804801 - Simplify mValidRegion after painting

Status: RESOLVED INVALID

Description

[Benoit Girard (:BenWa)]

Created attachment 674413 [details] [diff] [review]
patch

I've seen mValidRegion get very complex and this happens very frequently with progressive drawing. Here's a typical 16 part mValidRegion that is identical to mVisibleRegion after the paint:

I/Gecko   (29153): mValidRegion [513,768,2305,1280; 513,1280,1024,1536; 1024,1280,1792,3072; 1792,1280,2305,1536; 513,1536,1024,1792; 1792,1536,2305,1792; 513,1792,1024,2048; 1792,1792,2305,2048; 513,2048,1024,2304; 1792,2048,2305,2304; 513,2304,1024,2560; 1792,2304,2305,2560; 513,2560,1024,2816; 1792,2560,2305,2816; 513,2816,1024,3072; 1792,2816,2305,3072]
I/Gecko   (29153): mVisibleRegion [513,768,2305,3072]

The trick is simple, assign mVisibleRegion to mValidRegion after a progressive paint to simplify the region.

Comment 1

[Chris Lord [:cwiiis]]

Comment on attachment 674413 [details] [diff] [review]
patch

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

Well caught - nit about the comment, but nicely done.

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +358,5 @@
>      // The transaction is completed, store the last scroll offset.
>      mLastScrollOffset = aScrollOffset;
> +    NS_ASSERTION(mValidRegion.IsEqual(mVisibleRegion),
> +                 "valid region doesn't match visible region after progressive paint.");
> +    // mVisibleRegion is equal to mValidRegion but is a simpler reduce so assigning it here

Maybe just because it's late, but 'is a simpler reduce' doesn't read well to me - s/reduce/region/? or reduction? Or perhaps just

// mVisibleRegion is equal to mValidRegion, but simpler, so assign it to the
// latter to avoid unnecessarily complex regions.

Comment 2

[Benoit Girard (:BenWa)]

Pushed to try since I added an assert and I'm not running under debug to catch it.
https://tbpl.mozilla.org/?tree=Try&rev=a8585e6b5622

Comment 3

[Kartikaya Gupta (email:[e-mail]) (parental leave)]

Is this patch still valid? Should it be landed?