Closed Bug 995721 Opened 6 years ago Closed 6 years ago

ClientLayerManager::BeginTransactionWithTarget doesn't honor translation on target

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(2 files, 2 obsolete files)

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].
(I'm going to attach a patch for review, although I'm not quite ready to claim that that patch is correct.)
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?
Attachment #8405838 - Flags: review?(matt.woodrow)
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.
... 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 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.
Attachment #8405838 - Flags: review?(matt.woodrow)
And hopefully a final try run with all three bugs, and new mochitests:
https://tbpl.mozilla.org/?tree=Try&rev=ef0b8637f70e
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?
Attachment #8405927 - Flags: review?(matt.woodrow)
Attachment #8405838 - Attachment is obsolete: true
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.)
Attachment #8405928 - Flags: review?(matt.woodrow)
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.
Attachment #8405927 - Flags: review?(matt.woodrow) → review+
(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 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.
Attachment #8405927 - Flags: review+ → review-
Attachment #8405928 - Flags: review?(matt.woodrow) → review+
This switches from DrawTarget::CopySurface (which ignores the transform)
to DrawSurface (which honors the transform).
Attachment #8405942 - Flags: review?(matt.woodrow)
Attachment #8405927 - Attachment is obsolete: true
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.
Attachment #8405942 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/4428373539b7
https://hg.mozilla.org/mozilla-central/rev/7ba6e4eeb392
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.