Patch that "fixes" compositor-x11

[Kristian Høgsberg]

On Tue, Jun 5, 2012 at 3:11 PM, Bill Spitzak <spitzak at gmail.com> wrote:
> Kristian Høgsberg wrote:
>
>> What do you mean by "produces a composite" and how do you know?  The
>> x11 backend renders every 10ms when it's rendering continuously, not
>> after every X event.  At least that's how it's supposed to work (and
>> how it works here), if that doesn't happen for you, it would be nice
>> if you could get some more details on what actually happens and why.
>
>
> If weston compositor-x11 is doing some heavy work (for instance when it is
> starting up, or if you launch about 10 applications) and then move the mouse
> quickly in circles, it will move the mouse cursor (and any other changes)
> much more slowly at a smooth even speed, falling way behind my hand
> movements. It can take tens of seconds after I stop moving the mouse for it
> to track through all the events.
>
> I placed a print statement in window.c in the run-loop function after it
> gets a block of events to print how many events were read. It never printed
> any number other than 1 or 0, confirming my suspicion of what was wrong.
>
> I then moved the print statement to compositor-x11 to where it calls
> xcb_poll functions, with the intention of finding out if the bug was in the
> x11 portion, or in wayland itself. I was surprised to discover the print
> statement *fixed* the problem. Now the window.c function gets 6-15 events at
> a time, and the cursor now keeps up with mouse movements, jumping over
> intermediate positions if weston gets slow.
>
> The clickdot motion trail is now also straight line segments, since it
> apparently does not record all the motion events it gets, but instead draws
> a straight line to the current position when redraw is called.
>
> I do not have an explanation as to why the print statement makes a
> difference. My best guess is that it produces a blocking i/o call, during
> which time the x server changes it's state in such a way that xcb_poll works
> a second time. But it is also very sensitive to the location of the print
> statment, as otherwise-identical code in window.c did not fix it.
>
>
>>> This patch also makes x11_compositor_handle_event return non-zero,
>>> which I believe was intended. But that does not fix the bug.
>>
>>
>> Is there a case where we return 0 but actually handled events in the
>> loop?  I don't see the code path where that may happen, if we handle
>> an event, 'event' will be non-NULL and we'll return 1.  The mainloop
>> only cares whether the return values is 0 or positive, the actual
>> number of events processed doesn't matter.
>
>
> The previous version of x11_compositor_handle_event always returned 0. This
> is because it did this (changing the out parameter to a return value):
>
>    while (event = getEvent()) { ... }
>    return event != 0;
>
> Since the while loop does not exit until getEvent() returns 0, p will always
> be 0 and this function always returns 0.
>
> My patch changes it to return how many times p was set to non-zero.
>
> It is also possible that x11_compositor_next_event is supposed to not write
> over the event parameter when it returns false, but that is not how it is
> written.

Argh, you're right, thanks.  But you said that just adding the count
and returning that alone didn't fix it, right?  I wonder if we need a

        xcb_flush(compositor->conn);

in x11_output_repaint() after the eglSwapBuffers() call.  Maybe just
before returning from
x11_compositor_handle_event()... not sure.  If you only handle one
event at the time in x11_compositor_handle_event(), it doesn't sound
like a missing flush issue.  Maybe try to force
x11_compositor_next_event() to always use xcb_poll_for_event(), that
is, don't use xcb_poll_for_queued_event()?

Kristian