Bug 84275 – [chromium] Image masks are considered opaque incorrectly

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Image masks are considered opaque incorrectly

[Dana Jansens]

Created attachment 137766 [details]
Patch

[Dana Jansens]

crbug is http://code.google.com/p/chromium/issues/detail?id=123089

[Dana Jansens]

Comment on attachment 137766 [details]
Patch

Would like to add one more unit test for the imageMask but pretend that the image was partly opaque (if this were possible in the future) by using fillRect instead.

[Adrienne Walker]

Comment on attachment 137766 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:158
> +    state.opaqueRect = SkRect::MakeEmpty();

Can this go in the constructor?

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:178
> +    // Track the copy of the whole layer we are popping off the stack, which may destroy opaque
> +    // stuff in the layer below. Then add the layer's opaque area back into the mix.
> +    didDrawUnbounded(context, layerPaint, transform, FillByCopy);
> +    didDraw(context, transform, layerOpaqueRect, layerPaint, 0, fillsBounds, FillOnly);

Doesn't doing this clear+add depend on the type of composite operation? Do you always need to do the didDrawUnbounded? (Also, it's kind of weird to say didDrawUnbounded, since it seems like you know what the paint rect is here?)

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:418
> +    SkRect* trackingRect = &m_opaqueRect;
> +    if (!m_canvasLayerStack.isEmpty())
> +        trackingRect = &m_canvasLayerStack.last().opaqueRect;

Helper function?

[Dana Jansens]

Comment on attachment 137766 [details]
Patch

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

thanks for looking at this.

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:158
>> +    state.opaqueRect = SkRect::MakeEmpty();
> 
> Can this go in the constructor?

yes. will do.

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:178
>> +    didDraw(context, transform, layerOpaqueRect, layerPaint, 0, fillsBounds, FillOnly);
> 
> Doesn't doing this clear+add depend on the type of composite operation? Do you always need to do the didDrawUnbounded? (Also, it's kind of weird to say didDrawUnbounded, since it seems like you know what the paint rect is here?)

it does depend on the operation, and that is embedded in this whole thing in the layerPaint.

didDrawUnbounded makes sense because we are copying the entire layer being popped off. What are the bounds of the layer? They are not the same as the tracked opaque rect. Perhaps we could know them.. but I don't know them here as it is now. But this is okay. If the draw operation is non-destructive (SrcOver for instance) then it does not do anything to the opaqueRect in the layer below. If it is destructive (DstIn for masking) then anything not known to be opaque in the layer above will destroy opaque in the layer below. So this gives the right answer given the information we are tracking.

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:418
>> +        trackingRect = &m_canvasLayerStack.last().opaqueRect;
> 
> Helper function?

I kinda thought about that too.. will do :)

[Dana Jansens]

Created attachment 137984 [details]
Patch

[Dana Jansens]

Comment on attachment 137984 [details]
Patch

(In reply to comment #5)
> > Doesn't doing this clear+add depend on the type of composite operation? Do you always need to do the didDrawUnbounded? (Also, it's kind of weird to say didDrawUnbounded, since it seems like you know what the paint rect is here?)

Hm, what's wrong here is that these need to combined into a single operation. What we want is a bounded chance to "preserveOpaque" under the layer's opaqueRect. Not to clear and then "preserveOpaque" over what we just cleared to be non-opaque.

[Dana Jansens]

Created attachment 138018 [details]
Patch

[Dana Jansens]

I added a unit test with an opaque area in the image mask, which exposed also the problem you were talking about I believe as well. So the structure of things is now as follows.

- Added a helper bool getTotalClippedBounds() method. Since we were computing the cliprect now in 3 places (didDraw, didDrawUnbounded, and the new applyLayerOpaqueRect).

- Added a helper SkRect& currentTrackingOpaqueRect() which gives the active SkRect that should be modified.

- Added applyLayerOpaqueRegion() which takes the tracked opaque region for a source layer, along with the current clip, and applies it to the current layer.

- Have popCanvasLayer compute the current clip and see if the source layer gets clipped away. Then have it see if it can bound the tracked opaque region in the source layer. If not, we just do an unbounded draw cuz we don't know what is opaque. Otherwise, use applyLayerOpaqueRegion().

[Dana Jansens]

Created attachment 138022 [details]
Patch

Rename applyLayerOpaqueRegion to applyOpaqueRegionFromLayer. And a little change to try keep logic out of popCanvasLayer and inside applyOpaqueRegionFromLayer instead.

[Adrienne Walker]

Comment on attachment 138022 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:162
> +        clippedBounds.setEmpty();

This function is really inconsistent about if and what it sets clippedBounds to when returning false.

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:197
> +    SkRect clippedBounds;

Whoa there, potentially uninitialized SkRect.

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:354
> +    if (clippedBounds.isEmpty())
> +        return;

Should clippedBounds be initialized to layerOpaqueRect in the case not having a clip? I'm guessing here that clipped bounds being empty is trying to imply that the entire layer is clipped away?

[Dana Jansens]

Comment on attachment 138022 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:162
>> +        clippedBounds.setEmpty();
> 
> This function is really inconsistent about if and what it sets clippedBounds to when returning false.

false is meant to mean there is no clippedBounds. If it returns false you cannot use the clippedBounds for anything.

true means that there is a valid rect in clippedBounds so you may use it. I can clarify this in the comment for the function.

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:197
>> +    SkRect clippedBounds;
> 
> Whoa there, potentially uninitialized SkRect.

Yes, but if getTotalClippedBounds() returns false, then fillsBounds = false and we never use the clippedBounds.

Do you want me to reorder this code to make that more clear?

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:354
>> +        return;
> 
> Should clippedBounds be initialized to layerOpaqueRect in the case not having a clip? I'm guessing here that clipped bounds being empty is trying to imply that the entire layer is clipped away?

Right it means exactly that.

clippedBounds is set to something valid if getTotalClippedBounds returned true. I think this comment depends on my previous responses.

[Adrienne Walker]

Comment on attachment 138022 [details]
Patch

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

>>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:197
>>> +    SkRect clippedBounds;
>> 
>> Whoa there, potentially uninitialized SkRect.
> 
> Yes, but if getTotalClippedBounds() returns false, then fillsBounds = false and we never use the clippedBounds.
> 
> Do you want me to reorder this code to make that more clear?

Oops, sorry.  I missed the 'return' there when reading the code and misunderstood the intention.  However, in that case, does that mean that you don't apply an opaque rect from an unclipped layer? (Maybe I just need more context about when layers are not clipped?)

[Dana Jansens]

Comment on attachment 138022 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:197
>>>> +    SkRect clippedBounds;
>>> 
>>> Whoa there, potentially uninitialized SkRect.
>> 
>> Yes, but if getTotalClippedBounds() returns false, then fillsBounds = false and we never use the clippedBounds.
>> 
>> Do you want me to reorder this code to make that more clear?
> 
> Oops, sorry.  I missed the 'return' there when reading the code and misunderstood the intention.  However, in that case, does that mean that you don't apply an opaque rect from an unclipped layer? (Maybe I just need more context about when layers are not clipped?)

It's not that the layer is 'unclipped', it's more that the current clip is something we can't represent as a rectangle. For example, it could be a path, or a diamond, or whatever.

In this case, we don't know what will be clipped from the layerOpaqueRect - essentially we have no layerOpaqueRect anymore, so we just do an "unboundedDraw". This will act like we drew something completely undefined/unknown and just check if it could possibly destroy already opaque areas.

[Dana Jansens]

I think this will be cleaner on top of bug #83608 so rebasing on top of it.

[Dana Jansens]

Created attachment 138154 [details]
Patch

Rebased and I removed the changes to didDrawUnbounded() and made the whole thing a whole lot cleaner!

I'll followup with a CL to fix up didDrawUnbounded().

[Dana Jansens]

Created attachment 138214 [details]
Patch

[Dana Jansens]

Rebased for ews

[Adrienne Walker]

Comment on attachment 138214 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:334
> +    if (!outsideSourceOpaqueRectPreservesOpaque)
> +        markRectAsNonOpaque(deviceClipRect);

Do you need to check for deviceClipIsARect here before you use deviceClipRect and maybe do a markAllAsNonOpaque if it's not? Can you add a test for a layer without a clip that destroys opacity?

[Dana Jansens]

Comment on attachment 138214 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:334
>> +        markRectAsNonOpaque(deviceClipRect);
> 
> Do you need to check for deviceClipIsARect here before you use deviceClipRect and maybe do a markAllAsNonOpaque if it's not? Can you add a test for a layer without a clip that destroys opacity?

oh yes

[Dana Jansens]

Comment on attachment 138214 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:159
> +    if (context->canvas()->getClipDeviceBounds(&deviceClipIRect))

Note this gives the bounds of the clip even if it is not a rect.

>>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:334
>>> +        markRectAsNonOpaque(deviceClipRect);
>> 
>> Do you need to check for deviceClipIsARect here before you use deviceClipRect and maybe do a markAllAsNonOpaque if it's not? Can you add a test for a layer without a clip that destroys opacity?
> 
> oh yes

I think what we want is to use the bounds always (and have getDeviceClipAsRect always return the actual bounds), but then treat it as a rect inside those bounds only if the clip is itself a rect (equal to the bounds). This is the intent of the didDraw code, with its "fillsBounds" variable.

[Dana Jansens]

--- a/Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp
+++ b/Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp
@@ -152,16 +152,14 @@ static inline bool paintIsOpaque(const SkPaint& paint, OpaqueRegionSkia::DrawTyp
 static inline bool getDeviceClipAsRect(const PlatformContextSkia* context, SkRect& deviceClipRect)
 {
     // Get the current clip in device coordinate space.
-    if (context->canvas()->getClipType() != SkCanvas::kRect_ClipType)
-        return false;
-
     SkIRect deviceClipIRect;
     if (context->canvas()->getClipDeviceBounds(&deviceClipIRect))
         deviceClipRect.set(deviceClipIRect);
     else
         deviceClipRect.setEmpty();
 
-    return true;
+    bool isARect = context->canvas()->getClipType() == SkCanvas::kRect_ClipType;
+    return isARect;
 }
 
 static inline SkRect& currentTrackingOpaqueRect(SkRect& rootOpaqueRect, Vector<OpaqueRegionSkia::CanvasLayerState, 3>& canvasLayerStack)
@@ -293,7 +291,7 @@ void OpaqueRegionSkia::didDraw(const PlatformContextSkia* context, const SkRect&
     SkRect deviceClipRect;
     if (!getDeviceClipAsRect(context, deviceClipRect))
         fillsBounds = false;
-    else if (!targetRect.intersect(deviceClipRect))
+    if (!targetRect.intersect(deviceClipRect))
         return;
 
     bool drawsOpaque = paintIsOpaque(paint, drawType, sourceBitmap);

[Dana Jansens]

Created attachment 138414 [details]
Patch

[Dana Jansens]

Created attachment 138415 [details]
Patch

Fix comment

[Adrienne Walker]

Comment on attachment 138415 [details]
Patch

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

popCanvasLayer looks good, but I still have some confusion around how the logic works in didDraw.

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:293
> +    if (!getDeviceClipAsRect(context, deviceClipRect))
>          fillsBounds = false;

Setting fillsBounds = false makes sense for kComplex_ClipType, but shouldn't fillsBounds = true for kEmpty_ClipType (which I'm assuming means no clip)? It seems like you should still be able to apply the opaque region, in that case.

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:295
> +    if (!targetRect.intersect(deviceClipRect))
> +        return;

Along the same lines, for kEmpty_ClipType, deviceClipRect is empty, and this will early out, even though it seems like you should be able to record an opaque rect?

Can you add a test where you draw into a layer with an empty clip rect?

[Adrienne Walker]

Comment on attachment 138415 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:323
> +    if (deviceClipIsARect && deviceClipRect.isEmpty())

Or, if kEmpty_ClipType means "clip everything", this should maybe be just a check for empty?

[Dana Jansens]

Comment on attachment 138415 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:323
>> +    if (deviceClipIsARect && deviceClipRect.isEmpty())
> 
> Or, if kEmpty_ClipType means "clip everything", this should maybe be just a check for empty?

this :)

[Dana Jansens]

Created attachment 138493 [details]
Patch

[Adrienne Walker]

Comment on attachment 138493 [details]
Patch

Thanks for patiently explaining how things work.  R=me.

[Dana Jansens]

Comment on attachment 138493 [details]
Patch

Thanks for your thorough review :)

[WebKit Review Bot]

Comment on attachment 138493 [details]
Patch

Clearing flags on attachment: 138493

Committed r115000: <http://trac.webkit.org/changeset/115000>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dana Jansens]

DstIn is not destroying opaque pixels on mac for some reason. Reopening.

[Dana Jansens]

Created attachment 138651 [details]
Patch for landing

[Dana Jansens]

Comment on attachment 138651 [details]
Patch for landing

Land-safely fail

[Dana Jansens]

Created attachment 138652 [details]
Patch for landing

[Dana Jansens]

Comment on attachment 138652 [details]
Patch for landing

Clearing flags on attachment: 138652

Committed r115139: <http://trac.webkit.org/changeset/115139>

[Dana Jansens]

All reviewed patches have been landed.  Closing bug.