Bug 197944 – [GTK] Use WPEBackend-fdo for accelerating compositing in Wayland instead of the nested compositor

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

The WPEBackend-fdo implementation is quite similar to our Wayland nested compositor, but more efficient (using linux dmabuf, resource caching, etc.). This allows us to share even more code with WPE port too.

[Carlos Garcia Campos]

Created attachment 370036 [details]
Patch

[EWS Watchlist]

Attachment 370036 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 370036 [details]
Patch

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

Cool.

No complaints about the code itself, but I think we need to discuss the preprocessor guards used here.

P.S. I'll keep an eye on the Fedora  packaging of libwpe. We'll need to discuss with Berto who will package libwpe for Debian.

> Source/WebCore/PlatformGTK.cmake:43
> +if (USE_WPE_RENDERER)
> +    list(APPEND WebCore_INCLUDE_DIRECTORIES
> +        "${WEBCORE_DIR}/platform/graphics/libwpe"
> +    )
> +endif ()

Hm, so this is like USE(LIBWPE), except more limited, we are only using the renderer component of libwpe?

I guess lots of things break if we turn on USE(LIBWPE)?

We need to think carefully about the guards we use here, because this has the potential to become a confusing mess. I don't think we should be using libwpe while having USE(LIBWPE) turned off. The whole point of USE(LIBWPE) was to allow using libwpe without being the WPE port. So we should use that. Except we can't, because that's too much libwpe for GTK. Am I correct?

We might need to introduce some WPE features concept, so PlayStation and GTK can both use separate bits of libwpe (USE_WPE_EVENTS, USE_WPE_RENDERER, USE_WPE_PASTEBOARD, USE_WPE_WHATEVER), while WPE port can use the whole thing. If we do that, then your USE_WPE_RENDERER option could live on, while USE_LIBWPE would either go away or could be set if any libwpe suboption is enabled.

> Source/WebCore/platform/graphics/PlatformDisplay.cpp:45
> -#if USE(LIBWPE)
> +#if USE(WPE_RENDERER)

See, this needs further discussion with Don.

> Source/WebKit/PlatformGTK.cmake:479
> +if (USE_WPE_RENDERER)
> +    list(APPEND WebKit_LIBRARIES
> +        PRIVATE

Oops! (PRIVATE doesn't work here.)

> Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp:119
> +        wpe_loader_init("libWPEBackend-fdo-1.0.so");

This looks frighteningly fragile. What happens when there's a soname bump? Does wpe_loader_init() crash (which would be OK), or is there a fallback (less good then a crash: users might not realize it's broken), or does everything just fail to work?

> Source/WebKit/UIProcess/gtk/WaylandCompositor.cpp:29
> +#if PLATFORM(WAYLAND) && USE(EGL) && !USE(WPE_RENDERER)

How long will we keep WaylandCompositor around as a fallback path?

It's only needed for accelerated rendering, right? So we don't really need to keep it. We could fallback to software rendering if libwpe is unavailable.

[Don Olmstead]

Comment on attachment 370036 [details]
Patch

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

If we want to split USE_LIBWPE that's fine. Just keep us in the loop and add whatever it is for the full USE(LIBWPE) into OptionsPlayStation.cmake.

>> Source/WebCore/PlatformGTK.cmake:43
>> +endif ()
> 
> Hm, so this is like USE(LIBWPE), except more limited, we are only using the renderer component of libwpe?
> 
> I guess lots of things break if we turn on USE(LIBWPE)?
> 
> We need to think carefully about the guards we use here, because this has the potential to become a confusing mess. I don't think we should be using libwpe while having USE(LIBWPE) turned off. The whole point of USE(LIBWPE) was to allow using libwpe without being the WPE port. So we should use that. Except we can't, because that's too much libwpe for GTK. Am I correct?
> 
> We might need to introduce some WPE features concept, so PlayStation and GTK can both use separate bits of libwpe (USE_WPE_EVENTS, USE_WPE_RENDERER, USE_WPE_PASTEBOARD, USE_WPE_WHATEVER), while WPE port can use the whole thing. If we do that, then your USE_WPE_RENDERER option could live on, while USE_LIBWPE would either go away or could be set if any libwpe suboption is enabled.

Original reason for the USE(LIBPWE) was PLATFORM(WPE) = USE(GLIB) + USE(LIBWPE) and PLATFORM(PLAYSTATION) = USE(LIBWPE). I'm ok with coming up with a good way to split things but we'd probably have to audit the code to see what is what. For the PlayStation port our plan is to use LIBWPE fully as an abstraction over our own APIs to get code sharing. A simple USE_WPE_RENDERER and USE_WPE_INPUT would probably be ok. I know there was a Windows WPE port at some point but the repo hasn't seen much in the way of commits. For that you might have to split things more. I'd assume the Windows pasteboard code is what they'd want to use for example.

>> Source/WebKit/PlatformGTK.cmake:479
>> +        PRIVATE
> 
> Oops! (PRIVATE doesn't work here.)

On the CMake side I've been debating making all libraries PRIVATE to better support the Apple internal builds. That would let you build each directory separately. Could also do ${target}_PRIVATE_LIBRARIES.

> Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:191
>      // FIXME: Enable this for WPE once it's possible to do these forced updates
>      // in a way that doesn't starve out the underlying graphics buffers.
> -#if PLATFORM(GTK)
> +#if PLATFORM(GTK) && !USE(WPE_RENDERER)

Can we address this FIXME? Fujii has a patch to turn on coordinated graphics on WinCairo and he modified this line as well.

If not wouldn't just !USE(WPE_RENDERER) be correct?

> Source/WebKit/SourcesGTK.txt:422
> +WebProcess/WebPage/libwpe/AcceleratedSurfaceLibWPE.cpp @no-unify

We've gotten non unified builds to work with WinCairo and Ross has been periodically doing that to make sure everything works. Perhaps that's something you all could spin up a bot for so we can figure out how to get rid of the @no-unify parts?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 370036 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370036&action=review
> 
> Cool.
> 
> No complaints about the code itself, but I think we need to discuss the
> preprocessor guards used here.
> 
> P.S. I'll keep an eye on the Fedora  packaging of libwpe. We'll need to
> discuss with Berto who will package libwpe for Debian.
> 
> > Source/WebCore/PlatformGTK.cmake:43
> > +if (USE_WPE_RENDERER)
> > +    list(APPEND WebCore_INCLUDE_DIRECTORIES
> > +        "${WEBCORE_DIR}/platform/graphics/libwpe"
> > +    )
> > +endif ()
> 
> Hm, so this is like USE(LIBWPE), except more limited, we are only using the
> renderer component of libwpe?

Yes.

> I guess lots of things break if we turn on USE(LIBWPE)?

Note that we use libwpe only for accelerated compositing implementation under wayland. We can't use libwpe in X11 or for non-ac mode. And in any case we really want to use GDK for everything else in all configurations.

> We need to think carefully about the guards we use here, because this has
> the potential to become a confusing mess. I don't think we should be using
> libwpe while having USE(LIBWPE) turned off. The whole point of USE(LIBWPE)
> was to allow using libwpe without being the WPE port. So we should use that.
> Except we can't, because that's too much libwpe for GTK. Am I correct?

Right, we don't want to use the pasteboard or input apis from libwpe, we want GDK.

> We might need to introduce some WPE features concept, so PlayStation and GTK
> can both use separate bits of libwpe (USE_WPE_EVENTS, USE_WPE_RENDERER,
> USE_WPE_PASTEBOARD, USE_WPE_WHATEVER), while WPE port can use the whole
> thing. If we do that, then your USE_WPE_RENDERER option could live on, while
> USE_LIBWPE would either go away or could be set if any libwpe suboption is
> enabled.

I never liked USE(LIBWPE) to be honest (even less the classes named FooLibWPE), but I agree that PLATFORM(WPE) and USE(WPE) would be even more confusing. In any case, if we want to split USE(LIBWPE) I would do it in a previous patch to this, or in a follow up.

> > Source/WebCore/platform/graphics/PlatformDisplay.cpp:45
> > -#if USE(LIBWPE)
> > +#if USE(WPE_RENDERER)
> 
> See, this needs further discussion with Don.

What exactly?

> > Source/WebKit/PlatformGTK.cmake:479
> > +if (USE_WPE_RENDERER)
> > +    list(APPEND WebKit_LIBRARIES
> > +        PRIVATE
> 
> Oops! (PRIVATE doesn't work here.)

What doesn't work? What should I do then, just remove it?

> > Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp:119
> > +        wpe_loader_init("libWPEBackend-fdo-1.0.so");
> 
> This looks frighteningly fragile. What happens when there's a soname bump?
> Does wpe_loader_init() crash (which would be OK), or is there a fallback
> (less good then a crash: users might not realize it's broken), or does
> everything just fail to work?

If there's a soname bump there will be two .so, so the old should exist. We depend on 1.0, so it should always exists. The same way we depend on gtk3.

> > Source/WebKit/UIProcess/gtk/WaylandCompositor.cpp:29
> > +#if PLATFORM(WAYLAND) && USE(EGL) && !USE(WPE_RENDERER)
> 
> How long will we keep WaylandCompositor around as a fallback path?

At least until we have tested the new path enough and all distros are shipping libwpe. Hopefully we can remove it after branching for 2.26, but I guess we will keep it for one more cycle.

> It's only needed for accelerated rendering, right? So we don't really need
> to keep it. We could fallback to software rendering if libwpe is unavailable.

No, let's give people and distros time to switch to libwpe before removing a feature like accelerated compositing.

[Carlos Garcia Campos]

(In reply to Don Olmstead from comment #4)
> Comment on attachment 370036 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370036&action=review
> 
> If we want to split USE_LIBWPE that's fine. Just keep us in the loop and add
> whatever it is for the full USE(LIBWPE) into OptionsPlayStation.cmake.
> 
> >> Source/WebCore/PlatformGTK.cmake:43
> >> +endif ()
> > 
> > Hm, so this is like USE(LIBWPE), except more limited, we are only using the renderer component of libwpe?
> > 
> > I guess lots of things break if we turn on USE(LIBWPE)?
> > 
> > We need to think carefully about the guards we use here, because this has the potential to become a confusing mess. I don't think we should be using libwpe while having USE(LIBWPE) turned off. The whole point of USE(LIBWPE) was to allow using libwpe without being the WPE port. So we should use that. Except we can't, because that's too much libwpe for GTK. Am I correct?
> > 
> > We might need to introduce some WPE features concept, so PlayStation and GTK can both use separate bits of libwpe (USE_WPE_EVENTS, USE_WPE_RENDERER, USE_WPE_PASTEBOARD, USE_WPE_WHATEVER), while WPE port can use the whole thing. If we do that, then your USE_WPE_RENDERER option could live on, while USE_LIBWPE would either go away or could be set if any libwpe suboption is enabled.
> 
> Original reason for the USE(LIBPWE) was PLATFORM(WPE) = USE(GLIB) +
> USE(LIBWPE) and PLATFORM(PLAYSTATION) = USE(LIBWPE). I'm ok with coming up
> with a good way to split things but we'd probably have to audit the code to
> see what is what. For the PlayStation port our plan is to use LIBWPE fully
> as an abstraction over our own APIs to get code sharing. A simple
> USE_WPE_RENDERER and USE_WPE_INPUT would probably be ok. I know there was a
> Windows WPE port at some point but the repo hasn't seen much in the way of
> commits. For that you might have to split things more. I'd assume the
> Windows pasteboard code is what they'd want to use for example.
> 
> >> Source/WebKit/PlatformGTK.cmake:479
> >> +        PRIVATE
> > 
> > Oops! (PRIVATE doesn't work here.)
> 
> On the CMake side I've been debating making all libraries PRIVATE to better
> support the Apple internal builds. That would let you build each directory
> separately. Could also do ${target}_PRIVATE_LIBRARIES.

I have no idea what that is for, so I'll do whatever yous guys tell me to do.

> > Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:191
> >      // FIXME: Enable this for WPE once it's possible to do these forced updates
> >      // in a way that doesn't starve out the underlying graphics buffers.
> > -#if PLATFORM(GTK)
> > +#if PLATFORM(GTK) && !USE(WPE_RENDERER)
> 
> Can we address this FIXME? Fujii has a patch to turn on coordinated graphics
> on WinCairo and he modified this line as well.

Yes, it's on top of my TODO when this patch lands, I have to see how many tests fail because of this. Unfortunately it's not easy to fix, because libwpe and fdo backend expect the frame cycle to be always the same and they contains asserts to ensure it.

> If not wouldn't just !USE(WPE_RENDERER) be correct?

Yes.

> > Source/WebKit/SourcesGTK.txt:422
> > +WebProcess/WebPage/libwpe/AcceleratedSurfaceLibWPE.cpp @no-unify
> 
> We've gotten non unified builds to work with WinCairo and Ross has been
> periodically doing that to make sure everything works. Perhaps that's
> something you all could spin up a bot for so we can figure out how to get
> rid of the @no-unify parts?

I don't remember why I added that, I'll check if it's still needed and why.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #5)
> I never liked USE(LIBWPE) to be honest (even less the classes named
> FooLibWPE), but I agree that PLATFORM(WPE) and USE(WPE) would be even more
> confusing. In any case, if we want to split USE(LIBWPE) I would do it in a
> previous patch to this, or in a follow up.

We need to discuss with Don what exactly to do with the include guards. Seems like that's the only controversy here.

> > > Source/WebKit/PlatformGTK.cmake:479
> > > +if (USE_WPE_RENDERER)
> > > +    list(APPEND WebKit_LIBRARIES
> > > +        PRIVATE
> > 
> > Oops! (PRIVATE doesn't work here.)
> 
> What doesn't work? What should I do then, just remove it?

Yeah, just remove it. PRIVATE is not a library and cannot be appended to WebKit_LIBRARIES.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #3)

> How long will we keep WaylandCompositor around as a fallback path?
> 
> It's only needed for accelerated rendering, right? So we don't really need
> to keep it. We could fallback to software rendering if libwpe is unavailable.

While Wayland is now available in FreeBSD (using a similar stack, with
libdrm + Mesa), AFAIU there is no support for DMA-BUF there because that
is Linux-specific. We may want to keep the nested compositor support for
systems where Wayland runs but are not Linux-based. I have no idea what
the situation is for the other BSDs, though.

As a reminder: Wayland is only communications protocol, just a peculiar
one with a very specific purpose (pushing pixels around, that is), and
there is not nothing to the its base protocols which is Linux-specific.

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #8)
> (In reply to Michael Catanzaro from comment #3)
> 
> > How long will we keep WaylandCompositor around as a fallback path?
> > 
> > It's only needed for accelerated rendering, right? So we don't really need
> > to keep it. We could fallback to software rendering if libwpe is unavailable.
> 
> While Wayland is now available in FreeBSD (using a similar stack, with
> libdrm + Mesa), AFAIU there is no support for DMA-BUF there because that
> is Linux-specific. We may want to keep the nested compositor support for
> systems where Wayland runs but are not Linux-based. I have no idea what
> the situation is for the other BSDs, though.

WPEBackend-fdo works without linux dmabuf too, using the wayland resources the same way we do in our nested compositor.

> As a reminder: Wayland is only communications protocol, just a peculiar
> one with a very specific purpose (pushing pixels around, that is), and
> there is not nothing to the its base protocols which is Linux-specific.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #9)
> (In reply to Adrian Perez from comment #8)
> > (In reply to Michael Catanzaro from comment #3)
> > 
> > > How long will we keep WaylandCompositor around as a fallback path?
> > > 
> > > It's only needed for accelerated rendering, right? So we don't really need
> > > to keep it. We could fallback to software rendering if libwpe is unavailable.
> > 
> > While Wayland is now available in FreeBSD (using a similar stack, with
> > libdrm + Mesa), AFAIU there is no support for DMA-BUF there because that
> > is Linux-specific. We may want to keep the nested compositor support for
> > systems where Wayland runs but are not Linux-based. I have no idea what
> > the situation is for the other BSDs, though.
> 
> WPEBackend-fdo works without linux dmabuf too, using the wayland resources
> the same way we do in our nested compositor.

Great. No further objections, your honor!

[Carlos Garcia Campos]

Created attachment 370687 [details]
Patch

Rebased and updated.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #7)
> (In reply to Carlos Garcia Campos from comment #5)
> > I never liked USE(LIBWPE) to be honest (even less the classes named
> > FooLibWPE), but I agree that PLATFORM(WPE) and USE(WPE) would be even more
> > confusing. In any case, if we want to split USE(LIBWPE) I would do it in a
> > previous patch to this, or in a follow up.
> 
> We need to discuss with Don what exactly to do with the include guards.
> Seems like that's the only controversy here.

Ok, but I don't think the naming of the internal ifdefs should block this patch. We can discuss and change them on a follow up patch.

[EWS Watchlist]

Attachment 370687 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 370687 [details]
Patch

Fine, but please start that discussion in another bug and follow up on it.

[Carlos Garcia Campos]

Committed r245807: <https://trac.webkit.org/changeset/245807>