739679 - Add a Shadowable TiledThebesLayer implementation

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Benoit Girard (:BenWa)]

The goal of this bug is to provide an alternate implementation for shadowable layers that uses tiling instead of layers. Using a tiled buffer has a few advantages over the current implementation of buffer rotation:
- Buffer rotation hits cases where it must throw away the retained data (dirty region crosses the rotation boundary), this is very expensive.
- Tiled buffer can retain a complex, non rectangular, area.
- Tiled buffer don't need to memcpy into a temporary buffer with some drivers.
- Tiled buffer upload efficiently without using glTexSubImage2D.
- Tiled buffer allow the compositor to perform its own caching and decide to merge the current and new valid region efficiently.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: WIP

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 609765 [details] [diff] [review]
WIP

Review of attachment 609765 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxASurface.h
@@ +42,5 @@
>  #include "gfxRect.h"
>  #include "nsAutoPtr.h"
>  #include "nsAutoRef.h"
>  #include "nsThreadUtils.h"
> +#define MOZ_DUMP_PAINTING

--enable-dump-painting in your mozconfig :)

Comment 3

[Benoit Girard (:BenWa)]

I didn't want to clobber build

Comment 4

[Benoit Girard (:BenWa)]

Attachment: WIP (v2)

Improved performance by using the invalidate region rather then it's bounds.

Comment 5

[Joe Drew (not getting mail)]

This is the sort of bug that we probably need before beta if we're going to get it at all.

Comment 6

[Benoit Girard (:BenWa)]

+1 for beta blocker.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: WIP (v3)

This implementation now handles complex region and should be ready to build against.

Comment 8

[Benoit Girard (:BenWa)]

Attachment: WIP (v4)

Fixed leaks: gfxImageSurface/Textures

Comment 9

[Benoit Girard (:BenWa)]

Attachment: WIP (v5)

Support thebes layer that don't get a shadow copy by returning a basicthebeslayer for now.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: WIP (v6)

Significant performance improvement.
TODO: support RGBA, fix memory leak, move upload off EndTransaction

Comment 11

[Benoit Girard (:BenWa)]

Attachment: Part 1: TiledLayerBuffer + build changes

Comment 12

[Benoit Girard (:BenWa)]

Attachment: Part 2: BasicLayers Changes

Comment 13

[Benoit Girard (:BenWa)]

Attachment: Part 3: OGL Layers changes

Comment 14

[Benoit Girard (:BenWa)]

Comment on attachment 614560 [details] [diff] [review]
Part 1: TiledLayerBuffer + build changes

From the class description:
+// An abstract implementation of a tile buffer.
+// This code covers the logic of moving and reusing
+// tiles and leaves the validation up to the implementor.

Comment 15

[Benoit Girard (:BenWa)]

Comment on attachment 614563 [details] [diff] [review]
Part 2: BasicLayers Changes

I refactored BasicImplData out so that I could put BasicTiledThebesLayer into it's own file for simplicity.

This layer types isn't meant to be used outside ShadowableLayers and thus doesn't touch it's drawing target. This solve the problem of compositing in the main thread and should be a good step forward towards layers that separate validation and composition.

I don't support RGBA32 surfaces but that should be easy. Starting on that now.

Comment 16

[Benoit Girard (:BenWa)]

Attachment: Part 2.5: IPC Changes

Comment 17

[Benoit Girard (:BenWa)]

Attachment: Part 4: RGBA Support

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 614560 [details] [diff] [review]
Part 1: TiledLayerBuffer + build changes

Review of attachment 614560 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/TiledLayerBuffer.h
@@ +38,5 @@
> +    mRetainedTiles = o.mRetainedTiles;
> +    mRetainedWidth = o.mRetainedWidth;
> +    mRetainedHeight = o.mRetainedHeight;
> +    return *this;
> +  }

How is this different from the default copy constructor? It seems unnecessary.

@@ +44,5 @@
> +  T GetTile(const nsIntPoint& aTileOrigin) const;
> +  T GetTile(int x, int y) const;
> +  nsRegion GetTileRegion(int x, int y) const;
> +
> +  uint16_t GetTileSize() const { return 256; }

Can we make this a define/enum value instead of a magical constant?

@@ +78,5 @@
> +  int           mRetainedHeight; // in tiles
> +
> +private:
> +
> +  TiledLayerBuffer(const TiledLayerBuffer&);

MOZ_DELETE

@@ +101,5 @@
> +{
> +  // TODO Cache offsetX/offsetY
> +  int offsetX = (mValidRegion.GetBounds().x - mValidRegion.GetBounds().x % GetTileSize()) / GetTileSize();
> +  int offsetY = (mValidRegion.GetBounds().y - mValidRegion.GetBounds().y % GetTileSize()) / GetTileSize();
> +  int index = (aTileOrigin.x / GetTileSize() - offsetX) * mRetainedHeight + aTileOrigin.y / GetTileSize() - offsetY;

This logic isn't very easy to follow, comments would help.

Maybe just make this compute the tile x/y values from the origin and then call the other GetTile overload?

@@ +117,5 @@
> +void
> +TiledLayerBuffer<T>::update(const nsIntRegion& aNewValidRegion, const nsIntRegion& aInvalidateRegion)
> +{
> +  nsTArray< T >  newRetainedTiles;
> +  nsTArray< T >& oldRetainedTiles = mRetainedTiles;

Can't we just operate directly on mRetainedTiles here instead of making a copy?

I can't see anything that touches it until we overwrite it at the end.

@@ +124,5 @@
> +  const nsIntPoint oldBufferOrigin(oldBound.x - oldBound.x % GetTileSize(), oldBound.y - oldBound.y % GetTileSize());
> +  const nsIntPoint newBufferOrigin(newBound.x - newBound.x % GetTileSize(), newBound.y - newBound.y % GetTileSize());
> +  const nsIntRegion& oldValidRegion = mValidRegion;
> +  const nsIntRegion& newValidRegion = aNewValidRegion;
> +  const int oldRetainedHeight = mRetainedHeight;

Same with a few other variables here, I can't see any reason we need to copy mValidRegion, mRetainedHeight, aNewValidRegion etc.

@@ +132,5 @@
> +  // TODO: Add a tile pool to reduce new allocation
> +  int tileX = 0;
> +  int tileY;
> +  for (int x = newBound.x; x < newBound.x + newBound.width;) {
> +    int w = GetTileSize() - x % GetTileSize();

Call this width?

@@ +140,5 @@
> +    tileY = 0;
> +    for (int y = newBound.y; y < newBound.y + newBound.height;) {
> +      int h = GetTileSize() - y % GetTileSize();
> +      if (y + h > newBound.y + newBound.height)
> +        h = newBound.y + newBound.height - y;

This loop logic isn't very easy to understand. I don't have any ideas though, so maybe just a few comments?

@@ +148,5 @@
> +        int tileX = (x - oldBufferOrigin.x) / GetTileSize();
> +        int tileY = (y - oldBufferOrigin.y) / GetTileSize();
> +        int index = tileX * oldRetainedHeight + tileY;
> +
> +        NS_ABORT_IF_FALSE(oldRetainedTiles.SafeElementAt(index, GetPlaceholderTile()) != GetPlaceholderTile(), "Expected tile");

We could use GetTile() here if we are using mRetainedTiles

@@ +153,5 @@
> +
> +        T tileWithPartialValidContent = oldRetainedTiles[index];
> +        newRetainedTiles.AppendElement(tileWithPartialValidContent);
> +        // Flag the tile as used, but don't remove it since
> +        // we need to leave the buffer not to impact the lookup index.

'since we need to maintain the buffer indexes' ?

Comment 19

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 614563 [details] [diff] [review]
Part 2: BasicLayers Changes

Review of attachment 614563 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/basic/BasicLayers.cpp
@@ +3132,5 @@
> +    return layer.forget();
> +  }
> +  nsRefPtr<BasicTiledThebesLayer> layer =
> +    new BasicTiledThebesLayer(this);
> +  printf("Create\n\n\n\n\n\n\n");

Debugging printf.

How about:

#ifdef FORCE_BASICTILEDTHEBESLAYER
if (HasShadowManager() {
  // create tiled thebes layer

} else
#endif
{
  //create normal thebes layer
}

To avoid duplicating code.

::: gfx/layers/basic/BasicLayers.h
@@ +277,5 @@
>  
>    LayerRefArray mKeepAlive;
>  };
>  
> +class BasicShadowableThebesLayer;

Can you put all the moving code into a separate patch (one that applies/builds without the rest).

It'd be better to have that separate from new code.

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +68,5 @@
> +  long start = PR_IntervalNow();
> +#endif
> +  if (UseSinglePaintBuffer()) {
> +    const nsIntRect bounds = aInvalidateRegion.GetBounds();
> +    mSinglePaintBuffer = new gfxImageSurface(gfxIntSize(bounds.width, bounds.height), gfxASurface::ImageFormatRGB16_565);

We probably want to try recycle these don't we?

@@ +97,5 @@
> +  }
> +  start = PR_IntervalNow();
> +#endif
> +
> +  update(aNewValidRegion, aInvalidateRegion);

Capital U?

@@ +124,5 @@
> +#ifdef GFX_TILEDLAYER_PREF_WARNINGS
> +    printf_stderr("Complex region\n");
> +    for (const nsIntRect* rect = it.Next(); rect != NULL; rect = it.Next()) {
> +      printf_stderr(" break into subrect %i, %i, %i, %i\n", rect->x, rect->y, rect->width, rect->height);
> +      aTile = validateTile(aTile, aTileOrigin, nsIntRegion(*rect));

Should this really be within the #ifdef?

@@ +133,5 @@
> +
> +
> +  if (aTile == GetPlaceholderTile()) {
> +    gfxImageSurface* tmpTile = new gfxImageSurface(gfxIntSize(GetTileSize(), GetTileSize()), gfxASurface::ImageFormatRGB16_565);
> +    aTile = BasicTiledLayerTile(tmpTile);

I'd prefer if you didn't modify the input parameter, and made a local var to store this.

@@ +136,5 @@
> +    gfxImageSurface* tmpTile = new gfxImageSurface(gfxIntSize(GetTileSize(), GetTileSize()), gfxASurface::ImageFormatRGB16_565);
> +    aTile = BasicTiledLayerTile(tmpTile);
> +  }
> +
> +  gfxRect drawRect(gfxPoint(aDirtyRegion.GetBounds().x-aTileOrigin.x, aDirtyRegion.GetBounds().y-aTileOrigin.y), gfxSize(aDirtyRegion.GetBounds().width, aDirtyRegion.GetBounds().height));

Split this across multiple lines (do you need the gfxPoint/gfxSize constructor? just passing 4 parameters would work fine)

@@ +192,5 @@
> +    return;
> +  }
> +
> +  if (!HasShadow()) {
> +    printf_stderr("No shadow paint\n");

NS_WARNING/ERROR?

::: gfx/layers/basic/BasicTiledThebesLayer.h
@@ +72,5 @@
> +  }
> +
> +  void ReadUnlock() {
> +    for (size_t i = 0; i < mRetainedTiles.Length(); i++) {
> +      if (mRetainedTiles[i] == GetPlaceholderTile()) continue;

newline before continue

Comment 20

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 614563 [details] [diff] [review]
Part 2: BasicLayers Changes

Review of attachment 614563 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/basic/BasicTiledThebesLayer.h
@@ +70,5 @@
> +  {
> +    return mPlaceholder;
> +  }
> +
> +  void ReadUnlock() {

What actually calls this? I can't see it anywhere.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 614564 [details] [diff] [review]
Part 3: OGL Layers changes

Review of attachment 614564 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +987,5 @@
>    , LayerOGL(aManager)
>    , mUploadTask(nsnull)
>  {
> +#ifdef FORCE_BASICTILEDTHEBESLAYER
> +  abort();

NS_ABORT

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +16,5 @@
> +    return;
> +
> +  mContext->MakeCurrent();
> +  for (size_t i = 0; i < mRetainedTiles.Length(); i++) {
> +    if (mRetainedTiles[i] == GetPlaceholderTile()) continue;

newline

@@ +24,5 @@
> +
> +void
> +TiledLayerBufferOGL::ReleaseTile(TiledTexture aTile)
> +{
> +  if (aTile == GetPlaceholderTile()) return;

\n

@@ +25,5 @@
> +void
> +TiledLayerBufferOGL::ReleaseTile(TiledTexture aTile)
> +{
> +  if (aTile == GetPlaceholderTile()) return;
> +  mContext->fDeleteTextures(1, &aTile);

Don't we need to MakeCurrent() before calling this? (at least in theory, it might not matter with context sharing)

@@ +34,5 @@
> +                            const BasicTiledLayerBuffer* aMainMemoryTiledBuffer,
> +                            const nsIntRegion& aNewValidRegion,
> +                            const nsIntRegion& aInvalidateRegion)
> +{
> +  printf_stderr("Upload %i, %i, %i, %i\n", aInvalidateRegion.GetBounds().x, aInvalidateRegion.GetBounds().y, aInvalidateRegion.GetBounds().width, aInvalidateRegion.GetBounds().height);

Lots of debugging printfs, #ifdef these out?

@@ +39,5 @@
> +  long start = PR_IntervalNow();
> +  mMainMemoryTiledBuffer = aMainMemoryTiledBuffer;
> +  mContext->MakeCurrent();
> +  update(aNewValidRegion, aInvalidateRegion);
> +  mMainMemoryTiledBuffer = NULL;

nsnull

@@ +105,5 @@
> +
> +  mVideoMemoryTiledBuffer.Upload(this, &mMainMemoryTiledBuffer, mMainMemoryTiledBuffer.GetValidRegion(), mRegionToUpload);
> +  mValidRegion = mVideoMemoryTiledBuffer.GetValidRegion();
> +
> +  mMainMemoryTiledBuffer.ReadUnlock();

Found it! Ignore my previous question.

@@ +131,5 @@
> +    for( size_t y = visibleRect.y; y < visibleRect.y + visibleRect.height;) {
> +      uint16_t tileStartY = y % mVideoMemoryTiledBuffer.GetTileSize();
> +      uint16_t h = mVideoMemoryTiledBuffer.GetTileSize() - tileStartY;
> +      if (y + h > visibleRect.y + visibleRect.height)
> +        h = visibleRect.y + visibleRect.height - y;

Again with the scarily complicated loop logic. I wonder if we can write an iterator helper here, will think about it.

::: gfx/layers/opengl/TiledThebesLayerOGL.h
@@ +36,5 @@
> +              const BasicTiledLayerBuffer* aMainMemoryTiledBuffer,
> +              const nsIntRegion& aNewValidRegion,
> +              const nsIntRegion& aInvalidateRegion);
> +
> +  virtual TiledTexture GetPlaceholderTile() const { return NULL; }

nsnull

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 614560 [details] [diff] [review]
Part 1: TiledLayerBuffer + build changes

Review of attachment 614560 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Makefile.in
@@ +66,5 @@
>          LayerManagerOGL.h \
>          LayerManagerOGLProgram.h \
>          ReadbackLayer.h \
> +        BasicTiledThebesLayer.h \
> +        BasicImplData.h \

Why are you exporting these? Also, why are you exporting these in this patch? Each patch-queue prefix should build, and this patch won't.

::: gfx/layers/TiledLayerBuffer.h
@@ +7,5 @@
> +
> +// Debug defines
> +#define FORCE_BASICTILEDTHEBESLAYER
> +//#define GFX_TILEDLAYER_DEBUG_OVERLAY
> +//#define GFX_TILEDLAYER_PREF_WARNINGS

Clean these up

@@ +19,5 @@
> +
> +// An abstract implementation of a tile buffer.
> +// This code covers the logic of moving and reusing
> +// tiles and leaves the validation up to the implementor.
> +template <class T>

Need more comments here. For example, explain what T is, and what interface T needs to support.

@@ +42,5 @@
> +  }
> +
> +  T GetTile(const nsIntPoint& aTileOrigin) const;
> +  T GetTile(int x, int y) const;
> +  nsRegion GetTileRegion(int x, int y) const;

Document these.

@@ +55,5 @@
> +
> +  // A temporary placeholder tile used as a marker. This placeholder tile must
> +  // never be returned by validateTile and must be == to every instance
> +  // of a placeholder tile.
> +  virtual T GetPlaceholderTile() const = 0;

Instead of making this virtual, would it make sense for T to have a static method ::Placeholder() or something like that?

@@ +62,5 @@
> +  // The implementor should call update() to change
> +  // the new valid region. This implementation will call
> +  // validateTile on each tile that is dirty, which is left
> +  // to the implementor.
> +  void update(const nsIntRegion& aNewValidRegion, const nsIntRegion& aInvalidateRegion);

Update

@@ +66,5 @@
> +  void update(const nsIntRegion& aNewValidRegion, const nsIntRegion& aInvalidateRegion);
> +
> +  // The implementor will give an implementation that will validate
> +  // the dirtyRect. The return'ed T will replace the tile.
> +  virtual T validateTile(T aTile, const nsIntPoint& aTileOrigin, const nsIntRegion& aDirtyRect) = 0;

ValidateTile

@@ +72,5 @@
> +  virtual void ReleaseTile(T aTile) = 0;
> +
> +  nsIntRegion   mValidRegion;
> +  nsIntRegion   mLastPaintRegion;
> +  nsTArray< T > mRetainedTiles;

Lose spaces around T

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 614563 [details] [diff] [review]
Part 2: BasicLayers Changes

Review of attachment 614563 [details] [diff] [review]:
-----------------------------------------------------------------

Can you split part 2 into a patch that just moves the existing code out into the header file, and a separate patch that makes your real changes?

Comment 24

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> @@ +117,5 @@
> > +void
> > +TiledLayerBuffer<T>::update(const nsIntRegion& aNewValidRegion, const nsIntRegion& aInvalidateRegion)
> > +{
> > +  nsTArray< T >  newRetainedTiles;
> > +  nsTArray< T >& oldRetainedTiles = mRetainedTiles;
> 
> Can't we just operate directly on mRetainedTiles here instead of making a
> copy?
> 
> I can't see anything that touches it until we overwrite it at the end.

Most of these are only an alias (const X&). I just found it much easier to reason about the code with names such as oldValidRegion/newValidRegion then aValidRegion/mValidRegion. AFAIK there's no run time costs to this. If you object I'll gladly do the replacement.


(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> > +
> > +  // A temporary placeholder tile used as a marker. This placeholder tile must
> > +  // never be returned by validateTile and must be == to every instance
> > +  // of a placeholder tile.
> > +  virtual T GetPlaceholderTile() const = 0;
> 
> Instead of making this virtual, would it make sense for T to have a static
> method ::Placeholder() or something like that?
> 

I don't think so. The idea was to allow tiling types that shouldn't know that they are being tiled such as gfxSharedImageSurface and gl textures. I've been thinking of storing T in a wrapper class in TiledLayerBuffer. That would solve this problem and let us track more metadata under #DEBUG.

Comment 25

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> > +  virtual TiledTexture GetPlaceholderTile() const { return NULL; }
> 
> nsnull

Our coding style is NULL instead of nsnull in new code:
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#C.2FC.2B.2B_practices

Do we overwrite this rule?

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You could use a traits class to provide the extra functionality. E.g. TileTraits<T>::Placeholder().

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Benoit Girard (:BenWa) from comment #25)
> Our coding style is NULL instead of nsnull in new code:
> https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#C.2FC.2B.
> 2B_practices
> 
> Do we overwrite this rule?

I don't think that rule is correct...

Comment 28

[Benoit Girard (:BenWa)]

I'll use nsnull and I'll post on dev.platform so that we can get this rule clarified.

Comment 29

[Benoit Girard (:BenWa)]

Attachment: Part 1: TiledLayerBuffer

Comment 30

[Benoit Girard (:BenWa)]

Attachment: Part 2: BasicLayers Refactor

Comment 31

[Benoit Girard (:BenWa)]

Attachment: Part 3: BasicTiledThebesLayer

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 614893 [details] [diff] [review]
Part 2.5: IPC Changes

We need this code to work across processes.  Let's discuss a non-pointer-sharing impl.

Comment 33

[Patrick Walton (:pcwalton)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #32)
> Comment on attachment 614893 [details] [diff] [review]
> Part 2.5: IPC Changes
> 
> We need this code to work across processes.  Let's discuss a
> non-pointer-sharing impl.

I think that shouldn't block this landing for now. Fennec desperately needs this.

Comment 34

[Patrick Walton (:pcwalton)]

Furthermore, I build on this work to share textures/EGLImageKHR across threads, and there's no way to make that work across processes. So this will never be totally IPC safe.

Comment 35

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Removing the raw pointer here may not be much work.  I suspect it shouldn't be.  That's why I'd like to discuss it.

EGLImageKHR not working across processes is fine, we don't take that path when going cross-process.  We only care about that configuration on b2g and we'll have gralloc powers there.

Comment 36

[Benoit Girard (:BenWa)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #35)
> Removing the raw pointer here may not be much work.  I suspect it shouldn't
> be.  That's why I'd like to discuss it.
> 
> EGLImageKHR not working across processes is fine, we don't take that path
> when going cross-process.  We only care about that configuration on b2g and
> we'll have gralloc powers there.

I'm starting the work to make them IPC safe right now otherwise it will keep getting pushed back, however I'd like to land this as-is and address as a follow in a few short days (really!).

Comment 37

[Benoit Girard (:BenWa)]

Attachment: part 4: TiledThebesLayerOGL

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 615525 [details] [diff] [review]
Part 1: TiledLayerBuffer

Review of attachment 615525 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/TiledLayerBuffer.h
@@ +22,5 @@
> +// An abstract implementation of a tile buffer.
> +// This code covers the logic of moving and reusing
> +// tiles and leaves the validation up to the implementor.
> +// class T can be any type as long as the  Derived class
> +// can validate and return tiles of that type.

I guess you should say that T should be a reference type or something else that can be cheaply copied.

@@ +56,5 @@
> +
> +  // A temporary placeholder tile used as a marker. This placeholder tile must
> +  // never be returned by validateTile and must be == to every instance
> +  // of a placeholder tile.
> +  virtual T GetPlaceholderTile() const = 0;

What happened to the traits class idea?

@@ +69,5 @@
> +  // The implementor will give an implementation that will validate
> +  // the dirtyRect. The return'ed T will replace the tile.
> +  virtual T ValidateTile(T aTile, const nsIntPoint& aTileOrigin, const nsIntRegion& aDirtyRect) = 0;
> +
> +  virtual void ReleaseTile(T aTile) = 0;

Having templates *and* virtual functions still seems wrong. We should be able to avoid having virtual functions here.

@@ +75,5 @@
> +  nsIntRegion   mValidRegion;
> +  nsIntRegion   mLastPaintRegion;
> +  nsTArray<T>   mRetainedTiles;
> +  int           mRetainedWidth;  // in tiles
> +  int           mRetainedHeight; // in tiles

Can you document the invariants here?

For example it's totally unclear how mRetainedTiles is organised, or what mRetainedWidth and mRetainedHeight are for.

Basically you need comments explaining how this actually works.

@@ +92,5 @@
> +  /**
> +   * Update the current retained layer with the
> +   * update layer data.
> +   */
> +  virtual void PaintedTiledLayerBuffer(const BasicTiledLayerBuffer* aTiledBuffer) = 0;

What's the role of this class? Please document.

@@ +97,5 @@
> +};
> +
> +template <class T>
> +T
> +TiledLayerBuffer<T>::GetTile(const nsIntPoint& aTileOrigin) const

I think 'template' and 'T' could go on the same line.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 615527 [details] [diff] [review]
Part 2: BasicLayers Refactor

Review of attachment 615527 [details] [diff] [review]:
-----------------------------------------------------------------

My review is enough for this.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 615583 [details] [diff] [review]
Part 3: BasicTiledThebesLayer

Review of attachment 615583 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/basic/BasicLayers.cpp
@@ +3128,5 @@
> +  if (!HasShadowManager()) {
> +    nsRefPtr<BasicShadowableThebesLayer> layer =
> +      new BasicShadowableThebesLayer(this);
> +    MAYBE_CREATE_SHADOW(Thebes);
> +    return layer.forget();

This is confusing. The code for !HasShadowManager() is the same as for HasShadowManager()?

::: gfx/layers/basic/BasicTiledThebesLayer.h
@@ +14,5 @@
> +namespace mozilla {
> +namespace layers {
> +
> +struct BasicTiledLayerTile {
> +  nsRefPtr<gfxReusableSurfaceWrapper> mSurface;

Needs documentation!

@@ +56,5 @@
> +  }
> +};
> +
> +class BasicTiledThebesLayer;
> +class BasicTiledLayerBuffer : public TiledLayerBuffer<BasicTiledLayerTile>

Needs documentation!

@@ +105,5 @@
> +};
> +
> +class BasicTiledThebesLayer : public ThebesLayer,
> +                              public BasicImplData,
> +                              public BasicShadowableLayer

Needs documentation!

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 614898 [details] [diff] [review]
Part 4: RGBA Support

Review of attachment 614898 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like you have to part 4s now

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +137,5 @@
>    }
>  
>  
> +  gfxImageFormat imgFormat = mThebesLayer->CanUseOpaqueSurface() ? gfxASurface::ImageFormatRGB16_565
> +                                                                 : gfxASurface::ImageFormatARGB32;

Create a helper function that returns the right format so we don't have to repeat this expression.

@@ +143,2 @@
>    if (aTile == GetPlaceholderTile()) {
> +    gfxImageSurface* tmpTile = new gfxImageSurface(gfxIntSize(GetTileSize(), GetTileSize()), imgFormat);

We should probably name GetTileSize to GetTileLength or something to indicate that it returns a scalar, not a size. Or, we should just make it return a gfxIntSize. (Could it possibly make sense to allow non-square tiles?)

::: gfx/layers/ipc/CompositorParent.cpp
@@ +187,4 @@
>  #ifdef COMPOSITOR_PERFORMANCE_WARNING
>      mExpectedComposeTime = mozilla::TimeStamp::Now() + TimeDuration::FromMilliseconds(15 - delta.ToMilliseconds());
>  #endif
> +    MessageLoop::current()->PostDelayedTask(FROM_HERE, mCurrentCompositeTask, 15 - delta.ToMilliseconds());

Looks like this change doesn't belong in this patch.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 615823 [details] [diff] [review]
part 4: TiledThebesLayerOGL

Review of attachment 615823 [details] [diff] [review]:
-----------------------------------------------------------------

Would it make sense to merge this with the RGBA support patch? The latter patch changes some fundamental things here.

::: gfx/layers/opengl/TiledThebesLayerOGL.h
@@ +18,5 @@
> +}
> +
> +namespace layers {
> +
> +using mozilla::gl::GLContext;

Can't use 'using' in header files.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 614893 [details] [diff] [review]
Part 2.5: IPC Changes

Review of attachment 614893 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this needs my review.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +187,4 @@
>  #ifdef COMPOSITOR_PERFORMANCE_WARNING
>      mExpectedComposeTime = mozilla::TimeStamp::Now() + TimeDuration::FromMilliseconds(15 - delta.ToMilliseconds());
>  #endif
> +    MessageLoop::current()->PostDelayedTask(FROM_HERE, mCurrentCompositeTask, 30 - delta.ToMilliseconds());

uh, this cancels out the change in the other patch?

Comment 44

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> Comment on attachment 614893 [details] [diff] [review]
> Part 2.5: IPC Changes
> 
> Review of attachment 614893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think this needs my review.
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +187,4 @@
> >  #ifdef COMPOSITOR_PERFORMANCE_WARNING
> >      mExpectedComposeTime = mozilla::TimeStamp::Now() + TimeDuration::FromMilliseconds(15 - delta.ToMilliseconds());
> >  #endif
> > +    MessageLoop::current()->PostDelayedTask(FROM_HERE, mCurrentCompositeTask, 30 - delta.ToMilliseconds());
> 
> uh, this cancels out the change in the other patch?

Right, I just tried to undo the change and it got lump into the wrong patch.

Comment 45

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 615583 [details] [diff] [review]
Part 3: BasicTiledThebesLayer

Review of attachment 615583 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/basic/BasicLayers.cpp
@@ +3128,5 @@
> +  if (!HasShadowManager()) {
> +    nsRefPtr<BasicShadowableThebesLayer> layer =
> +      new BasicShadowableThebesLayer(this);
> +    MAYBE_CREATE_SHADOW(Thebes);
> +    return layer.forget();

I assume the test is meant to be reversed here, and it should create a tiled layer instead.

This may be a bit annoying, but you could create an intermediate class inheriting from both ThebesLayer and BasicShadowableLayer, and have BasicShadowableThebesLayer and BasicTiledThebesLayer both inherit that.

Then the refptr, MAYBE_CREATE_SHADOW and return lines can be shared outside of the branch. Suggestion only.

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +126,5 @@
> +    for (const nsIntRect* rect = it.Next(); rect != nsnull; rect = it.Next()) {
> +      printf_stderr(" break into subrect %i, %i, %i, %i\n", rect->x, rect->y, rect->width, rect->height);
> +      aTile = ValidateTile(aTile, aTileOrigin, nsIntRegion(*rect));
> +    }
> +#endif

#ifdef in wrong place

@@ +193,5 @@
> +  }
> +
> +  if (!HasShadow()) {
> +    printf_stderr("No shadow paint\n");
> +    return;

NS_ASSERTION

Comment 46

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 615823 [details] [diff] [review]
part 4: TiledThebesLayerOGL

Review of attachment 615823 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +25,5 @@
> +
> +void
> +TiledLayerBufferOGL::ReleaseTile(TiledTexture aTile)
> +{
> +  // We've made current pior to calling TiledLayerBufferOGL::update

'We've called MakeCurrent() on mGLContext/our GL context' 

prior

@@ +43,5 @@
> +  long start = PR_IntervalNow();
> +#endif
> +  mMainMemoryTiledBuffer = aMainMemoryTiledBuffer;
> +  mContext->MakeCurrent();
> +  update(aNewValidRegion, aInvalidateRegion);

Update

::: gfx/layers/opengl/TiledThebesLayerOGL.h
@@ +38,5 @@
> +              const nsIntRegion& aInvalidateRegion);
> +
> +  virtual TiledTexture GetPlaceholderTile() const { return nsnull; }
> +protected:
> +  virtual TiledTexture validateTile(TiledTexture aTile,

ValidateTile

Comment 47

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> 
> Can you document the invariants here?
> 
> For example it's totally unclear how mRetainedTiles is organised, or what
> mRetainedWidth and mRetainedHeight are for.
> 
> Basically you need comments explaining how this actually works.
> 

  /**
   * mRetainedTiles is a rectangular buffer of mRetainedWidth x mRetainedWidth
   * stored as column major with the same origin as mValidRegion.GetBounds().
   * Any tile that does not intersect mValidRegion is a PlaceholderTile.
   * Only the region intersecting with mValidRegion should be read from a tile,
   * another other region is assumed to be uninitialized.
   */

> @@ +92,5 @@
> > +  /**
> > +   * Update the current retained layer with the
> > +   * update layer data.
> > +   */
> > +  virtual void PaintedTiledLayerBuffer(const BasicTiledLayerBuffer* aTiledBuffer) = 0;
> 
> What's the role of this class? Please document.
> 

  /**
   * Update the current retained layer with the updated layer data.
   * The BasicTiledLayerBuffer is expected to be in the ReadLock state
   * prior to this being called. aTiledBuffer is copy constructed and
   * is retained until it has been uploaded/copyed and unlocked.
   */

Comment 48

[Patrick Walton (:pcwalton)]

(In reply to Benoit Girard (:BenWa) from comment #47)
>   /**
>    * mRetainedTiles is a rectangular buffer of mRetainedWidth x
> mRetainedWidth

Do you mean mRetainedWidth * mRetainedHeight?

Comment 49

[Benoit Girard (:BenWa)]

fixed

Comment 50

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40)
> Comment on attachment 615583 [details] [diff] [review]
> Part 3: BasicTiledThebesLayer
> 
> Review of attachment 615583 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicLayers.cpp
> @@ +3128,5 @@
> > +  if (!HasShadowManager()) {
> > +    nsRefPtr<BasicShadowableThebesLayer> layer =
> > +      new BasicShadowableThebesLayer(this);
> > +    MAYBE_CREATE_SHADOW(Thebes);
> > +    return layer.forget();
> 
> This is confusing. The code for !HasShadowManager() is the same as for
> HasShadowManager()?

I messed this up when using matt's suggestion, fixed now.

> 
> ::: gfx/layers/basic/BasicTiledThebesLayer.h
> @@ +14,5 @@
> > +namespace mozilla {
> > +namespace layers {
> > +
> > +struct BasicTiledLayerTile {
> > +  nsRefPtr<gfxReusableSurfaceWrapper> mSurface;
> 
> Needs documentation!

/**
 * Represent a single tile in tiled buffer. It's backed
 * by a gfxReusableSurfaceWrapper that implements a
 * copy-on-write mechanism while locked. The tile should be
 * locked before being sent to the compositor and unlocked
 * as soon as it is uploaded to prevent a copy.
 * Ideal place to store per tile debug information.
 */

> 
> @@ +56,5 @@
> > +  }
> > +};
> > +
> > +class BasicTiledThebesLayer;
> > +class BasicTiledLayerBuffer : public TiledLayerBuffer<BasicTiledLayerTile>
> 
> Needs documentation!

/**
 * Provide an instance of TiledLayerBuffer backed by image surfaces.
 * This buffer provides an implementation to ValidateTile using a
 * thebes callback and can support painting using a single paint buffer
 * which is much faster then painting directly into the tiles.
 */

> 
> @@ +105,5 @@
> > +};
> > +
> > +class BasicTiledThebesLayer : public ThebesLayer,
> > +                              public BasicImplData,
> > +                              public BasicShadowableLayer
> 
> Needs documentation!

/**
 * An implementation of ThebesLayer that ONLY supports remote
 * composition that is backed by tiles. This thebes layer implementation
 * is better suited to mobile hardware to work around slow implementation
 * of glTexImage2D (for OGL compositors), and restrait memory bandwidth.
 */

Comment 51

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> @@ +143,2 @@
> >    if (aTile == GetPlaceholderTile()) {
> > +    gfxImageSurface* tmpTile = new gfxImageSurface(gfxIntSize(GetTileSize(), GetTileSize()), imgFormat);
> 
> We should probably name GetTileSize to GetTileLength or something to
> indicate that it returns a scalar, not a size. Or, we should just make it
> return a gfxIntSize. (Could it possibly make sense to allow non-square
> tiles?)
> 

I can't see of any use case for non square tiles, if you think there is a use case you should let me know so I can fix it before other things that will build on top of tiling assume square tiles.

Comment 52

[Benoit Girard (:BenWa)]

Attachment: Part 1: TiledLayerBuffer

Note that I haven't fixed GetPlaceholder and the virtual calls yet

Comment 53

[Benoit Girard (:BenWa)]

Attachment: Part 3: BasicTiledThebesLayer

Comment 54

[Benoit Girard (:BenWa)]

Attachment: Part 4: TiledThebesLayerOGL + RGBA Support

Comment 55

[Benoit Girard (:BenWa)]

Attachment: Part 5: Remove memset for 565 surfaces (feature from bug 746344 on inbound)

This gives us a 5-8% drawing performance win.

Comment 56

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 616387 [details] [diff] [review]
Part 5: Remove memset for 565 surfaces (feature from bug 746344 on inbound)

Review of attachment 616387 [details] [diff] [review]:
-----------------------------------------------------------------

Can you land the profiler label changes separately please :)

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +70,5 @@
>                                     const nsIntRegion& aInvalidateRegion,
>                                     LayerManager::DrawThebesLayerCallback aCallback,
>                                     void* aCallbackData)
>  {
> +  SAMPLE_LABEL("BasicTiledLayerBuffer", "PaintThebesSingleBuffer");

Wrong name? We're not necessarily doing SingleBuffer here.

Comment 57

[Benoit Girard (:BenWa)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #56)
> Comment on attachment 616387 [details] [diff] [review]
> Part 5: Remove memset for 565 surfaces (feature from bug 746344 on inbound)
> 
> Review of attachment 616387 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you land the profiler label changes separately please :)

Sure

> 
> ::: gfx/layers/basic/BasicTiledThebesLayer.cpp
> @@ +70,5 @@
> >                                     const nsIntRegion& aInvalidateRegion,
> >                                     LayerManager::DrawThebesLayerCallback aCallback,
> >                                     void* aCallbackData)
> >  {
> > +  SAMPLE_LABEL("BasicTiledLayerBuffer", "PaintThebesSingleBuffer");
> 
> Wrong name? We're not necessarily doing SingleBuffer here.

SingleBuffer means we're using the path were we draw into a temporary single buffer before copying it out into tiles.

Comment 58

[Matt Woodrow (:mattwoodrow)]

(In reply to Benoit Girard (:BenWa) from comment #57)

> > Wrong name? We're not necessarily doing SingleBuffer here.
> 
> SingleBuffer means we're using the path were we draw into a temporary single
> buffer before copying it out into tiles.

Yes, but that label is outside of that if branch.

Comment 59

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 616381 [details] [diff] [review]
Part 3: BasicTiledThebesLayer

Review of attachment 616381 [details] [diff] [review]:
-----------------------------------------------------------------

The RGBA changes relevant to these files should probably be part of this patch.

Comment 60

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 616382 [details] [diff] [review]
Part 4: TiledThebesLayerOGL + RGBA Support

Review of attachment 616382 [details] [diff] [review]:
-----------------------------------------------------------------

I think the gfxReusableSurfaceWrapper changes should be a separate patch, and the BasicTiledThebesLayer changes should be folded into part 3. Rest looks good though!

Comment 61

[Patrick Walton (:pcwalton)]

Comment on attachment 616382 [details] [diff] [review]
Part 4: TiledThebesLayerOGL + RGBA Support

Review of attachment 616382 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +109,5 @@
> +  // TODO: Mark as read-only, copy-on-write
> +  // Keep a reference to the tile buffer and flag it as copy-on-write,
> +  // this lets us move the upload outside the sync transaction.
> +  mMainMemoryTiledBuffer = *mTiledBuffer;
> +  mRegionToUpload = mMainMemoryTiledBuffer.GetLastPaintRegion();

I think you want to this to be mRegionToUpload.Or(mRegionToUpload, mMainMemoryTiledBuffer.GetLastPaintRegion());

That way, when we start performing uploads on demand instead of eagerly, we won't drop uploads if multiple PLayers updates come in per frame.

Comment 62

[Patrick Walton (:pcwalton)]

Attachment: Virtual removal.

This patch removes virtual in favor of the curiously recurring template pattern.

Comment 63

[Patrick Walton (:pcwalton)]

Attachment: Virtual removal, version 2.

Virtual removal version 2 cleans it up a bit.

Comment 64

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 2: Tiled layer buffer.

Part 1 version 2 merges the virtual removal changes.

Comment 65

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 2: Basic tiled Thebes layer.

Merged virtual changes with part 3.

Comment 66

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 2: Tiled OpenGL Thebes layer.

Part 4 version 2 rolls up the virtual changes.

Comment 67

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 616813 [details] [diff] [review]
Proposed patch, part 1, version 2: Tiled layer buffer.

Review of attachment 616813 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/TiledLayerBuffer.h
@@ +21,5 @@
> +
> +// An abstract implementation of a tile buffer. This code covers the logic of
> +// moving and reusing tiles and leaves the validation up to the implementor. To
> +// avoid the overhead of virtual dispatch, we employ the curiously recurring
> +// template pattern.

Say something about the valid and last-paint regions and what they mean. We're still not really saying that this class does.

@@ +94,5 @@
> +  void Update(const nsIntRegion& aNewValidRegion, const nsIntRegion& aInvalidateRegion);
> +
> +  /**
> +   * mRetainedTiles is a rectangular buffer of mRetainedWidth x mRetainedHeight
> +   * stored as column major with the same origin as mValidRegion.GetBounds().

why column-major? That seems like a slightly odd decision to me.

Move this comment next to the declaration of mRetainedTiles.

@@ +135,5 @@
> +  // TODO Cache firstTileOriginX/firstTileOriginY
> +  // Find the tile x/y of the first tile and the target tile relative to the (0, 0)
> +  // origin, the difference is the tile x/y relative to the start of the tile buffer.
> +  int firstTileX = (mValidRegion.GetBounds().x - mValidRegion.GetBounds().x % GetTileLength()) / GetTileLength();
> +  int firstTileY = (mValidRegion.GetBounds().y - mValidRegion.GetBounds().y % GetTileLength()) / GetTileLength();

why aren't these just mValidRegion.GetBounds().x/GetTileLength() etc?

@@ +151,5 @@
> +TiledLayerBuffer<Derived>::Update(const nsIntRegion& aNewValidRegion,
> +                                  const nsIntRegion& aInvalidateRegion)
> +{
> +  nsTArray< Tile >  newRetainedTiles;
> +  nsTArray< Tile >& oldRetainedTiles = mRetainedTiles;

Drop the spaces around Tile

@@ +155,5 @@
> +  nsTArray< Tile >& oldRetainedTiles = mRetainedTiles;
> +  const nsIntRect oldBound = mValidRegion.GetBounds();
> +  const nsIntRect newBound = aNewValidRegion.GetBounds();
> +  const nsIntPoint oldBufferOrigin(oldBound.x - oldBound.x % GetTileLength(), oldBound.y - oldBound.y % GetTileLength());
> +  const nsIntPoint newBufferOrigin(newBound.x - newBound.x % GetTileLength(), newBound.y - newBound.y % GetTileLength());

Let's have a helper method RoundDownToTileEdge(v) that return v - v%GetTileLength().

@@ +168,5 @@
> +  // TODO: Add a tile pool to reduce new allocation
> +  int tileX = 0;
> +  int tileY;
> +  // Iterate over the new drawing bounds in steps of tiles.
> +  for (int x = newBound.x; x < newBound.x + newBound.width;) {

newBound.XMost()

@@ +172,5 @@
> +  for (int x = newBound.x; x < newBound.x + newBound.width;) {
> +    // Compute tileRect(x,y,width,height) in layer space coordinate
> +    // giving us the rect of the tile that hits the newBounds.
> +    int width = GetTileLength() - x % GetTileLength();
> +    if (x + width > newBound.x + newBound.width)

newBound.XMost()

@@ +173,5 @@
> +    // Compute tileRect(x,y,width,height) in layer space coordinate
> +    // giving us the rect of the tile that hits the newBounds.
> +    int width = GetTileLength() - x % GetTileLength();
> +    if (x + width > newBound.x + newBound.width)
> +      width = newBound.x + newBound.width - x;

newBound.XMost()

{} around if body.

it might be clearer to write this as width = NS_MIN(newBounds.XMost(), x + width) - x though.

@@ +176,5 @@
> +    if (x + width > newBound.x + newBound.width)
> +      width = newBound.x + newBound.width - x;
> +
> +    tileY = 0;
> +    for (int y = newBound.y; y < newBound.y + newBound.height;) {

newBound.YMost()

@@ +179,5 @@
> +    tileY = 0;
> +    for (int y = newBound.y; y < newBound.y + newBound.height;) {
> +      int height = GetTileLength() - y % GetTileLength();
> +      if (y + height > newBound.y + newBound.height)
> +        height = newBound.y + newBound.height - y;

as above

@@ +221,5 @@
> +  mRetainedHeight = tileY;
> +
> +  NS_ABORT_IF_FALSE(aNewValidRegion.Contains(aInvalidateRegion), "Invalidating a region outside the visible region");
> +
> +  nsIntRegion regionToPaint(aInvalidateRegion);

Shouldn't this be (oldValidRegion - aInvalidateRegion) AND aNewValidRegion?

@@ +248,5 @@
> +        height = newBound.y + newBound.height - y;
> +
> +      const nsIntRect tileRect(x,y,width,height);
> +      nsIntRegion tileDrawRegion(tileRect);
> +      tileDrawRegion.And(tileDrawRegion, regionToPaint);

tileDrawRegion.And(tileRect, regionToPaint);

@@ +264,5 @@
> +                              != AsDerived().GetPlaceholderTile(),
> +                           "If we don't draw a tile we shouldn't have a placeholder there.");
> +#endif
> +        tileY++;
> +        y += height;

Move tileY++ and y += height into the third clause of the y for-loop. Similar for x, and similar in the previous pass.

@@ +275,5 @@
> +      NS_ABORT_IF_FALSE(index >= 0 && index < newRetainedTiles.Length(), "index out of range");
> +      Tile newTile = newRetainedTiles[index];
> +      while (newTile == AsDerived().GetPlaceholderTile() && oldRetainedTiles.Length() > 0) {
> +        newTile = oldRetainedTiles[oldRetainedTiles.Length()-1];
> +        oldRetainedTiles.RemoveElementAt(oldRetainedTiles.Length()-1);

It might be worth defining a Swap operation on tiles that you can call here (and elsewhere?) to avoid refcount churn.

It might also be worth defining an IsPlaceholder() method on tiles to simplify the code and reduce the number of temporaries floating around.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually I kind of wonder if the traits class is needed. It seems like very little work to just require the Tile type to have all necessary methods, and if a tile is fundamentally some existing simple type, just wrap that up.

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 616815 [details] [diff] [review]
Proposed patch, part 3, version 2: Basic tiled Thebes layer.

Review of attachment 616815 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those fixed.

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +131,5 @@
> +    for (const nsIntRect* rect = it.Next(); rect != nsnull; rect = it.Next()) {
> +#ifdef GFX_TILEDLAYER_PREF_WARNINGS
> +      printf_stderr(" break into subrect %i, %i, %i, %i\n", rect->x, rect->y, rect->width, rect->height);
> +#endif
> +      aTile = ValidateTile(aTile, aTileOrigin, nsIntRegion(*rect));

Would be a bit cleaner to have a ValidateTile(const nsIntRect&) and have the ValidateTile(const nsIntRegion&) simply call that one or more times.

@@ +142,5 @@
> +    gfxImageSurface* tmpTile = new gfxImageSurface(gfxIntSize(GetTileLength(), GetTileLength()), gfxASurface::ImageFormatRGB16_565);
> +    aTile = BasicTiledLayerTile(tmpTile);
> +  }
> +
> +  gfxRect drawRect(gfxPoint(aDirtyRegion.GetBounds().x-aTileOrigin.x, aDirtyRegion.GetBounds().y-aTileOrigin.y), gfxSize(aDirtyRegion.GetBounds().width, aDirtyRegion.GetBounds().height));

do some CSE here please!

@@ +155,5 @@
> +  nsRefPtr<gfxContext> ctxt = new gfxContext(writableSurface);
> +  ctxt->NewPath();
> +  ctxt->SetOperator(gfxContext::OPERATOR_CLEAR);
> +  ctxt->SetDeviceColor(gfxRGBA(0.0, 0.0, 0.0, 0.0));
> +  ctxt->Rectangle(drawRect, true);

I don't think you need to set the color here, do you?

::: gfx/layers/basic/BasicTiledThebesLayer.h
@@ +108,5 @@
> +  }
> +protected:
> +  virtual BasicTiledLayerTile ValidateTile(BasicTiledLayerTile aTile,
> +                                        const nsIntPoint& aTileRect,
> +                                        const nsIntRegion& dirtyRect);

This doesn't need to be virtual. Also, fix indent.

@@ +110,5 @@
> +  virtual BasicTiledLayerTile ValidateTile(BasicTiledLayerTile aTile,
> +                                        const nsIntPoint& aTileRect,
> +                                        const nsIntRegion& dirtyRect);
> +
> +  bool UseSinglePaintBuffer() { return true; }

Document this.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 616816 [details] [diff] [review]
Proposed patch, part 4, version 2: Tiled OpenGL Thebes layer.

Review of attachment 616816 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those fixed.

BTW I'm no expert on GL, but I thought that binding textures was relatively expensive and RenderLayer would be much faster if you stored all the tiles in a single texture so you could draw all tiles in a single draw call?

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +164,5 @@
>    nsRefPtr<gfxContext> ctxt = new gfxContext(writableSurface);
> +  if (!mThebesLayer->CanUseOpaqueSurface()) {
> +    // We need to clear the alpha channel
> +    ctxt->SetOperator(gfxContext::OPERATOR_CLEAR);
> +    ctxt->SetDeviceColor(gfxRGBA(0.0, 0.0, 0.0, 0.0));

I don't think you need to set this color here.

::: gfx/layers/opengl/TiledThebesLayerOGL.h
@@ +20,5 @@
> +namespace layers {
> +
> +class TiledTexture {
> +public:
> +  // Placeholder

Documentation!

::: mobile/android/app/mobile.js
@@ +373,5 @@
>  pref("gfx.displayport.strategy_fm.multiplier", -1); // displayport dimension multiplier
>  pref("gfx.displayport.strategy_fm.danger_x", -1); // danger zone on x-axis when multiplied by viewport width
>  pref("gfx.displayport.strategy_fm.danger_y", -1); // danger zone on y-axis when multiplied by viewport height
>  // velocity bias strategy options
> +pref("gfx.displayport.strategy_vb.multiplier", 3000); // displayport dimension multiplier

This change shouldn't be in this patch.

Comment 71

[Patrick Walton (:pcwalton)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline April 15-18) from comment #68)
> Actually I kind of wonder if the traits class is needed. It seems like very
> little work to just require the Tile type to have all necessary methods, and
> if a tile is fundamentally some existing simple type, just wrap that up.

Not sure what you mean here, sorry... The Tile type will be different depending on the kind of tiled layer buffer Derived is (basic or OGL). If we wanted to get rid of the traits class while keeping only one template parameter, we'd have to have a virtual copy constructor, operator=, and operator== on the Tile. I figured a Traits class was more robust than having two template parameters (one for Derived and one for Tile) that have to be coordinated, but it can be changed if you'd like.

Comment 72

[Patrick Walton (:pcwalton)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline April 15-18) from comment #70)
> BTW I'm no expert on GL, but I thought that binding textures was relatively
> expensive and RenderLayer would be much faster if you stored all the tiles
> in a single texture so you could draw all tiles in a single draw call?

We'd need to use glTexSubImage2D() then to change individual textures, which is unfortunately expensive on most mobile GPUs (it involves readbacks, unlike desktop drivers).

Comment 73

[Patrick Walton (:pcwalton)]

Sorry, to change individual *tiles*.

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Patrick Walton (:pcwalton) from comment #71)
> Not sure what you mean here, sorry... The Tile type will be different
> depending on the kind of tiled layer buffer Derived is (basic or OGL). If we
> wanted to get rid of the traits class while keeping only one template
> parameter, we'd have to have a virtual copy constructor, operator=, and
> operator== on the Tile. I figured a Traits class was more robust than having
> two template parameters (one for Derived and one for Tile) that have to be
> coordinated, but it can be changed if you'd like.

Oh right, I keep forgetting that TiledLayerBuffer can't write
  typedef typename Derived::Tile Tile;
because Derived hasn't been declared where we instantiate TiledLayerBuffer.

When I hit this in MediaStreams I just used two template parameters. I think that's probably the way to go here too.

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Patrick Walton (:pcwalton) from comment #72)
> We'd need to use glTexSubImage2D() then to change individual textures, which
> is unfortunately expensive on most mobile GPUs (it involves readbacks,
> unlike desktop drivers).

Yikes.

Given that, and the fact that drawing into the buffer now requires either an extra copy or many DrawThebesLayer calls, I'm amazed the tiling approach is fast!

Comment 76

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 3: Tiled layer buffer.

Patch part 1, version 3 addresses most of the review comments, but I'll leave the final revisions to Benoit.

Comment 77

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 3: Basic tiled Thebes layer.

Part part 3, version 3 addresses most of the review comments.

Comment 78

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 3: Tiled OpenGL Thebes layer.

Patch part 4, version 3 addresses most of the review comments.

Comment 79

[Benoit Girard (:BenWa)]

Comment on attachment 616884 [details] [diff] [review]
Proposed patch, part 1, version 3: Tiled layer buffer.

Review of attachment 616884 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks pcwalton, this is great :D!

::: gfx/layers/TiledLayerBuffer.h
@@ +24,5 @@
> +// avoid the overhead of virtual dispatch, we employ the curiously recurring
> +// template pattern.
> +//
> +// This tile buffer stores a valid region, which defines the areas that are
> +// still valid and have not changed since the last paint. Tiles within this

The valid region can certainly have changed in the last paint. It is any region from which you can read the tiles and get update to data content. Reading from tiles outside the valid region will you get uninitialized (or stale) content.

Comment 80

[Benoit Girard (:BenWa)]

Comment on attachment 616885 [details] [diff] [review]
Proposed patch, part 3, version 3: Basic tiled Thebes layer.

Review of attachment 616885 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/basic/BasicTiledThebesLayer.h
@@ +115,5 @@
> +  bool UseSinglePaintBuffer() { return true; }
> +
> +  void ReleaseTile(BasicTiledLayerTile aTile) { /* No-op. */ }
> +
> +  void SwapTiles(BasicTiledLayerTile& aTileA, BasicTiledLayerTile& aTileB) {

Why did we add Swap? I don't see it used anywhere.

Comment 81

[Benoit Girard (:BenWa)]

Comment on attachment 616886 [details] [diff] [review]
Proposed patch, part 4, version 3: Tiled OpenGL Thebes layer.

Review of attachment 616886 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +65,5 @@
> +#endif
> +  if (aTile == GetPlaceholderTile()) {
> +    mContext->fGenTextures(1, &aTile.mTextureHandle);
> +    mContext->fBindTexture(LOCAL_GL_TEXTURE_2D, aTile.mTextureHandle);
> +    mContext->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_LINEAR);

I see you took the changes to switch this to LINEAR. This also needs to land with GL_CLAMP_TO_EDGE or we will get lines between the tiles.

@@ +72,5 @@
> +    mContext->fBindTexture(LOCAL_GL_TEXTURE_2D, aTile.mTextureHandle);
> +  }
> +
> +  nsRefPtr<gfxReusableSurfaceWrapper> reusableSurface = mMainMemoryTiledBuffer->GetTile(aTileOrigin).mSurface.get();
> +  GLenum format = reusableSurface->Format() == gfxASurface::ImageFormatRGB16_565 ? LOCAL_GL_RGB

I had moved these into a helper function like roc suggested. Not sure what happened to that.

Comment 82

[Benoit Girard (:BenWa)]

Attachment: part 5: Fix tile jumping

Comment 83

[Benoit Girard (:BenWa)]

Attachment: Part 2.5: IPC Changes

I couldn't think of a good way to check multi-process for run time. Regardless we're only going to be using this layer type for mobile when we enable it (none of the patches here enable it).

Comment 84

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 4: Tiled layer buffer.

Patch part 1, version 4 addresses feedback and remaining review comments.

Comment 85

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 3, version 4: Basic tiled Thebes layer.

Patch part 3, version 4 addresses review and feedback comments.

Comment 86

[Benoit Girard (:BenWa)]

Attachment: Part 2.5: IPC Changes

Comment 87

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 4, version 4: Tiled OpenGL Thebes layer.

Patch part 4, version 4 addresses review comments.

Comment 88

[Patrick Walton (:pcwalton)]

Comment on attachment 617130 [details] [diff] [review]
part 5: Fix tile jumping

Looks good, merged into part 4.

Comment 89

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 617136 [details] [diff] [review]
Proposed patch, part 1, version 4: Tiled layer buffer.

Review of attachment 617136 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/TiledLayerBuffer.h
@@ +194,5 @@
> +    tileY = 0;
> +    for (int y = newBound.y; y < newBound.YMost(); tileY++) {
> +      int height = GetTileLength() - y % GetTileLength();
> +      if (y + height > newBound.y + newBound.height)
> +        height = newBound.y + newBound.height - y;

{}

@@ +237,5 @@
> +  mRetainedHeight = tileY;
> +
> +  NS_ABORT_IF_FALSE(aNewValidRegion.Contains(aInvalidateRegion), "Invalidating a region outside the visible region");
> +
> +  nsIntRegion regionToPaint(aInvalidateRegion);

You missed my comment:
> Shouldn't this be (oldValidRegion - aInvalidateRegion) AND aNewValidRegion?

At least tell me why it isn't :-).

@@ +282,5 @@
> +                                         SafeElementAt(index, AsDerived().GetPlaceholderTile())),
> +                          "If we don't draw a tile we shouldn't have a placeholder there.");
> +#endif
> +        tileY++;
> +        y += height;

Isn't this a bug? You're incrementing tileY twice now.

Also, why not move y+=height and x+=width into the for loop too?

Comment 90

[Patrick Walton (:pcwalton)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline April 22-25) from comment #89)
> Isn't this a bug? You're incrementing tileY twice now.

Yes, fixing.

> Also, why not move y+=height and x+=width into the for loop too?

"height" and "width" aren't in scope at that point.

Comment 91

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 617138 [details] [diff] [review]
Part 2.5: IPC Changes

>diff --git a/gfx/layers/ipc/PLayers.ipdl b/gfx/layers/ipc/PLayers.ipdl

>+struct OpPaintTiledLayerBuffer {
>+  PLayer layer;
>+  uintptr_t tiledLayerBuffer;

This needs a bug filed and FIXME comment pointing at the bug.  Let's
chat about the fix if you want.

>diff --git a/gfx/layers/ipc/ShadowLayers.cpp b/gfx/layers/ipc/ShadowLayers.cpp

>+void
>+ShadowLayerForwarder::PaintedTiledLayerBuffer(ShadowableLayer* aLayer,
>+                                              BasicTiledLayerBuffer* aTiledLayerBuffer)
>+{

Move the RUNTIMEABORT() to here, and note the FIXME bug in the abort
message.  Abort if in a content process (type != default).

>diff --git a/gfx/layers/ipc/ShadowLayersParent.cpp b/gfx/layers/ipc/ShadowLayersParent.cpp

>+      if (XRE_GetProcessType() != GeckoProcessType_Default)
>+        NS_RUNTIMEABORT("OpPaintTiledLayerBuffer must be made IPC safe (not share pointers)");

These updates are only received in the "Default" process type so far,
so this check is meaningless.  See above.

>+      NS_ASSERTION(tileCompoler, "shadowLayer is not a tile composer");
>+

This won't compile.

very sad r=me with those changes.  We need to fix this asap.

Comment 92

[Patrick Walton (:pcwalton)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline April 22-25) from comment #89)
> > +  NS_ABORT_IF_FALSE(aNewValidRegion.Contains(aInvalidateRegion), "Invalidating a region outside the visible region");
> > +
> > +  nsIntRegion regionToPaint(aInvalidateRegion);
> 
> You missed my comment:
> > Shouldn't this be (oldValidRegion - aInvalidateRegion) AND aNewValidRegion?
> 
> At least tell me why it isn't :-).

During PaintThebes(), BasicTiledThebesLayer calls Update() with aInvalidateRegion set to mVisibleRegion - mValidRegion and aNewValidRegion set to mVisibleRegion. So subtracting out aInvalidateRegion will cause the invalid region to not be repainted.

Comment 93

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 5: Tiled layer buffer.

Patch part 1 version 5 addresses review comments.

Comment 94

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 617172 [details] [diff] [review]
Proposed patch, part 1, version 5: Tiled layer buffer.

Review of attachment 617172 [details] [diff] [review]:
-----------------------------------------------------------------

Alright. Actually aInvalidateRegion is misnamed here. It should aPaintRegion. Same in BasicTiledLayerBuffer::PaintThebes and BasicTiledThebesLayer::PaintThebes, etc.

I think the assertion should be that oldValidRegion OR aPaintRegion contains aNewValidRegion. You can leave the existing assertion in as well if you want.

I suggest moving width and height outside the 'for' scope so that x/y can be updated in the for statement.

r+ with those fixed.

Comment 95

[Benoit Girard (:BenWa)]

Attachment: part 1, version 6: Tiled layer buffer.

Comment 96

[Benoit Girard (:BenWa)]

Landed part1+2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad583c18d336
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ffd7065ef5

Comment 97

[Benoit Girard (:BenWa)]

Attachment: Part 2.5: IPC Changes, r=(sad)cjones

Comment 98

[Kartikaya Gupta (email:[e-mail])]

With these patches applied (+ the tile snapping patches, plus a few more), I get a crash whenever I try loading cnn.com. It also happens in other situations but that's the easiest to repro. GDB says:

Breakpoint 1, gfxReusableSurfaceWrapper::~gfxReusableSurfaceWrapper (this=0x67d21be0, __in_chrg=<optimized out>)
    at /Users/kats/zspace/mozilla-git/gfx/thebes/gfxReusableSurfaceWrapper.cpp:20
20	  NS_ABORT_IF_FALSE(mReadCount == 0, "Should not be locked when released");
(gdb) bt
#0  gfxReusableSurfaceWrapper::~gfxReusableSurfaceWrapper (this=0x67d21be0, __in_chrg=<optimized out>)
    at /Users/kats/zspace/mozilla-git/gfx/thebes/gfxReusableSurfaceWrapper.cpp:20
#1  0x63321522 in gfxReusableSurfaceWrapper::Release (this=0x67d21be0) at ../../dist/include/gfxReusableSurfaceWrapper.h:31
#2  0x63333dc0 in ~nsRefPtr (this=<optimized out>, __in_chrg=<optimized out>) at ../../dist/include/nsAutoPtr.h:908
#3  ~BasicTiledLayerTile (this=<optimized out>, __in_chrg=<optimized out>) at ../../dist/include/BasicTiledThebesLayer.h:26
#4  Destruct (e=<optimized out>) at ../../dist/include/nsTArray.h:380
#5  DestructRange (start=<optimized out>, count=<optimized out>, this=<optimized out>) at ../../dist/include/nsTArray.h:1243
#6  RemoveElementsAt (count=<optimized out>, start=<optimized out>, this=<optimized out>) at ../../dist/include/nsTArray.h:963
#7  Clear (this=<optimized out>) at ../../dist/include/nsTArray.h:974
#8  ~nsTArray (this=<optimized out>, __in_chrg=<optimized out>) at ../../dist/include/nsTArray.h:462
#9  ~TiledLayerBuffer (this=<optimized out>, __in_chrg=<optimized out>) at /Users/kats/zspace/mozilla-git/gfx/layers/TiledLayerBuffer.h:72
#10 ~BasicTiledLayerBuffer (this=<optimized out>, __in_chrg=<optimized out>) at ../../dist/include/BasicTiledThebesLayer.h:78
#11 mozilla::layers::TiledThebesLayerOGL::~TiledThebesLayerOGL (this=0x67d32c00, __in_chrg=<optimized out>)
    at /Users/kats/zspace/mozilla-git/gfx/layers/opengl/TiledThebesLayerOGL.h:109
#12 0x63333e56 in mozilla::layers::TiledThebesLayerOGL::~TiledThebesLayerOGL (this=0x67d21be0, __in_chrg=<optimized out>)
    at /Users/kats/zspace/mozilla-git/gfx/layers/opengl/TiledThebesLayerOGL.h:109
#13 0x6284b64c in mozilla::layers::Layer::Release (this=0x67d32c90) at ../../dist/include/Layers.h:564
#14 0x63339bb0 in assign_assuming_AddRef (newPtr=<optimized out>, this=<optimized out>) at ../../dist/include/nsAutoPtr.h:896
#15 assign_with_AddRef (rawPtr=<optimized out>, this=<optimized out>) at ../../dist/include/nsAutoPtr.h:880
#16 operator= (rhs=<optimized out>, this=<optimized out>) at ../../dist/include/nsAutoPtr.h:964
#17 mozilla::layers::ShadowLayerParent::ActorDestroy (this=0x67d21440, why=128) at /Users/kats/zspace/mozilla-git/gfx/layers/ipc/ShadowLayerParent.cpp:108
#18 0x631aa992 in mozilla::plugins::PPluginBackgroundDestroyerParent::DestroySubtree (this=0x67d21440, 
    why=mozilla::ipc::IProtocolManager<mozilla::ipc::RPCChannel::RPCListener>::Deletion)
    at /Users/kats/zspace/mozilla-git/obj-android-debug/ipc/ipdl/PPluginBackgroundDestroyerParent.cpp:315
#19 0x631dd692 in mozilla::layers::PLayerParent::OnMessageReceived (this=0x67d21440, __msg=...)
    at /Users/kats/zspace/mozilla-git/obj-android-debug/ipc/ipdl/PLayerParent.cpp:170
#20 0x631dbbaa in mozilla::layers::PCompositorParent::OnMessageReceived (this=0x63f3f200, __msg=...)
    at /Users/kats/zspace/mozilla-git/obj-android-debug/ipc/ipdl/PCompositorParent.cpp:288
#21 0x631a1dda in mozilla::ipc::AsyncChannel::OnDispatchMessage (this=0x63f3f208, msg=...) at /Users/kats/zspace/mozilla-git/ipc/glue/AsyncChannel.cpp:495
#22 0x631a6cdc in mozilla::ipc::RPCChannel::OnMaybeDequeueOne (this=0x63f3f208) at /Users/kats/zspace/mozilla-git/ipc/glue/RPCChannel.cpp:434
#23 0x6318edfc in DispatchToMethod<mozilla::plugins::PluginInstanceChild, void (mozilla::plugins::PluginInstanceChild::*)()> (arg=<optimized out>, 
    method=<optimized out>, obj=<optimized out>) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/tuple.h:383
#24 RunnableMethod<mozilla::plugins::PluginInstanceChild, void (mozilla::plugins::PluginInstanceChild::*)(), Tuple0>::Run (this=<optimized out>)
    at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/task.h:307
#25 0x631a50be in Run (this=<optimized out>) at ../../dist/include/mozilla/ipc/RPCChannel.h:462
#26 mozilla::ipc::RPCChannel::DequeueTask::Run (this=0x67d213a0) at ../../dist/include/mozilla/ipc/RPCChannel.h:485
#27 0x632c92ee in MessageLoop::RunTask (this=0x645ffdd4, task=0x67d213a0) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:318
#28 0x632c9af8 in MessageLoop::DeferOrRunPendingTask (this=0x67d21be0, pending_task=<optimized out>)
    at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:326
#29 0x632ca7e6 in MessageLoop::DoWork (this=0x645ffdd4) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:426
#30 0x632cab52 in base::MessagePumpDefault::Run (this=0x641f33a0, delegate=0x645ffdd4) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_pump_default.cc:23
#31 0x632c988a in MessageLoop::RunInternal (this=0x645ffdd4) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:208
#32 0x632c98ea in RunHandler (this=<optimized out>) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:201
#33 MessageLoop::Run (this=0x645ffdd4) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:175
#34 0x632d3388 in base::Thread::ThreadMain (this=0x5db0a7f0) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/thread.cc:156
#35 0x632e0b82 in ThreadFunc (closure=0x67d21be0) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/platform_thread_posix.cc:26
#36 0x4003fc20 in __thread_entry () from /Users/kats/android/jdb/moz-gdb/lib/01466E640801401C/system/lib/libc.so
#37 0x4003f774 in pthread_create () from /Users/kats/android/jdb/moz-gdb/lib/01466E640801401C/system/lib/libc.so

Comment 99

[Benoit Girard (:BenWa)]

Thanks kats, I have a fix for that somewhere. I plan on landing the fix along with tiles.

Comment 100

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/ad583c18d336

Comment 101

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/a0ffd7065ef5

Comment 102

[Benoit Girard (:BenWa)]

Attachment: part 6: Fix unlocking (Debug asserts failing)

Comment 103

[Patrick Walton (:pcwalton)]

Comment on attachment 617710 [details] [diff] [review]
part 6: Fix unlocking (Debug asserts failing)

Review of attachment 617710 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +135,5 @@
>    mVideoMemoryTiledBuffer.Upload(&mMainMemoryTiledBuffer, mMainMemoryTiledBuffer.GetValidRegion(), mRegionToUpload);
>    mValidRegion = mVideoMemoryTiledBuffer.GetValidRegion();
>  
>    mMainMemoryTiledBuffer.ReadUnlock();
> +  mMainMemoryTiledBuffer = BasicTiledLayerBuffer();

Please add a comment here saying what's going on. I take it you're recreating a tiled layer buffer so that you don't try to lock it or unlock it again... or something?

Comment 104

[Benoit Girard (:BenWa)]

Attachment: part 6: Fix unlocking (simpler)

Comment 105

[Patrick Walton (:pcwalton)]

Comment on attachment 617717 [details] [diff] [review]
part 6: Fix unlocking (simpler)

Review of attachment 617717 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, much simpler. r=me

Comment 106

[Benoit Girard (:BenWa)]

Attachment: part 6: Fix unlocking

Comment 107

[Patrick Walton (:pcwalton)]

Comment on attachment 617724 [details] [diff] [review]
part 6: Fix unlocking

I get it now. r=me

Comment 108

[Benoit Girard (:BenWa)]

Attachment: part 7: enable

Comment 109

[Patrick Walton (:pcwalton)]

Comment on attachment 617751 [details] [diff] [review]
part 7: enable

Review of attachment 617751 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/TiledLayerBuffer.h
@@ +7,5 @@
>  
>  #define TILEDLAYERBUFFER_TILE_SIZE 256
>  
>  // Debug defines
> +#ifdef ANDROID

MOZ_WIDGET_ANDROID please. r=me with that.

Comment 110

[Benoit Girard (:BenWa)]

(In reply to Patrick Walton (:pcwalton) from comment #109)
> Comment on attachment 617751 [details] [diff] [review]
> part 7: enable
> 
> Review of attachment 617751 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/TiledLayerBuffer.h
> @@ +7,5 @@
> >  
> >  #define TILEDLAYERBUFFER_TILE_SIZE 256
> >  
> >  // Debug defines
> > +#ifdef ANDROID
> 
> MOZ_WIDGET_ANDROID please. r=me with that.

I missed this comment, I am quite tired already. I'll fix this with up coming improvements.

Comment 111

[Ed Morley [:emorley]]

Landed on inbound, but then backed out for XUL Android bustage:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Android&rev=37b4c6fd16ed

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9228e7f3ef7

Comment 112

[Ed Morley [:emorley]]

s/XUL/XUL and native/

Comment 113

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/2f5d290dc234
https://hg.mozilla.org/mozilla-central/rev/6aac1347aa35
https://hg.mozilla.org/mozilla-central/rev/a8f489c62eb0
https://hg.mozilla.org/mozilla-central/rev/baffceac44f9
https://hg.mozilla.org/mozilla-central/rev/58736fb3b001
https://hg.mozilla.org/mozilla-central/rev/82cb595c2a03