Closed
Bug 749063
Opened 12 years ago
Closed 12 years ago
Improve progressive tile drawing order/priority
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: BenWa, Assigned: cwiiis)
References
Details
Attachments
(3 files, 2 obsolete files)
30.00 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
6.10 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
4.19 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Summary: Tile Priority Drawing → Improve progressive tile drawing order/priority
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 years ago
|
||
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•12 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•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
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•12 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•12 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
Comment 12•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•