The default bug view has changed. See this FAQ.

Add a Shadowable TiledThebesLayer implementation

RESOLVED FIXED in mozilla14

Status

()

Core
Graphics
P1
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Depends on: 1 bug, Blocks: 4 bugs)

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

Firefox Tracking Flags

(blocking-fennec1.0 beta+)

Details

Attachments

(8 attachments, 32 obsolete attachments)

12.83 KB, patch
roc
: review+
BenWa
: checkin+
Details | Diff | Splinter Review
4.30 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
17.15 KB, patch
Details | Diff | Splinter Review
24.45 KB, patch
Details | Diff | Splinter Review
13.89 KB, patch
BenWa
: checkin+
Details | Diff | Splinter Review
6.98 KB, patch
Details | Diff | Splinter Review
3.62 KB, patch
pcwalton
: review+
Details | Diff | Splinter Review
908 bytes, patch
pcwalton
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 609765 [details] [diff] [review]
WIP
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
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 :)
(Assignee)

Comment 3

5 years ago
I didn't want to clobber build
(Assignee)

Comment 4

5 years ago
Created attachment 610290 [details] [diff] [review]
WIP (v2)

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

Updated

5 years ago
Depends on: 740557
Blocks: 717774
This is the sort of bug that we probably need before beta if we're going to get it at all.
blocking-fennec1.0: --- → ?
(Assignee)

Comment 6

5 years ago
+1 for beta blocker.
Blocks: 737510

Updated

5 years ago
blocking-fennec1.0: ? → beta+
(Assignee)

Comment 7

5 years ago
Created attachment 611924 [details] [diff] [review]
WIP (v3)

This implementation now handles complex region and should be ready to build against.
Attachment #609765 - Attachment is obsolete: true
Attachment #610290 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 612028 [details] [diff] [review]
WIP (v4)

Fixed leaks: gfxImageSurface/Textures
Attachment #611924 - Attachment is obsolete: true

Updated

5 years ago
Priority: -- → P1

Updated

5 years ago
Blocks: 742757

Updated

5 years ago
Blocks: 742366

Updated

5 years ago
Blocks: 730718
Blocks: 743751
(Assignee)

Comment 9

5 years ago
Created attachment 614006 [details] [diff] [review]
WIP (v5)

Support thebes layer that don't get a shadow copy by returning a basicthebeslayer for now.
Attachment #612028 - Attachment is obsolete: true
Whiteboard: [gfx]

Updated

5 years ago
Blocks: 744241

Updated

5 years ago
No longer blocks: 744241
(Assignee)

Comment 10

5 years ago
Created attachment 614228 [details] [diff] [review]
WIP (v6)

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

Updated

5 years ago
Attachment #614006 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 614560 [details] [diff] [review]
Part 1: TiledLayerBuffer + build changes
Attachment #614228 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #614560 - Attachment is patch: true
(Assignee)

Comment 12

5 years ago
Created attachment 614563 [details] [diff] [review]
Part 2: BasicLayers Changes
(Assignee)

Comment 13

5 years ago
Created attachment 614564 [details] [diff] [review]
Part 3: OGL Layers changes
(Assignee)

Comment 14

5 years ago
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.
Attachment #614560 - Flags: review?(matt.woodrow)
(Assignee)

Comment 15

5 years ago
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.
Attachment #614563 - Flags: review?(matt.woodrow)
(Assignee)

Updated

5 years ago
Attachment #614564 - Flags: review?(matt.woodrow)
Attachment #614560 - Flags: review?(roc)
Attachment #614563 - Flags: review?(roc)
Attachment #614564 - Flags: review?(roc)

Updated

5 years ago
Blocks: 745177
(Assignee)

Comment 16

5 years ago
Created attachment 614893 [details] [diff] [review]
Part 2.5: IPC Changes
Attachment #614893 - Flags: review?(roc)
Attachment #614893 - Flags: review?(matt.woodrow)
(Assignee)

Comment 17

5 years ago
Created attachment 614898 [details] [diff] [review]
Part 4: RGBA Support
Attachment #614898 - Flags: review?(roc)
Attachment #614898 - Flags: review?(matt.woodrow)
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 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 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.
Attachment #614893 - Flags: review?(matt.woodrow) → review?(jones.chris.g)
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 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 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?
(Assignee)

Comment 24

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

Comment 25

5 years ago
(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?
You could use a traits class to provide the extra functionality. E.g. TileTraits<T>::Placeholder().
(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...
(Assignee)

Comment 28

5 years ago
I'll use nsnull and I'll post on dev.platform so that we can get this rule clarified.
(Assignee)

Comment 29

5 years ago
Created attachment 615525 [details] [diff] [review]
Part 1: TiledLayerBuffer
Attachment #614560 - Attachment is obsolete: true
Attachment #614560 - Flags: review?(roc)
Attachment #614560 - Flags: review?(matt.woodrow)
Attachment #615525 - Flags: review?(roc)
Attachment #615525 - Flags: review?(matt.woodrow)
(Assignee)

Comment 30

5 years ago
Created attachment 615527 [details] [diff] [review]
Part 2: BasicLayers Refactor
Attachment #615527 - Flags: review?(roc)
Attachment #615527 - Flags: review?(matt.woodrow)
(Assignee)

Updated

5 years ago
Attachment #614563 - Attachment is obsolete: true
Attachment #614563 - Flags: review?(roc)
Attachment #614563 - Flags: review?(matt.woodrow)
(Assignee)

Comment 31

5 years ago
Created attachment 615583 [details] [diff] [review]
Part 3: BasicTiledThebesLayer
Attachment #614564 - Attachment is obsolete: true
Attachment #614564 - Flags: review?(roc)
Attachment #614564 - Flags: review?(matt.woodrow)
Attachment #615583 - Flags: review?(roc)
Attachment #615583 - Flags: review?(matt.woodrow)
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.
Attachment #614893 - Flags: review?(jones.chris.g) → review-
(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.
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.
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.
Whiteboard: [gfx] → [gfx][has patches][needs review roc][needs review matt]
(Assignee)

Comment 36

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

Comment 37

5 years ago
Created attachment 615823 [details] [diff] [review]
part 4: TiledThebesLayerOGL
Attachment #615823 - Flags: review?(roc)
Attachment #615823 - Flags: review?(matt.woodrow)
(Assignee)

Updated

5 years ago
Depends on: 746344
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 on attachment 615527 [details] [diff] [review]
Part 2: BasicLayers Refactor

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

My review is enough for this.
Attachment #615527 - Flags: review?(roc)
Attachment #615527 - Flags: review?(matt.woodrow)
Attachment #615527 - Flags: review+
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 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 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 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?
Attachment #614893 - Flags: review?(roc)
(Assignee)

Comment 44

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

Comment 47

5 years ago
(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.
   */
(In reply to Benoit Girard (:BenWa) from comment #47)
>   /**
>    * mRetainedTiles is a rectangular buffer of mRetainedWidth x
> mRetainedWidth

Do you mean mRetainedWidth * mRetainedHeight?
(Assignee)

Comment 49

5 years ago
fixed
(Assignee)

Comment 50

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

Comment 51

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

Updated

5 years ago
No longer blocks: 743751
(Assignee)

Comment 52

5 years ago
Created attachment 616380 [details] [diff] [review]
Part 1: TiledLayerBuffer

Note that I haven't fixed GetPlaceholder and the virtual calls yet
Attachment #615525 - Attachment is obsolete: true
Attachment #615525 - Flags: review?(roc)
Attachment #615525 - Flags: review?(matt.woodrow)
Attachment #616380 - Flags: review?(roc)
Attachment #616380 - Flags: review?(matt.woodrow)
(Assignee)

Comment 53

5 years ago
Created attachment 616381 [details] [diff] [review]
Part 3: BasicTiledThebesLayer
Attachment #615583 - Attachment is obsolete: true
Attachment #615583 - Flags: review?(roc)
Attachment #615583 - Flags: review?(matt.woodrow)
Attachment #616381 - Flags: review?(roc)
Attachment #616381 - Flags: review?(matt.woodrow)
(Assignee)

Comment 54

5 years ago
Created attachment 616382 [details] [diff] [review]
Part 4: TiledThebesLayerOGL + RGBA Support
Attachment #614898 - Attachment is obsolete: true
Attachment #615823 - Attachment is obsolete: true
Attachment #614898 - Flags: review?(roc)
Attachment #614898 - Flags: review?(matt.woodrow)
Attachment #615823 - Flags: review?(roc)
Attachment #615823 - Flags: review?(matt.woodrow)
Attachment #616382 - Flags: review?(roc)
Attachment #616382 - Flags: review?(matt.woodrow)
(Assignee)

Comment 55

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

This gives us a 5-8% drawing performance win.
Attachment #616387 - Flags: review?(matt.woodrow)
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.
Attachment #616387 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 57

5 years ago
(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.
Attachment #616380 - Flags: review?(matt.woodrow) → review+
(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 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.
Attachment #616381 - Flags: review?(matt.woodrow) → review+
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!
Attachment #616382 - Flags: review?(matt.woodrow) → review+
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.
Created attachment 616780 [details] [diff] [review]
Virtual removal.

This patch removes virtual in favor of the curiously recurring template pattern.
Created attachment 616783 [details] [diff] [review]
Virtual removal, version 2.

Virtual removal version 2 cleans it up a bit.
Attachment #616780 - Attachment is obsolete: true
Created attachment 616813 [details] [diff] [review]
Proposed patch, part 1, version 2: Tiled layer buffer.

Part 1 version 2 merges the virtual removal changes.
Attachment #616380 - Attachment is obsolete: true
Attachment #616380 - Flags: review?(roc)
Attachment #616813 - Attachment is patch: true
Created attachment 616815 [details] [diff] [review]
Proposed patch, part 3, version 2: Basic tiled Thebes layer.

Merged virtual changes with part 3.
Attachment #616381 - Attachment is obsolete: true
Attachment #616381 - Flags: review?(roc)
Created attachment 616816 [details] [diff] [review]
Proposed patch, part 4, version 2: Tiled OpenGL Thebes layer.

Part 4 version 2 rolls up the virtual changes.
Attachment #616382 - Attachment is obsolete: true
Attachment #616783 - Attachment is obsolete: true
Attachment #616382 - Flags: review?(roc)
Attachment #616816 - Flags: review?(roc)
Attachment #616813 - Flags: review?(roc)
Attachment #616815 - Flags: review?(roc)
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.
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 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.
Attachment #616815 - Flags: review?(roc) → review+
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.
Attachment #616816 - Flags: review?(roc) → review+
(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.
(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).
Sorry, to change individual *tiles*.
(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.
(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!
Created attachment 616884 [details] [diff] [review]
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.
Attachment #616813 - Attachment is obsolete: true
Attachment #616813 - Flags: review?(roc)
Attachment #616884 - Flags: feedback?(bgirard)
Created attachment 616885 [details] [diff] [review]
Proposed patch, part 3, version 3: Basic tiled Thebes layer.

Part part 3, version 3 addresses most of the review comments.
Attachment #616815 - Attachment is obsolete: true
Attachment #616885 - Flags: feedback?(bgirard)
Created attachment 616886 [details] [diff] [review]
Proposed patch, part 4, version 3: Tiled OpenGL Thebes layer.

Patch part 4, version 3 addresses most of the review comments.
Attachment #616816 - Attachment is obsolete: true
Attachment #616886 - Flags: feedback?(bgirard)
(Assignee)

Comment 79

5 years ago
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.
Attachment #616884 - Flags: feedback?(bgirard) → feedback+
(Assignee)

Comment 80

5 years ago
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.
Attachment #616885 - Flags: feedback?(bgirard) → feedback+
(Assignee)

Comment 81

5 years ago
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.
Attachment #616886 - Flags: feedback?(bgirard) → feedback+
(Assignee)

Comment 82

5 years ago
Created attachment 617130 [details] [diff] [review]
part 5: Fix tile jumping
Attachment #617130 - Flags: review?(pwalton)
(Assignee)

Comment 83

5 years ago
Created attachment 617133 [details] [diff] [review]
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).
Attachment #617133 - Flags: review?(jones.chris.g)
Created attachment 617136 [details] [diff] [review]
Proposed patch, part 1, version 4: Tiled layer buffer.

Patch part 1, version 4 addresses feedback and remaining review comments.
Attachment #616884 - Attachment is obsolete: true
Attachment #617136 - Flags: review?(roc)
Created attachment 617137 [details] [diff] [review]
Proposed patch, part 3, version 4: Basic tiled Thebes layer.

Patch part 3, version 4 addresses review and feedback comments.
Attachment #616885 - Attachment is obsolete: true
(Assignee)

Comment 86

5 years ago
Created attachment 617138 [details] [diff] [review]
Part 2.5: IPC Changes
Attachment #614893 - Attachment is obsolete: true
Attachment #617133 - Attachment is obsolete: true
Attachment #617133 - Flags: review?(jones.chris.g)
Attachment #617138 - Flags: review?(jones.chris.g)
Created attachment 617139 [details] [diff] [review]
Proposed patch, part 4, version 4: Tiled OpenGL Thebes layer.

Patch part 4, version 4 addresses review comments.
Attachment #616886 - Attachment is obsolete: true
Comment on attachment 617130 [details] [diff] [review]
part 5: Fix tile jumping

Looks good, merged into part 4.
Attachment #617130 - Attachment is obsolete: true
Attachment #617130 - Flags: review?(pwalton)
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?
Attachment #617136 - Flags: review?(roc) → review-
(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 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.
Attachment #617138 - Flags: review?(jones.chris.g) → review+
(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.
Created attachment 617172 [details] [diff] [review]
Proposed patch, part 1, version 5: Tiled layer buffer.

Patch part 1 version 5 addresses review comments.
Attachment #617136 - Attachment is obsolete: true
Attachment #617172 - Flags: review?(roc)
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.
Attachment #617172 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Blocks: 747811
(Assignee)

Updated

5 years ago
Whiteboard: [gfx][has patches][needs review roc][needs review matt] → [gfx][has patches]
(Assignee)

Comment 95

5 years ago
Created attachment 617381 [details] [diff] [review]
part 1, version 6: Tiled layer buffer.
(Assignee)

Comment 96

5 years ago
Landed part1+2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad583c18d336
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ffd7065ef5
(Assignee)

Updated

5 years ago
Attachment #617381 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #615527 - Flags: checkin+
(Assignee)

Updated

5 years ago
Whiteboard: [gfx][has patches] → [gfx][has patches], keep-open
(Assignee)

Comment 97

5 years ago
Created attachment 617620 [details] [diff] [review]
Part 2.5: IPC Changes, r=(sad)cjones
(Assignee)

Updated

5 years ago
Attachment #617138 - Attachment is obsolete: true
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
(Assignee)

Comment 99

5 years ago
Thanks kats, I have a fix for that somewhere. I plan on landing the fix along with tiles.
https://hg.mozilla.org/mozilla-central/rev/ad583c18d336
https://hg.mozilla.org/mozilla-central/rev/a0ffd7065ef5
(Assignee)

Comment 102

5 years ago
Created attachment 617710 [details] [diff] [review]
part 6: Fix unlocking (Debug asserts failing)
Attachment #617710 - Flags: review?(pwalton)
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?
(Assignee)

Comment 104

5 years ago
Created attachment 617717 [details] [diff] [review]
part 6: Fix unlocking (simpler)
Attachment #617717 - Flags: review?(pwalton)
(Assignee)

Updated

5 years ago
Attachment #617710 - Attachment is obsolete: true
Attachment #617710 - Flags: review?(pwalton)
Comment on attachment 617717 [details] [diff] [review]
part 6: Fix unlocking (simpler)

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

Ah, much simpler. r=me
Attachment #617717 - Flags: review?(pwalton) → review+
(Assignee)

Comment 106

5 years ago
Created attachment 617724 [details] [diff] [review]
part 6: Fix unlocking
Attachment #617717 - Attachment is obsolete: true
Attachment #617724 - Flags: review?(pwalton)
(Assignee)

Updated

5 years ago
Attachment #617724 - Attachment is patch: true
Comment on attachment 617724 [details] [diff] [review]
part 6: Fix unlocking

I get it now. r=me
Attachment #617724 - Flags: review?(pwalton) → review+
(Assignee)

Comment 108

5 years ago
Created attachment 617751 [details] [diff] [review]
part 7: enable
Attachment #617172 - Attachment is obsolete: true
Attachment #617751 - Flags: review?(pwalton)
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.
Attachment #617751 - Flags: review?(pwalton) → review+
(Assignee)

Comment 110

5 years ago
(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.
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
s/XUL/XUL and native/
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

Updated

5 years ago
Target Milestone: --- → mozilla14
(Assignee)

Updated

5 years ago
Blocks: 748501
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [gfx][has patches], keep-open

Updated

5 years ago
Depends on: 748721

Updated

5 years ago
Depends on: 749107
(Assignee)

Updated

5 years ago
Depends on: 749778
No longer blocks: 598873
Blocks: 598873
Depends on: 756555
Depends on: 761959
Depends on: 1179298
You need to log in before you can comment on or make changes to this bug.