Last Comment Bug 771219 - Tile base progressive painting
: Tile base progressive painting
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Benoit Girard (:BenWa)
:
: Milan Sreckovic [:milan]
Mentors:
: 751629 (view as bug list)
Depends on: 775848 800041 800475 800483 800944
Blocks: 749062 check2 783368 794130 794200 794465 796107 796117 796939 797187 803390
  Show dependency treegraph
 
Reported: 2012-07-05 10:32 PDT by Benoit Girard (:BenWa)
Modified: 2013-05-31 07:36 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Copy the displaylist for repeating paints (1.13 KB, patch)
2012-07-05 10:33 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Repeatable transaction (2.07 KB, patch)
2012-07-05 10:39 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Use Repeatable Transaction (4.89 KB, patch)
2012-07-05 11:34 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Use Repeatable Transaction (6.43 KB, patch)
2012-07-05 11:43 PDT, Benoit Girard (:BenWa)
ajuma.bugzilla: review+
Details | Diff | Splinter Review
Restore the display items (1.79 KB, patch)
2012-07-06 10:57 PDT, Benoit Girard (:BenWa)
roc: review+
Details | Diff | Splinter Review
Repeatable transaction (2.16 KB, patch)
2012-07-06 11:00 PDT, Benoit Girard (:BenWa)
ajuma.bugzilla: review+
Details | Diff | Splinter Review
Use Repeatable Transaction (6.51 KB, patch)
2012-07-06 11:37 PDT, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review
Add a pref (3.22 KB, patch)
2012-07-06 11:40 PDT, Benoit Girard (:BenWa)
ajuma.bugzilla: review+
Details | Diff | Splinter Review
Turn this on by default (3.22 KB, patch)
2012-10-02 13:28 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Turn this on by default (1.02 KB, patch)
2012-10-02 13:50 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-07-05 10:32:22 PDT
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) 2012-07-05 10:33:58 PDT
Created attachment 639390 [details] [diff] [review]
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) 2012-07-05 10:39:03 PDT
Created attachment 639393 [details] [diff] [review]
Repeatable transaction
Comment 3 Benoit Girard (:BenWa) 2012-07-05 11:34:45 PDT
Created attachment 639412 [details] [diff] [review]
Use Repeatable Transaction
Comment 4 Benoit Girard (:BenWa) 2012-07-05 11:43:07 PDT
Created attachment 639417 [details] [diff] [review]
Use Repeatable Transaction

Forgot the mResolution changes
Comment 5 Ali Juma [:ajuma] 2012-07-05 13:41:00 PDT
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] 2012-07-05 13:55:39 PDT
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) 2012-07-05 17:59:58 PDT
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) 2012-07-05 18:24:52 PDT
It should also be documented somewhere that it's OK to call DrawThebesLayer more than once.
Comment 9 Benoit Girard (:BenWa) 2012-07-06 08:48:32 PDT
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) 2012-07-06 10:57:54 PDT
Created attachment 639725 [details] [diff] [review]
Restore the display items
Comment 11 Benoit Girard (:BenWa) 2012-07-06 11:00:17 PDT
Created attachment 639727 [details] [diff] [review]
Repeatable transaction
Comment 12 Benoit Girard (:BenWa) 2012-07-06 11:28:22 PDT
Comment on attachment 639725 [details] [diff] [review]
Restore the display items

Made the wrong patch obsolete.
Comment 13 Benoit Girard (:BenWa) 2012-07-06 11:37:10 PDT
Created attachment 639739 [details] [diff] [review]
Use Repeatable Transaction

Carry forward r=ajuma
Comment 14 Benoit Girard (:BenWa) 2012-07-06 11:40:58 PDT
Created attachment 639741 [details] [diff] [review]
Add a pref

I didn't add this to the pref.js on purpose.
Comment 15 Benoit Girard (:BenWa) 2012-07-07 08:35:46 PDT
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 17 Ryan VanderMeulen [:RyanVM] 2012-07-08 10:39:17 PDT
Looks like this was breaking opt R2 and R3 as well.
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-07-08 17:51:20 PDT
Leaving open based on comments 15 and 17.
Comment 21 Benoit Girard (:BenWa) 2012-09-02 21:49:41 PDT
*** Bug 751629 has been marked as a duplicate of this bug. ***
Comment 22 Benoit Girard (:BenWa) 2012-10-02 13:28:40 PDT
Created attachment 667135 [details] [diff] [review]
Turn this on by default

Hoping to land this by default on Friday.
Comment 23 Benoit Girard (:BenWa) 2012-10-02 13:50:21 PDT
Created attachment 667148 [details] [diff] [review]
Turn this on by default
Comment 24 Benoit Girard (:BenWa) 2012-10-15 07:59:09 PDT
Comment on attachment 667148 [details] [diff] [review]
Turn this on by default

Tracked in bug 795259
Comment 25 Benoit Girard (:BenWa) 2012-10-15 07:59:37 PDT
This feature is working, moving to track the blocking work over to bug 795259.

Note You need to log in before you can comment on or make changes to this bug.