Bug 161530 – [GTK] Stop using glReadPixels() to blit AC surfaces in the UIProcess under Wayland

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Emanuele Aina]

glReadPixels() is quite obviously suboptimal in the AC paint loop under Wayland[1], and we need a better mechanism to blit surfaces on the window.

One option would be to rely on gdk_cairo_draw_from_gl()[1], but unfortunately we can't trivially use that as it results in the image being y-flipped on screen. Apparently gdk_cairo_draw_from_gl() is already doing its own y-flipping[3], so in my current limited understanding of how GL and its GTK+ integration works the best option would be to flip rendering in the UIProcess and then pipe the resulting surfaces through gdk_cairo_draw_from_gl().

I'm not sure if the current rendering is wrong or it's a weird GL inconsistency that we would have to work around, but at least this approach would just work with current GTK+. :)

[1] https://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp?rev=205116#L74
[2] https://git.gnome.org/browse/gtk+/tree/gdk/gdkgl.c#n330
[3] https://git.gnome.org/browse/gtk+/tree/gdk/gdkgl.c#n455

[Emanuele Aina]

Flipping while rendering with the below patch and then piping through gdk_cairo_draw_from_gl() seem to work fine under Wayland as long as the display scaling factor is 1 (under X11 we should just not flip).

Weirdly when the device scaling factor is 2 (`weston --scale=2`) the page is split in two half, with the top half being drawn at the bottom and viceversa (they are otherwise correct).

+++ w/Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp
@@ -224,6 +224,8 @@ void ThreadedCompositor::renderLayerTree()
     FloatRect clipRect(0, 0, m_viewportSize.width(), m_viewportSize.height());
 
     TransformationMatrix viewportTransform;
+    viewportTransform.flipY();
+    viewportTransform.translate(0, -m_viewportSize.height());
     FloatPoint scrollPostion = m_viewportController->visibleContentsRect().location();
     viewportTransform.scale(m_viewportController->pageScaleFactor() * m_deviceScaleFactor);
     viewportTransform.translate(-scrollPostion.x(), -scrollPostion.y());

[Emanuele Aina]

Gustavo found the cause of the split image, and it's a bug in GTK+: https://bugzilla.gnome.org/show_bug.cgi?id=771060

Given that AC was not available under Wayland I would suggest to disable it if Wayland && HiDPI && GTK+ < 3.22 (assuming a fix will land) are detected rather than preserving the glReadPixels() codepath, I don't think it's worth carrying it for what I suspect is a case that nobody will ever encounter.

[Gustavo Noronha (kov)]

*** Bug 161798 has been marked as a duplicate of this bug. ***

[Gustavo Noronha (kov)]

Created attachment 288419 [details]
Patch

[Emanuele Aina]

Comment on attachment 288419 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288419&action=review

So in the end it was not a bug in GTK+ and this patch works with any GTK+ recent enough to have gdk_cairo_draw_from_gl() (ie. 3.16). \o/

There are some issues in the current glReadPixels() code (color format mismatches between Cairo/Pixman and GL, PACK_ROW_LENGTH not being available on GLES2 and needing workarounds) but most important glReadPixels() is inherently slow: I don't think it's worth the complexity it brings, and I have an hard time believing people building on GTK+ < 3.16 would care about having AC available under Wayland, so I suggest to simply drop the glReadPixels() codepath in a later commit if building with GTK+ < 3.16 and rely on non-AC instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1183
> +#if GTK_CHECK_VERSION(3, 16, 0) && PLATFORM(WAYLAND)

Should this be guarded by USE(COORDINATED_GRAPHICS_THREADED) too as WebPageProxy::setPaintsMirrored() is guarded by it?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:392
> +#if USE(COORDINATED_GRAPHICS_THREADED) || PLATFORM(GTK)

I'd be tempted to say that there's nothing specific to GTK here, I would just drop the PLATFORM(GTK) guard.

If you chose to drop it here, also drop it nearly everywhere else. :)

> Source/WebKit2/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:78
> +    gdk_cairo_draw_from_gl(cr, gtk_widget_get_window(m_webPage.viewWidget()), texture, GL_TEXTURE, m_webPage.deviceScaleFactor(), scaledClipRect.x(), scaledClipRect.y(), scaledClipRect.width(), scaledClipRect.height());

Do we prefer to drop the glReadPixels() in a later commit or just drop it here?

I think it's fair to disable AC under Wayland in this case: if building with an old GTK+ I'd say that perfomance on Wayland is not a big concern, and running without AC is the current status quo.

> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:341
> +#if PLATFORM(WAYLAND) && GTK_CHECK_VERSION(3, 16, 0)

Nothing Wayland-specific, I would drop PLATFORM(WAYLAND).

> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:342
> +    if (m_webPage.paintsMirrored())

I know it's probably overkill here, but for consistency I'd rather drop the USE(COORDINATED_GRAPHICS_THREADED) guards and use the same mechanism for both kind of LayerTreeHosts.

[Carlos Garcia Campos]

(In reply to comment #5)
> Comment on attachment 288419 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288419&action=review
> 
> So in the end it was not a bug in GTK+ and this patch works with any GTK+
> recent enough to have gdk_cairo_draw_from_gl() (ie. 3.16). \o/
> 
> There are some issues in the current glReadPixels() code (color format
> mismatches between Cairo/Pixman and GL, PACK_ROW_LENGTH not being available
> on GLES2 and needing workarounds) but most important glReadPixels() is
> inherently slow: I don't think it's worth the complexity it brings, and I
> have an hard time believing people building on GTK+ < 3.16 would care about
> having AC available under Wayland, so I suggest to simply drop the
> glReadPixels() codepath in a later commit if building with GTK+ < 3.16 and
> rely on non-AC instead.

I disagree, glReadPixels() is exactly what gdk_cairo_draw_from_gl does as fallback too when using the software path. I'm getting the same performance results here with wayland using glReadPixels(), than with X11 so I don't see why we have to disable AC if we have a working path with previous versions of GTK+. Users can always disable AC if they want.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1183
> > +#if GTK_CHECK_VERSION(3, 16, 0) && PLATFORM(WAYLAND)
> 
> Should this be guarded by USE(COORDINATED_GRAPHICS_THREADED) too as
> WebPageProxy::setPaintsMirrored() is guarded by it?
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:392
> > +#if USE(COORDINATED_GRAPHICS_THREADED) || PLATFORM(GTK)
> 
> I'd be tempted to say that there's nothing specific to GTK here, I would
> just drop the PLATFORM(GTK) guard.
> 
> If you chose to drop it here, also drop it nearly everywhere else. :)
> 
> > Source/WebKit2/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:78
> > +    gdk_cairo_draw_from_gl(cr, gtk_widget_get_window(m_webPage.viewWidget()), texture, GL_TEXTURE, m_webPage.deviceScaleFactor(), scaledClipRect.x(), scaledClipRect.y(), scaledClipRect.width(), scaledClipRect.height());
> 
> Do we prefer to drop the glReadPixels() in a later commit or just drop it
> here?
> 
> I think it's fair to disable AC under Wayland in this case: if building with
> an old GTK+ I'd say that perfomance on Wayland is not a big concern, and
> running without AC is the current status quo.

Again, if performance is not a big concern I don't see why it's a problem to use glReadPixels() in that case, instead of disabling a so important feature like AC that is required by more and more web sites nowadays. And again, it's not slow here, if that depends on the platform, users can disable AC on their platform both at compile time or at build time.

> > Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:341
> > +#if PLATFORM(WAYLAND) && GTK_CHECK_VERSION(3, 16, 0)
> 
> Nothing Wayland-specific, I would drop PLATFORM(WAYLAND).
> 
> > Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:342
> > +    if (m_webPage.paintsMirrored())
> 
> I know it's probably overkill here, but for consistency I'd rather drop the
> USE(COORDINATED_GRAPHICS_THREADED) guards and use the same mechanism for
> both kind of LayerTreeHosts.

Haven't looked at the patch yet, I'll review it tomorrow

[Gustavo Noronha (kov)]

Comment on attachment 288419 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288419&action=review

>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1183
>>> +#if GTK_CHECK_VERSION(3, 16, 0) && PLATFORM(WAYLAND)
>> 
>> Should this be guarded by USE(COORDINATED_GRAPHICS_THREADED) too as WebPageProxy::setPaintsMirrored() is guarded by it?
> 
> Again, if performance is not a big concern I don't see why it's a problem to use glReadPixels() in that case, instead of disabling a so important feature like AC that is required by more and more web sites nowadays. And again, it's not slow here, if that depends on the platform, users can disable AC on their platform both at compile time or at build time.

I think it makes sense to keep the glReadPixels implementation for earlier GTK+ versions. I think it actually lowers the number of codepaths we need to care about, overall, counter-intuitively.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:392
>> +#if USE(COORDINATED_GRAPHICS_THREADED) || PLATFORM(GTK)
> 
> I'd be tempted to say that there's nothing specific to GTK here, I would just drop the PLATFORM(GTK) guard.
> 
> If you chose to drop it here, also drop it nearly everywhere else. :)

Reason why I went with the || PLATFORM(GTK) is we want to set the flag even if the threaded compositor is disabled, for the GTK+ port, in which case we will use this boolean in LayerTreeHostGtk. An option we have, of course, is to ditch the non-threadedcompositor codepaths for GTK.

>>> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:341
>>> +#if PLATFORM(WAYLAND) && GTK_CHECK_VERSION(3, 16, 0)
>> 
>> Nothing Wayland-specific, I would drop PLATFORM(WAYLAND).
> 
> Haven't looked at the patch yet, I'll review it tomorrow

THanks =)

>> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:342
>> +    if (m_webPage.paintsMirrored())
> 
> I know it's probably overkill here, but for consistency I'd rather drop the USE(COORDINATED_GRAPHICS_THREADED) guards and use the same mechanism for both kind of LayerTreeHosts.

Erm… I probably made a mistake when creating this last patch because using the same mechanism here was exactly the reason why I added the || PLATFORM(GTK) elsewhere =) I'll upload a fixed patch.

[Emanuele Aina]

My concerns about the glReadPixels() codepath are that:

* it's a non-trivial piece of code with several compile-time and runtime branches, some of which hardware-dependent
* it currently has some nasty issues with color format mismatches between Cairo/Pixman and GLES (current GL_RGBA usage is not correct, CAIRO_FORMAT_ARGB32 aka PIXMAN_a8r8g8b8 for GLES should be GL_BGRA_EXT/GL_UNSIGNED_BYTE for little-endian, which requires the EXT_read_format_bgra extension while nothing matches for big-endian)
* nobody will likely ever test it as I suppose that all the developers testing under Wayland use recent versions of GTK+ (for several reasons, including the fact that the xdg_shell protocol had several breaking changes and old GTK+ releases won't work on current compositors)

I just said that glReadPixels() is slow because it has been a big issue on any low-powered ARM board I worked on: I agree that on my laptop performance is good even with it, as the CPU is probably fast enough and the GPU→CPU memory bandwidth here copes well enough with the extra traffic.

My suggestions was just to avoid increasing further the testing matrix with several hard-to-setup conditions: I personally see it as dead code, but if you want to keep it around that's more than welcome for me. :)

[Emanuele Aina]

Erhm, I need to correct myself: the current usage of GL_RGBA is correct since we do a tight loop right after glReadPixels() to manually swap bytes one by one.

I'm still really dubious about the usefulness of such a CPU/memory bandwidth heavy approach in a codepath targeted at GLES2-only hardware, which I guess it's safe to assume does not have desktop-grade CPU/GPU performance.

[Gustavo Noronha (kov)]

Created attachment 288720 [details]
Patch

[Build Bot]

Comment on attachment 288720 [details]
Patch

Attachment 288720 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2067693

New failing tests:
fast/scrolling/ios/scroll-events-back-forward.html

[Build Bot]

Created attachment 288731 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Carlos Garcia Campos]

Comment on attachment 288720 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288720&action=review

I don't think we need to make the decision on the UI process, keep it there and send it to the web process. The conditions are always the same and are not going to change at runtime, so this can be a lot simpler by making the decision in the web process once.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:125
> +void ThreadedCompositor::setPaintsMirrored(bool paintsMirrored)
> +{
> +    m_compositingRunLoop->performTask([this, protectedThis = Ref<ThreadedCompositor>(*this), paintsMirrored] {
> +        m_paintsMirrored = paintsMirrored;
> +        scheduleDisplayImmediately();
> +    });
> +}

I think we should pass this to the ThreadedCompositor constructor, and not allow to change it. It simplifies everything. Also, even though PaintMirrored is the only TextureMapper paint flag, we can make it more generic and use TextureMapper::PaintFlags directly instead a of a boolean.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:208
> -    m_scene->paintToCurrentGLContext(viewportTransform, 1, clipRect, Color::transparent, !m_drawsBackground, m_scrollPosition);
> +    TextureMapper::PaintFlags paintFlags = m_paintsMirrored ? TextureMapper::PaintingMirrored : 0;
> +    m_scene->paintToCurrentGLContext(viewportTransform, 1, clipRect, Color::transparent, !m_drawsBackground, m_scrollPosition, paintFlags);

And here we would use m_paintFlags directly.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1186
> +#if GTK_CHECK_VERSION(3, 16, 0) && PLATFORM(WAYLAND)
> +    if (PlatformDisplay::sharedDisplayForCompositing().type() == PlatformDisplay::Type::Wayland)
> +        priv->pageProxy->setPaintsMirrored(true);
> +#endif

This doesn't work. The shared display for compositing is set in the web process only, here it will always be the shared display. If we eventually switch to use the wayland compositor in X11, this will return the X11 shared display, not the wayland one. Also, I think we could always paint mirrored in Wayland, and in case of using older versions of GTK+, simply do a transformation when rendering in the widget cairo context. That way we reduce the conditions.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:66
>          m_layerTreeContext.contextID = m_surface->surfaceID();
>      } else
>          m_compositor = ThreadedCompositor::create(m_compositorClient);
> +
> +    m_compositor->setPaintsMirrored(webPage.paintsMirrored());

And here we could make the decision to pass the paint flags to the threaded compositor constructor. To avoid ifdefs here, we could even let the surface decide, adding AcceleratedSurface::paintFlags().

[Carlos Garcia Campos]

I've tried this and I've noticed only a couple of things that don't work. 

- The gray background we use for the auth dialog is not rendered. I don't know why, because background colors works, even with transparencies.

- When the inspector is attached, the inspected web view is not correctly rendered, it's like if tiles were not positioned in the right order or something like that.

Both things work perfectly when using glReadPixels. Apart from that everything seems to work great.

[Carlos Garcia Campos]

(In reply to comment #1)
> Flipping while rendering with the below patch and then piping through
> gdk_cairo_draw_from_gl() seem to work fine under Wayland as long as the
> display scaling factor is 1 (under X11 we should just not flip).
> 
> Weirdly when the device scaling factor is 2 (`weston --scale=2`) the page is
> split in two half, with the top half being drawn at the bottom and viceversa
> (they are otherwise correct).

Wait, this is what happens when the inspector is attached when using gdk_cairo_draw_from_gl(), even with 1 device scale factor.

[Gustavo Noronha (kov)]

(In reply to comment #13)
> Comment on attachment 288720 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288720&action=review
> 
> I don't think we need to make the decision on the UI process, keep it there
> and send it to the web process. The conditions are always the same and are
> not going to change at runtime, so this can be a lot simpler by making the
> decision in the web process once.

One of the reasons why I made the decision happen in the UI process was we have a branch of WebKit2GTK+ where we wrap the WebView in a ClutterActor, and WebViewBase is aware of it. In that case we do not want to flip the painting, but we still want the GTK+ widget to keep working. We can deal with it on our side, I suppose, it's not a huge diff. Do you know if webkit for wayland or other threaded-compositor-using ports could benefit from deciding on the UI process?

> I think we should pass this to the ThreadedCompositor constructor, and not
> allow to change it. It simplifies everything. Also, even though
> PaintMirrored is the only TextureMapper paint flag, we can make it more
> generic and use TextureMapper::PaintFlags directly instead a of a boolean.

Can do. The reason why it is not being used in the constructor and is being allowed to change is we only know the WebView is being wrapper by the clutter widget after its creation (so after the threaded compositor is created).
 
> This doesn't work. The shared display for compositing is set in the web
> process only, here it will always be the shared display. If we eventually
> switch to use the wayland compositor in X11, this will return the X11 shared
> display, not the wayland one. Also, I think we could always paint mirrored
> in Wayland, and in case of using older versions of GTK+, simply do a
> transformation when rendering in the widget cairo context. That way we
> reduce the conditions.

I see. I'll give that a try.

(In reply to comment #15)
> (In reply to comment #1)
> > Flipping while rendering with the below patch and then piping through
> > gdk_cairo_draw_from_gl() seem to work fine under Wayland as long as the
> > display scaling factor is 1 (under X11 we should just not flip).
> > 
> > Weirdly when the device scaling factor is 2 (`weston --scale=2`) the page is
> > split in two half, with the top half being drawn at the bottom and viceversa
> > (they are otherwise correct).
> 
> Wait, this is what happens when the inspector is attached when using
> gdk_cairo_draw_from_gl(), even with 1 device scale factor.

Whoa. I'll investigate. Thanks!

[Carlos Garcia Campos]

(In reply to comment #16)
> (In reply to comment #13)
> > Comment on attachment 288720 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=288720&action=review
> > 
> > I don't think we need to make the decision on the UI process, keep it there
> > and send it to the web process. The conditions are always the same and are
> > not going to change at runtime, so this can be a lot simpler by making the
> > decision in the web process once.
> 
> One of the reasons why I made the decision happen in the UI process was we
> have a branch of WebKit2GTK+ where we wrap the WebView in a ClutterActor,
> and WebViewBase is aware of it. In that case we do not want to flip the
> painting, but we still want the GTK+ widget to keep working. We can deal
> with it on our side, I suppose, it's not a huge diff. Do you know if webkit
> for wayland or other threaded-compositor-using ports could benefit from
> deciding on the UI process?

I don't think so, this is only needed because of GTK+, so other ports shouldn't need to render mirrored at all. So, I prefer to keep tings simple upstream.

> > I think we should pass this to the ThreadedCompositor constructor, and not
> > allow to change it. It simplifies everything. Also, even though
> > PaintMirrored is the only TextureMapper paint flag, we can make it more
> > generic and use TextureMapper::PaintFlags directly instead a of a boolean.
> 
> Can do. The reason why it is not being used in the constructor and is being
> allowed to change is we only know the WebView is being wrapper by the
> clutter widget after its creation (so after the threaded compositor is
> created).

You can do both, like we do for the native surface handle. But then we would have an unused method upstream, so I prefer if we keeo the constructor parameter only in upstream.
  
> > This doesn't work. The shared display for compositing is set in the web
> > process only, here it will always be the shared display. If we eventually
> > switch to use the wayland compositor in X11, this will return the X11 shared
> > display, not the wayland one. Also, I think we could always paint mirrored
> > in Wayland, and in case of using older versions of GTK+, simply do a
> > transformation when rendering in the widget cairo context. That way we
> > reduce the conditions.
> 
> I see. I'll give that a try.
> 
> (In reply to comment #15)
> > (In reply to comment #1)
> > > Flipping while rendering with the below patch and then piping through
> > > gdk_cairo_draw_from_gl() seem to work fine under Wayland as long as the
> > > display scaling factor is 1 (under X11 we should just not flip).
> > > 
> > > Weirdly when the device scaling factor is 2 (`weston --scale=2`) the page is
> > > split in two half, with the top half being drawn at the bottom and viceversa
> > > (they are otherwise correct).
> > 
> > Wait, this is what happens when the inspector is attached when using
> > gdk_cairo_draw_from_gl(), even with 1 device scale factor.
> 
> Whoa. I'll investigate. Thanks!

[Gustavo Noronha (kov)]

Created attachment 288842 [details]
Patch

[Gustavo Noronha (kov)]

I could not reproduce the inspector issue with this new patch. I am building the old one to see if I can with it, can you try this one out? What version of GTK+ do you have?

[Carlos Garcia Campos]

(In reply to comment #19)
> I could not reproduce the inspector issue with this new patch. I am building
> the old one to see if I can with it, can you try this one out? What version
> of GTK+ do you have?

I've tried it and got same results. I don't know what GTK+ version I was using, 3.21.something, but I've just updated to git master, and also weston and wayland and I'm getting the same. I'm also seeing artifacts when resizing the window, that doesn't happen when using glRedPixels. This is an intel gpu with mesa 12.0.2 (from debian testing) FWIW. I'll submit screenshots.

[Carlos Garcia Campos]

Created attachment 288934 [details]
Screenshot of the inspector issue

It's not this way all the time, it happens when you hover elements in the inspector, and then after a moment it's rendered correctly, but then incorrectly again.

[Carlos Garcia Campos]

Created attachment 288935 [details]
Screenshot of the resizing artifacts

This is when making the window smaller, there are artifacts while rendering inside the web view, but also that black rectangles outside the window. I thought this could be a weston or wayland issue, but it only happens when using gdk_cairo_draw_from_gl

[Carlos Garcia Campos]

Comment on attachment 288842 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288842&action=review

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:48
> -Ref<ThreadedCompositor> ThreadedCompositor::create(Client& client, uint64_t nativeSurfaceHandle, ShouldDoFrameSync doFrameSync)
> +Ref<ThreadedCompositor> ThreadedCompositor::create(Client& client, uint64_t nativeSurfaceHandle, ShouldDoFrameSync doFrameSync, TextureMapper::PaintFlags paintFlags)
>  {
> -    return adoptRef(*new ThreadedCompositor(client, nativeSurfaceHandle, doFrameSync));
> +    return adoptRef(*new ThreadedCompositor(client, nativeSurfaceHandle, doFrameSync, paintFlags));
>  }

Maybe we could merge ShouldDoFrameSync and paintFlags into a single enum flags ThreadedCompositor::Options that we can grow in the future if needed without adding more construction parameters. We could even add draws background too. But this could be done ins a separate patch, in any case.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:64
> +#if GTK_CHECK_VERSION(3, 16, 0)

As I suggested before, I think we can simplify the conditions if we always flip Y when rendering to the nested wayland compositor. When using previous versions of GTK+ we only need to apply a transform in the cairo context when rendering the image, and it shouldn't affect the performance that much, I think.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:65
> +    if (PlatformDisplay::sharedDisplay().type() == PlatformDisplay::Type::Wayland)

In this case it's he sharedDisplayForCompositing what we want to check, since we are in the web process.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:77
> -        m_compositor = ThreadedCompositor::create(m_compositorClient);
> +        m_compositor = ThreadedCompositor::create(m_compositorClient, paintFlags);

We only want to flip Y when rendering to the nested wayland compositor, never in this case. This path should only happen on X11 when building without the redirected window. But it's wrong in any case, because the second parameter is nto the paint flags, but the ShouldDoFrameSync.

> Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:346
> +#if GTK_CHECK_VERSION(3, 16, 0) && PLATFORM(WAYLAND)
> +    if (PlatformDisplay::sharedDisplay().type() == PlatformDisplay::Type::Wayland)
> +        paintFlags |= TextureMapper::PaintingMirrored;
> +#endif

Same comments here. I still think the code would be cleaner if the decision is made by the surface, in which case we don't need to check the display again, AcceleratedSurfaceX11 returns false and AcceleratedSurfaceWayland true, and we don't need ifdefs either.

[Gustavo Noronha (kov)]

(In reply to comment #21)
> Created attachment 288934 [details]
> Screenshot of the inspector issue
> 
> It's not this way all the time, it happens when you hover elements in the
> inspector, and then after a moment it's rendered correctly, but then
> incorrectly again.

This is really weird. Could be a bug in mesa or in the intel driver I suppose… can you try with a recent GTK+ with and without this patch applied, maybe? https://bug771060.bugzilla-attachments.gnome.org/attachment.cgi?id=335188

[Gustavo Noronha (kov)]

Don't bother, I managed to reproduce, will investigate! \o/

[Gustavo Noronha (kov)]

Is something like this what you're looking after?

ThreadedCoordinatedLayerTreeHost::ThreadedCoordinatedLayerTreeHost(WebPage& webPage)
    : CoordinatedLayerTreeHost(webPage)
    , m_compositorClient(*this)
    , m_surface(AcceleratedSurface::create(webPage))
{
    if (m_surface) {
        // Do not do frame sync when rendering offscreen in the web process to ensure that SwapBuffers never blocks.
        // Rendering to the actual screen will happen later anyway since the UI process schedules a redraw for every update,
        // the compositor will take care of syncing to vblank.
        TextureMapper::PaintFlags paintFlags = 0;

        if (m_surface.paintsMirrored())
            paintFlags |= TextureMapper::PaintingMirrored;

        m_compositor = ThreadedCompositor::create(m_compositorClient, m_surface->window(), ThreadedCompositor::ShouldDoFrameSync::No, paintFlags);
        m_layerTreeContext.contextID = m_surface->surfaceID();
    } else
        m_compositor = ThreadedCompositor::create(m_compositorClient);
}

[Carlos Garcia Campos]

(In reply to comment #26)
> Is something like this what you're looking after?

Yes!

> ThreadedCoordinatedLayerTreeHost::ThreadedCoordinatedLayerTreeHost(WebPage&
> webPage)
>     : CoordinatedLayerTreeHost(webPage)
>     , m_compositorClient(*this)
>     , m_surface(AcceleratedSurface::create(webPage))
> {
>     if (m_surface) {
>         // Do not do frame sync when rendering offscreen in the web process
> to ensure that SwapBuffers never blocks.
>         // Rendering to the actual screen will happen later anyway since the
> UI process schedules a redraw for every update,
>         // the compositor will take care of syncing to vblank.
>         TextureMapper::PaintFlags paintFlags = 0;
> 
>         if (m_surface.paintsMirrored())

It's not the surface the one who paints mirrored, so I would call this shouldPaintMirrored(), for example.

>             paintFlags |= TextureMapper::PaintingMirrored;

And I would move the sync frame comment here, right before the flag is used.

>         m_compositor = ThreadedCompositor::create(m_compositorClient,
> m_surface->window(), ThreadedCompositor::ShouldDoFrameSync::No, paintFlags);
>         m_layerTreeContext.contextID = m_surface->surfaceID();
>     } else
>         m_compositor = ThreadedCompositor::create(m_compositorClient);
> }

Thanks!

[Gustavo Noronha (kov)]

(In reply to comment #27)
> It's not the surface the one who paints mirrored, so I would call this
> shouldPaintMirrored(), for example.

Makes sense!
 
> >             paintFlags |= TextureMapper::PaintingMirrored;
> 
> And I would move the sync frame comment here, right before the flag is used.

Heh, noticed that and moved right after I posted the comment =).

By the way, I noticed while poking this code that we seem to be creating a cairo surface double the size we need in the glReadPixels case, since the texture is already created scaled to the device scale factor, and we scale that again for creating the surface. I'll file a separate bug about that.

Back to the issue we were seeing: apparently the problem we're hitting is caused by using the clippedRect, which come to think of it makes no sense since it refers to the cairo context. If we just use the texture coordinates it behaves like the  glReadPixels version. Will upload a patch in a bit.

[Gustavo Noronha (kov)]

Created attachment 288978 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #28)
> (In reply to comment #27)
> > It's not the surface the one who paints mirrored, so I would call this
> > shouldPaintMirrored(), for example.
> 
> Makes sense!
>  
> > >             paintFlags |= TextureMapper::PaintingMirrored;
> > 
> > And I would move the sync frame comment here, right before the flag is used.
> 
> Heh, noticed that and moved right after I posted the comment =).
> 
> By the way, I noticed while poking this code that we seem to be creating a
> cairo surface double the size we need in the glReadPixels case, since the
> texture is already created scaled to the device scale factor, and we scale
> that again for creating the surface. I'll file a separate bug about that.

You are right! Did you file the bug in end?

> Back to the issue we were seeing: apparently the problem we're hitting is
> caused by using the clippedRect, which come to think of it makes no sense
> since it refers to the cairo context. If we just use the texture coordinates
> it behaves like the  glReadPixels version. Will upload a patch in a bit.

Yes, that fixed the inspector issues, but I still the weird "shadow" when shrinking the window. I've only tried in weston.

[Gustavo Noronha (kov)]

(In reply to comment #30)
> > By the way, I noticed while poking this code that we seem to be creating a
> > cairo surface double the size we need in the glReadPixels case, since the
> > texture is already created scaled to the device scale factor, and we scale
> > that again for creating the surface. I'll file a separate bug about that.
> 
> You are right! Did you file the bug in end?

I did! https://bugs.webkit.org/show_bug.cgi?id=162025
 
> > Back to the issue we were seeing: apparently the problem we're hitting is
> > caused by using the clippedRect, which come to think of it makes no sense
> > since it refers to the cairo context. If we just use the texture coordinates
> > it behaves like the  glReadPixels version. Will upload a patch in a bit.
> 
> Yes, that fixed the inspector issues, but I still the weird "shadow" when
> shrinking the window. I've only tried in weston.

Yeah. I believe that fixes the issues we had with the inspector and when enlarging the window. The shrinking bit, that seems like a bug in GTK+ that is being exposed, since the window frame is also being rendered improperly. I'm investigating, let's see what I find.

[Carlos Garcia Campos]

(In reply to comment #14)
> I've tried this and I've noticed only a couple of things that don't work. 
> 
> - The gray background we use for the auth dialog is not rendered. I don't
> know why, because background colors works, even with transparencies.
> 

I've been investigating this and I haven't been ale to find the reason. However, I've noticed that whatever I try to render on the cairo context after gdk_cairo_draw_from_gl is not rendered (or least not on top), but container propagate draws that happen for the same cairo context work. That's why we don't see the shadow, but we see the actual dialog (rendered after gdk_cairo_draw_from_gl() on the same cairo context). So, I have found a workaround, I've moved the shadow to the auth dialog widget itself. Instead of allocating the size specific for the dialog, we give the whole view to the auth dialog, that render the shadow and centers its child. I'll file a new bug report and attach a patch.

[Gustavo Noronha (kov)]

I think I found the issue with the black border when shrinking, reported it to GTK+, will poke Alex about whether my patch is good enough or if it should be fixed in a different fashion, but I think we can probably go ahead with this change on our front.

https://bugzilla.gnome.org/show_bug.cgi?id=771553

[Carlos Garcia Campos]

Comment on attachment 288978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288978&action=review

> Source/WebKit2/ChangeLog:30
> [phone]  Gustavo Noronha Silva  <[e-mail]>
> +
> +        [GTK] Stop using glReadPixels() to blit AC surfaces in the UIProcess under Wayland
> +        https://bugs.webkit.org/show_bug.cgi?id=161530
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Use gdk_cairo_draw_from_gl when all necessary conditions exist.

Double ChangeLog

> Source/WebKit2/WebProcess/WebPage/gtk/AcceleratedSurface.h:44
> +    virtual bool shouldPaintMirrored() const { return false; }

This is supposed to be pure virtual, but it's not because we allow to build X11 only with no redirected window, in which case this won't be used, but an implementation won't be compiled either. So, I prefer if you add and assert here and give an implementation to X11, like we do for window() and surfaceID() methods.

[Gustavo Noronha (kov)]

Committed r206080: <http://trac.webkit.org/changeset/206080>