Bug 746883; fix the bustage possibly caused by 746896. r=mattwoodrow

gfx/layers/d3d9/CanvasLayerD3D9.cpp

@@ -32,6 +32,7 @@ CanvasLayerD3D9::Initialize(const Data& aData)
 
   if (aData.mDrawTarget) {
     mDrawTarget = aData.mDrawTarget;
+    mSurface = gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mDrawTarget);
     mNeedsYFlip = false;
     mDataIsPremultiplied = true;
   } else if (aData.mSurface) {
@@ -139,18 +140,11 @@ CanvasLayerD3D9::UpdateSurface()
     D3DLOCKED_RECT lockedRect = textureLock.GetLockRect();
 
     nsRefPtr<gfxImageSurface> sourceSurface;
-    nsRefPtr<gfxASurface> tempSurface;
-    if (mDrawTarget) {
-      tempSurface = gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mDrawTarget);
-    }
-    else {
-      tempSurface = mSurface;
-    }
 
-    if (tempSurface->GetType() == gfxASurface::SurfaceTypeWin32) {
+    if (mSurface->GetType() == gfxASurface::SurfaceTypeWin32) {
-      sourceSurface = tempSurface->GetAsImageSurface();
+      sourceSurface = mSurface->GetAsImageSurface();
-    } else if (tempSurface->GetType() == gfxASurface::SurfaceTypeImage) {
+    } else if (mSurface->GetType() == gfxASurface::SurfaceTypeImage) {
-      sourceSurface = static_cast<gfxImageSurface*>(tempSurface.get());
+      sourceSurface = static_cast<gfxImageSurface*>(mSurface.get());
       if (sourceSurface->Format() != gfxASurface::ImageFormatARGB32 &&
           sourceSurface->Format() != gfxASurface::ImageFormatRGB24)
       {
@@ -161,7 +155,7 @@ CanvasLayerD3D9::UpdateSurface()
                                           gfxASurface::ImageFormatARGB32);
       nsRefPtr<gfxContext> ctx = new gfxContext(sourceSurface);
       ctx->SetOperator(gfxContext::OPERATOR_SOURCE);
-      ctx->SetSource(tempSurface);
+      ctx->SetSource(mSurface);
       ctx->Paint();
     }
 

gfx/layers/opengl/CanvasLayerOGL.cpp

@@ -75,6 +75,7 @@ CanvasLayerOGL::Initialize(const Data& aData)
 
   if (aData.mDrawTarget) {
     mDrawTarget = aData.mDrawTarget;
+    mCanvasSurface = gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mDrawTarget);
     mNeedsYFlip = false;
   } else if (aData.mSurface) {
     mCanvasSurface = aData.mSurface;
@@ -162,11 +163,7 @@ CanvasLayerOGL::UpdateSurface()
   } else {
     nsRefPtr<gfxASurface> updatedAreaSurface;
 
-    if (mDrawTarget) {
+    if (mCanvasSurface) {
-      // TODO: This is suboptimal - We should have direct handling for the surface types instead of
-      // going via a gfxASurface.
-      updatedAreaSurface = gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mDrawTarget);
-    } else if (mCanvasSurface) {
       updatedAreaSurface = mCanvasSurface;
     } else if (mCanvasGLContext) {
       gfxIntSize size(mBounds.width, mBounds.height);
@@ -226,13 +223,8 @@ CanvasLayerOGL::RenderLayer(int aPreviousDestination,
     
     drawRect.IntersectRect(drawRect, GetEffectiveVisibleRegion().GetBounds());
 
-    nsRefPtr<gfxASurface> surf = mCanvasSurface;
-    if (mDrawTarget) {
-      surf = gfxPlatform::GetPlatform()->GetThebesSurfaceForDrawTarget(mDrawTarget);
-    }
-
     mLayerProgram =
-      gl()->UploadSurfaceToTexture(surf,
+      gl()->UploadSurfaceToTexture(mCanvasSurface,
                                    nsIntRect(0, 0, drawRect.width, drawRect.height),
                                    mTexture,
                                    true,

gfx/thebes/gfxPlatform.cpp

@@ -615,57 +615,53 @@ gfxPlatform::GetScaledFontForFont(DrawTarget* aTarget, gfxFont *aFont)
   return scaledFont;
 }
 
-UserDataKey kThebesSurfaceKey;
+cairo_user_data_key_t kDrawSourceSurface;
-void
+static void
-DestroyThebesSurface(void *data)
+DataSourceSurfaceDestroy(void *dataSourceSurface)
 {
-  gfxASurface *surface = static_cast<gfxASurface*>(data);
+  static_cast<DataSourceSurface*>(dataSourceSurface)->Release();
-  surface->Release();
+}
+
+cairo_user_data_key_t kDrawTargetForSurface;
+static void
+DataDrawTargetDestroy(void *aTarget)
+{
+  static_cast<DrawTarget*>(aTarget)->Release();
 }
 
 already_AddRefed<gfxASurface>
 gfxPlatform::GetThebesSurfaceForDrawTarget(DrawTarget *aTarget)
 {
-  // If we have already created a thebes surface, we can just return it.
-  void *surface = aTarget->GetUserData(&kThebesSurfaceKey);
-  if (surface) {
-    nsRefPtr<gfxASurface> surf = static_cast<gfxASurface*>(surface);
-    return surf.forget();
-  }
-
-  nsRefPtr<gfxASurface> surf;
   if (aTarget->GetType() == BACKEND_CAIRO) {
     cairo_surface_t* csurf =
       static_cast<cairo_surface_t*>(aTarget->GetNativeSurface(NATIVE_SURFACE_CAIRO_SURFACE));
-    surf = gfxASurface::Wrap(csurf);
+    return gfxASurface::Wrap(csurf);
-  } else {
-    // The semantics of this part of the function are sort of weird. If we
-    // don't have direct support for the backend, we snapshot the first time
-    // and then return the snapshotted surface for the lifetime of the draw
-    // target. Sometimes it seems like this works out, but it seems like it
-    // might result in no updates ever.
-    RefPtr<SourceSurface> source = aTarget->Snapshot();
-    RefPtr<DataSourceSurface> data = source->GetDataSurface();
-
-    if (!data) {
-      return NULL;
-    }
-
-    IntSize size = data->GetSize();
-    gfxASurface::gfxImageFormat format = OptimalFormatForContent(ContentForFormat(data->GetFormat()));
-
-    // We need to make a copy here because data might change its data under us
-    nsRefPtr<gfxImageSurface> imageSurf = new gfxImageSurface(gfxIntSize(size.width, size.height), format, false);
- 
-    bool resultOfCopy = imageSurf->CopyFrom(source);
-    NS_ASSERTION(resultOfCopy, "Failed to copy surface.");
-    surf = imageSurf;
   }
 
-  // add a reference to be held by the drawTarget
+  // The semantics of this part of the function are sort of weird. If we
-  // careful, the reference graph is getting complicated here
+  // don't have direct support for the backend, we snapshot the first time
-  surf->AddRef();
+  // and then return the snapshotted surface for the lifetime of the draw
-  aTarget->AddUserData(&kThebesSurfaceKey, surf.get(), DestroyThebesSurface);
+  // target. Sometimes it seems like this works out, but it seems like it
+  // might result in no updates ever.
+  RefPtr<SourceSurface> source = aTarget->Snapshot();
+  RefPtr<DataSourceSurface> data = source->GetDataSurface();
+
+  if (!data) {
+    return NULL;
+  }
+
+  IntSize size = data->GetSize();
+  gfxASurface::gfxImageFormat format = OptimalFormatForContent(ContentForFormat(data->GetFormat()));
+
+
+  nsRefPtr<gfxASurface> surf =
+    new gfxImageSurface(data->GetData(), gfxIntSize(size.width, size.height),
+                        data->Stride(), format);
+
+  surf->SetData(&kDrawSourceSurface, data.forget().drop(), DataSourceSurfaceDestroy);
+  // keep the draw target alive as long as we need its data
+  aTarget->AddRef();
+  surf->SetData(&kDrawTargetForSurface, aTarget, DataDrawTargetDestroy);
 
   return surf.forget();
 }

gfx/thebes/gfxPlatform.h

@@ -38,9 +38,6 @@ class gfxTextRun;
 class nsIURI;
 class nsIAtom;
 
-extern mozilla::gfx::UserDataKey kThebesSurfaceKey;
-void DestroyThebesSurface(void *data);
-
 extern cairo_user_data_key_t kDrawTarget;
 
 // pref lang id's for font prefs
@@ -175,9 +172,12 @@ public:
       CreateDrawTargetForSurface(gfxASurface *aSurface, const mozilla::gfx::IntSize& aSize);
 
     /*
-     * Creates a SourceSurface for a gfxASurface. This surface should -not- be
+     * Creates a SourceSurface for a gfxASurface. This function does no caching,
-     * held around by the user after the underlying gfxASurface has been
+     * so the caller should cache the gfxASurface if it will be used frequently.
-     * destroyed as a copy of the data is not guaranteed.
+     * The returned surface keeps a reference to aTarget, so it is OK to keep the
+     * surface, even if aTarget changes.
+     * aTarget should not keep a reference to the returned surface because that
+     * will cause a cycle.
      */
     virtual mozilla::RefPtr<mozilla::gfx::SourceSurface>
       GetSourceSurfaceForSurface(mozilla::gfx::DrawTarget *aTarget, gfxASurface *aSurface);

gfx/thebes/gfxPlatformMac.cpp

@@ -380,26 +380,16 @@ already_AddRefed<gfxASurface>
 gfxPlatformMac::GetThebesSurfaceForDrawTarget(DrawTarget *aTarget)
 {
   if (aTarget->GetType() == BACKEND_COREGRAPHICS) {
-    void *surface = aTarget->GetUserData(&kThebesSurfaceKey);
+    CGContextRef cg = static_cast<CGContextRef>(aTarget->GetNativeSurface(NATIVE_SURFACE_CGCONTEXT));
-    if (surface) {
-      nsRefPtr<gfxASurface> surf = static_cast<gfxQuartzSurface*>(surface);
-      return surf.forget();
-    } else {
-      CGContextRef cg = static_cast<CGContextRef>(aTarget->GetNativeSurface(NATIVE_SURFACE_CGCONTEXT));
 
-      //XXX: it would be nice to have an implicit conversion from IntSize to gfxIntSize
+    //XXX: it would be nice to have an implicit conversion from IntSize to gfxIntSize
-      IntSize intSize = aTarget->GetSize();
+    IntSize intSize = aTarget->GetSize();
-      gfxIntSize size(intSize.width, intSize.height);
+    gfxIntSize size(intSize.width, intSize.height);
 
-      nsRefPtr<gfxASurface> surf =
+    nsRefPtr<gfxASurface> surf =
-        new gfxQuartzSurface(cg, size);
+      new gfxQuartzSurface(cg, size);
 
-      // add a reference to be held by the drawTarget
+    return surf.forget();
-      surf->AddRef();
-      aTarget->AddUserData(&kThebesSurfaceKey, surf.get(), DestroyThebesSurface);
-
-      return surf.forget();
-    }
   }
 
   return gfxPlatform::GetThebesSurfaceForDrawTarget(aTarget);

gfx/thebes/gfxWindowsPlatform.cpp

@@ -809,40 +809,26 @@ gfxWindowsPlatform::GetThebesSurfaceForDrawTarget(DrawTarget *aTarget)
 {
 #ifdef XP_WIN
   if (aTarget->GetType() == BACKEND_DIRECT2D) {
-    void *surface = aTarget->GetUserData(&kThebesSurfaceKey);
+    if (!GetD2DDevice()) {
-    if (surface) {
+      // We no longer have a D2D device, can't do this.
-      nsRefPtr<gfxASurface> surf = static_cast<gfxASurface*>(surface);
+      return NULL;
-      return surf.forget();
-    } else {
-      if (!GetD2DDevice()) {
-        // We no longer have a D2D device, can't do this.
-        return NULL;
-      }
-
-      RefPtr<ID3D10Texture2D> texture =
-        static_cast<ID3D10Texture2D*>(aTarget->GetNativeSurface(NATIVE_SURFACE_D3D10_TEXTURE));
-
-      if (!texture) {
-        return gfxPlatform::GetThebesSurfaceForDrawTarget(aTarget);
-      }
-
-      aTarget->Flush();
-
-      nsRefPtr<gfxASurface> surf =
-        new gfxD2DSurface(texture, ContentForFormat(aTarget->GetFormat()));
-
-      // add a reference to be held by the drawTarget
-      surf->AddRef();
-      aTarget->AddUserData(&kThebesSurfaceKey, surf.get(), DestroyThebesSurface);
-      /* "It might be worth it to clear cairo surfaces associated with a drawtarget.
-	  The strong reference means for example for D2D that cairo's scratch surface
-	  will be kept alive (well after a user being done) and consume extra VRAM.
-	  We can deal with this in a follow-up though." */
-
-      // shouldn't this hold a reference?
-      surf->SetData(&kDrawTarget, aTarget, NULL);
-      return surf.forget();
     }
+
+    RefPtr<ID3D10Texture2D> texture =
+      static_cast<ID3D10Texture2D*>(aTarget->GetNativeSurface(NATIVE_SURFACE_D3D10_TEXTURE));
+
+    if (!texture) {
+      return gfxPlatform::GetThebesSurfaceForDrawTarget(aTarget);
+    }
+
+    aTarget->Flush();
+
+    nsRefPtr<gfxASurface> surf =
+      new gfxD2DSurface(texture, ContentForFormat(aTarget->GetFormat()));
+
+    // shouldn't this hold a reference?
+    surf->SetData(&kDrawTarget, aTarget, NULL);
+    return surf.forget();
   }
 #endif