If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Improve progressive tile drawing order/priority

RESOLVED FIXED in mozilla19

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: BenWa, Assigned: cwiiis)

Tracking

(Blocks: 1 bug)

unspecified
mozilla19
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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)

Updated

5 years ago
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Summary: Tile Priority Drawing → Improve progressive tile drawing order/priority
(Assignee)

Updated

5 years ago
Blocks: 795259
(Assignee)

Comment 1

5 years ago
Created attachment 670740 [details] [diff] [review]
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).
Attachment #670740 - Flags: review?(bgirard)
(Assignee)

Comment 2

5 years ago
Created attachment 670741 [details] [diff] [review]
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.
Attachment #670741 - Flags: feedback?(bgirard)
(Assignee)

Comment 3

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

Comment 4

5 years ago
Created attachment 670751 [details] [diff] [review]
Prioritise visible tiles

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)
(Reporter)

Comment 5

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

Looks good
Attachment #670740 - Flags: review?(bgirard) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 670826 [details] [diff] [review]
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.
Attachment #670751 - Attachment is obsolete: true
Attachment #670751 - Flags: feedback?(bgirard)
Attachment #670826 - Flags: review?(bgirard)
(Reporter)

Comment 7

5 years ago
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+
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
Created attachment 670850 [details] [diff] [review]
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.
Attachment #670850 - Flags: review?(bgirard)
(Reporter)

Comment 10

5 years ago
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+
(Assignee)

Comment 11

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/f8114d23854f
https://hg.mozilla.org/mozilla-central/rev/9706761f3533
https://hg.mozilla.org/mozilla-central/rev/1c267806256f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.