1280653 - [glx] Part or all of popup window under search box is rendered in black

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

STR:
  1. Run Nightly on Linux.
     I'm running on Debian Stable with KDE.
  2. Set the pref "layers.acceleration.force-enabled" to true.
  3. Type characters into the search box and observe the popup
     window after each character. Also, erase characters and
     observe the popup window after each character.

After typing or erasing a character, I sometimes see part or all of the popup window under the search box to be rendered in black rather than being rendered with content. This happens particularly when the size of the popup window changes.

Please see the attached screenshots for examples.

Disabling layers acceleration makes the problem go away.

Comment 1

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #0)
> Disabling layers acceleration makes the problem go away.

We also observed today that using DRI instead of llvmpipe makes the problem go away.

Comment 2

[Andrew Comminos [:acomminos]]

So I believe I've found the cause here;

If we trigger a composite with the new widget dimensions via an invalidation in nsWindow::OnSizeAllocate before the X server dispatches the ConfigureNotify event, DRI will invalidate the drawable in DRI2ConfigNotify after we've drawn to it.

We should thus composite to the window after ConfigureNotify, possibly in nsWindow::OnConfigureEvent, to ensure that Mesa handles the invalidated buffer appropriately.

Comment 3

[Andrew Comminos [:acomminos]]

The question here is really whether or not "size-allocate" should be called after "configure-event". Looking into this.

Comment 4

[Andrew Comminos [:acomminos]]

Attachment: Bug 1280653 - Read into the X11 event queue in GLContextGLX::MakeCurrent to queue DRI invalidation events.

Review commit: https://reviewboard.mozilla.org/r/72096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72096/

Comment 5

[Botond Ballo [:botond]]

I can confirm that this patch fixes the issue where popup window after a resize is rendered black *and stays black* until something triggers another composite of it.

I do still see it briefly flash to black before being rendered properly. Andrew tells me that would be tricker to fix, so I'll file a follow-up for that.

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)
> I do still see it briefly flash to black before being rendered properly.
> Andrew tells me that would be tricker to fix, so I'll file a follow-up for
> that.

Filed bug 1296054.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8781749 [details]
Bug 1280653 - Read into the X11 event queue in GLContextGLX::MakeCurrent to queue DRI invalidation events.

https://reviewboard.mozilla.org/r/72096/#review70104

It feels to me that this is not on a path to full solution, and I don't
actually understand why it makes a difference.

I think the issues here are mainly graphics architecture issues rather than
GTK issues and so I think Lee would be better placed than I to offer
suggestions.

On IRC we discussed that there were two problems:

1) With DRI2, drawing after resize occurs on the wrong buffer until the GL
   implementation receives the DRI2_InvalidateBuffers event to notify of a
   buffer change. [1]

2) The resize and drawing can get out of order, either because separate
   display connections are used on resize and composite threads or because X
   is used for resize and GL for composite.

To address issue 2 some kind of barrier is required.

I thought the plan was to listen for ConfigureNotify on the compositor
connection, expecting that to provide notification of resize for issue 2 and
was assuming that would happen after DRI2_InvalidateBuffers.  However, it
looks like ConfigureNotify may be sent before DRI2_InvalidateBuffers [2].

The approach of this patch is listening for ConfigureNotify on the resize
thread to invalidate.  That eventually triggers a draw signal which will send
a message to the compositor thread to composite again.  The delays involved
likely mean that the compositor thread has received DRI2_InvalidateBuffers by
that point, but there is nothing to guarantee this AFAIK.

Something that I still don't understand is that merely making a window larger
will generate expose events that should make invalidation on ConfigureNotify
unnecessary.  I assume expose events arrive after ConfigureNotify (because
that is the only way it would work), and so why does invalidating on
ConfigureNotify help?  Is the black only when the window gets smaller in every
dimension?

Similarly I wonder why DRI2InvalidateDrawable would not be generating an
expose event to cover over this problem.

To further influence things here, AIUI the set_sync_counter() in
gdk_x11_window_end_frame() sends a message to the window manager/compositor
that the painting is finished.  This may be what is causing the black flash if
the composite during the draw event wasn't successful.  After being told that
drawing has completed, the WM compositor composites what it has, which is
nothing.

Is the composite during draw synchronous wrt returning from the draw event
handler?

I wonder whether the compositor thread should glXWaitX() or XSync() when
responding to draw events, so that it completes a good composite before
set_sync_counter is called.  Another kind of barrier may be possible, but more
complicated.  The main thread may need to XSync() also to ensure that its
requests have reached the server.

That could address issue 2.  If issue 1 only exists on resize, then
perhaps that is addressed also.

Comment 8

[Karl Tomlinson (:karlt)]

Lee, would you be able to comment on a solution here?

Comment 9

[Lee Salzman [:lsalzman]]

(In reply to Karl Tomlinson (:karlt) from comment #8)
> Lee, would you be able to comment on a solution here?

Without further digging deeper into the code I couldn't offer a solution offhand. I think your analysis was fairly thorough and points out the issues.

If Andrew's patch works as a stop-gap, we'd want to use it I think so long as, as you pointed out, we can figure out why it works. If it is simply a matter of lucky timing/delays incurred by invalidation, then this might argue against it... except if a real solution to this problem is not easy/forthcoming and the quick hack is fairly reliable despite the uncertainty of it (weighted against the annoyance of the bug itself).

Comment 10

[Andrew Comminos [:acomminos]]

Thanks for the analysis, Karl; I agree it's worth looking into some more before anything lands.

This approach works for a less convincing reason than I initially suspected; it works because we trigger a second invalidate after our "too early" (before mesa knows about DRI buffer invalidation) composite completes. After the failed early composite, Mesa has received the DRI2_InvalidateBuffers event, and draws into the new buffer on the next swap. If we alter OnExposeEvent to trigger a new (second) composite, things work similarly.

> I thought the plan was to listen for ConfigureNotify on the compositor
> connection, expecting that to provide notification of resize for issue 2 and
> was assuming that would happen after DRI2_InvalidateBuffers.  However, it
> looks like ConfigureNotify may be sent before DRI2_InvalidateBuffers [2].

For some reason I thought we dropped this solution; my mistake. This is pretty sane, but we'd just ensure that we poll the X11 fd on the compositor thread.

> Something that I still don't understand is that merely making a window larger
> will generate expose events that should make invalidation on ConfigureNotify
> unnecessary.  I assume expose events arrive after ConfigureNotify (because
> that is the only way it would work), and so why does invalidating on
> ConfigureNotify help?  Is the black only when the window gets smaller in every
> dimension?

It's on both. Again, the patch works only because of the second composite.

> To further influence things here, AIUI the set_sync_counter() in
> gdk_x11_window_end_frame() sends a message to the window manager/compositor
> that the painting is finished.  This may be what is causing the black flash if
> the composite during the draw event wasn't successful.  After being told that
> drawing has completed, the WM compositor composites what it has, which is
> nothing.

This issue occurs even without an X11 compositor (using metacity --no-composite), so luckily this issue (while legitimate) appears orthogonal. Blocking on the compositor in the draw event handler would solve this.

> Is the composite during draw synchronous wrt returning from the draw event
> handler?

No. We could make it, though- I'm not too keen on blocking the main thread for a composite (if we have a non compositing window manager, we could end up blocking the main thread a lot). We could potentially limit sync exposes to when the window size changes.

Comment 11

[Andrew Comminos [:acomminos]]

Just to clarify, we do receive expose events *before* ConfigureNotify for popups;

308963136[7f9b1129e4a0]: nsWindow::NativeResize [7f9ad1e7d000] 189 186
308963136[7f9b1129e4a0]: size_allocate [7f9ad1e7d000] 0 0 189 186
308963136[7f9b1129e4a0]:        119641360 189 186 -785169664
308963136[7f9b1129e4a0]: sending expose event [7f9ad1e7d000] 7f9ad66f8340 0x1a0003e (rects follow):
308963136[7f9b1129e4a0]: configure event [7f9ad1e7d000] 2530 605 189 186

Comment 12

[Andrew Comminos [:acomminos]]

Some testing has revealed that the compositor display receives DRI2_InvalidateBuffers by the time CompositorOGL::BeginFrame is called- however, Mesa's DRI2WireToEvent function to handle the invalidation event doesn't get called until the event is read into the display's event queue, which doesn't typically happen until XSync in glXSwapBuffers. This is too late, as drawing commands have already been dispatched to the invalid buffer.

I propose then that we read into the X event queue using a call to XPending in GLContextGLX::MakeCurrent(force=true), which is called whenever the widget size changes (or the GL context itself).

Comment 13

[Andrew Comminos [:acomminos]]

Comment on attachment 8781749 [details]
Bug 1280653 - Read into the X11 event queue in GLContextGLX::MakeCurrent to queue DRI invalidation events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72096/diff/1-2/

Comment 14

[Lee Salzman [:lsalzman]]

Comment on attachment 8781749 [details]
Bug 1280653 - Read into the X11 event queue in GLContextGLX::MakeCurrent to queue DRI invalidation events.

https://reviewboard.mozilla.org/r/72096/#review71874

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d78a3a4fdaf9
Read into the X11 event queue in GLContextGLX::MakeCurrent to queue DRI invalidation events. r=lsalzman

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d78a3a4fdaf9

Comment 17

[Botond Ballo [:botond]]

We've had a user report a very similar problem in Firefox 50 (bug 1306974). Andrew, do you think it's plausible that it's the same issue? If so, how would you feel about uplifting this patch to 50?

Comment 18

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #17)
> We've had a user report a very similar problem in Firefox 50 (bug 1306974).
> Andrew, do you think it's plausible that it's the same issue? If so, how
> would you feel about uplifting this patch to 50?

Never mind - it's looking like bug 1306974 is a different issue.