Oh wow, I was playing around with outputs, and never realized output_handle_done was being called after any geometry change, not just after the output was created.
On Thu, May 14, 2015 at 2:58 AM, Marek Chalupa <[email protected]> wrote: > output.done event can be sent even on some property change, not only > when announcing the output. Therefore we must check if we already have it > otherwise we may corrupt the list by adding it multiple times. > > This fixes bug when xwayland looped indefinitely in output.done handler > and that can be reproduced following these steps (under X without > multi-monitor setup): > 1) run weston --output-count=2 > 2) run xterm, move it so that half is on one output > and half on the other > 3) close second output, try run weston-terminal > (I can only repro it after closing the first output, not the second one; is that what you meant?) > weston sends updated outputs which trigger this bug. > > Signed-off-by: Marek Chalupa <[email protected]> > --- > hw/xwayland/xwayland-output.c | 25 +++++++++++++++++++------ > 1 file changed, 19 insertions(+), 6 deletions(-) > > diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c > index 155cbc1..0c96e87 100644 > --- a/hw/xwayland/xwayland-output.c > +++ b/hw/xwayland/xwayland-output.c > @@ -116,15 +116,28 @@ output_handle_mode(void *data, struct wl_output > *wl_output, uint32_t flags, > static void > output_handle_done(void *data, struct wl_output *wl_output) > { > - struct xwl_output *xwl_output = data; > + struct xwl_output *it, *xwl_output = data; > struct xwl_screen *xwl_screen = xwl_output->xwl_screen; > - int width, height; > + int width = 0, height = 0, has_this_output = 0; > + > + xorg_list_for_each_entry(it, &xwl_screen->output_list, link) { > + /* output done event is sent even when some property > + * of output is changed. That means that we may already > + * have this output. If it is true, we must not add it > + * into the output_list otherwise we'll corrupt it */ > + if (it == xwl_output) > + has_this_output = 1; > + > + if (width < it->x + it->width) > + width = it->x + it->width; > + if (height < it->y + it->height) > + height = it->y + it->height; > + } > > - xorg_list_append(&xwl_output->link, &xwl_screen->output_list); > + if (!has_this_output) { > + xorg_list_append(&xwl_output->link, &xwl_screen->output_list); > I think this line should also be moved here: xwl_screen->expecting_event--; (It might not matter since it's only used by xwl_screen_init - but the code seems to assume it would only be decremented once after the output is created.) - width = 0; > - height = 0; > - xorg_list_for_each_entry(xwl_output, &xwl_screen->output_list, link) { > + /* we did not check this output for new screen size, do it now */ > if (width < xwl_output->x + xwl_output->width) > width = xwl_output->x + xwl_output->width; > if (height < xwl_output->y + xwl_output->height) > -- > 2.1.0 > > _______________________________________________ > wayland-devel mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > Just a thought... You could instead: - check if the output already exists - add it if necessary - update the width and height That way, the width/height calculation code won't be duplicated. (Though it will require iterating over output_list twice.) Anyways, it's up to you.
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
