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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

With OGL, the compositor's texture acts as layers' front buffers.

Comment 1

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

Needs to block because with GL, we're wasting half the memory allocated by shadow layers (like, literally not using it).

Comment 2

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

(System memory, to be clear.)

Comment 3

[Oleg Romashin (:romaxa)]

can we just delete received front buffer? instead of remembering it into mDeathWeight?

Comment 4

[Oleg Romashin (:romaxa)]

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...

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Can we do it this way?

Comment 6

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

(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 7

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

Comment on attachment 498108 [details] [diff] [review]
Can we do it this way?

Approach looks good.  See comment 6 for feedback.

Comment 8

[Oleg Romashin (:romaxa)]

Attachment: Hopefully all coments adressed

Comment 9

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

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.

Comment 10

[Oleg Romashin (:romaxa)]

Attachment: Updated patch

Comment 11

[Oleg Romashin (:romaxa)]

Attachment: Updated patch

Comment 12

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

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.

Comment 13

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

> The method we want here is, ShouldDoubleBuffer() or something, which
> can just return |mParentBackend > LAYERS_BASIC|.

Er sorry, check |mParentBackend == LAYERS_BASIC|.

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Fixed comment 12

Comment 15

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

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.

Comment 16

[Oleg Romashin (:romaxa)]

> >+  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.....

Comment 17

[Oleg Romashin (:romaxa)]

> something wrong here.....
oh, ok too late and I'm stupid now... will push it tomorrow with comment fixed

Comment 18

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/b083bc8b79ab