Use a draw target in ThebesLayerBuffer::PaintState

RESOLVED FIXED in mozilla29

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

(Blocks: 1 bug)

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 825639 [details] [diff] [review]
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).
(Assignee)

Comment 1

4 years ago
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.
Assignee: nobody → ncameron
Flags: needinfo?(nical.bugzilla)
(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
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 3

4 years ago
Blocking bug 946567 because I want to reduce the amount of locking and flushing we do, and this makes it possible.
Blocks: 946567
(Assignee)

Comment 4

4 years ago
Created attachment 8346305 [details] [diff] [review]
Preqrequisites - Matrix::HasNonIntegerTranslation and SetAntialiasingFlags
Attachment #8346305 - Flags: review?(matt.woodrow)
(Assignee)

Comment 5

4 years ago
Created attachment 8346307 [details] [diff] [review]
Change PaintState to use a DrawTarget instead of a gfxContext
Attachment #825639 - Attachment is obsolete: true
Attachment #8346307 - Flags: review?(matt.woodrow)
Attachment #8346305 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8346307 - Flags: review?(matt.woodrow) → review+
(Assignee)

Updated

4 years ago
Depends on: 950550
(Assignee)

Comment 8

4 years ago
Created attachment 8348322 [details] [diff] [review]
Change PaintState to use a DrawTarget instead of a gfxContextTLB_thebes.patch

Quite a lot has changed, so requesting re-review.
Attachment #8346307 - Attachment is obsolete: true
Attachment #8348322 - Flags: review?(matt.woodrow)
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
Attachment #8348322 - Flags: review?(matt.woodrow) → review+
(Assignee)

Updated

4 years ago
Blocks: 951556
(Assignee)

Updated

4 years ago
No longer blocks: 946567
https://hg.mozilla.org/mozilla-central/rev/aa070938b018
https://hg.mozilla.org/mozilla-central/rev/acb5541fdea6
https://hg.mozilla.org/mozilla-central/rev/b668ebb1d700
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.