Use SurfaceDescriptor for ImageLayer and probably CanvasLayer

RESOLVED FIXED in mozilla6

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
mozilla6
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 488694 [details] [diff] [review]
Switch ImageLayer to surfaceDescriptor

In order to make OGL rendering without copies we should do next:
1) Switch IPC protocol to XID surface descriptors (enable Use X for IPC layers)
2) Similar to Bug 570625, part 5: Mostly mechanically switch  BasicShadowable/ShadowImage over to the new SurfaceDescriptor API. so we can send XID across the processes and Bind to texture
Attachment #488694 - Flags: review?(jones.chris.g)

Comment 1

7 years ago
BasicShadowImageLayer::Swap(...)
{
     ....
      mFrontBufferDescriptor = aNewFront;
     ....
}

Why cache this variable during the swap?
(Assignee)

Comment 2

7 years ago
Created attachment 489096 [details] [diff] [review]
Updated to m-c and fixed Swap logic
Attachment #488694 - Attachment is obsolete: true
Attachment #488694 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Attachment #489096 - Flags: review?(jones.chris.g)
Comment on attachment 489096 [details] [diff] [review]
Updated to m-c and fixed Swap logic

>--- a/gfx/layers/basic/BasicLayers.cpp
>+++ b/gfx/layers/basic/BasicLayers.cpp
> void
> BasicShadowableImageLayer::Paint(gfxContext* aContext,
>                                  LayerManager::DrawThebesLayerCallback aCallback,
>                                  void* aCallbackData)
> {
>   gfxIntSize oldSize = mSize;
>   nsRefPtr<gfxPattern> pat = GetAndPaintCurrentImage(aContext, GetEffectiveOpacity());
>   if (!pat || !HasShadow())
>     return;
> 
>   if (oldSize != mSize) {
>     NS_ASSERTION(oldSize == gfxIntSize(-1, -1), "video changed size?");
> 
>-    if (mBackSurface) {
>-      BasicManager()->ShadowLayerForwarder::DestroySharedSurface(mBackSurface);
>-      mBackSurface = nsnull;
>-
>+    if (IsSurfaceDescriptorValid(mBackBuffer)) {
>       BasicManager()->DestroyedImageBuffer(BasicManager()->Hold(this));
>+      mBackBuffer = SurfaceDescriptor();
>     }

Oops!  You're going to leak the old back buffer here.  (If we ever
were to hit this in practice.)

>-  nsRefPtr<gfxSharedImageSurface> mFrontSurface;
>+  SurfaceDescriptor mFrontBufferDescriptor;

Call this |mFrontBuffer| to save space and be consistent.

>diff --git a/gfx/layers/opengl/ImageLayerOGL.cpp b/gfx/layers/opengl/ImageLayerOGL.cpp
>--- a/gfx/layers/opengl/ImageLayerOGL.cpp
>+++ b/gfx/layers/opengl/ImageLayerOGL.cpp
>@@ -724,54 +727,54 @@ ShadowImageLayerOGL::ShadowImageLayerOGL
> {
>   mImplData = static_cast<LayerOGL*>(this);
> }  
> 
> ShadowImageLayerOGL::~ShadowImageLayerOGL()
> {}
> 
> PRBool
>-ShadowImageLayerOGL::Init(gfxSharedImageSurface* aFront,
>+ShadowImageLayerOGL::Init(const SurfaceDescriptor& aFront,
>                           const nsIntSize& aSize)
> {
>   mDeadweight = aFront;
>-  gfxSize sz = mDeadweight->GetSize();
>+  nsRefPtr<gfxASurface> surf = ShadowLayerForwarder::OpenDescriptor(aFront);
>+  gfxSize sz = surf->GetSize();
>   mTexImage = gl()->CreateTextureImage(nsIntSize(sz.width, sz.height),
>-                                       mDeadweight->GetContentType(),
>+                                       surf->GetContentType(),
>                                        LOCAL_GL_CLAMP_TO_EDGE);
>   return PR_TRUE;
> }
> 
>-already_AddRefed<gfxSharedImageSurface>
>-ShadowImageLayerOGL::Swap(gfxSharedImageSurface* aNewFront)
>+void
>+ShadowImageLayerOGL::Swap(const SurfaceDescriptor& aNewFront, SurfaceDescriptor* aNewBack)
> {
>   if (!mDestroyed && mTexImage) {
>+    nsRefPtr<gfxASurface> surf = ShadowLayerForwarder::OpenDescriptor(aNewFront);
>     // XXX this is always just ridiculously slow
>-
>-    gfxSize sz = aNewFront->GetSize();
>+    gfxSize sz = surf->GetSize();
>     nsIntRegion updateRegion(nsIntRect(0, 0, sz.width, sz.height));
>     // NB: this gfxContext must not escape EndUpdate() below
>     nsRefPtr<gfxContext> dest = mTexImage->BeginUpdate(updateRegion);
> 
>     dest->SetOperator(gfxContext::OPERATOR_SOURCE);
>-    dest->DrawSurface(aNewFront, aNewFront->GetSize());
>+    dest->DrawSurface(surf, surf->GetSize());
> 
>     mTexImage->EndUpdate();
>   }
>-
>-  return aNewFront;
>+  *aNewBack = mDeadweight;
>+  mDeadweight = aNewFront;

No need to swap here.  Let's the keep the code as it was previously,
and return aNewFront.  |mDeadweight| is going to die soon.
Attachment #489096 - Flags: review?(jones.chris.g)
(Assignee)

Comment 4

7 years ago
Created attachment 497210 [details] [diff] [review]
Fixed comments
Assignee: nobody → romaxa
Attachment #489096 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #497210 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
Blocks: 619056
Comment on attachment 497210 [details] [diff] [review]
Fixed comments

I didn't review the followup because I didn't want to take a cleanup patch for no gain.  Thought I'd commented here, sorry for not doing that.

I'm OK with taking this change now, but this code has evolved too much for this version of the patch to be relevant.  Please post a rebased version if you still want this.
Attachment #497210 - Flags: review?(jones.chris.g)
(Assignee)

Comment 6

6 years ago
Created attachment 522924 [details] [diff] [review]
Updated to mc tip
Attachment #497210 - Attachment is obsolete: true
Attachment #522924 - Flags: review?(jones.chris.g)
Comment on attachment 522924 [details] [diff] [review]
Updated to mc tip

>diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp
>+  virtual PRBool SupportsSurfaceDescriptor() const { return PR_TRUE; }

It'd be great to kill this off.  Want to write a
gfxASurface->SurfaceDescriptor patch for canvas too? :)

>diff --git a/gfx/layers/opengl/ImageLayerOGL.cpp b/gfx/layers/opengl/ImageLayerOGL.cpp
>+  if (SurfaceDescriptor::T__None != mDeadweight.type()) {

We should be using the IsSurfaceDescriptorValid() helper for this, but
I'm OK with factoring that out of BasicLayers.cpp in the future
CanvasLayer patch.
Attachment #522924 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 8

6 years ago
Created attachment 523820 [details] [diff] [review]
Canvas part + some cleanup
Attachment #523820 - Flags: review?(jones.chris.g)
Comment on attachment 523820 [details] [diff] [review]
Canvas part + some cleanup

Mostly looks good.

>diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp
> BasicShadowCanvasLayer::Initialize(const Data& aData)
>+  if (gfxSharedImageSurface::IsSharedImage(aData.mSurface)) {
>+    mFrontSurface = static_cast<gfxSharedImageSurface*>(aData.mSurface)->GetShmem();
>+#ifdef MOZ_X11
>+  } else if (static_cast<gfxASurface*>(aData.mSurface)->GetType() == gfxASurface::SurfaceTypeXlib) {
>+    mFrontSurface = SurfaceDescriptorX11(static_cast<gfxXlibSurface*>(aData.mSurface));
>+#endif
>+  } else {
>+    NS_RUNTIMEABORT("Incompatibe surface type");
>+  }
>+

We need to share this code.  The easiest way is probably to add an
Initialize() method to ShadowCanvasLayer that takes a RemoteData
struct or something like that, which has a SurfaceDescriptor member.
That would allow us to pass the descriptor down to ShadowCanvasLayer
without going through this ugly unpack-to-gfxASurface hack.  Then, the
*ShadowCanvasLayer impls would make their Initialize(Data) methods
just RUNTIMEABORT(), since they shouldn't be called
(Initialize(RemoteData) should be called instead).
Attachment #523820 - Flags: review?(jones.chris.g)
(Assignee)

Comment 10

6 years ago
Created attachment 526137 [details] [diff] [review]
Fixed review comment, canvas part
Attachment #523820 - Attachment is obsolete: true
Attachment #526137 - Flags: review?(jones.chris.g)
Attachment #526137 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 11

6 years ago
Created attachment 527568 [details] [diff] [review]
Combined patch

Fixed build errors on non-linux
(Assignee)

Comment 12

6 years ago
Created attachment 527574 [details] [diff] [review]
interdiff
Pushed romaxa's merged and non-linux-build-error-fixed cset from
   http://hg.mozilla.org/try/raw-rev/f9d29e392cac
to m-c: http://hg.mozilla.org/mozilla-central/rev/0736b014d4a5
Target Milestone: --- → mozilla6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Backed out for breaking canvas reftests on the mobile tbpl:
  http://hg.mozilla.org/mozilla-central/rev/d74c502ac764

Reftest logs (from http://tbpl.mozilla.org/?tree=Mobile ):
http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1303407435.1303410604.29100.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1303407435.1303408938.21513.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1303409056.1303411624.1775.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
One more reftest log (just to complete the set -- 2 from the push with this bug's fix, and 2 from the next push):
http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1303421235.1303422808.17278.gz

In all the failures, it looks like the testcase is blank.
(Assignee)

Comment 16

6 years ago
ok, will check that more carefully.
(Assignee)

Comment 17

6 years ago
Created attachment 528026 [details] [diff] [review]
Compilation and mobile reftests fix

Ok, reason of failed reftests, was 
-  if (!mFrontSurface) {
+  if (IsSurfaceDescriptorValid(mFrontSurface)) {
     return;
   }
change.
Here is the additional change to already reviewed patches which is fixing reftests failure.
Attachment #527574 - Attachment is obsolete: true
Attachment #528026 - Flags: review?(jones.chris.g)
(Assignee)

Comment 18

6 years ago
Created attachment 528027 [details] [diff] [review]
Combined patch against m-c tip
Attachment #527568 - Attachment is obsolete: true
Attachment #528026 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 19

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