Re: [PATCH weston] hmi-controller: remove duplicate commit_changes in random mode

[Pekka Paalanen]

On Wed, 10 Feb 2016 08:54:47 +
"Ucan, Emre (ADITG/SW1)" <eu...@de.adit-jv.com> wrote:

> Hi Pekka,
> 
> My comments are below

Hi Emre

> > -Original Message-
> > From: Pekka Paalanen [mailto:ppaala...@gmail.com]
> > Sent: Montag, 8. Februar 2016 10:21
> > To: Ucan, Emre (ADITG/SW1)
> > Cc: Nobuhiko Tanibata; securitych...@denso.co.jp; Natsume, Wataru
> > (ADITJ/SWG); wayland-devel@lists.freedesktop.org; Friedrich, Eugen
> > (ADITG/SW1)
> > Subject: Re: [PATCH weston] hmi-controller: remove duplicate
> > commit_changes in random mode
> > 
> > On Fri, 5 Feb 2016 21:42:02 +
> > "Ucan, Emre (ADITG/SW1)" <eu...@de.adit-jv.com> wrote:
> >   
> > > Hi Pekka,
> > >
> > > First Question:
> > > An ivi-layer should have properties so that a group of surfaces can be
> > > scaled and clipped according to destination and source rectangle of
> > > the layer.
> > >
> > > Second Question:
> > > We have requirements for compositing an ivi-surface on many layers
> > > and/or screens. But I think it is reasonable to restrict an ivi-layer
> > > to a single screen.  
> > 
> > Hi Emre,
> > 
> > ok, and you are going to introduce ivi-view as a first-class concept in the
> > ivi_layout API?
> > 
> > Below are some things that came to my mind and they are only food for
> > thought, not requirements.
> > 
> > Do I understand the following right?
> > 
> > ivi-surface:
> > - created by Wayland clients (ivi apps) with a specific ivi-id
> > - has properties like size, but not position or src/dst rects
> > - what about opacity etc.? here or in ivi-view?
> > - may be associated with several ivi-views
> > - backed by a weston_surface
> >   
> 
> The surface will have buffer size as property. It will not have
> opacity and visibility.

Ok, buffer size is a matter of opinion. In my view, buffer size is a
property of the buffer. Then wl_surface has things like
buffer_transform, buffer_scale, and wl_viewport settings, and all these
together produce the surface size. This is all specified by the Wayland
protocol and controlled by the client, so they are read-only in
the compositor.

> > If opacity is not settable by the app, it would make more sense in
> > ivi-view.
> > 
> > ivi-view:
> > - is associated with exactly one ivi-surface
> > - created by the window manager system as a Weston/ivi-shell
> > internal object
> > - gets content from the ivi-surface
> > - links the ivi-surface to an ivi-layer
> > - property src rect given in ivi-surface coordinates
> > - property dst rect given in ivi-layer coordinate space
> > - backed by a weston_view
> >   
> 
> View will have opacity and visibility. Src rectangle will be used for
> weston_view_set_mask. Dst rect will be used to calculate the scaling
> factor of view. Dst rect is given in ivi-layer coordinate space. The
> parts of the view are not shown, which are outside of the dest rect
> of its ivi-layer. 

Agreed.

> > ivi-layer:
> > - groups ivi-views into an ordered set
> > - created by the window manager system as a Weston/ivi-shell
> > internal object
> > - links the set of ivi-views to exactly zero or one ivi-screen
> > - property src rect given in ivi-layer coordinate space
> > - property dst rect given in ivi-screen coordinate space
> > - IOW, maps content from ivi-layer space to ivi-screen space
> > - other properties like opacity? (note that correctly implementing
> >   opacity will have the same problem as sub-surfaces: you usually
> > need an intermediate composite, which Weston does not implement
> > atm.)
> > - may not be backed by any particular Weston core structure  
> 
> Basically similar to my ideas. I have to think about visibility and
> opacity. We have to support the legacy behavior of LayerManagerClient
> APIs. Please check this:
> http://projects.genivi.org/wayland-ivi-extension/layer-manager-apis
> to understand how ivi-layers and ivi-surfaces should behave. Most
> likely, we have to adjust some APIs. But it must be discussed in
> GENIVI community. We should not break the workflow of the users.

Yes, I am familiar with that page. It is mostly doable, just the need
for an intermediate composite could be an issue.

> > 
> > ivi-screen:
> > - represents a single output (monitor)
> > - contents specified by multiple ordered ivi-layers
> > - no adjustable properties
> > - created by the window manager system as a Weston/ivi-shell
> > internal object
> > - 1:1 with a weston_output
> >

RE: [PATCH weston] hmi-controller: remove duplicate commit_changes in random mode

[Ucan, Emre (ADITG/SW1)]

Hi Pekka,

My comments are below

> -Original Message-
> From: Pekka Paalanen [mailto:ppaala...@gmail.com]
> Sent: Montag, 8. Februar 2016 10:21
> To: Ucan, Emre (ADITG/SW1)
> Cc: Nobuhiko Tanibata; securitych...@denso.co.jp; Natsume, Wataru
> (ADITJ/SWG); wayland-devel@lists.freedesktop.org; Friedrich, Eugen
> (ADITG/SW1)
> Subject: Re: [PATCH weston] hmi-controller: remove duplicate
> commit_changes in random mode
> 
> On Fri, 5 Feb 2016 21:42:02 +
> "Ucan, Emre (ADITG/SW1)" <eu...@de.adit-jv.com> wrote:
> 
> > Hi Pekka,
> >
> > First Question:
> > An ivi-layer should have properties so that a group of surfaces can be
> > scaled and clipped according to destination and source rectangle of
> > the layer.
> >
> > Second Question:
> > We have requirements for compositing an ivi-surface on many layers
> > and/or screens. But I think it is reasonable to restrict an ivi-layer
> > to a single screen.
> 
> Hi Emre,
> 
> ok, and you are going to introduce ivi-view as a first-class concept in the
> ivi_layout API?
> 
> Below are some things that came to my mind and they are only food for
> thought, not requirements.
> 
> Do I understand the following right?
> 
> ivi-surface:
> - created by Wayland clients (ivi apps) with a specific ivi-id
> - has properties like size, but not position or src/dst rects
> - what about opacity etc.? here or in ivi-view?
> - may be associated with several ivi-views
> - backed by a weston_surface
> 

The surface will have buffer size as property. It will not have opacity and 
visibility.

> If opacity is not settable by the app, it would make more sense in ivi-view.
> 
> ivi-view:
> - is associated with exactly one ivi-surface
> - created by the window manager system as a Weston/ivi-shell internal
>   object
> - gets content from the ivi-surface
> - links the ivi-surface to an ivi-layer
> - property src rect given in ivi-surface coordinates
> - property dst rect given in ivi-layer coordinate space
> - backed by a weston_view
> 

View will have opacity and visibility. Src rectangle will be used for 
weston_view_set_mask.
Dst rect will be used to calculate the scaling factor of view. Dst rect is 
given in ivi-layer coordinate space.
The parts of the view are not shown, which are outside of the dest rect of its 
ivi-layer. 

> ivi-layer:
> - groups ivi-views into an ordered set
> - created by the window manager system as a Weston/ivi-shell internal
>   object
> - links the set of ivi-views to exactly zero or one ivi-screen
> - property src rect given in ivi-layer coordinate space
> - property dst rect given in ivi-screen coordinate space
> - IOW, maps content from ivi-layer space to ivi-screen space
> - other properties like opacity? (note that correctly implementing
>   opacity will have the same problem as sub-surfaces: you usually need
>   an intermediate composite, which Weston does not implement atm.)
> - may not be backed by any particular Weston core structure

Basically similar to my ideas. I have to think about visibility and opacity.
We have to support the legacy behavior of LayerManagerClient APIs.
Please check this: 
http://projects.genivi.org/wayland-ivi-extension/layer-manager-apis to 
understand how ivi-layers and ivi-surfaces should behave.
Most likely, we have to adjust some APIs. But it must be discussed in GENIVI 
community. We should not break the workflow of the users.
> 
> ivi-screen:
> - represents a single output (monitor)
> - contents specified by multiple ordered ivi-layers
> - no adjustable properties
> - created by the window manager system as a Weston/ivi-shell internal
>   object
> - 1:1 with a weston_output
> 
> In this scheme, you can place an ivi-surface on any ivi-layers at any number 
> of
> times by creating an ivi-view for each. The ivi-view specifies the position, 
> so
> each "clone" of the ivi-surface is completely independently mappable.
> 
> Putting an ivi-layer on multiple ivi-screens can be implemented by creating an
> ivi-layer per ivi-screen, and associating ivi-surfaces with each ivi-layer by
> creating the necessary ivi-views.
> 
> About the ivi_layout API and its implementation:
> 
> I would recommend to have the API use pointers to struct ivi-surface, ivi-
> view, etc. as the primary means of object reference, and leave the ivi-id
> usage just for getting the pointer from an ivi-id and functions creating
> objects. I think most of the API already is this way, but the implementations
> are still matching a lot by ivi-id, when they already have the pointer they 
> are
> looking for.

You are right we do not need to use ivi-ids internally. Pointers will be easier 
to handle.
I want to get rid

Re: [PATCH weston] hmi-controller: remove duplicate commit_changes in random mode

[Pekka Paalanen]

On Fri, 5 Feb 2016 21:42:02 +
"Ucan, Emre (ADITG/SW1)" <eu...@de.adit-jv.com> wrote:

> Hi Pekka,
> 
> First Question:
> An ivi-layer should have properties so that a group of surfaces can
> be scaled and clipped according to destination and source rectangle
> of the layer.
> 
> Second Question:
> We have requirements for compositing an ivi-surface on many layers
> and/or screens. But I think it is reasonable to restrict an ivi-layer
> to a single screen.

Hi Emre,

ok, and you are going to introduce ivi-view as a first-class concept in
the ivi_layout API?

Below are some things that came to my mind and they are only food for
thought, not requirements.

Do I understand the following right?

ivi-surface:
- created by Wayland clients (ivi apps) with a specific ivi-id
- has properties like size, but not position or src/dst rects
- what about opacity etc.? here or in ivi-view?
- may be associated with several ivi-views
- backed by a weston_surface

If opacity is not settable by the app, it would make more sense in
ivi-view.

ivi-view:
- is associated with exactly one ivi-surface
- created by the window manager system as a Weston/ivi-shell internal
  object
- gets content from the ivi-surface
- links the ivi-surface to an ivi-layer
- property src rect given in ivi-surface coordinates
- property dst rect given in ivi-layer coordinate space
- backed by a weston_view

ivi-layer:
- groups ivi-views into an ordered set
- created by the window manager system as a Weston/ivi-shell internal
  object
- links the set of ivi-views to exactly zero or one ivi-screen
- property src rect given in ivi-layer coordinate space
- property dst rect given in ivi-screen coordinate space
- IOW, maps content from ivi-layer space to ivi-screen space
- other properties like opacity? (note that correctly implementing
  opacity will have the same problem as sub-surfaces: you usually need
  an intermediate composite, which Weston does not implement atm.)
- may not be backed by any particular Weston core structure

ivi-screen:
- represents a single output (monitor)
- contents specified by multiple ordered ivi-layers
- no adjustable properties
- created by the window manager system as a Weston/ivi-shell internal
  object
- 1:1 with a weston_output

In this scheme, you can place an ivi-surface on any ivi-layers at any
number of times by creating an ivi-view for each. The ivi-view
specifies the position, so each "clone" of the ivi-surface is
completely independently mappable.

Putting an ivi-layer on multiple ivi-screens can be implemented by
creating an ivi-layer per ivi-screen, and associating ivi-surfaces with
each ivi-layer by creating the necessary ivi-views.

About the ivi_layout API and its implementation:

I would recommend to have the API use pointers to struct ivi-surface,
ivi-view, etc. as the primary means of object reference, and leave the
ivi-id usage just for getting the pointer from an ivi-id and
functions creating objects. I think most of the API already is this
way, but the implementations are still matching a lot by ivi-id, when
they already have the pointer they are looking for.

The transition framework seems to be using a timer to get a callback
for advancing animations. Weston already has an animation framework,
but it is quite specific to weston_views and weston_spring, so I'm not
sure reusing that directly makes sense. However, what it does right is
hooking up to the output repaint cycle instead of using timers or idle
callbacks. This make sure that animation state is updated exactly the
right time, and not too late or too often.


Thanks,
pq

> -Original Message-
> From: wayland-devel
> [mailto:wayland-devel-boun...@lists.freedesktop.org] On Behalf Of
> Pekka Paalanen Sent: Freitag, 5. Februar 2016 15:08 To: Ucan, Emre
> (ADITG/SW1) Cc: Nobuhiko Tanibata; securitych...@denso.co.jp;
> Natsume, Wataru (ADITJ/SWG); wayland-devel@lists.freedesktop.org
> Subject: Re: [PATCH weston] hmi-controller: remove duplicate
> commit_changes in random mode
> 
> On Thu, 4 Feb 2016 16:31:50 +
> "Ucan, Emre (ADITG/SW1)" <eu...@de.adit-jv.com> wrote:
> 
> > Hi Pekka,
> > 
> > I am for removing the remaining bits of code which is wrongly
> > assuming that a surface can be shown in multiple layers.  
> 
> Hi Emre,
> 
> sounds good.
> 
> >  Afterwards we have to redesign a big part of ivi_layout API to 
> > support this feauture.
> > 
> > My ideas for the redesign so far:
> > The most basic approach would be that an ivi_surface would have a 
> > weston_view for every layer/screen combination.  
> 
> Yes, I believe that is how it could/should work.
> 
> > But ivi_layout API
> > does not make much sense than. Because when controller calls 
> > ivi_layout_surface_set_destination_rectangle, should we scale all 

RE: [PATCH weston] hmi-controller: remove duplicate commit_changes in random mode

[Ucan, Emre (ADITG/SW1)]

Hi Pekka,

First Question:
An ivi-layer should have properties so that a group of surfaces can be scaled 
and clipped according to destination and source rectangle of the layer.

Second Question:
We have requirements for compositing an ivi-surface on many layers and/or 
screens. But I think it is reasonable to restrict an ivi-layer to a single 
screen.

Best regards

Emre Ucan
Software Group I (ADITG/SW1)

Tel. +49 5121 49 6937
-Original Message-
From: wayland-devel [mailto:wayland-devel-boun...@lists.freedesktop.org] On 
Behalf Of Pekka Paalanen
Sent: Freitag, 5. Februar 2016 15:08
To: Ucan, Emre (ADITG/SW1)
Cc: Nobuhiko Tanibata; securitych...@denso.co.jp; Natsume, Wataru (ADITJ/SWG); 
wayland-devel@lists.freedesktop.org
Subject: Re: [PATCH weston] hmi-controller: remove duplicate commit_changes in 
random mode

On Thu, 4 Feb 2016 16:31:50 +
"Ucan, Emre (ADITG/SW1)" <eu...@de.adit-jv.com> wrote:

> Hi Pekka,
> 
> I am for removing the remaining bits of code which is wrongly assuming 
> that a surface can be shown in multiple layers.

Hi Emre,

sounds good.

>  Afterwards we have to redesign a big part of ivi_layout API to 
> support this feauture.
> 
> My ideas for the redesign so far:
> The most basic approach would be that an ivi_surface would have a 
> weston_view for every layer/screen combination.

Yes, I believe that is how it could/should work.

> But ivi_layout API
> does not make much sense than. Because when controller calls 
> ivi_layout_surface_set_destination_rectangle, should we scale all 
> views ? I think it does not make sense to scale all views.

Indeed, that was a bit weird, like a surface's position on any layer being 
recorded with the surface, which means you cannot have per-layer positions. The 
same with layer vs. screen, IIRC.

> Therefore,
> the set APIs should be changed to get ivi_views and not ivi_surfaces.
> This would mean:
> 
> - Modifying every ivi_layout_surface_set* API
> - Implementing new APIs to get layer/screen specific ivi_views for a 
> given ivi_surface
> - ivi-layers should list ivi_views and not ivi_surfaces
> - The implementation of commit_changes should be updated accordingly.
> 
> I am planning to do all these changes, but at first we can remove old 
> code. Because it is very unlikely that we can reuse.

Do you still think that ivi-layer should have a size and a position and all the 
other attributes?
Or should it be used only for grouping and z-ordering ivi-surfaces/views, and 
linking them to a screen?

Could an ivi-layer be on multiple screens? If you can restrict an ivi-layer to 
a single ivi-screen, I think that would simplify things a lot.

It seems you are moving closer to weston's internal architecture with surfaces, 
views, and layers. :-)


Thanks,
pq

> -Original Message-
> From: wayland-devel
> [mailto:wayland-devel-boun...@lists.freedesktop.org] On Behalf Of 
> Pekka Paalanen Sent: Dienstag, 2. Februar 2016 16:43 To: Nobuhiko 
> Tanibata Cc: securitych...@denso.co.jp; Natsume, Wataru (ADITJ/SWG); 
> wayland-devel@lists.freedesktop.org Subject: Re: [PATCH weston]
> hmi-controller: remove duplicate commit_changes in random mode
> 
> Hi,
> 
> I don't think this patch is ready to be merged, more below.
> 
> TL;DR: It would probably be best to just ignore which ivilayer an 
> ivisurface is currently on, and always just remove and add. That would 
> be the simplest solution, if the ivilayout API just allows it.
> 
> Specifically, do not clear the layer's surface list in advance. Just 
> go over each surface and reassign the layer for it. Then doing a 
> single commit_changes() in the end will ensure that ivisurfaces get 
> atomically moved from layer to layer as appropriate, and there won't 
> be any multiple assignments.
> 
> 
> On Fri, 25 Dec 2015 13:47:15 +0900
> Nobuhiko Tanibata <nobuhiko_tanib...@xddp.denso.co.jp> wrote:
> 
> > From: Nobuhiko Tanibata <nobuhiko_tanib...@xddp.denso.co.jp>
> > 
> > Previous code cleaned up surfaces in layer once and then added 
> > surfaces to a layer in random. In this flow, two commitchanges are 
> > required.
> > 
> > This patch proposes that it avoids calling add_surface if a surface 
> > is already added to a layer in random. In this flow, cleaning up 
> > surfaces is not required.
> > 
> > Signed-off-by: Nobuhiko Tanibata
> > <nobuhiko_tanib...@xddp.denso.co.jp> ---  ivi-shell/hmi-controller.c 
> > | 25 -
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/ivi-shell/hmi-controller.c b/ivi-shell/hmi-controller.c 
> > index 77426bc..8a81f5c 100644
> > --- a/ivi-shell/hmi-controller.c
> > +++ b/ivi-shell/hmi-controller.c
> > @@ -4

Re: [PATCH weston] hmi-controller: remove duplicate commit_changes in random mode

[Pekka Paalanen]

On Thu, 4 Feb 2016 16:31:50 +
"Ucan, Emre (ADITG/SW1)" <eu...@de.adit-jv.com> wrote:

> Hi Pekka,
> 
> I am for removing the remaining bits of code which is wrongly
> assuming that a surface can be shown in multiple layers.

Hi Emre,

sounds good.

>  Afterwards we have to redesign a big part of ivi_layout API to
> support this feauture.
> 
> My ideas for the redesign so far:
> The most basic approach would be that an ivi_surface would have a
> weston_view for every layer/screen combination.

Yes, I believe that is how it could/should work.

> But ivi_layout API
> does not make much sense than. Because when controller calls
> ivi_layout_surface_set_destination_rectangle, should we scale all
> views ? I think it does not make sense to scale all views.

Indeed, that was a bit weird, like a surface's position on any layer
being recorded with the surface, which means you cannot have per-layer
positions. The same with layer vs. screen, IIRC.

> Therefore,
> the set APIs should be changed to get ivi_views and not ivi_surfaces.
> This would mean:
> 
> - Modifying every ivi_layout_surface_set* API
> - Implementing new APIs to get layer/screen specific ivi_views for a
> given ivi_surface
> - ivi-layers should list ivi_views and not ivi_surfaces
> - The implementation of commit_changes should be updated accordingly.
> 
> I am planning to do all these changes, but at first we can remove old
> code. Because it is very unlikely that we can reuse.

Do you still think that ivi-layer should have a size and a position and
all the other attributes?
Or should it be used only for grouping and z-ordering
ivi-surfaces/views, and linking them to a screen?

Could an ivi-layer be on multiple screens? If you can restrict an
ivi-layer to a single ivi-screen, I think that would simplify things a
lot.

It seems you are moving closer to weston's internal architecture with
surfaces, views, and layers. :-)


Thanks,
pq

> -Original Message-
> From: wayland-devel
> [mailto:wayland-devel-boun...@lists.freedesktop.org] On Behalf Of
> Pekka Paalanen Sent: Dienstag, 2. Februar 2016 16:43 To: Nobuhiko
> Tanibata Cc: securitych...@denso.co.jp; Natsume, Wataru (ADITJ/SWG);
> wayland-devel@lists.freedesktop.org Subject: Re: [PATCH weston]
> hmi-controller: remove duplicate commit_changes in random mode
> 
> Hi,
> 
> I don't think this patch is ready to be merged, more below.
> 
> TL;DR: It would probably be best to just ignore which ivilayer an
> ivisurface is currently on, and always just remove and add. That
> would be the simplest solution, if the ivilayout API just allows it.
> 
> Specifically, do not clear the layer's surface list in advance. Just
> go over each surface and reassign the layer for it. Then doing a
> single commit_changes() in the end will ensure that ivisurfaces get
> atomically moved from layer to layer as appropriate, and there won't
> be any multiple assignments.
> 
> 
> On Fri, 25 Dec 2015 13:47:15 +0900
> Nobuhiko Tanibata <nobuhiko_tanib...@xddp.denso.co.jp> wrote:
> 
> > From: Nobuhiko Tanibata <nobuhiko_tanib...@xddp.denso.co.jp>
> > 
> > Previous code cleaned up surfaces in layer once and then added 
> > surfaces to a layer in random. In this flow, two commitchanges are
> > required.
> > 
> > This patch proposes that it avoids calling add_surface if a surface
> > is already added to a layer in random. In this flow, cleaning up
> > surfaces is not required.
> > 
> > Signed-off-by: Nobuhiko Tanibata
> > <nobuhiko_tanib...@xddp.denso.co.jp> ---
> >  ivi-shell/hmi-controller.c | 25 -
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/ivi-shell/hmi-controller.c
> > b/ivi-shell/hmi-controller.c index 77426bc..8a81f5c 100644
> > --- a/ivi-shell/hmi-controller.c
> > +++ b/ivi-shell/hmi-controller.c
> > @@ -418,24 +418,18 @@ mode_random_replace(struct hmi_controller
> > *hmi_ctrl, struct ivi_layout_surface *ivisurf  = NULL;
> > const uint32_t duration =
> > hmi_ctrl->hmi_setting->transition_duration; int32_t i = 0;
> > +   int32_t j = 0;
> > int32_t layer_idx = 0;
> > +   int32_t surface_len_on_layer = 0;
> > +   struct ivi_layout_surface **ivisurfs = NULL;
> >  
> > layers = MEM_ALLOC(sizeof(*layers) * hmi_ctrl->screen_num);
> >  
> > wl_list_for_each(application_layer, layer_list, link) {
> > layers[layer_idx] = application_layer;
> > -
> > ivi_layout_interface->layer_set_render_order(layers[layer_idx]->ivilayer,
> > -   NULL, 0);
> > layer_idx++;
> > 

RE: [PATCH weston] hmi-controller: remove duplicate commit_changes in random mode

[Ucan, Emre (ADITG/SW1)]

Hi Pekka,

I am for removing the remaining bits of code which is wrongly assuming that a 
surface can be shown in multiple layers.

 Afterwards we have to redesign a big part of ivi_layout API to support this 
feauture.

My ideas for the redesign so far:
The most basic approach would be that an ivi_surface would have a weston_view 
for every layer/screen combination.
But ivi_layout API does not make much sense than. Because when controller calls 
ivi_layout_surface_set_destination_rectangle,
should we scale all views ? I think it does not make sense to scale all views. 
Therefore, the set APIs should be changed to get ivi_views and not ivi_surfaces.
This would mean:

- Modifying every ivi_layout_surface_set* API
- Implementing new APIs to get layer/screen specific ivi_views for a given 
ivi_surface
- ivi-layers should list ivi_views and not ivi_surfaces
- The implementation of commit_changes should be updated accordingly.

I am planning to do all these changes, but at first we can remove old code. 
Because it is very unlikely that we can reuse.

Best regards

Emre Ucan
Software Group I (ADITG/SW1)

Tel. +49 5121 49 6937

-Original Message-
From: wayland-devel [mailto:wayland-devel-boun...@lists.freedesktop.org] On 
Behalf Of Pekka Paalanen
Sent: Dienstag, 2. Februar 2016 16:43
To: Nobuhiko Tanibata
Cc: securitych...@denso.co.jp; Natsume, Wataru (ADITJ/SWG); 
wayland-devel@lists.freedesktop.org
Subject: Re: [PATCH weston] hmi-controller: remove duplicate commit_changes in 
random mode

Hi,

I don't think this patch is ready to be merged, more below.

TL;DR: It would probably be best to just ignore which ivilayer an ivisurface is 
currently on, and always just remove and add. That would be the simplest 
solution, if the ivilayout API just allows it.

Specifically, do not clear the layer's surface list in advance. Just go over 
each surface and reassign the layer for it. Then doing a single
commit_changes() in the end will ensure that ivisurfaces get atomically moved 
from layer to layer as appropriate, and there won't be any multiple assignments.


On Fri, 25 Dec 2015 13:47:15 +0900
Nobuhiko Tanibata <nobuhiko_tanib...@xddp.denso.co.jp> wrote:

> From: Nobuhiko Tanibata <nobuhiko_tanib...@xddp.denso.co.jp>
> 
> Previous code cleaned up surfaces in layer once and then added 
> surfaces to a layer in random. In this flow, two commitchanges are required.
> 
> This patch proposes that it avoids calling add_surface if a surface is 
> already added to a layer in random. In this flow, cleaning up surfaces 
> is not required.
> 
> Signed-off-by: Nobuhiko Tanibata <nobuhiko_tanib...@xddp.denso.co.jp>
> ---
>  ivi-shell/hmi-controller.c | 25 -
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/ivi-shell/hmi-controller.c b/ivi-shell/hmi-controller.c 
> index 77426bc..8a81f5c 100644
> --- a/ivi-shell/hmi-controller.c
> +++ b/ivi-shell/hmi-controller.c
> @@ -418,24 +418,18 @@ mode_random_replace(struct hmi_controller *hmi_ctrl,
>   struct ivi_layout_surface *ivisurf  = NULL;
>   const uint32_t duration = hmi_ctrl->hmi_setting->transition_duration;
>   int32_t i = 0;
> + int32_t j = 0;
>   int32_t layer_idx = 0;
> + int32_t surface_len_on_layer = 0;
> + struct ivi_layout_surface **ivisurfs = NULL;
>  
>   layers = MEM_ALLOC(sizeof(*layers) * hmi_ctrl->screen_num);
>  
>   wl_list_for_each(application_layer, layer_list, link) {
>   layers[layer_idx] = application_layer;
> - 
> ivi_layout_interface->layer_set_render_order(layers[layer_idx]->ivilayer,
> - NULL, 0);
>   layer_idx++;
>   }
>  
> - /*
> -  * This commit change is needed because ivisurface can not belongs to 
> several layers
> -  * at the same time. So ivisurfaces shall be removed from layers once 
> and then set them
> -  * to layers randomly.
> -  */
> - ivi_layout_interface->commit_changes();

This is a lengthy side-comment that came to my mind while looking at the code:

An ivisurface indeed cannot belong to several layers at the same time, but I 
don't think that would happen (break anything) even if you literally removed 
only this commit_changes() call.

That is because the staging list (ivi_layout_surface::pending) and the current 
list (ivi_layout_surface::order) are separate. I believe it is fine to have an 
ivi_layout_surface on one layer in the current list and on a different layer in 
the staging list. You have to make that work anyway, because it is the only way 
to move an ivisurface from one layer to another without potentially causing it 
to disappear from the screen in between.

When the connecting structures, that were meant to allow an ivisurface to be on 
several

Re: [PATCH weston] hmi-controller: remove duplicate commit_changes in random mode

[Pekka Paalanen]

Hi,

I don't think this patch is ready to be merged, more below.

TL;DR: It would probably be best to just ignore which ivilayer an
ivisurface is currently on, and always just remove and add. That would be
the simplest solution, if the ivilayout API just allows it.

Specifically, do not clear the layer's surface list in advance. Just go
over each surface and reassign the layer for it. Then doing a single
commit_changes() in the end will ensure that ivisurfaces get atomically
moved from layer to layer as appropriate, and there won't be any
multiple assignments.


On Fri, 25 Dec 2015 13:47:15 +0900
Nobuhiko Tanibata  wrote:

> From: Nobuhiko Tanibata 
> 
> Previous code cleaned up surfaces in layer once and then added surfaces
> to a layer in random. In this flow, two commitchanges are required.
> 
> This patch proposes that it avoids calling add_surface if a surface is
> already added to a layer in random. In this flow, cleaning up
> surfaces is not required.
> 
> Signed-off-by: Nobuhiko Tanibata 
> ---
>  ivi-shell/hmi-controller.c | 25 -
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/ivi-shell/hmi-controller.c b/ivi-shell/hmi-controller.c
> index 77426bc..8a81f5c 100644
> --- a/ivi-shell/hmi-controller.c
> +++ b/ivi-shell/hmi-controller.c
> @@ -418,24 +418,18 @@ mode_random_replace(struct hmi_controller *hmi_ctrl,
>   struct ivi_layout_surface *ivisurf  = NULL;
>   const uint32_t duration = hmi_ctrl->hmi_setting->transition_duration;
>   int32_t i = 0;
> + int32_t j = 0;
>   int32_t layer_idx = 0;
> + int32_t surface_len_on_layer = 0;
> + struct ivi_layout_surface **ivisurfs = NULL;
>  
>   layers = MEM_ALLOC(sizeof(*layers) * hmi_ctrl->screen_num);
>  
>   wl_list_for_each(application_layer, layer_list, link) {
>   layers[layer_idx] = application_layer;
> - 
> ivi_layout_interface->layer_set_render_order(layers[layer_idx]->ivilayer,
> - NULL, 0);
>   layer_idx++;
>   }
>  
> - /*
> -  * This commit change is needed because ivisurface can not belongs to 
> several layers
> -  * at the same time. So ivisurfaces shall be removed from layers once 
> and then set them
> -  * to layers randomly.
> -  */
> - ivi_layout_interface->commit_changes();

This is a lengthy side-comment that came to my mind while looking at
the code:

An ivisurface indeed cannot belong to several layers at the same time,
but I don't think that would happen (break anything) even if you
literally removed only this commit_changes() call.

That is because the staging list (ivi_layout_surface::pending) and the
current list (ivi_layout_surface::order) are separate. I believe it is
fine to have an ivi_layout_surface on one layer in the current list and
on a different layer in the staging list. You have to make that work
anyway, because it is the only way to move an ivisurface from one layer
to another without potentially causing it to disappear from the screen
in between.

When the connecting structures, that were meant to allow an ivisurface
to be on several layers, were removed, the code automatically became
such that it naturally avoids attempting to have an ivisurface on
multiple layers, even if you tried to do that from the ivilayout
external API. The list manipulations just work, and the last assignment
will prevail.

There are also leftovers in the code, that go through a list of all
ivisurfaces just to find an ivisurface with the right ivi-id when you
already have a pointer to the ivisurface with the ivi-id you are
looking for. Since there cannot be multiple ivisurfaces with the same
ivi-id, the search will only check if the given ivisurface is on the
given list. If the list by definition contains all ivisurfaces, the
check is moot. I think this happens in functions like
ivi_layout_layer_add_surface() and ivi_layout_layer_set_render_order().

But, as said, that is an aside. I think there is a lot that could be
simplified and cleaned up in the surface/layer/screen management, but
that's off-topic here.

> -
>   for (i = 0; i < surface_length; i++) {
>   ivisurf = pp_surface[i];
>  
> @@ -463,6 +457,19 @@ mode_random_replace(struct hmi_controller *hmi_ctrl,
>surface_width,
>surface_height);
>  
> + ivi_layout_interface
> + ->get_surfaces_on_layer(layers[layer_idx]->ivilayer,
> + _len_on_layer,
> + );

Please, try not to split around -> as it looks odd.

This call allocates memory and stores the pointer to 'ivisurfs', which
is then never freed, leaking.

> +
> + for (j =