Simplify mValidRegion after painting

RESOLVED INVALID

Status

()

RESOLVED INVALID
6 years ago
3 years ago

People

(Reporter: BenWa, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.
Attachment #674413 - Flags: review?(chrislord.net)
(Reporter)

Updated

6 years ago
Attachment #674413 - Attachment is patch: true

Comment 1

6 years ago
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.
Attachment #674413 - Flags: review?(chrislord.net) → review+
(Reporter)

Updated

6 years ago
Blocks: 796117
(Reporter)

Comment 2

6 years ago
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
Is this patch still valid? Should it be landed?
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.