Don't use double-buffered shadowed layers with OGL compositing

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: cjones, Assigned: romaxa)

Tracking

(Blocks 1 bug)

unspecified
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b4+)

Details

Attachments

(1 attachment, 4 obsolete attachments)

With OGL, the compositor's texture acts as layers' front buffers.
Needs to block because with GL, we're wasting half the memory allocated by shadow layers (like, literally not using it).
tracking-fennec: --- → ?
(System memory, to be clear.)
(Assignee)

Comment 3

9 years ago
can we just delete received front buffer? instead of remembering it into mDeathWeight?
(Assignee)

Comment 4

9 years ago
Probably we should do it this way:
1) export from Chrome process to content process MOZ_ACCELERATED env variable
2) create function AllocBuffer instead of double buffer, and if parent is accelerated, then alloc only one backBuffer... and send fake buffer to Chrome
struct FakeSurfaceDescriptor {
  nsIntSize size;
  PRUint32 contentType;
};


Also it would be nice in this case make BackBuffer as small as possible (paint size) and destroy by timer...
(Assignee)

Comment 5

9 years ago
Posted patch Can we do it this way? (obsolete) — Splinter Review
Attachment #498108 - Flags: feedback?(jones.chris.g)
(In reply to comment #3)
> can we just delete received front buffer? instead of remembering it into
> mDeathWeight?

Judging by your later comment, you probably realized we don't want to do this because it would require a useless alloc and increased OOM risk for no benefit.

(In reply to comment #4)
> Probably we should do it this way:
> 1) export from Chrome process to content process MOZ_ACCELERATED env variable

This is ostensibly covered by bug 619488, but we can do that work here, doesn't matter.  Hacking with env vars sounds ugly and fragile, and I'd rather just ask the current layer manager for its type when shadow forwarders are created (or ask just when the first is created and save the value away).

> 2) create function AllocBuffer instead of double buffer, and if parent is
> accelerated, then alloc only one backBuffer... and send fake buffer to Chrome

I don't think we need to do this.  I think instead we can change CreateThebesBuffer() to use OptionalThebesSurface, send null_t when using OGL, and on the first OpPaintThebesBuffer, create+upload the texture using glTexImage2D.  The rest of the logic can stay the same, AFAIK.

> Also it would be nice in this case make BackBuffer as small as possible (paint
> size) and destroy by timer...

Yes, that's worth trying, but it's not a clear win.  Let's do that in a followup though.
Comment on attachment 498108 [details] [diff] [review]
Can we do it this way?

Approach looks good.  See comment 6 for feedback.
Attachment #498108 - Flags: feedback?(jones.chris.g) → feedback+
(Assignee)

Updated

9 years ago
Blocks: 619898
(Assignee)

Comment 8

8 years ago
Assignee: nobody → romaxa
Attachment #498108 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #498995 - Flags: review?(jones.chris.g)
tracking-fennec: ? → 2.0b4+
Comment on attachment 498995 [details] [diff] [review]
Hopefully all coments adressed

>diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp
>--- a/gfx/layers/basic/BasicLayers.cpp
>+++ b/gfx/layers/basic/BasicLayers.cpp

>+  PRBool parentOpenGL = BasicManager()->GetParentBackendType() == LayerManager::LAYERS_OPENGL;

s/parentOpenGL/userDoubleBuffer/.  Probably best to split this logic
into a helper function/method; we'll need it for d3d too.

>@@ -2014,32 +2021,38 @@ private:
> void
>-BasicShadowThebesLayer::SetFrontBuffer(const ThebesBuffer& aNewFront,
>+BasicShadowThebesLayer::SetFrontBuffer(const OptionalThebesBuffer& aNewFront,
>                                        const nsIntRegion& aValidRegion,
>                                        float aXResolution, float aYResolution)
> {
>   mValidRegion = mOldValidRegion = aValidRegion;
>   mXResolution = mOldXResolution = aXResolution;
>   mYResolution = mOldYResolution = aYResolution;
>+
>+  if (OptionalThebesBuffer::Tnull_t == aNewFront.type()) {
>+    return;
>+  }
>+

This will never happen with SW compositing.  Make this a hard-assert
that aNewFront isn't null_t.

>diff --git a/gfx/layers/ipc/ShadowLayerUtilsX11.cpp b/gfx/layers/ipc/ShadowLayerUtilsX11.cpp
>--- a/gfx/layers/ipc/ShadowLayerUtilsX11.cpp
>+++ b/gfx/layers/ipc/ShadowLayerUtilsX11.cpp
>@@ -98,41 +98,49 @@ SurfaceDescriptorX11::OpenForeign() cons
> }
> 
> PRBool
> ShadowLayerForwarder::PlatformAllocDoubleBuffer(const gfxIntSize& aSize,
>                                                 gfxASurface::gfxContentType aContent,
>                                                 SurfaceDescriptor* aFrontBuffer,
>                                                 SurfaceDescriptor* aBackBuffer)
> {
>+  if (!PlatformAllocBuffer(aSize, aContent, aFrontBuffer) ||
>+      !PlatformAllocBuffer(aSize, aContent, aBackBuffer)) {
>+    return PR_FALSE;
>+  }
>+
>+  return PR_TRUE;

  return (PlatformAllocBuffer(aSize, aContent, aFrontBuffer) &&
          PlatformAllocBuffer(aSize, aContent, aBackBuffer));

>diff --git a/gfx/layers/ipc/ShadowLayers.cpp b/gfx/layers/ipc/ShadowLayers.cpp
>--- a/gfx/layers/ipc/ShadowLayers.cpp
>+++ b/gfx/layers/ipc/ShadowLayers.cpp
>@@ -184,16 +184,25 @@ ShadowLayerForwarder::CreatedCanvasLayer
> void
> ShadowLayerForwarder::CreatedThebesBuffer(ShadowableLayer* aThebes,
>                                           const nsIntRegion& aFrontValidRegion,
>                                           float aXResolution,
>                                           float aYResolution,
>                                           const nsIntRect& aBufferRect,
>                                           const SurfaceDescriptor& aTempFrontBuffer)
> {
>+  if (aTempFrontBuffer.type() == SurfaceDescriptor::T__None) {
>+    mTxn->AddEdit(OpCreateThebesBuffer(NULL, Shadow(aThebes),
>+                                       null_t(),
>+                                       aFrontValidRegion,
>+                                       aXResolution,
>+                                       aYResolution));
>+    return;
>+
>+  }
>   mTxn->AddEdit(OpCreateThebesBuffer(NULL, Shadow(aThebes),
>                                      ThebesBuffer(aTempFrontBuffer,
>                                                   aBufferRect,
>                                                   nsIntPoint(0, 0)),
>                                      aFrontValidRegion,
>                                      aXResolution,
>                                      aYResolution));

Can share more code by doing

  OptionalThebesBuffer buf(null_t());
  if (none != aTempFrontBuffer.type())
    buf = ThebesBuffer(...);
  mTxn->AddEdit(..., buf, ...);

> PRBool
>+ShadowLayerForwarder::AllocBuffer(const gfxIntSize& aSize,
>+                                  gfxASurface::gfxContentType aContent,
>+                                  gfxSharedImageSurface** aBuffer)
>+{
>+  NS_ABORT_IF_FALSE(HasShadowManager(), "no manager to forward to");
>+
>+  gfxASurface::gfxImageFormat format = OptimalFormatFor(aContent);
>+  SharedMemory::SharedMemoryType shmemType = OptimalShmemType();
>+
>+  nsRefPtr<gfxSharedImageSurface> back = new gfxSharedImageSurface();
>+  if (!back->InitUnsafe(mShadowManager, aSize, format, shmemType))
>+    return PR_FALSE;
>+
>+  *aBuffer = NULL;

nsnull.  My fault from the original rev of this.

Need to refactor the AllocDoubleBuffer() functions to use the new
AllocBuffer()s.  We need to share the alloc code.

>diff --git a/gfx/layers/opengl/ThebesLayerOGL.cpp b/gfx/layers/opengl/ThebesLayerOGL.cpp
>--- a/gfx/layers/opengl/ThebesLayerOGL.cpp
>+++ b/gfx/layers/opengl/ThebesLayerOGL.cpp
>+ShadowThebesLayerOGL::SetFrontBuffer(const OptionalThebesBuffer& aNewFront,
>                                      const nsIntRegion& aValidRegion,
>                                      float aXResolution, float aYResolution)
> {
>   if (mDestroyed) {
>     return;
>   }
> 
>   if (!mBuffer) {
>     mBuffer = new ShadowBufferOGL(this);
>   }
> 
>-  nsRefPtr<gfxASurface> surf = ShadowLayerForwarder::OpenDescriptor(aNewFront.buffer());
>+  if (OptionalThebesBuffer::Tnull_t == aNewFront.type()) {
>+    return;
>+  }

As things stand now, we will always have a null_t initial front
buffer.  I'd prefer leaving in a weak check (NS_ASSERT()) that the
initial front is null_t and don't even attempt to deal with non-null_t
below; just delete that code.
 
>-  mDeadweight = aNewFront.buffer();
>+  mDeadweight = newFront.buffer();

Just nuke mDeadweight from orbit.

Looking good, would like to see another rev.
Attachment #498995 - Flags: review?(jones.chris.g)
(Assignee)

Comment 10

8 years ago
Posted patch Updated patch (obsolete) — Splinter Review
Attachment #498995 - Attachment is obsolete: true
(Assignee)

Comment 11

8 years ago
Posted patch Updated patch (obsolete) — Splinter Review
Attachment #499721 - Attachment is obsolete: true
Attachment #499722 - Flags: review?(jones.chris.g)
Comment on attachment 499722 [details] [diff] [review]
Updated patch

>diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp
>--- a/gfx/layers/basic/BasicLayers.cpp
>+++ b/gfx/layers/basic/BasicLayers.cpp
>@@ -1686,21 +1686,27 @@ BasicShadowableThebesLayer::CreateBuffer
>   if (IsSurfaceDescriptorValid(mBackBuffer)) {
>     BasicManager()->DestroyedThebesBuffer(BasicManager()->Hold(this),
>                                           mBackBuffer);
>     mBackBuffer = SurfaceDescriptor();
>   }
> 
>   // XXX error handling
>   SurfaceDescriptor tmpFront;
>+  if (BasicManager()->IsParentAccelerated()) {

See below re: name of this method.

>+    if (!BasicManager()->AllocBuffer(gfxIntSize(aSize.width, aSize.height),
>+                                     aType,
>+                                     &mBackBuffer))
>+      NS_RUNTIMEABORT("creating ThebesLayer 'back buffer' failed!");

Need braces around the stmt in the inner if-stmt.

>+  } else

Braces here.

>+    if (!BasicManager()->AllocDoubleBuffer(gfxIntSize(aSize.width, aSize.height),
>+                                           aType,
>+                                           &tmpFront,
>+                                           &mBackBuffer))

Braces here.

>-BasicShadowThebesLayer::SetFrontBuffer(const ThebesBuffer& aNewFront,
>+BasicShadowThebesLayer::SetFrontBuffer(const OptionalThebesBuffer& aNewFront,
>                                        const nsIntRegion& aValidRegion,
>                                        float aXResolution, float aYResolution)
> {
>   mValidRegion = mOldValidRegion = aValidRegion;
>   mXResolution = mOldXResolution = aXResolution;
>   mYResolution = mOldYResolution = aYResolution;
>+
>+  NS_ASSERTION(OptionalThebesBuffer::Tnull_t != aNewFront.type(),
>+               "aNewFront must be valid here!");

NS_ASSERTION is a "weak assert"; NS_ABORT_IF_FALSE is the one we want
here.

>diff --git a/gfx/layers/ipc/ShadowLayers.cpp b/gfx/layers/ipc/ShadowLayers.cpp
>--- a/gfx/layers/ipc/ShadowLayers.cpp
>+++ b/gfx/layers/ipc/ShadowLayers.cpp
>-LayersBackend
>-ShadowLayerForwarder::GetParentBackendType()
>+bool
>+ShadowLayerForwarder::IsParentAccelerated()

I don't see any benefit in removing GetParentBackendType(): that's a
useful method on its own.

The method we want here is, ShouldDoubleBuffer() or something, which
can just return |mParentBackend > LAYERS_BASIC|.  No need to check for
< LAYERS_LAST since that will never be returned.  That new method
needs a short comment describing why we don't "double buffer" in
system memory with GPU rendering, i.e. the texture is the front
buffer.

>diff --git a/gfx/layers/opengl/ThebesLayerOGL.cpp b/gfx/layers/opengl/ThebesLayerOGL.cpp
>--- a/gfx/layers/opengl/ThebesLayerOGL.cpp
>+++ b/gfx/layers/opengl/ThebesLayerOGL.cpp
> void
>-ShadowThebesLayerOGL::SetFrontBuffer(const ThebesBuffer& aNewFront,
>+ShadowThebesLayerOGL::SetFrontBuffer(const OptionalThebesBuffer& aNewFront,
>                                      const nsIntRegion& aValidRegion,
>                                      float aXResolution, float aYResolution)
>+  NS_ASSERTION(OptionalThebesBuffer::Tnull_t != aNewFront.type(),
>+               "aNewFront must not be null");

Er, we want to assert |null_t == front.type()| here.

Looking good.  Would like to see one more rev.
Attachment #499722 - Flags: review?(jones.chris.g)
> The method we want here is, ShouldDoubleBuffer() or something, which
> can just return |mParentBackend > LAYERS_BASIC|.

Er sorry, check |mParentBackend == LAYERS_BASIC|.
(Assignee)

Comment 14

8 years ago
Attachment #499722 - Attachment is obsolete: true
Attachment #499859 - Flags: review?(jones.chris.g)
Comment on attachment 499859 [details] [diff] [review]
Fixed comment 12

Thanks!

>+ShadowThebesLayerOGL::SetFrontBuffer(const OptionalThebesBuffer& aNewFront,
>                                      const nsIntRegion& aValidRegion,
>                                      float aXResolution, float aYResolution)
>-  mDeadweight = aNewFront.buffer();
>+  NS_ASSERTION(OptionalThebesBuffer::Tnull_t == aNewFront.type(),
>+               "aNewFront must not be null");

This message is incorrect.  Something like "Only one system-memory buffer expected" seems reasonable.

r=me with that fix.
Attachment #499859 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 16

8 years ago
> >+  NS_ASSERTION(OptionalThebesBuffer::Tnull_t != aNewFront.type(),
> >+               "aNewFront must not be null");
> 
> Er, we want to assert |null_t == front.type()| here.

.....
> >+  NS_ASSERTION(OptionalThebesBuffer::Tnull_t == aNewFront.type(),
> >+               "aNewFront must not be null");
> 
> This message is incorrect.  Something like "Only one system-memory buffer
> expected" seems reasonable.

something wrong here.....
(Assignee)

Comment 17

8 years ago
> something wrong here.....
oh, ok too late and I'm stupid now... will push it tomorrow with comment fixed
(Assignee)

Comment 18

8 years ago
http://hg.mozilla.org/mozilla-central/rev/b083bc8b79ab
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.