[PATCH weston 04/15] libweston: let add/remove_output handle the lists

[Pekka Paalanen]

On Mon, 24 Jul 2017 16:08:49 +0300
Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Fri, 7 Apr 2017 20:58:45 +0200
> Armin Krezović <krezovic.armin at gmail.com> wrote:
> 
> > On 04.04.2017 12:58, Pekka Paalanen wrote:  
> > > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > > 
> > > A weston_output available to the compositor should always be either in
> > > the pending or the live outputs list. Let weston_compositor_add_output()
> > > and weston_compositor_remove_output() handle the moves between the
> > > lists.
> > > 
> > > This way weston_output_enable() does not need to remove and
> > > oops-it-failed-add-it-back. weston_output_disable() does not need to
> > > manually re-add the output back to the pending list.
> > > 
> > > To make everything nicely symmetric and fix any unbalancing caused by
> > > this:
> > > - weston_output_destroy() explicitly wl_list_remove()s
> > > - weston_compositor_add_pending_output() first removes then inserts, as
> > > we have the assumption that the link is always valid, even if empty.
> > > 
> > > Update the documentations, too.

> > > @@ -4555,7 +4561,6 @@ weston_compositor_remove_output(struct weston_output *output)
> > >  	weston_presentation_feedback_discard_list(&output->feedback_list);
> > >  
> > >  	weston_compositor_reflow_outputs(compositor, output, output->width);
> > > -	wl_list_remove(&output->link);
> > >      
> > 
> > When I looked more closely at this part when I applied your patches, I found a
> > potential problem with moving this down below. The original code first removed
> > the output from the enabled output list, then emitted the signals.
> > 
> > Signal listeners might iterate through output list before this function "returns",
> > ie, to pick a new output for a view, and could potentially pick this output again.
> > 
> > Even worse, wl_list_empty(&c->output_list) will return false even if this is the
> > last output going away, throwing previous work about "all outputs gone at runtime"
> > down the drain.  
> 
> Hi Armin,
> 
> yes, a very good catch. I'll fix that, probably by moving both remove
> and insert to where the remove used to be. In fact, I'm thinking if
> that list manipulation should be the very first thing that function
> does.

Meh, reflow_outputs relies on the output still being in the list, so
making it the very first thing is for another time.

Thanks,
pq

> > >  	wl_signal_emit(&compositor->output_destroyed_signal, output);
> > >  	wl_signal_emit(&output->destroy_signal, output);
> > > @@ -4563,6 +4568,9 @@ weston_compositor_remove_output(struct weston_output *output)
> > >  	wl_resource_for_each(resource, &output->resource_list) {
> > >  		wl_resource_set_destructor(resource, NULL);
> > >  	}
> > > +
> > > +	wl_list_remove(&output->link);
> > > +	wl_list_insert(compositor->pending_output_list.prev, &output->link);
> > >  }
> > >      
> > 
> >   
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170724/52f4f924/attachment.sig>