Closed Bug 1170189 Opened 5 years ago Closed 5 years ago

Simplify the tiling code

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: nical, Assigned: nical)

Details

Attachments

(6 files, 3 obsolete files)

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.
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)
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 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-
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?
Attachment #8614113 - Flags: review?(bgirard) → review+
(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.
Attachment #8613555 - Flags: review?(jmuizelaar) → review?(bgirard)
Attachment #8613555 - Flags: review?(bgirard) → review+
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 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 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).
Whiteboard: [leave-open]
61 additions
305 removals
\o/
Attachment #8625863 - Flags: review?(bgirard)
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)
Attachment #8616649 - Flags: checkin+
Attachment #8614113 - Flags: checkin+
Attachment #8613555 - Flags: checkin+
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)
Attachment #8625890 - Attachment is patch: true
Attached patch fix commentsSplinter Review
I mixed up two comments while rebasing one of the earlier patches on this bug.
Attachment #8625964 - Flags: review?(bgirard)
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 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 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 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+
(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).
Note that you can use relative include paths for things you don't want to export. Like #include "../ipc/foo.h"
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.
Flags: needinfo?(nical.bugzilla)
(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)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
You need to log in before you can comment on or make changes to this bug.