On Mon, 16 May 2016 16:29:18 +0200 Armin Krezović <[email protected]> wrote:
> On 10.05.2016 16:10, Pekka Paalanen wrote: > > From: Pekka Paalanen <[email protected]> > > > > a7af70436b7dccfacd736626d6719b3e751fd985 converted the surface list into > > a view list. > > > > It looks like weston_surface::output's comment about surface list does > > not apply to view list. Still, many places assume weston_surface::output > > is not NULL when processing "visible" surfaces, e.g. those reachable via > > the view list. > > > > The comment on weston_view::output is updated, but I could not figure > > out the actual relationship between that and being on the view list. > > > > weston_view::link is documented to be in weston_compositor::view_list, > > and weston_compositor::view_list is documented to contain weston_views. > > > > It is always nice to see someone documenting the code. I've been banging > my head lately to understand the codebase, and the currently present > comments really help there. > > With the issue noted below fixed, this patch is: > > Reviewed-by: Armin Krezović <[email protected]> > > > Signed-off-by: Pekka Paalanen <[email protected]> > > --- > > src/compositor.h | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/src/compositor.h b/src/compositor.h > > index a95f05d..7851000 100644 > > --- a/src/compositor.h > > +++ b/src/compositor.h > > @@ -754,7 +754,7 @@ struct weston_compositor { > > struct wl_list output_list; > > struct wl_list seat_list; > > struct wl_list layer_list; > > - struct wl_list view_list; > > + struct wl_list view_list; /* struct weston_view::link */ > > struct wl_list plane_list; > > struct wl_list key_binding_list; > > struct wl_list modifier_binding_list; > > @@ -890,7 +890,7 @@ struct weston_view { > > struct wl_list surface_link; > > struct wl_signal destroy_signal; > > > > - struct wl_list link; > > + struct wl_list link; /* weston_compositor::view_list */ > > struct weston_layer_entry layer_link; /* part of geometry */ > > struct weston_plane *plane; > > > > @@ -951,7 +951,7 @@ struct weston_view { > > /* > > * Which output to vsync this surface to. > > * Used to determine, whether to send or queue frame events. > > - * Must be NULL, if 'link' is not in weston_compositor::surface_list. > > + * Must be NULL, if 'link' is not in weston_compositor::view_list. > > I couldn't find if anything checks for this, and view->output can only be null > if it got NULL from an output list. So I think the comment is redundant. Btw. you cannot get a NULL from an output list, unless you first explicitly check that the list is empty and bail out. Code that simply does wl_container_of(output_list.next, output, link) or similar will silently get a bad pointer and cause havoc later. The 'next' of an empty list points to the list head itself, it's not NULL. And list head is almost always not a valid list member. Cool. I'm re-checking just to be sure. Marking weston_view::link as deprecated, the compiler points me to the following cases: weston_view_unmap(): Bails out via weston_view_is_mapped() if view->output == NULL. Otherwise removes *and* initializes 'link'. So this is a potential issue, but the init hints that 'link' is intended to be always removable, which would void the issue. weston_view_create() unconditionally inits 'link', and weston_view_destroy() unconditionally removes 'link', so these confirm that 'link' is always removable. view_list_add_subsurface_view(): Bails out if surface->output is NULL. (Not view->output!) Otherwise it does an unconditional wl_list_insert(view->link), which means the list 'link' was previously in, is now corrupt because it did not remove first. view_list_add(): Also does an unconditional wl_list_insert(view->link), which means the list 'link' was previously in, is now corrupt. However, the corruption created there does not hurt, because those functions are called from weston_compositor_build_view_list(), which does wl_list_init() on the view_list, leaving all member of the list orphaned and with corrupted 'link' to begin with. But, there may be a danger there. If some views are not put back in the view list during that weston_compositor_build_view_list() call, they will be left with a corrupted 'link'. If you then remove 'link', it may corrupt others. This path could use more investigation, and maybe we should just fix it to keep all 'link' members always valid anyway by doing the proper removes. (You can also remove a list head, it means the elements remain in a headless list you cannot safely traverse, but remove will work just fine.) weston_plane_release(): Traverses the view_list to reset plane pointers to NULL since the plane is being destroyed... looks like it might miss any views not in view_list though, which might cause problems if such view taken into use again. weston_output_destroy(): Calls weston_view_assign_output() for all views in view_list who overlapped the output being destroyed. Presumably shell plugin should be repositioning view before this to avoid the output becoming NULL... but the destroy signals are only emitted afterwards. How does that work? That was the compositor.c uses of weston_view::link. Renderers and assign_planes() implementations also use the view_list, but I don't think they care what the weston_view::output is. IOW, did not find a connection between link and output there. I think I'll tweak the comments in these two patches a bit, and maybe push then. > > */ > > struct weston_output *output; > > > > @@ -1021,7 +1021,6 @@ struct weston_surface { > > /* > > * Which output to vsync this surface to. > > * Used to determine, whether to send or queue frame events. > > - * Must be NULL, if 'link' is not in weston_compositor::surface_list. > > */ > > struct weston_output *output; > > > > > Thanks, pq
pgpqikkHeBP_E.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
