<div dir="ltr">Actually, why not just move "<span style="font-size:12.8000001907349px">xorg_list_append(&xwl_output-></span><span style="font-size:12.8000001907349px">link, &xwl_screen->output_list);" to xwl_output_create?</span><div><span style="font-size:12.8000001907349px"><br></span></div><div><span style="font-size:12.8000001907349px">I can't tell if there's a reason it's in xwl_output_done, or if it's just an oversight.</span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 14, 2015 at 9:37 AM, Dima Ryazanov <span dir="ltr"><<a href="mailto:dima@gmail.com" target="_blank">dima@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Oh wow, I was playing around with outputs, and never realized <span style="font-size:12.8000001907349px">output_handle_done was being called after any geometry change, not just after the output was created.</span><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Thu, May 14, 2015 at 2:58 AM, Marek Chalupa <span dir="ltr"><<a href="mailto:mchqwerty@gmail.com" target="_blank">mchqwerty@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">output.done event can be sent even on some property change, not only<br>
when announcing the output. Therefore we must check if we already have it<br>
otherwise we may corrupt the list by adding it multiple times.<br>
<br>
This fixes bug when xwayland looped indefinitely in output.done handler<br>
and that can be reproduced following these steps (under X without<br>
multi-monitor setup):<br>
1) run weston --output-count=2<br>
2) run xterm, move it so that half is on one output<br>
and half on the other<br>
3) close second output, try run weston-terminal<br></blockquote><div><br></div></span><div>(I can only repro it after closing the first output, not the second one; is that what you meant?)</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">weston sends updated outputs which trigger this bug.<br>
<br>
Signed-off-by: Marek Chalupa <<a href="mailto:mchqwerty@gmail.com" target="_blank">mchqwerty@gmail.com</a>><br>
---<br>
hw/xwayland/xwayland-output.c | 25 +++++++++++++++++++------<br>
1 file changed, 19 insertions(+), 6 deletions(-)<br>
<br>
diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c<br>
index 155cbc1..0c96e87 100644<br>
--- a/hw/xwayland/xwayland-output.c<br>
+++ b/hw/xwayland/xwayland-output.c<br>
@@ -116,15 +116,28 @@ output_handle_mode(void *data, struct wl_output *wl_output, uint32_t flags,<br>
static void<br>
output_handle_done(void *data, struct wl_output *wl_output)<br>
{<br>
- struct xwl_output *xwl_output = data;<br>
+ struct xwl_output *it, *xwl_output = data;<br>
struct xwl_screen *xwl_screen = xwl_output->xwl_screen;<br>
- int width, height;<br>
+ int width = 0, height = 0, has_this_output = 0;<br>
+<br>
+ xorg_list_for_each_entry(it, &xwl_screen->output_list, link) {<br>
+ /* output done event is sent even when some property<br>
+ * of output is changed. That means that we may already<br>
+ * have this output. If it is true, we must not add it<br>
+ * into the output_list otherwise we'll corrupt it */<br>
+ if (it == xwl_output)<br>
+ has_this_output = 1;<br>
+<br>
+ if (width < it->x + it->width)<br>
+ width = it->x + it->width;<br>
+ if (height < it->y + it->height)<br>
+ height = it->y + it->height;<br>
+ }<br>
<br>
- xorg_list_append(&xwl_output->link, &xwl_screen->output_list);<br>
+ if (!has_this_output) {<br>
+ xorg_list_append(&xwl_output->link, &xwl_screen->output_list);<br></blockquote><div><br></div></div></div><div>I think this line should also be moved here:</div><div><div> xwl_screen->expecting_event--;</div></div><div><br></div><div>(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.)</div><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">- width = 0;<br>
- height = 0;<br>
- xorg_list_for_each_entry(xwl_output, &xwl_screen->output_list, link) {<br>
+ /* we did not check this output for new screen size, do it now */<br>
if (width < xwl_output->x + xwl_output->width)<br>
width = xwl_output->x + xwl_output->width;<br>
if (height < xwl_output->y + xwl_output->height)<br>
<span><font color="#888888">--<br>
2.1.0<br>
<br>
_______________________________________________<br>
wayland-devel mailing list<br>
<a href="mailto:wayland-devel@lists.freedesktop.org" target="_blank">wayland-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a><br>
</font></span></blockquote></span></div><br></div><div class="gmail_extra">Just a thought... You could instead:</div><div class="gmail_extra">- check if the output already exists</div><div class="gmail_extra">- add it if necessary</div><div class="gmail_extra">- update the width and height</div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div><div class="gmail_extra"><br></div></div>
</blockquote></div><br></div>