Last Comment Bug 610155 - Use SurfaceDescriptor for ImageLayer and probably CanvasLayer
: Use SurfaceDescriptor for ImageLayer and probably CanvasLayer
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla6
Assigned To: Oleg Romashin (:romaxa)
:
Mentors:
Depends on:
Blocks: 619056
  Show dependency treegraph
 
Reported: 2010-11-06 13:55 PDT by Oleg Romashin (:romaxa)
Modified: 2011-04-29 03:47 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Switch ImageLayer to surfaceDescriptor (18.06 KB, patch)
2010-11-06 13:55 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Updated to m-c and fixed Swap logic (17.69 KB, patch)
2010-11-08 20:30 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Fixed comments (17.62 KB, patch)
2010-12-12 19:44 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Updated to mc tip (17.32 KB, patch)
2011-03-29 22:02 PDT, Oleg Romashin (:romaxa)
cjones.bugs: review+
Details | Diff | Splinter Review
Canvas part + some cleanup (23.66 KB, patch)
2011-04-02 14:41 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Fixed review comment, canvas part (20.76 KB, patch)
2011-04-14 16:02 PDT, Oleg Romashin (:romaxa)
cjones.bugs: review+
Details | Diff | Splinter Review
Combined patch (36.73 KB, patch)
2011-04-21 10:15 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
interdiff (1.14 KB, patch)
2011-04-21 10:19 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Compilation and mobile reftests fix (1.89 KB, patch)
2011-04-24 15:36 PDT, Oleg Romashin (:romaxa)
cjones.bugs: review+
Details | Diff | Splinter Review
Combined patch against m-c tip (32.15 KB, patch)
2011-04-24 15:38 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2010-11-06 13:55:20 PDT
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
Comment 1 tom brinkman 2010-11-08 16:32:30 PST
BasicShadowImageLayer::Swap(...)
{
     ....
      mFrontBufferDescriptor = aNewFront;
     ....
}

Why cache this variable during the swap?
Comment 2 Oleg Romashin (:romaxa) 2010-11-08 20:30:32 PST
Created attachment 489096 [details] [diff] [review]
Updated to m-c and fixed Swap logic
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-09 20:20:20 PST
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.
Comment 4 Oleg Romashin (:romaxa) 2010-12-12 19:44:50 PST
Created attachment 497210 [details] [diff] [review]
Fixed comments
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-25 14:21:54 PDT
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.
Comment 6 Oleg Romashin (:romaxa) 2011-03-29 22:02:44 PDT
Created attachment 522924 [details] [diff] [review]
Updated to mc tip
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-30 17:54:16 PDT
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.
Comment 8 Oleg Romashin (:romaxa) 2011-04-02 14:41:17 PDT
Created attachment 523820 [details] [diff] [review]
Canvas part + some cleanup
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-06 17:13:40 PDT
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).
Comment 10 Oleg Romashin (:romaxa) 2011-04-14 16:02:32 PDT
Created attachment 526137 [details] [diff] [review]
Fixed review comment, canvas part
Comment 11 Oleg Romashin (:romaxa) 2011-04-21 10:15:24 PDT
Created attachment 527568 [details] [diff] [review]
Combined patch

Fixed build errors on non-linux
Comment 12 Oleg Romashin (:romaxa) 2011-04-21 10:19:26 PDT
Created attachment 527574 [details] [diff] [review]
interdiff
Comment 13 Daniel Holbert [:dholbert] 2011-04-21 10:24:45 PDT
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
Comment 15 Daniel Holbert [:dholbert] 2011-04-21 15:25:56 PDT
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.
Comment 16 Oleg Romashin (:romaxa) 2011-04-21 17:46:36 PDT
ok, will check that more carefully.
Comment 17 Oleg Romashin (:romaxa) 2011-04-24 15:36:30 PDT
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.
Comment 18 Oleg Romashin (:romaxa) 2011-04-24 15:38:51 PDT
Created attachment 528027 [details] [diff] [review]
Combined patch against m-c tip
Comment 19 Oleg Romashin (:romaxa) 2011-04-29 03:47:09 PDT
http://hg.mozilla.org/mozilla-central/rev/8ceab62fda1d

Note You need to log in before you can comment on or make changes to this bug.