749063 - Improve progressive tile drawing order/priority

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

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.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Replace shouldAbortProgressiveUpdate call with ProgressiveUpdateCallback

This replaces shouldAbort with a more generic callback that provides more contextual data to the layer (specifically, current viewport and zoom).

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Prioritise visible tiles

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.

Comment 3

[Chris Lord [:cwiiis]]

Note, this also is pretty dumb about tile boundaries, so you end up with tiles that get updated twice... Will fix that too.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Prioritise visible tiles

Fix some silly, obvious bugs - shouldn't be crashy/cause endless update loops anymore.

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 670740 [details] [diff] [review]
Replace shouldAbortProgressiveUpdate call with ProgressiveUpdateCallback

Looks good

Comment 6

[Chris Lord [:cwiiis]]

Attachment: Prioritise visible tiles

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.

Comment 7

[Benoit Girard (:BenWa)]

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.

Comment 8

[Chris Lord [:cwiiis]]

(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.

Comment 9

[Chris Lord [:cwiiis]]

Attachment: Simplify/robustify tile drawing order

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.

Comment 10

[Benoit Girard (:BenWa)]

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.

Comment 11

[Chris Lord [:cwiiis]]

Pushed with nits addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8114d23854f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9706761f3533
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c267806256f

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f8114d23854f
https://hg.mozilla.org/mozilla-central/rev/9706761f3533
https://hg.mozilla.org/mozilla-central/rev/1c267806256f