892966 - Fix bugs with Quartz azure content

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Take a reference to the CGContext when create a DT wrapping it

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Make gfxQuartzNativeDrawing support Moz2D surface

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Make gfxQuartzNativeDrawing support DrawTargetDual surfaces

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Reset our cached SourceSurface when MarkDirty/Flush is called on the gfxASurface

Const cast is pretty awful, but imglib seems to only call Flush and not MarkDirty for quartz surfaces.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix a crash when drawing native menus

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Allow Quartz azure content to be used

This should pass tests now.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 774626 [details] [diff] [review]
Take a reference to the CGContext when create a DT wrapping it

Yes please.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Respect the filter property of SurfacePattern

This fixes at least 3 reftest failures.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 774634 [details] [diff] [review]
Respect the filter property of SurfacePattern

Fixed by bug 892968

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 774628 [details] [diff] [review]
Make gfxQuartzNativeDrawing support DrawTargetDual surfaces

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

::: gfx/thebes/gfxQuartzNativeDrawing.cpp
@@ +48,5 @@
> +                            NSToIntFloor(mNativeRect.height * mBackingScale));
> +        CGColorSpaceRef colorSpace = CGColorSpaceCreateDeviceRGB();
> +        mCGContext = CGBitmapContextCreate(nullptr, backingSize.width, backingSize.height,
> +                                           8, backingSize.width * 4, colorSpace,
> +                                           kCGBitmapByteOrder32Host | kCGImageAlphaPremultipliedFirst);

Instead of doing this we should just create a new DrawTargetCG. We can change the borrow api to have a separate Init function instead of using the constructor. That gets rid of the null check and we can reuse the the same BorrowedCG for both cases.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 774627 [details] [diff] [review]
Make gfxQuartzNativeDrawing support Moz2D surface

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

This should be joined with the DrawTargetDual stuff.

::: gfx/2d/DrawTargetCG.cpp
@@ +1181,5 @@
>  BorrowedCGContext::BorrowCGContextFromDrawTarget(DrawTarget *aDT)
>  {
> +  if (!aDT) {
> +    return nullptr;
> +  }

How about moving this check to the constructor and explaining it a bit there.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Make gfxQuartzNativeDrawing support Moz2D surfaces

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Add CreateSourceSurfaceFromNativeSurface for CG

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 774694 [details] [diff] [review]
Make gfxQuartzNativeDrawing support Moz2D surfaces

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

::: gfx/2d/2D.h
@@ +1016,5 @@
>  public:
> +  BorrowedCGContext()
> +    : cg(nullptr)
> +    , mDT(nullptr)
> +  { }

How would feel about having two versions of the constructor. One that takes a DrawTarget and one that doesn't. This will make the code in nsChildView.mm a bit more concise.

@@ +1018,5 @@
> +    : cg(nullptr)
> +    , mDT(nullptr)
> +  { }
> +
> +  CGContextRef Init(DrawTarget *aDT)

Maybe add a comment like "Init is separated from construction because we may not know the DrawTarget we're going to use at construction time if the BorrowedCGContext is a member of another class".

::: gfx/thebes/gfxQuartzNativeDrawing.cpp
@@ +37,5 @@
>  {
>      NS_ASSERTION(!mQuartzSurface, "BeginNativeDrawing called when drawing already in progress");
>  
> +    if (!mContext->IsCairo()) {
> +      mDrawTarget = mContext->GetDrawTarget();

I don't think you need to set mDrawTarget when we're not creating a new one.

i.e. if (mContext->GetDrawTarget()->IsDualDrawTarget()) should work fine.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 774745 [details] [diff] [review]
Add CreateSourceSurfaceFromNativeSurface for CG

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

::: gfx/2d/SourceSurfaceCG.cpp
@@ +254,5 @@
> +{
> +  MOZ_ASSERT(cg);
> +  MOZ_ASSERT(GetContextType(cg) == CG_CONTEXT_TYPE_BITMAP);
> +
> +  mCg = CGContextRetain(cg);

Add to the member declaration about how it is now a strong reference to a CGContext.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Setup dashing correctly with StrokeRect

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 774865 [details] [diff] [review]
Setup dashing correctly with StrokeRect

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

::: gfx/2d/DrawTargetCG.h
@@ +68,5 @@
>  }
>  
> +static inline void
> +SetStrokeOptions(CGContextRef cg, const StrokeOptions &aStrokeOptions)
> +{

Let's just have SetStrokeOptions and use that everywhere.

Comment 18

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69062da26093
https://hg.mozilla.org/integration/mozilla-inbound/rev/97bd67a11baa
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dcbef737c1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd8fb493121
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2d53dd91fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/7160624804ff

Comment 19

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/67eb0198434e as being the first most likely thing in the wide range when we weren't building on Mac at all to have caused https://tbpl.mozilla.org/php/getParsedLog.php?id=25242777&tree=Mozilla-Inbound once we started building again.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 774629 [details] [diff] [review]
Reset our cached SourceSurface when MarkDirty/Flush is called on the gfxASurface

Turns out we do need this.

I guess Quartz has internal caches of CGImages (the downscaled ones maybe?) that don't get updated when we modify the bits.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Landed everything except 'Add CreateSourceSurfaceFromNativeSurface for CG' since it causes test failures in test_canvas.html that I haven't resolved yet.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f51008f3261
https://hg.mozilla.org/integration/mozilla-inbound/rev/d74f325c5139
https://hg.mozilla.org/integration/mozilla-inbound/rev/c14de13134bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/71ae24eae0d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/120178591e35
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f79144c775

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1f51008f3261
https://hg.mozilla.org/mozilla-central/rev/d74f325c5139
https://hg.mozilla.org/mozilla-central/rev/c14de13134bf
https://hg.mozilla.org/mozilla-central/rev/71ae24eae0d1
https://hg.mozilla.org/mozilla-central/rev/120178591e35
https://hg.mozilla.org/mozilla-central/rev/c8f79144c775