771219 - Tile base progressive painting

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

Tile base progressive painting is a pre-requisite for priority tiled base drawing. The design is:
- Validate the buffer in groups of tiles (1,3,9).
- Send the progress as individual transactions.
- Have the compositor receive these transactions, upload and present it as it arrives.
- If the layers are not fully painted repeat the transaction imediate rather then discarding the display list.

The benefits are:
- Tiles start appearing much faster to user.
- The compositor can start to upload while content is still drawing tiles.
- We COULD (this needs another bug) abort painting between the group of tiles safely if we decide that we're no longer painting something useful.
- This provides a pre-requesite for efficient drawing at high and low resolution. This is required to replace the screenshot.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Copy the displaylist for repeating paints

I know we discussed this patch before. Whats the most representative way to measure the performance impact of this patch? If needed I can make this change android only.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Repeatable transaction

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Use Repeatable Transaction

Comment 4

[Benoit Girard (:BenWa)]

Attachment: Use Repeatable Transaction

Forgot the mResolution changes

Comment 5

[Ali Juma [:ajuma]]

Comment on attachment 639393 [details] [diff] [review]
Repeatable transaction

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

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +974,5 @@
>    ForwardTransaction();
>    if (mShadowTarget) {
>      ShadowLayerForwarder::ShadowDrawToTarget(mShadowTarget);
>      mShadowTarget = nsnull;
>    }

When we have a repeat transaction, we shouldn't call ShadowDrawToTarget right away (since this will cause an incomplete draw to the target). Instead, we should wait until no further repeating is needed, and then call ShadowDrawToTarget. (So, essentially, move the code above to an 'else' case of the code below.)

Comment 6

[Ali Juma [:ajuma]]

Comment on attachment 639417 [details] [diff] [review]
Use Repeatable Transaction

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

Nice work!

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +244,5 @@
> +    //             user interaction events.
> +    int paintTileStartX = mTiledBuffer.RoundDownToTileEdge(rect->x);
> +    int paintTileStartY = mTiledBuffer.RoundDownToTileEdge(rect->y);
> +
> +    nsIntRegion maxPaint(nsIntRect(paintTileStartX, paintTileStartY, 768, 768));

Use 3*GetTileLength() instead of 768.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 639390 [details] [diff] [review]
Copy the displaylist for repeating paints

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

I think instead of this, at the end of DrawThebesLayer you can re-get the ThebesLayerItemsEntry and just swap the elements back into place.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It should also be documented somewhere that it's OK to call DrawThebesLayer more than once.

Comment 9

[Benoit Girard (:BenWa)]

Talos results are looking great:
Changeset f5dd704b112f

http://graphs.mozilla.org/graph.html#tests=[[204,23,20]]&sel=none&displayrange=7&datatype=running

I'm seeing some bad results in dhtml:
First run was +100%, modified run was +8%
http://graphs.mozilla.org/graph.html#tests=[[19,23,20]]

I think we're pushing pixels to the screen faster, uploading in parallel but overall we block the main thread for longer so I expect Talos to reflect this. I'm going to land this with a pref since it's easy to maintain a non progressive paint code path.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: Restore the display items

Comment 11

[Benoit Girard (:BenWa)]

Attachment: Repeatable transaction

Comment 12

[Benoit Girard (:BenWa)]

Comment on attachment 639725 [details] [diff] [review]
Restore the display items

Made the wrong patch obsolete.

Comment 13

[Benoit Girard (:BenWa)]

Attachment: Use Repeatable Transaction

Carry forward r=ajuma

Comment 14

[Benoit Girard (:BenWa)]

Attachment: Add a pref

I didn't add this to the pref.js on purpose.

Comment 15

[Benoit Girard (:BenWa)]

I'm going to be landing this pref'ed off shortly. The reason is because it will overplay the talos improvement. This is because the first progressive paint we update the viewport and this makes the front end and the Talos measurement think that we're done painting the page when we've only done a single tile.

The solution is to compute the region that we have painted and pass that to the front end.

Comment 16

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c9802cfef5
https://hg.mozilla.org/integration/mozilla-inbound/rev/265258de61ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5cdfedc324d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a4ee4b39ca3

I meant to disable by default within part 4 but forgot:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02207487d5e5

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Looks like this was breaking opt R2 and R3 as well.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b7c9802cfef5
https://hg.mozilla.org/mozilla-central/rev/265258de61ec
https://hg.mozilla.org/mozilla-central/rev/b5cdfedc324d
https://hg.mozilla.org/mozilla-central/rev/6a4ee4b39ca3
https://hg.mozilla.org/mozilla-central/rev/02207487d5e5

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Leaving open based on comments 15 and 17.

Comment 20

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6a4ee4b39ca3
https://tbpl.mozilla.org/php/getParsedLog.php?id=13332343&tree=Mozilla-Inbound&full=1

Comment 22

[Benoit Girard (:BenWa)]

Attachment: Turn this on by default

Hoping to land this by default on Friday.

Comment 23

[Benoit Girard (:BenWa)]

Attachment: Turn this on by default

Comment 24

[Benoit Girard (:BenWa)]

Comment on attachment 667148 [details] [diff] [review]
Turn this on by default

Tracked in bug 795259

Comment 25

[Benoit Girard (:BenWa)]

This feature is working, moving to track the blocking work over to bug 795259.