Closed
Bug 1170189
Opened 9 years ago
Closed 9 years ago
Simplify the tiling code
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
People
(Reporter: nical, Assigned: nical)
Details
Attachments
(6 files, 3 obsolete files)
2.49 KB,
patch
|
jrmuizel
:
review+
nical
:
checkin+
|
Details | Diff | Splinter Review |
19.56 KB,
patch
|
BenWa
:
review+
nical
:
checkin+
|
Details | Diff | Splinter Review |
9.97 KB,
patch
|
BenWa
:
review+
nical
:
checkin+
|
Details | Diff | Splinter Review |
27.97 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
2.05 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
29.00 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
The tiling code evolved very poorly and we are in the process of enabling it on more platform. Now is a good time to clean it up. I'll start with some simplification in the compositor-side code following up on bug 1150549, and get to the content-side code soon after that.
Assignee | ||
Comment 1•9 years ago
|
||
if GetFormat is called before SetCompositor, we crash in the X11 TextureHost. This fixes it and simplifies the branches in UseTileTexture.
Attachment #8613555 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 2•9 years ago
|
||
This simplifies greatly reorganizing tiles because we have a TilesPlacement for the old tile buffer and one for the new one, and we this lets us go from on to the other easily since TileIntPoint is in the same coordinate space for both TilesPlacement. In followup patches I will move more code to us this rather than compute indices manually, but this will involve changing a bit how we iterate over tiles so I am splitting this over several patches.
Attachment #8614034 -
Flags: review?(bgirard)
Comment 3•9 years ago
|
||
Comment on attachment 8614034 [details] [diff] [review] Use TilesPlacement to handle position <-> index conversion. Review of attachment 8614034 [details] [diff] [review]: ----------------------------------------------------------------- r-, let's get a better comment for that class. ::: gfx/layers/TiledLayerBuffer.h @@ +115,5 @@ > +typedef gfx::IntSizeTyped<TileUnit> TileIntSize; > +typedef gfx::IntPointTyped<TileUnit> TileIntPoint; > + > +/** > + * Handles the computaion to swtch between tile indices and positions, either in computation* but that's redundant. switch* This comment doesn't really do a good job at describing what this class does. This does more than just mappings, his stores the origin and the size of the tile buffer.
Attachment #8614034 -
Flags: review?(bgirard) → review-
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8614034 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8614113 -
Flags: review?(bgirard)
Comment on attachment 8613555 [details] [diff] [review] fix crash on Linux Review of attachment 8613555 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/TiledContentHost.cpp @@ +111,5 @@ > const IntRect& aUpdateRect, > TextureHost* aNewTexture, > Compositor* aCompositor) > { > + MOZ_ASSERT(aNewTexture); This assert is more there to show us when this happens, rather than actually asserting that it will never happen, right? You may want to consider using gfxCriticalError() inside the if (!aNewTexture) which will give us a message, an assert, and a trail in release builds. If we are actually interested in when this happens? @@ +118,5 @@ > } > + > + if (aTexture) { > + aTexture->SetCompositor(aCompositor); > + aNewTexture->SetCompositor(aCompositor); We want to do this even if aCompositor is null? @@ +132,2 @@ > > + Seems a bit awkward that we start using aTexture instead of aNewTexture, unconditionally? Makes it a bit harder to read, why aren't we just using aNewTexture in all the code below instead of doing aTexture = aNewTexture; and then using aTexture?
Updated•9 years ago
|
Attachment #8614113 -
Attachment is patch: true
Updated•9 years ago
|
Attachment #8614113 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #5) > ::: gfx/layers/composite/TiledContentHost.cpp > @@ +111,5 @@ > > const IntRect& aUpdateRect, > > TextureHost* aNewTexture, > > Compositor* aCompositor) > > { > > + MOZ_ASSERT(aNewTexture); > > This assert is more there to show us when this happens, rather than actually > asserting that it will never happen, right? You may want to consider using > gfxCriticalError() inside the if (!aNewTexture) which will give us a > message, an assert, and a trail in release builds. > If we are actually interested in when this happens? hm, if aNewTexture is null it means the ipdl message was malformed. If we want to catch it, it's probably best to test against it in the caller and return false if it happens (which will kill the child process). > > + aNewTexture->SetCompositor(aCompositor); > > We want to do this even if aCompositor is null? The calling function asserts that aCompsoitor is not null and returns false if it ever happens in a release build so aCompositor can never be null here. > Seems a bit awkward that we start using aTexture instead of aNewTexture, > unconditionally? Makes it a bit harder to read, why aren't we just using > aNewTexture in all the code below instead of doing aTexture = aNewTexture; > and then using aTexture? aTexture is &tile.mTextureHost[OnWhite], so my thinking was "assign the new texture to the tile and work with the tile". it's what makes most sense the way I think about the flow but I don't have a strong opinion. I can do aTexture = aNewTexture and use aNewTexture in the rest of this function if you feel strongly about it.
Assignee | ||
Updated•9 years ago
|
Attachment #8613555 -
Flags: review?(jmuizelaar) → review?(bgirard)
Updated•9 years ago
|
Attachment #8613555 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 7•9 years ago
|
||
Less code overall, much less computation and juggling around coordinate spaces, and renders the background color before going through the tiles to avoid switching back and forth between the two shaders.
Attachment #8616649 -
Flags: review?(bgirard)
Comment 8•9 years ago
|
||
Comment on attachment 8616649 [details] [diff] [review] Simplify the tile composition code Review of attachment 8616649 [details] [diff] [review]: ----------------------------------------------------------------- Nice
Attachment #8616649 -
Flags: review?(bgirard) → review+
Comment 9•9 years ago
|
||
Comment on attachment 8616649 [details] [diff] [review] Simplify the tile composition code Review of attachment 8616649 [details] [diff] [review]: ----------------------------------------------------------------- Nice :) Fly-by comment. ::: gfx/layers/composite/TiledContentHost.cpp @@ +503,5 @@ > aVisibleRegion.ScaleRoundOut(layerScale.width, layerScale.height); > } > > + // Make sure the high precision buffer masks the low precision buffer out to > + // avoid overdraw and rendering artifacts with non-opaque layers. This comment has high and low mixed up (The low precision buffer masks the high precision out, not the other way round).
Assignee | ||
Updated•9 years ago
|
Whiteboard: [leave-open]
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e8d13cb9fdb https://hg.mozilla.org/integration/mozilla-inbound/rev/615a3fe84722 https://hg.mozilla.org/integration/mozilla-inbound/rev/9afdfcdabd4f
https://hg.mozilla.org/mozilla-central/rev/9e8d13cb9fdb https://hg.mozilla.org/mozilla-central/rev/615a3fe84722 https://hg.mozilla.org/mozilla-central/rev/9afdfcdabd4f
Assignee | ||
Comment 12•9 years ago
|
||
61 additions 305 removals \o/
Attachment #8625863 -
Flags: review?(bgirard)
Assignee | ||
Comment 13•9 years ago
|
||
TiledLayerComposer is used in few enough places that it's not really worth having this interface to hide TiledContentHost. Using TiledContentHost directly is just as simple and removes code.
Attachment #8625890 -
Flags: review?(bgirard)
Assignee | ||
Updated•9 years ago
|
Attachment #8616649 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8614113 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8613555 -
Flags: checkin+
Assignee | ||
Comment 14•9 years ago
|
||
In the previous version of the patch I had removed a test (along with the code that it tested). Since removing tests doesn't look good, I added a test for TilesPlacement which contains the most important logic that is used on both the Client and host sides, and represents more or less the part that was tested previously.
Attachment #8625863 -
Attachment is obsolete: true
Attachment #8625863 -
Flags: review?(bgirard)
Attachment #8625962 -
Flags: review?(bgirard)
Assignee | ||
Updated•9 years ago
|
Attachment #8625890 -
Attachment is patch: true
Assignee | ||
Comment 15•9 years ago
|
||
I mixed up two comments while rebasing one of the earlier patches on this bug.
Attachment #8625964 -
Flags: review?(bgirard)
Assignee | ||
Comment 16•9 years ago
|
||
Sorry for the churn, there was a mistake in the test.
Attachment #8625962 -
Attachment is obsolete: true
Attachment #8625962 -
Flags: review?(bgirard)
Attachment #8625971 -
Flags: review?(bgirard)
Comment 17•9 years ago
|
||
Comment on attachment 8625971 [details] [diff] [review] simplify the client-side tiling code Review of attachment 8625971 [details] [diff] [review]: ----------------------------------------------------------------- Consider using mozreview for your own benefit (./mach mercurial-setup will set it up). It's really handy for patch queues and will let you push to try easily. ::: gfx/layers/client/TiledContentClient.cpp @@ +1125,5 @@ > + const TileIntPoint tilePosition = oldTiles.TilePosition(oldIndex); > + const size_t newIndex = newTiles.TileIndex(tilePosition); > + // First, get the already existing tiles to the right place in the new array. > + // Leave placeholders (default constructor) where there was no tile. > + if (!newTiles.HasTile(tilePosition)) { nit: Can you invert the logic here. It will flow better with your comment and remove the negation: if (newTiles.HasTile(tilePosition)) { <reuse> } else { <release> } ::: gfx/tests/gtest/TestTiledLayerBuffer.cpp @@ +25,3 @@ > > + const TilesPlacement p1 = TilesPlacement(firstX, firstY, width, height); > + // Check that HasTile returns fals with some positions that we know false
Attachment #8625971 -
Flags: review?(bgirard) → review+
Comment 18•9 years ago
|
||
Comment on attachment 8625890 [details] [diff] [review] Remove a bunch of unused methods and the TiledLayerComposer interface. Review of attachment 8625890 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/CompositableTransactionParent.cpp @@ +21,5 @@ > #include "mozilla/layers/LayersSurfaces.h" // for SurfaceDescriptor > #include "mozilla/layers/LayersTypes.h" // for MOZ_LAYERS_LOG > #include "mozilla/layers/TextureHost.h" // for TextureHost > #include "mozilla/layers/TextureHostOGL.h" // for TextureHostOGL > +#include "mozilla/layers/TiledContentHost.h" You should be able to import this without mozilla/layers here and remove the moz.build export, just like we do for 'CompositableHost.h'. If that wasn't possible I'm not sure what our policy is for using the local include vs. export. I'd imagine it's if you only need it at one place then use local include. ::: gfx/layers/moz.build @@ +130,5 @@ > 'composite/ImageLayerComposite.h', > 'composite/LayerManagerComposite.h', > 'composite/PaintedLayerComposite.h', > 'composite/TextureHost.h', > + 'composite/TiledContentHost.h', We shouldn't be exporting *Host includes to the outside without a good reason. They are layers implementation details. Looks like someone else let TextureHost.h slip by already.
Attachment #8625890 -
Flags: review?(bgirard) → review+
Comment 19•9 years ago
|
||
Comment on attachment 8625964 [details] [diff] [review] fix comments Review of attachment 8625964 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/TiledContentHost.cpp @@ +499,5 @@ > gfx::Size layerScale(1, 1); > > + // We assume that the current frame resolution is the one used in our high > + // precision layer buffer. Compensate for a changing frame resolution when > + // rendering the low precision buffer. nsIntRegion maskRegion; maskRegion made it into the comment somehow.
Attachment #8625964 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 20•9 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #18) > We shouldn't be exporting *Host includes to the outside without a good > reason. They are layers implementation details. Looks like someone else let > TextureHost.h slip by already. I agree in principle. The reason they are currently exported is for the layers/ipc directory to see them. Perhaps we should move the files in layers/ipc to be in the client/ and composite/ directories (and rename "composite/" into "host/" in the process). This way we could remove the export of most (all?) of the *Host.h headers (I'd rather do that as a followup bug).
Comment 21•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a4d42725bd0 https://hg.mozilla.org/integration/mozilla-inbound/rev/cc37d935a68c https://hg.mozilla.org/integration/mozilla-inbound/rev/46fe95bd71fe
Comment 22•9 years ago
|
||
Note that you can use relative include paths for things you don't want to export. Like #include "../ipc/foo.h"
Comment 23•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2a4d42725bd0 https://hg.mozilla.org/mozilla-central/rev/cc37d935a68c https://hg.mozilla.org/mozilla-central/rev/46fe95bd71fe
Comment 24•9 years ago
|
||
Comment on attachment 8614113 [details] [diff] [review] Use TilesPlacement to handle position <-> index conversion v2 Review of attachment 8614113 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/TiledLayerBuffer.h @@ +42,5 @@ > # define TILING_LOG(...) > #endif > > +// Normal integer division truncates towards zero, > +// we instead want to floor to hangle negative numbers. "hangle" seems like a typo.
Updated•9 years ago
|
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #24) > "hangle" seems like a typo. Indeed. That was copy-pasted from the previous place the comment was at. There's more cleanup to do so I'll fix it in the next patch.
Flags: needinfo?(nical.bugzilla)
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
You need to log in
before you can comment on or make changes to this bug.
Description
•