933549 - Use a draw target in ThebesLayerBuffer::PaintState

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nick Cameron [:nrc]]

Attachment: WIP patch

Once we get rid of the Thebes paths from ThebesLayerBuffer (soon to be RotatedContentBuffer) we should replace mContext in ThebesLayerBuffer::PaintState with a draw target.

This is a little bit complicated because we will have to pass around a transform (just a translation) for the offset into the buffer due to rotation. We then have to apply and remove this whenever we use the DT (or wrap it in a gfxContext to pass to layout).

The WIP patch is basically missing that (the transform in the above para) so rendering is pretty broken (may also be broken for other reasons).

Comment 1

[Nick Cameron [:nrc]]

Hmm, so it looks like this would be pretty useful actually. To improve perf (esp. on Windows) we would like to keep hold of the DrawTarget underlying the TLB and have more control over flushing and locking.

Nical - just checking you've not started this too.

Comment 2

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #1)
> Nical - just checking you've not started this too.

I haven't touched ContentClient stuff so far

Comment 3

[Nick Cameron [:nrc]]

Blocking bug 946567 because I want to reduce the amount of locking and flushing we do, and this makes it possible.

Comment 4

[Nick Cameron [:nrc]]

Attachment: Preqrequisites - Matrix::HasNonIntegerTranslation and SetAntialiasingFlags

Comment 5

[Nick Cameron [:nrc]]

Attachment: Change PaintState to use a DrawTarget instead of a gfxContext

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8346307 [details] [diff] [review]
Change PaintState to use a DrawTarget instead of a gfxContext

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

::: gfx/layers/RotatedBuffer.h
@@ +206,2 @@
>      DrawRegionClip mClip;
> +    bool mDidSelfCopy;    

Whitespace!

@@ +356,5 @@
>     */
> +  TemporaryRef<gfx::DrawTarget>
> +  GetTargetForQuadrantUpdate(const nsIntRect& aBounds,
> +                             ContextSource aSource,
> +                             gfx::IntPoint* aTopLeft);

This is a bit worrying. We're passing the top-left as an outparam and expecting any callers to set (and then restore) the appropriate transform.

At minimum we need a comment here explaining what you need to use aTopLeft for, that you need to restore matrix change, and that you need to do so before trying to do anything else with this RotatedBuffer.

Ideally I guess we'd want a different API that only gives temporary access to the DT, and can handle cleanup itself.

::: gfx/layers/client/ContentClient.cpp
@@ +649,5 @@
> +  }
> +
> +  void Translate(const IntPoint& aOffset)
> +  {
> +    mOldTransform.Translate(-aOffset.x, -aOffset.y);

Having this translate by the inverse of aOffset is weird. How about swapping this, and inverting at the caller.

Comment 7

[Nick Cameron [:nrc]]

try push: https://tbpl.mozilla.org/?tree=Try&rev=c8d1aea2e304

Comment 8

[Nick Cameron [:nrc]]

Attachment: Change PaintState to use a DrawTarget instead of a gfxContextTLB_thebes.patch

Quite a lot has changed, so requesting re-review.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8348322 [details] [diff] [review]
Change PaintState to use a DrawTarget instead of a gfxContextTLB_thebes.patch

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

::: gfx/layers/RotatedBuffer.h
@@ +222,2 @@
>      DrawRegionClip mClip;
> +    bool mDidSelfCopy;    

Trailing whitespace

Comment 10

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/950dfd7a4ead
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8fa7087936

Comment 11

[Nick Cameron [:nrc]]

and out again: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef057726724

Comment 12

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa070938b018
https://hg.mozilla.org/integration/mozilla-inbound/rev/acb5541fdea6

Comment 13

[Nick Cameron [:nrc]]

and a fixup: https://hg.mozilla.org/integration/mozilla-inbound/rev/b668ebb1d700

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa070938b018
https://hg.mozilla.org/mozilla-central/rev/acb5541fdea6
https://hg.mozilla.org/mozilla-central/rev/b668ebb1d700