995721 - ClientLayerManager::BeginTransactionWithTarget doesn't honor translation on target

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Investigating the translation problem in bug 995410 has led me to find that the underlying problem seems to be that ClientLayerManager::BeginTransactionWithTarget doesn't honor the translation on target (unlike other layer managers, I believe).

This, in turn, leads to the translation issue I've been investigating in bug 995410 -- when CanvasRenderingContext2D::DrawWindow takes the codepath of creating a separate offscreen DrawTarget because the canvas and content azure backends don't match, *and* we're using OMTC (i.e., ClientLayerManager), drawWindow calls with x or y parameters nonzero end up drawing the wrong data to the right rectangle.

The stack that leads to this is the cairo_matrix_identity call in attachment 8405832 [details].

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I'm going to attach a patch for review, although I'm not quite ready to claim that that patch is correct.)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Honor the translation on mShadowTarget in ClientLayerManager::MakeSnapshotIfRequired

DrawTarget::CopySurface is documented as ignoring the transform; this
seems like a reasonable place to me to try to honor it, although I'm far
from being able to judge if it's the correct one.

REVIEW:  Is this the right API for BeginTransactionWithTarget to have?
And should there there be any difference between the treatment of
BeginTransactionWithTarget and SetShadowTarget?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The reftest results at https://tbpl.mozilla.org/?tree=Try&rev=e56757590f59 make me think there's (a) 1-pixel error somewhere and (b) something wrong with scrolling, maybe in this patch or maybe elsewhere.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... they all seem to be problems in this patch, since they all seem to go away with the cairo_identity_matrix call in attachment 8405832 [details] removed instead of having this patch.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8405838 [details] [diff] [review]
Honor the translation on mShadowTarget in ClientLayerManager::MakeSnapshotIfRequired

Better patch coming, but want to test it a bit more first.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=b605811c3f4b shows the new patch works

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And hopefully a final try run with all three bugs, and new mochitests:
https://tbpl.mozilla.org/?tree=Try&rev=ef0b8637f70e

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Honor the translation on mShadowTarget in ClientLayerManager::MakeSnapshotIfRequired

DrawTarget::CopySurface is documented as ignoring the transform; this
seems like a reasonable place to me to try to honor it, although I'm far
from being able to judge if it's the correct one.

REVIEW:  Is this the right API for BeginTransactionWithTarget to have?
And should there there be any difference between the treatment of
BeginTransactionWithTarget and SetShadowTarget?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Run drawWindow mochitests on a toplevel window

Without patch 1, the final 4 of the set of 7 tests fail on Linux with
accelerated layers (whereas all the ones they were copied from, added in
bug 995661, pass, given the patches in that bug but not this one).  (I
don't need to enable skia-canvas to see the failures without patch 1.  I
thought that didn't match my earlier testing, but I probably just never
tested that combination.  This means the failures happen even without
CanvasRenderingContext2D::DrawWindow going through the codepath where it
creates a DrawTarget.)

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8405927 [details] [diff] [review]
patch 1 - Honor the translation on mShadowTarget in ClientLayerManager::MakeSnapshotIfRequired

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

::: gfx/layers/client/ClientLayerManager.cpp
@@ +298,5 @@
> +        MOZ_ASSERT(mat.IsIntegerTranslation(),
> +                   "don't know how to handle matrix other than translation");
> +        IntPoint destPoint(RoundedToInt(mat.GetTranslation()));
> +        IntRect sourceRect(IntPoint(0, 0), widgetSize);
> +        dt->CopySurface(surf, sourceRect, destPoint);

I wonder if any extensions/addons are relying on being able to drawWindow with arbitrary transforms set. I guess not, since nobody has complained about it not working. We could use DrawSurface instead to fix that, but I doubt it matters.

Comment 11

[Matt Woodrow (:mattwoodrow)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #8)
> 
> REVIEW:  Is this the right API for BeginTransactionWithTarget to have?
> And should there there be any difference between the treatment of
> BeginTransactionWithTarget and SetShadowTarget?

I think this is ok. Those two functions should have the same behaviour, though it looks like SetShadowTarget is dead code.

If you want to use DrawSurface instead, then we'll need to do dt->PushClipRect(sourceRect), and then use the SOURCE operator for drawing.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8405927 [details] [diff] [review]
patch 1 - Honor the translation on mShadowTarget in ClientLayerManager::MakeSnapshotIfRequired

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

Let's just use DrawSurface here to match the other path in CanvasRenderingContext2D. It makes more sense this way, so that transparent parts of the window won't erase existing content. Users like reftests that want existing content to be overwritten can be responsible for ensuring opaque content (using the background color) or by explicitly clearing the canvas beforehand.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Honor the translation on mShadowTarget in ClientLayerManager::MakeSnapshotIfRequired

This switches from DrawTarget::CopySurface (which ignores the transform)
to DrawSurface (which honors the transform).

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And one more try run with the revisions:
https://tbpl.mozilla.org/?tree=Try&rev=ba9ace35812d

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8405942 [details] [diff] [review]
patch 1 - Honor the translation on mShadowTarget in ClientLayerManager::MakeSnapshotIfRequired

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

::: gfx/layers/client/ClientLayerManager.cpp
@@ +295,5 @@
> +        Rect widgetRect(Point(0, 0), Size(widgetSize.width, widgetSize.height));
> +        dt->PushClipRect(widgetRect);
> +        dt->DrawSurface(surf, widgetRect, widgetRect,
> +                        DrawSurfaceOptions(),
> +                        DrawOptions(1.0f, CompositionOp::OP_SOURCE));

I think we can just use OVER here and skip the clipping.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4428373539b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba6e4eeb392

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4428373539b7
https://hg.mozilla.org/mozilla-central/rev/7ba6e4eeb392