836130 - Enable accelerated quartz by default

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Add a direct azure version of DrawMissingGlyph

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Remove GetThebesSurfaceForDrawTarget dependencies

We can't really support this with accelerated quartz (without patching cairo significantly), so this makes us no longer require it.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Share the code for creating a CGImage from data

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Correctly handle SURFACE_DATA surfaces that aren't also a DataSourceSurfaceCG.

This prevents us from making an invalid cast when we encounter a raw data surface.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Remove GetThebesSurfaceForDrawTarget for accelerated quartz

This really doesn't work at all, it just creates an entirely separate image surface that never gets any further updates.

With the prior patches, we no longer require this to return a valid surface, so this removes it.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix some compiler warnings

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Avoid readback whenever possible when creating a snapshot of an accelerated quartz DT

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Mark some reftests fuzzy since we are using a different renderer for content/canvas

Comment 9

[Matt Woodrow (:mattwoodrow)]

This gets us fairly close on 10.7 at least: https://tbpl.mozilla.org/?tree=Try&rev=bf1498e215fc

The biggest remaining issue is reftests on 10.8. We fail a large amount with what appears to be a blending bug.

Any test that has a small invalidation update (and calls drawWindow with that small area) ends up with incorrect results. If I patch reftest.js to pad out the drawWindow sizes to at least 151x151, then the tests pass.

The implementation of drawWindow allocates a gfxImageSurface and draws the required contents into that. I've verified that the pixel data is identical in both failing/passing cases. We then create a CGImage for this data, and draw it to the accelerated quartz context. Using OPERATOR_SOURCE for the blit doesn't appear to fix the bug either.

My guess is that the internal implementation is doing something different for these 'small' surfaces. The fact that it is limited to 10.8 also suggests that it's unlikely to be something in our code.

There are also a few crash bugs found by fuzzing (marked as deps of this bug) that need to be resolved.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Apply filtering to the right texture target

Comment 11

[Benoit Girard (:BenWa)]

Comment on attachment 707910 [details] [diff] [review]
Remove GetThebesSurfaceForDrawTarget for accelerated quartz

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

::: gfx/thebes/gfxPlatformMac.cpp
@@ +403,5 @@
>  gfxPlatformMac::GetThebesSurfaceForDrawTarget(DrawTarget *aTarget)
>  {
>    if (aTarget->GetType() == BACKEND_COREGRAPHICS_ACCELERATED) {
> +    // There is no thebes support for accelerated CGContexts
> +    return nullptr;

Are you sure nothing here will need this? We could just read back the results.

Comment 12

[Bas Schouten (:bas.schouten)]

Comment on attachment 707906 [details] [diff] [review]
Remove GetThebesSurfaceForDrawTarget dependencies

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

Looks good to me! Some incorrect whitespace here, make sure to fix that before landing.

Comment 13

[Bas Schouten (:bas.schouten)]

Comment on attachment 707905 [details] [diff] [review]
Add a direct azure version of DrawMissingGlyph

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

::: gfx/2d/2D.h
@@ +371,5 @@
>     * if there is no active subpath.
>     */
>    virtual Point CurrentPoint() const = 0;
> +
> +  void Rectangle(const Rect& aRect) {

I'm not sure I like this API inside PathSink. I need to think about it a little more though.

Comment 14

[Matt Woodrow (:mattwoodrow)]

(In reply to Benoit Girard (:BenWa) from comment #11)
> Are you sure nothing here will need this? We could just read back the
> results.

Positive, I went through every caller and made sure of it.

Reading back doesn't work, because the callers expect the surface returned to be a 'live' copy of the DrawTarget's data. This is what we had previously, and broke things.

An example:

We create a new, blank, canvas. We call drawWindow, which does a readback (of blank pixels) inside GetThebesSurfaceForDrawTarget, and then draws the window contents into the cached gfxImageSurface.

We then use canvas API's to draw something ontop of the drawWindow contents (fillRect maybe?). These go into the DT (which is still blank), and don't touch the drawWindow contents which are in the gfxImageSurface.

We then use asDataURL to retrieve our composited image, which grbas the cached gfxImageSurface and doesn't contain the changes that our drawRect should have done.

We could stop caching the gfxImageSurface, but then the drawWindow contents would be lost instead.

I'm pretty sure my patches are the only sane way to fix this :)

Comment 15

[Matt Woodrow (:mattwoodrow)]

(In reply to Bas Schouten (:bas.schouten) from comment #13)

> 
> I'm not sure I like this API inside PathSink. I need to think about it a
> little more though.

I assume we want it *somewhere* though right? It's pretty basic boilerplate code that we don't want to be repeating all over the place.

Comment 16

[Benoit Girard (:BenWa)]

Comment on attachment 707910 [details] [diff] [review]
Remove GetThebesSurfaceForDrawTarget for accelerated quartz

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

r+ with a summary of your explanation in the code base why it's not supported.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 707907 [details] [diff] [review]
Share the code for creating a CGImage from data

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

::: gfx/2d/SourceSurfaceCG.cpp
@@ -165,5 @@
>        bitsPerPixel = 32;
>        break;
>  
>      case FORMAT_A8:
> -      // XXX: why don't we set a colorspace here?

Don't drop the comment

@@ +121,5 @@
>  
> +  dataProvider = CGDataProviderCreateWithData(data,
> +                                              data,
> +					                          aSize.height * aStride,
> +					                          (aOwner > ExternalData) ? releaseCallback : nullptr);

This function call mixes lines and spaces.

::: gfx/2d/SourceSurfaceCG.h
@@ +26,5 @@
> +CreateImageFromData(unsigned char *aData,
> +                    const IntSize &aSize,
> +                    int32_t aStride,
> +                    SurfaceFormat aFormat,
> +                    DataOwnership aOwner = ExternalData);

Don't have a default parameter. I'd prefer to have all the callers be explicit.

Comment 19

[Matt Woodrow (:mattwoodrow)]

This is currently on hold because of the crash bugs (listed as dependencies) that we currently have no solution to.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So we're planning to use SkiaGL instead, right?

Comment 21

[Matt Woodrow (:mattwoodrow)]

I think we should, yes, since we can patch it as needed.

CC'ing gw280 who has been working on SkiaGL the most.

Comment 22

[Bas Schouten (:bas.schouten)]

Comment on attachment 707905 [details] [diff] [review]
Add a direct azure version of DrawMissingGlyph

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

If this is still relevant it looks fine.

::: gfx/2d/2D.h
@@ +371,5 @@
>     * if there is no active subpath.
>     */
>    virtual Point CurrentPoint() const = 0;
> +
> +  void Rectangle(const Rect& aRect) {

We have a helper for this outside of pathsink now.

Comment 23

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/message/reply/question/etc.)]

What are we doing with this bug?

Comment 24

[Jeff Muizelaar [:jrmuizel]]

The high risk of problems and are inability to fix system libraries means that accelerated CoreGraphics is not worth the risk.