Closed Bug 1184534 Opened 6 years ago Closed 5 years ago

GL compositor only clears known render bounds, causing artifacts around content while resizing on Linux


(Core :: Graphics: Layers, defect)

Not set



Tracking Status
firefox42 --- fixed


(Reporter: acomminos, Assigned: acomminos)


(Blocks 1 open bug)



(2 files, 4 obsolete files)

Currently we restrict the clear region using GL_SCISSOR_TEST to the render bounds as last reported by the widget implementation. I suspect that on X11, the window's size change may be reported to the compositor too late, resulting in GLX displaying untouched data. Disabling scissor test when clearing fixes the issue.

We can't just remove the scissor test when clearing due to the regression mentioned in bug 1065256.
Assignee: nobody → acomminos
See Also: → 963549
To fix this correctly, I don't think it's sufficient to pull the GLX drawable's size in the compositor thread (as the window size can change at any time during composition). I suspect that a solution using something like _NET_WM_SYNC_REQUEST may be better. Ideally we could stop the window manager from resizing the window frame until our compositor has prepared a frame with the correct size.
We could also hook up GLX events to the OGL compositor to listen for buffer clobbers while rendering. If our back buffer's size changed while compositing, we could discard the frame or redraw.
This adds support to GLContext for fetching the native surface size, and a GLX implementation. This helps the compositor check for back buffer clobbers.
Attachment #8636225 - Flags: review?(jgilbert)
This makes CompositorOGL pull the viewport size from the GL surface, when available. Frames are ignored if the surface size changes between BeginFrame and EndFrame, which is okay since widgets' expose handlers send a request to composite after being resized anyway.
Attachment #8636226 - Flags: review?(nical.bugzilla)
See Also: → 1177091
Attachment #8636226 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8636225 [details] [diff] [review]
Add support for target size retrieval to GLX backend.

Review of attachment 8636225 [details] [diff] [review]:

Minimize the number of configurations which receive workarounds.

::: gfx/gl/GLContext.h
@@ +3229,5 @@
> +    /*
> +     * Retrieves the size of the native windowing system drawable.
> +     */
> +    virtual Maybe<gfx::IntSize> GetTargetSize() {

I would rather see `bool GetTargetSize(gfx::IntSize* const out)`, but this does work too.

::: gfx/gl/GLContextProviderGLX.cpp
@@ +813,5 @@
>      if (context) {
> +        GLXDrawable target;
> +        if (!isOffscreen && sGLXLibrary.GLXVersionCheck(1, 3)) {
> +            // Wrap X11 window around GLXWindow to workaround mesa bug 54080.

Only do this on Mesa.
Attachment #8636225 - Flags: review?(jgilbert) → review-
To be clear, Maybe<> is fine. It just seems less explicit to me. (Rather than "get me the target size, or fail I'll do something else)
Thanks. Here's a simpler implementation, we should be able to get the necessary information via XGetGeometry for all effective drawable types.
Attachment #8636225 - Attachment is obsolete: true
Attachment #8638758 - Flags: review?(jgilbert)
Attachment #8638758 - Flags: review?(jgilbert) → review+
Updated to only set the scissor rect if the size changed in BeginFrame.
Attachment #8636226 - Attachment is obsolete: true
Attachment #8640577 - Flags: review+
Keywords: checkin-needed
Viewport patch needs rebasing on top of inbound tip.
Keywords: checkin-needed
Keywords: checkin-needed
the 2nd patch failed to apply:

patching file gfx/layers/opengl/CompositorOGL.cpp
Hunk #2 FAILED at 1389
1 out of 2 hunks FAILED -- saving rejects to file gfx/layers/opengl/CompositorOGL.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug-1184534---Fetch-viewport-size-from-context-in-.patch
Tomcats-MacBook-Pro:mozilla-inbound Tomcat$ hg outgoing

could you take a look ?
Flags: needinfo?(acomminos)
Keywords: checkin-needed
Flags: needinfo?(acomminos)
Keywords: checkin-needed
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Duplicate of this bug: 1177091
You need to log in before you can comment on or make changes to this bug.