Closed Bug 749063 Opened 8 years ago Closed 7 years ago

Improve progressive tile drawing order/priority

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: BenWa, Assigned: cwiiis)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

We want to start drawing tiles using a priority system. This will let us support a larger displayports while being able to fill the content of the screen faster by prioritizing certain tiles in a partial update.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Summary: Tile Priority Drawing → Improve progressive tile drawing order/priority
Blocks: 795259
This replaces shouldAbort with a more generic callback that provides more contextual data to the layer (specifically, current viewport and zoom).
Attachment #670740 - Flags: review?(bgirard)
Attached patch Prioritise visible tiles (obsolete) — Splinter Review
I think this works, and the numbers it gets after the transforms all look correct, but I've not tested it extensively yet. It will also break if an intermediate surface is used.

I'm not sure what situations end up with intermediate surface to know if it's worth fixing, or just bailing out in that case - will investigate.
Attachment #670741 - Flags: feedback?(bgirard)
Note, this also is pretty dumb about tile boundaries, so you end up with tiles that get updated twice... Will fix that too.
Attached patch Prioritise visible tiles (obsolete) — Splinter Review
Fix some silly, obvious bugs - shouldn't be crashy/cause endless update loops anymore.
Attachment #670741 - Attachment is obsolete: true
Attachment #670741 - Flags: feedback?(bgirard)
Attachment #670751 - Flags: feedback?(bgirard)
Comment on attachment 670740 [details] [diff] [review]
Replace shouldAbortProgressiveUpdate call with ProgressiveUpdateCallback

Looks good
Attachment #670740 - Flags: review?(bgirard) → review+
I couldn't craft a page that would trigger intermediate surfaces, but I think this code will work. It's not a big deal if it doesn't, but if we discover that, we should revisit this.
Attachment #670751 - Attachment is obsolete: true
Attachment #670751 - Flags: feedback?(bgirard)
Attachment #670826 - Flags: review?(bgirard)
Comment on attachment 670826 [details] [diff] [review]
Prioritise visible tiles

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

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +379,4 @@
>        BasicManager()->SetRepeatTransaction();
>  
>        // Make sure that tiles that fall outside of the visible region are discarded.
>        mValidRegion.And(mValidRegion, mVisibleRegion);

Your patch looks good.
Unrelated: I can't remember why we only do this in this branch and not the 'else' block. This appears wrong to me.
Attachment #670826 - Flags: review?(bgirard) → review+
(In reply to Benoit Girard (:BenWa) from comment #7)
> Unrelated: I can't remember why we only do this in this branch and not the
> 'else' block. This appears wrong to me.

It only actually needs to be done on the first transaction, but we don't currently have a way of singling that transaction out, so we do it for all but the last - it could just as well be outside of that block though.
This simplifies the tile drawing code by making it region-based instead of relying on properties of nsIntRegion and using rect iteration. It is less efficient than the previous code, but the previous code no longer worked with the prioritisation patch.
Attachment #670850 - Flags: review?(bgirard)
Comment on attachment 670850 [details] [diff] [review]
Simplify/robustify tile drawing order

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

r+ with some nits

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +351,5 @@
> +    // Find a tile to draw.
> +    nsIntRect tileBounds(startX, startY,
> +                         mTiledBuffer.GetTileLength(),
> +                         mTiledBuffer.GetTileLength());
> +    for(;;) {

This should be while (true) and have a comment explaining why this isn't an infinite loop. Here's my reasoning, edit to your likings:

// This will always terminate because there must be at least
// one tile in the area we are iterating over otherwise
// the bound would have been smaller.
while (true)

@@ +352,5 @@
> +    nsIntRect tileBounds(startX, startY,
> +                         mTiledBuffer.GetTileLength(),
> +                         mTiledBuffer.GetTileLength());
> +    for(;;) {
> +      regionToPaint.And(invalidRegion, tileBounds);

Can we add a debug assertion that we still intersection the paint bounds in case for a sanity check? A small logic error and we're in an infinite loop. I don't think it's possible since I don't think we could get here with an empty region.
Attachment #670850 - Flags: review?(bgirard) → review+
You need to log in before you can comment on or make changes to this bug.