1196927 - Plugins that render to RGB24 surfaces have incorrect alpha values when compositing with Skia content backend

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Lee Salzman [:lsalzman]]

When using Skia as a content backend, RGB24 surfaces are only properly supported if their alpha channel is opaque (filled with 255). Internally, we tell Skia these surfaces have all opaque alpha, which breaks down when blending an opaque bitmap onto a non-opaque bitmap because the alpha values are interpreted.

In most cases, this is fine since we use Skia to render these bitmaps in the first place, ensuring the alpha channel is correctly filled...

... Except for plugins. The shared surface that is generated by a plugin instance is not rendered with Skia (i.e. X11 ends up using Cairo image/xlib surfaces), which will not properly fill the alpha channel for such RGB24 surfaces.

So when this shared plugin surface is finally passed to a Skia draw target for compositing, the Skia draw target composites this surface incorrectly with the faulty alpha values.

Currently this causes most of our plugin reftests to fail when setting Skia as the content backend.

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: use BGRA for plugin shared surface with Skia content backend

This core idea of this patch is to use a ARGB32 surface for sharing between plugin child and parent in the case of Skia. This ensures the alpha channel is correctly filled by the time this surface gets composited.

When Cairo copies from the RGB24 surface into the ARGB32 shared surface, it will handle correctly filling the alpha channel with opaque values for us.

Most of the patch is just poking at IPDL glue to pass in the content backend type to the plugin child so we can actually use that information.

Once there, the only specific semantic change this patch makes is to check if the content backend is Skia and if the shared image surface would have been RGB24, and if so, force it to ARGB32 instead.

The actual plugin rendering is left on the RGB24 surface to minimize chance of breakage, only making the ARGB32 surface come into play during transport.

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: convert plugin BGRX data to BGRA when shown to Skia backend

This is an alternative version of the patch that only modifies PluginInstanceParent so that the backend type is not communicated with PluginInstanceChild like in the other patch.

It does its work in the RecvShow handler because that's the last place we have any info on the dirty rect for the plugin data, after which such information becomes lost.

The downside of this version of the patch is that the format conversion work is no longer contained in the plugin thread, so the performance cost is no longer contained there but moves higher up.

Comment 4

[Benoit Girard (:BenWa)]

I'm warming up to the idea that having the content type in the plugin process might not be as bad as I originally though.

But first let's get some performance data to see how big the overhead is of the second patch.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Can't we just do the read back to a surface that BGRA and otherwise treat it as BGRX without having to tell anyone else that it's BGRA?

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: force plugin BGRX image surface data to always have valid alpha

As discussed with Jeff, went for door #3: don't create the surface as BGRA, but rather temporarily reinterpret it as such during the Xlib (or other) readback. The compositor now still gets its usual BGRX surface but is none-the-wiser that it was not generated by Skia, if such is the case.

This is now always done, since almost all cases but Skia will never hit that patch anyway. And even so, we determined that the Cairo fast-path for BGRX -> BGRA copies are more or less the same performance-wise as BGRX -> BGRX, especially once any readbacks are factored in. This avoids having to pass backend information to the plugin child, while letting us still isolate any potential costs to the plugin process.

Incidental cleanup as well of CreateDrawTargetForSurface as well to more closely match its current version in Factory and avoid some potential crashes there.

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: force plugin BGRX image surface data to always have valid alpha

Limit format conversion to where a copy was already taking place.

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 8652083 [details] [diff] [review]
force plugin BGRX image surface data to always have valid alpha

Review of attachment 8652083 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +3257,5 @@
> +      if (aSurface == mCurrentSurface &&
> +          aSurface->GetType() == gfxSurfaceType::Image &&
> +          aSurface->GetSurfaceFormat() == SurfaceFormat::B8G8R8X8) {
> +        gfxImageSurface* imageSurface = static_cast<gfxImageSurface*>(aSurface);
> +        // Reinterpret target surface as BGRA to fill alpha with opaque

This could use a more information comment.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8652083 [details] [diff] [review]
force plugin BGRX image surface data to always have valid alpha

Review of attachment 8652083 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +3257,5 @@
> +      if (aSurface == mCurrentSurface &&
> +          aSurface->GetType() == gfxSurfaceType::Image &&
> +          aSurface->GetSurfaceFormat() == SurfaceFormat::B8G8R8X8) {
> +        gfxImageSurface* imageSurface = static_cast<gfxImageSurface*>(aSurface);
> +        // Reinterpret target surface as BGRA to fill alpha with opaque

Yes please.

Comment 10

[Lee Salzman [:lsalzman]]

Attachment: force plugin BGRX image surface data to always have valid alpha

Added more descriptive code comment as requested by Ben, otherwise changed no code (so carried r+).

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee0bcdb8c038
The try run more or less just proves things build, since none of our platforms currently deploy Skia as content backend yet, so it shouldn't affect anything yet.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/338581c45777

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/338581c45777