1008729 - Use CAIRO DrawTarget for CanvasClient2D when reading back from GL

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Bas Schouten (:bas.schouten)]

Currently we use the canvas backend for CanvasClient2D. Normally for a 2D canvas this makes sense, when using it with readback it doesn't though. LockBits isn't implemented for Skia nor is CopySurface with a DataSourceSurface, so currently when Skia is the canvas backend CopyableCanvasLayer::UpdateTarget doesn't work at all. For Cairo LockBits is implemented so we should just use a cairo backend, this will prevent an additional copy and make things work.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Use Cairo backend when dealing with WebGL

Comment 2

[Andreas Gal :gal]

We are trying to kill Cairo. How about we implement the right bits for Skia instead?

Comment 3

[Andreas Gal :gal]

Attachment: Enable CopySurface for DataSourceSurfaces in skia DT

Comment 4

[Andreas Gal :gal]

Removing this might be enough. GetBitmapForSurface supports data surfaces, so if we fall into that this should just work.

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8420715 [details] [diff] [review]
Enable CopySurface for DataSourceSurfaces in skia DT

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

We'd still need to check for the surface types we do support.

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to Andreas Gal :gal from comment #4)
> Removing this might be enough. GetBitmapForSurface supports data surfaces,
> so if we fall into that this should just work.

Regardless, that is still a bad idea, currently we use Cairo on windows for most things outside of Canvas. This means we can get into situations where the DrawTargets we have here may be used in conjunction with other DrawTargets, which will be Cairo, and as such will need cairo source surfaces or jump through all sorts of slow hoops.

At the moment Cairo is what we use and it's the most functional and well tested backend. Wanting to switch to Skia is fine, and of course patches to implement LockBits (the most important bit here!) and improve CopySurface as your patch already mostly does (other than lacking the check) are very welcome. I don't want to do two things at once though. First let's fix what's broken about the current code in such a way that it causes the minimal possible risk to OMTC without causing regressions, then let's see about switching things to Skia :).

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 8420667 [details] [diff] [review]
Use Cairo backend when dealing with WebGL

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

::: gfx/layers/client/CanvasClient.cpp
@@ +71,5 @@
>        flags |= TextureFlags::NEEDS_Y_FLIP;
>      }
> +
> +    if (aLayer->IsGLLayer()) {
> +      mBuffer = CreateBufferTextureClient(gfx::ImageFormatToSurfaceFormat(format),

Please add a comment here explaining why we specifically use cairo for readbacks before landing.

Comment 8

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2430884972b6

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2430884972b6