Bug 1401672 - Make display items for the same WebRenderBridgeParent/Child share the ImageKey for shared surfaces. r=jrmuizel
authorAndrew Osmond <aosmond@mozilla.com>
Wed, 15 Nov 2017 14:31:13 -0500
changeset 392048 0ad4e810bab1313edd3d53b71f69afa692f5dc46
parent 392047 b6a8db01f25a57d6b70a769ab96c3a407b490945
child 392049 f41930a869a84af81df1a88d8e82323ff3a6509a
child 392126 1eb3b7a14ff75113daa94f32f77ddbecb3a663a3
push id32909
push usercbrindusan@mozilla.com
push dateWed, 15 Nov 2017 22:25:14 +0000
treeherdermozilla-central@f41930a869a8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs1401672
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1401672 - Make display items for the same WebRenderBridgeParent/Child share the ImageKey for shared surfaces. r=jrmuizel
gfx/layers/ipc/SharedSurfacesChild.cpp
gfx/layers/ipc/SharedSurfacesChild.h
gfx/layers/ipc/SharedSurfacesParent.cpp
gfx/layers/ipc/SharedSurfacesParent.h
gfx/layers/wr/WebRenderBridgeChild.cpp
gfx/layers/wr/WebRenderBridgeChild.h
gfx/layers/wr/WebRenderBridgeParent.cpp
gfx/layers/wr/WebRenderUserData.cpp
gfx/layers/wr/WebRenderUserData.h
--- a/gfx/layers/ipc/SharedSurfacesChild.cpp
+++ b/gfx/layers/ipc/SharedSurfacesChild.cpp
@@ -1,109 +1,187 @@
 /* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 2 -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "SharedSurfacesChild.h"
 #include "SharedSurfacesParent.h"
 #include "CompositorManagerChild.h"
+#include "mozilla/layers/IpcResourceUpdateQueue.h"
 #include "mozilla/layers/SourceSurfaceSharedData.h"
+#include "mozilla/layers/WebRenderBridgeChild.h"
+#include "mozilla/layers/WebRenderLayerManager.h"
 #include "mozilla/SystemGroup.h"        // for SystemGroup
 
 namespace mozilla {
 namespace layers {
 
 using namespace mozilla::gfx;
 
+class SharedSurfacesChild::ImageKeyData final
+{
+public:
+  RefPtr<WebRenderLayerManager> mManager;
+  wr::ImageKey mImageKey;
+  uint32_t mGenerationId;
+};
+
 class SharedSurfacesChild::SharedUserData final
 {
 public:
   explicit SharedUserData(const wr::ExternalImageId& aId)
     : mId(aId)
     , mShared(false)
   { }
 
   ~SharedUserData()
   {
     if (mShared) {
       mShared = false;
       if (NS_IsMainThread()) {
-        SharedSurfacesChild::Unshare(mId);
+        SharedSurfacesChild::Unshare(mId, mKeys);
       } else {
-        wr::ExternalImageId id = mId;
-        SystemGroup::Dispatch(TaskCategory::Other,
-                              NS_NewRunnableFunction("DestroySharedUserData",
-                                                     [id]() -> void {
-          SharedSurfacesChild::Unshare(id);
-        }));
+        class DestroyRunnable final : public Runnable
+        {
+        public:
+          DestroyRunnable(const wr::ExternalImageId& aId,
+                          nsTArray<ImageKeyData>&& aKeys)
+            : Runnable("SharedSurfacesChild::SharedUserData::DestroyRunnable")
+            , mId(aId)
+            , mKeys(aKeys)
+          { }
+
+          NS_IMETHOD Run() override
+          {
+            SharedSurfacesChild::Unshare(mId, mKeys);
+            return NS_OK;
+          }
+
+        private:
+          wr::ExternalImageId mId;
+          AutoTArray<ImageKeyData, 1> mKeys;
+        };
+
+        nsCOMPtr<nsIRunnable> task = new DestroyRunnable(mId, Move(mKeys));
+        SystemGroup::Dispatch(TaskCategory::Other, task.forget());
       }
     }
   }
 
   const wr::ExternalImageId& Id() const
   {
     return mId;
   }
 
   void SetId(const wr::ExternalImageId& aId)
   {
     mId = aId;
+    mKeys.Clear();
     mShared = false;
   }
 
   bool IsShared() const
   {
     return mShared;
   }
 
   void MarkShared()
   {
     MOZ_ASSERT(!mShared);
     mShared = true;
   }
 
+  wr::ImageKey UpdateKey(WebRenderLayerManager* aManager,
+                         wr::IpcResourceUpdateQueue& aResources,
+                         bool aForceUpdate,
+                         uint32_t aGenerationId)
+  {
+    MOZ_ASSERT(aManager);
+    MOZ_ASSERT(!aManager->IsDestroyed());
+
+    // We iterate through all of the items to ensure we clean up the old
+    // WebRenderLayerManager references. Most of the time there will be few
+    // entries and this should not be particularly expensive compared to the
+    // cost of duplicating image keys. In an ideal world, we would generate a
+    // single key for the surface, and it would be usable on all of the
+    // renderer instances. For now, we must allocate a key for each WR bridge.
+    wr::ImageKey key;
+    bool found = false;
+    auto i = mKeys.Length();
+    while (i > 0) {
+      --i;
+      ImageKeyData& entry = mKeys[i];
+      if (entry.mManager->IsDestroyed()) {
+        mKeys.RemoveElementAt(i);
+      } else if (entry.mManager == aManager) {
+        found = true;
+        if (aForceUpdate || entry.mGenerationId != aGenerationId) {
+          aManager->AddImageKeyForDiscard(entry.mImageKey);
+          entry.mGenerationId = aGenerationId;
+          entry.mImageKey = aManager->WrBridge()->GetNextImageKey();
+          aResources.AddExternalImage(mId, entry.mImageKey);
+        }
+        key = entry.mImageKey;
+      }
+    }
+
+    if (!found) {
+      key = aManager->WrBridge()->GetNextImageKey();
+      mKeys.AppendElement(ImageKeyData { aManager, key, aGenerationId });
+      aResources.AddExternalImage(mId, key);
+    }
+
+    return key;
+  }
+
 private:
+  AutoTArray<ImageKeyData, 1> mKeys;
   wr::ExternalImageId mId;
   bool mShared : 1;
 };
 
 /* static */ void
 SharedSurfacesChild::DestroySharedUserData(void* aClosure)
 {
   MOZ_ASSERT(aClosure);
   auto data = static_cast<SharedUserData*>(aClosure);
   delete data;
 }
 
 /* static */ nsresult
 SharedSurfacesChild::Share(SourceSurfaceSharedData* aSurface,
-                           wr::ExternalImageId& aId)
+                           WebRenderLayerManager* aManager,
+                           wr::IpcResourceUpdateQueue& aResources,
+                           bool aForceUpdate,
+                           uint32_t aGenerationId,
+                           wr::ImageKey& aKey)
 {
   MOZ_ASSERT(NS_IsMainThread());
+  MOZ_ASSERT(aSurface);
+  MOZ_ASSERT(aManager);
 
   CompositorManagerChild* manager = CompositorManagerChild::GetInstance();
   if (NS_WARN_IF(!manager || !manager->CanSend())) {
     return NS_ERROR_NOT_INITIALIZED;
   }
 
   static UserDataKey sSharedKey;
   SharedUserData* data =
     static_cast<SharedUserData*>(aSurface->GetUserData(&sSharedKey));
   if (!data) {
     data = new SharedUserData(manager->GetNextExternalImageId());
     aSurface->AddUserData(&sSharedKey, data, DestroySharedUserData);
   } else if (!manager->OwnsExternalImageId(data->Id())) {
     // If the id isn't owned by us, that means the bridge was reinitialized, due
     // to the GPU process crashing. All previous mappings have been released.
-    MOZ_ASSERT(manager->OtherPid() != base::GetCurrentProcId());
     data->SetId(manager->GetNextExternalImageId());
   } else if (data->IsShared()) {
     // It has already been shared with the GPU process, reuse the id.
-    aId = data->Id();
+    aKey = data->UpdateKey(aManager, aResources, aForceUpdate, aGenerationId);
     return NS_OK;
   }
 
   // Ensure that the handle doesn't get released until after we have finished
   // sending the buffer to the GPU process and/or reallocating it.
   // FinishedSharing is not a sufficient condition because another thread may
   // decide we are done while we are in the processing of sharing our newly
   // reallocated handle. Once it goes out of scope, it may release the handle.
@@ -111,17 +189,17 @@ SharedSurfacesChild::Share(SourceSurface
 
   // If we live in the same process, then it is a simple matter of directly
   // asking the parent instance to store a pointer to the same data, no need
   // to map the data into our memory space twice.
   auto pid = manager->OtherPid();
   if (pid == base::GetCurrentProcId()) {
     SharedSurfacesParent::AddSameProcess(data->Id(), aSurface);
     data->MarkShared();
-    aId = data->Id();
+    aKey = data->UpdateKey(aManager, aResources, aForceUpdate, aGenerationId);
     return NS_OK;
   }
 
   // Attempt to share a handle with the GPU process. The handle may or may not
   // be available -- it will only be available if it is either not yet finalized
   // and/or if it has been finalized but never used for drawing in process.
   ipc::SharedMemoryBasic::Handle handle = ipc::SharedMemoryBasic::NULLHandle();
   nsresult rv = aSurface->ShareToProcess(pid, handle);
@@ -142,60 +220,73 @@ SharedSurfacesChild::Share(SourceSurface
     return rv;
   }
 
   SurfaceFormat format = aSurface->GetFormat();
   MOZ_RELEASE_ASSERT(format == SurfaceFormat::B8G8R8X8 ||
                      format == SurfaceFormat::B8G8R8A8, "bad format");
 
   data->MarkShared();
-  aId = data->Id();
-  manager->SendAddSharedSurface(aId,
+  manager->SendAddSharedSurface(data->Id(),
                                 SurfaceDescriptorShared(aSurface->GetSize(),
                                                         aSurface->Stride(),
                                                         format, handle));
+  aKey = data->UpdateKey(aManager, aResources, aForceUpdate, aGenerationId);
   return NS_OK;
 }
 
 /* static */ nsresult
 SharedSurfacesChild::Share(ImageContainer* aContainer,
-                           wr::ExternalImageId& aId,
-                           uint32_t& aGeneration)
+                           WebRenderLayerManager* aManager,
+                           wr::IpcResourceUpdateQueue& aResources,
+                           bool aForceUpdate,
+                           wr::ImageKey& aKey)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aContainer);
+  MOZ_ASSERT(aManager);
 
   if (aContainer->IsAsync()) {
     return NS_ERROR_NOT_IMPLEMENTED;
   }
 
+  uint32_t generation = 0;
   AutoTArray<ImageContainer::OwningImage,4> images;
-  aContainer->GetCurrentImages(&images, &aGeneration);
+  aContainer->GetCurrentImages(&images, &generation);
   if (images.IsEmpty()) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   RefPtr<gfx::SourceSurface> surface = images[0].mImage->GetAsSourceSurface();
   if (!surface) {
     return NS_ERROR_NOT_IMPLEMENTED;
   }
 
   if (surface->GetType() != SurfaceType::DATA_SHARED) {
     return NS_ERROR_NOT_IMPLEMENTED;
   }
 
   auto sharedSurface = static_cast<SourceSurfaceSharedData*>(surface.get());
-  return Share(sharedSurface, aId);
+  return Share(sharedSurface, aManager, aResources,
+               aForceUpdate, generation, aKey);
 }
 
 /* static */ void
-SharedSurfacesChild::Unshare(const wr::ExternalImageId& aId)
+SharedSurfacesChild::Unshare(const wr::ExternalImageId& aId,
+                             nsTArray<ImageKeyData>& aKeys)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
+  for (const auto& entry : aKeys) {
+    WebRenderBridgeChild* wrBridge = entry.mManager->WrBridge();
+    if (wrBridge) {
+      wrBridge->DeallocExternalImageId(aId);
+    }
+  }
+
   CompositorManagerChild* manager = CompositorManagerChild::GetInstance();
   if (MOZ_UNLIKELY(!manager || !manager->CanSend())) {
     return;
   }
 
   if (manager->OtherPid() == base::GetCurrentProcId()) {
     // We are in the combined UI/GPU process. Call directly to it to remove its
     // wrapper surface to free the underlying buffer.
--- a/gfx/layers/ipc/SharedSurfacesChild.h
+++ b/gfx/layers/ipc/SharedSurfacesChild.h
@@ -6,39 +6,55 @@
 #ifndef MOZILLA_GFX_SHAREDSURFACESCHILD_H
 #define MOZILLA_GFX_SHAREDSURFACESCHILD_H
 
 #include <stddef.h>                     // for size_t
 #include <stdint.h>                     // for uint32_t, uint64_t
 #include "mozilla/Attributes.h"         // for override
 #include "mozilla/RefPtr.h"             // for already_AddRefed
 #include "mozilla/StaticPtr.h"          // for StaticRefPtr
-#include "mozilla/webrender/WebRenderTypes.h" // for wr::ExternalImageId
+#include "mozilla/webrender/WebRenderTypes.h" // for wr::ImageKey
 
 namespace mozilla {
 namespace gfx {
 class SourceSurfaceSharedData;
 } // namespace gfx
 
+namespace wr {
+class IpcResourceUpdateQueue;
+} // namespace wr
+
 namespace layers {
 
 class CompositorManagerChild;
+class WebRenderLayerManager;
 
 class SharedSurfacesChild final
 {
 public:
-  static nsresult Share(gfx::SourceSurfaceSharedData* aSurface, wr::ExternalImageId& aId);
-  static nsresult Share(ImageContainer* aContainer, wr::ExternalImageId& aId, uint32_t& aGeneration);
+  static nsresult Share(gfx::SourceSurfaceSharedData* aSurface,
+                        WebRenderLayerManager* aManager,
+                        wr::IpcResourceUpdateQueue& aResources,
+                        bool aForceUpdate,
+                        uint32_t aGenerationId,
+                        wr::ImageKey& aKey);
+
+  static nsresult Share(ImageContainer* aContainer,
+                        WebRenderLayerManager* aManager,
+                        wr::IpcResourceUpdateQueue& aResources,
+                        bool aForceUpdate,
+                        wr::ImageKey& aKey);
 
 private:
   SharedSurfacesChild() = delete;
   ~SharedSurfacesChild() = delete;
 
+  class ImageKeyData;
   class SharedUserData;
 
-  static void Unshare(const wr::ExternalImageId& aId);
+  static void Unshare(const wr::ExternalImageId& aId, nsTArray<ImageKeyData>& aKeys);
   static void DestroySharedUserData(void* aClosure);
 };
 
 } // namespace layers
 } // namespace mozilla
 
 #endif
--- a/gfx/layers/ipc/SharedSurfacesParent.cpp
+++ b/gfx/layers/ipc/SharedSurfacesParent.cpp
@@ -37,31 +37,43 @@ SharedSurfacesParent::Initialize()
 /* static */ void
 SharedSurfacesParent::Shutdown()
 {
   MOZ_ASSERT(NS_IsMainThread());
   sInstance = nullptr;
 }
 
 /* static */ already_AddRefed<DataSourceSurface>
+SharedSurfacesParent::Get(const wr::ExternalImageId& aId)
+{
+  MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
+  if (!sInstance) {
+    return nullptr;
+  }
+
+  RefPtr<SourceSurfaceSharedDataWrapper> surface;
+  sInstance->mSurfaces.Get(wr::AsUint64(aId), getter_AddRefs(surface));
+  return surface.forget();
+}
+
+/* static */ already_AddRefed<DataSourceSurface>
 SharedSurfacesParent::Acquire(const wr::ExternalImageId& aId)
 {
   MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
   if (!sInstance) {
     return nullptr;
   }
 
   RefPtr<SourceSurfaceSharedDataWrapper> surface;
   sInstance->mSurfaces.Get(wr::AsUint64(aId), getter_AddRefs(surface));
 
   if (surface) {
     DebugOnly<bool> rv = surface->AddConsumer();
     MOZ_ASSERT(!rv);
   }
-
   return surface.forget();
 }
 
 /* static */ bool
 SharedSurfacesParent::Release(const wr::ExternalImageId& aId)
 {
   MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
   if (!sInstance) {
--- a/gfx/layers/ipc/SharedSurfacesParent.h
+++ b/gfx/layers/ipc/SharedSurfacesParent.h
@@ -29,16 +29,21 @@ namespace layers {
 class SharedSurfacesChild;
 
 class SharedSurfacesParent final
 {
 public:
   static void Initialize();
   static void Shutdown();
 
+  // Get without increasing the consumer count.
+  static already_AddRefed<gfx::DataSourceSurface>
+  Get(const wr::ExternalImageId& aId);
+
+  // Get but also increase the consumer count. Must call Release after finished.
   static already_AddRefed<gfx::DataSourceSurface>
   Acquire(const wr::ExternalImageId& aId);
 
   static bool Release(const wr::ExternalImageId& aId);
 
   static void Add(const wr::ExternalImageId& aId,
                   const SurfaceDescriptorShared& aDesc,
                   base::ProcessId aPid);
--- a/gfx/layers/wr/WebRenderBridgeChild.cpp
+++ b/gfx/layers/wr/WebRenderBridgeChild.cpp
@@ -226,17 +226,17 @@ WebRenderBridgeChild::AllocExternalImage
   MOZ_ASSERT(aCompositable->IsConnected());
 
   wr::ExternalImageId imageId = GetNextExternalImageId();
   SendAddExternalImageIdForCompositable(imageId, aCompositable->GetIPCHandle());
   return imageId;
 }
 
 void
-WebRenderBridgeChild::DeallocExternalImageId(wr::ExternalImageId& aImageId)
+WebRenderBridgeChild::DeallocExternalImageId(const wr::ExternalImageId& aImageId)
 {
   if (mDestroyed) {
     // This can happen if the IPC connection was torn down, because, e.g.
     // the GPU process died.
     return;
   }
   SendRemoveExternalImageId(aImageId);
 }
--- a/gfx/layers/wr/WebRenderBridgeChild.h
+++ b/gfx/layers/wr/WebRenderBridgeChild.h
@@ -91,17 +91,17 @@ public:
 
   void AddPipelineIdForAsyncCompositable(const wr::PipelineId& aPipelineId,
                                          const CompositableHandle& aHandlee);
   void AddPipelineIdForCompositable(const wr::PipelineId& aPipelineId,
                                     const CompositableHandle& aHandlee);
   void RemovePipelineIdForCompositable(const wr::PipelineId& aPipelineId);
 
   wr::ExternalImageId AllocExternalImageIdForCompositable(CompositableClient* aCompositable);
-  void DeallocExternalImageId(wr::ExternalImageId& aImageId);
+  void DeallocExternalImageId(const wr::ExternalImageId& aImageId);
 
   /**
    * Clean this up, finishing with SendShutDown() which will cause __delete__
    * to be sent from the parent side.
    */
   void Destroy(bool aIsSync);
   bool IPCOpen() const { return mIPCOpen && !mDestroyed; }
   bool IsDestroyed() const { return mDestroyed; }
--- a/gfx/layers/wr/WebRenderBridgeParent.cpp
+++ b/gfx/layers/wr/WebRenderBridgeParent.cpp
@@ -376,17 +376,17 @@ WebRenderBridgeParent::AddExternalImage(
                                         wr::ResourceUpdateQueue& aResources)
 {
   Range<wr::ImageKey> keys(&aKey, 1);
   // Check if key is obsoleted.
   if (keys[0].mNamespace != mIdNamespace) {
     return true;
   }
 
-  RefPtr<DataSourceSurface> dSurf = SharedSurfacesParent::Acquire(aExtId);
+  RefPtr<DataSourceSurface> dSurf = SharedSurfacesParent::Get(aExtId);
   if (dSurf) {
     if (!gfxEnv::EnableWebRenderRecording()) {
       wr::ImageDescriptor descriptor(dSurf->GetSize(), dSurf->Stride(),
                                      dSurf->GetFormat());
       aResources.AddExternalImage(aKey, descriptor, aExtId,
                                   wr::WrExternalImageBufferType::ExternalBuffer,
                                   0);
       return true;
@@ -900,20 +900,16 @@ WebRenderBridgeParent::RecvAddExternalIm
 
 mozilla::ipc::IPCResult
 WebRenderBridgeParent::RecvRemoveExternalImageId(const ExternalImageId& aImageId)
 {
   if (mDestroyed) {
     return IPC_OK();
   }
 
-  if (SharedSurfacesParent::Release(aImageId)) {
-    return IPC_OK();
-  }
-
   WebRenderImageHost* wrHost = mExternalImageIds.Get(wr::AsUint64(aImageId)).get();
   if (!wrHost) {
     return IPC_OK();
   }
 
   wrHost->ClearWrBridge();
   mExternalImageIds.Remove(wr::AsUint64(aImageId));
 
--- a/gfx/layers/wr/WebRenderUserData.cpp
+++ b/gfx/layers/wr/WebRenderUserData.cpp
@@ -46,32 +46,43 @@ WebRenderUserData::RemoveFromTable()
 WebRenderBridgeChild*
 WebRenderUserData::WrBridge() const
 {
   return mWRManager->WrBridge();
 }
 
 WebRenderImageData::WebRenderImageData(WebRenderLayerManager* aWRManager, nsDisplayItem* aItem)
   : WebRenderUserData(aWRManager, aItem)
-  , mGeneration(0)
+  , mOwnsKey(false)
 {
 }
 
 WebRenderImageData::~WebRenderImageData()
 {
   ClearCachedResources();
 }
 
 void
-WebRenderImageData::ClearCachedResources()
+WebRenderImageData::ClearImageKey()
 {
   if (mKey) {
-    mWRManager->AddImageKeyForDiscard(mKey.value());
+    // If we don't own the key, then the owner is responsible for discarding the
+    // key when appropriate.
+    if (mOwnsKey) {
+      mWRManager->AddImageKeyForDiscard(mKey.value());
+    }
     mKey.reset();
   }
+  mOwnsKey = false;
+}
+
+void
+WebRenderImageData::ClearCachedResources()
+{
+  ClearImageKey();
 
   if (mExternalImageId) {
     WrBridge()->DeallocExternalImageId(mExternalImageId.ref());
     mExternalImageId.reset();
   }
 
   if (mPipelineId) {
     WrBridge()->RemovePipelineIdForCompositable(mPipelineId.ref());
@@ -85,70 +96,68 @@ WebRenderImageData::UpdateImageKey(Image
                                    bool aForceUpdate)
 {
   MOZ_ASSERT(aContainer);
 
   if (mContainer != aContainer) {
     mContainer = aContainer;
   }
 
-  wr::ExternalImageId externalId;
-  uint32_t generation;
-  nsresult rv = SharedSurfacesChild::Share(aContainer, externalId, generation);
+  wr::WrImageKey key;
+  nsresult rv = SharedSurfacesChild::Share(aContainer, mWRManager, aResources,
+                                           aForceUpdate, key);
   if (NS_SUCCEEDED(rv)) {
-    if (mExternalImageId.isSome() && mExternalImageId.ref() == externalId) {
-      // The image container has the same surface as before, we can reuse the
-      // key if the generation matches and the caller allows us.
-      if (mKey && mGeneration == generation && !aForceUpdate) {
-        return mKey;
-      }
-    } else {
-      // The image container has a new surface, generate a new image key.
-      mExternalImageId = Some(externalId);
-    }
+    // Ensure that any previously owned keys are released before replacing. We
+    // don't own this key, the surface itself owns it, so that it can be shared
+    // across multiple elements.
+    ClearImageKey();
+    mKey = Some(key);
+    return mKey;
+  }
 
-    mGeneration = generation;
-  } else if (rv == NS_ERROR_NOT_IMPLEMENTED) {
-    CreateImageClientIfNeeded();
-    CreateExternalImageIfNeeded();
+  if (rv != NS_ERROR_NOT_IMPLEMENTED) {
+    // We should be using the shared surface but somehow sharing it failed.
+    ClearImageKey();
+    return Nothing();
+  }
 
-    if (!mImageClient || !mExternalImageId) {
-      return Nothing();
-    }
+  CreateImageClientIfNeeded();
+  CreateExternalImageIfNeeded();
 
-    MOZ_ASSERT(mImageClient->AsImageClientSingle());
+  if (!mImageClient || !mExternalImageId) {
+    return Nothing();
+  }
 
-    ImageClientSingle* imageClient = mImageClient->AsImageClientSingle();
-    uint32_t oldCounter = imageClient->GetLastUpdateGenerationCounter();
+  MOZ_ASSERT(mImageClient->AsImageClientSingle());
 
-    bool ret = imageClient->UpdateImage(aContainer, /* unused */0);
-    if (!ret || imageClient->IsEmpty()) {
-      // Delete old key
-      if (mKey) {
-        mWRManager->AddImageKeyForDiscard(mKey.value());
-        mKey = Nothing();
-      }
-      return Nothing();
-    }
+  ImageClientSingle* imageClient = mImageClient->AsImageClientSingle();
+  uint32_t oldCounter = imageClient->GetLastUpdateGenerationCounter();
 
-    // Reuse old key if generation is not updated.
-    if (!aForceUpdate && oldCounter == imageClient->GetLastUpdateGenerationCounter() && mKey) {
-      return mKey;
-    }
+  bool ret = imageClient->UpdateImage(aContainer, /* unused */0);
+  if (!ret || imageClient->IsEmpty()) {
+    // Delete old key
+    ClearImageKey();
+    return Nothing();
+  }
+
+  // Reuse old key if generation is not updated.
+  if (!aForceUpdate && oldCounter == imageClient->GetLastUpdateGenerationCounter() && mKey) {
+    return mKey;
   }
 
   // Delete old key, we are generating a new key.
   // TODO(nical): noooo... we need to reuse image keys.
   if (mKey) {
     mWRManager->AddImageKeyForDiscard(mKey.value());
   }
 
-  wr::WrImageKey key = WrBridge()->GetNextImageKey();
+  key = WrBridge()->GetNextImageKey();
   aResources.AddExternalImage(mExternalImageId.value(), key);
   mKey = Some(key);
+  mOwnsKey = true;
 
   return mKey;
 }
 
 already_AddRefed<ImageClient>
 WebRenderImageData::GetImageClient()
 {
   RefPtr<ImageClient> imageClient = mImageClient;
--- a/gfx/layers/wr/WebRenderUserData.h
+++ b/gfx/layers/wr/WebRenderUserData.h
@@ -96,24 +96,25 @@ public:
                                          const gfx::MaybeIntSize& aScaleToSize,
                                          const wr::ImageRendering& aFilter,
                                          const wr::MixBlendMode& aMixBlendMode,
                                          bool aIsBackfaceVisible);
 
   void CreateImageClientIfNeeded();
   void ClearCachedResources() override;
 protected:
+  void ClearImageKey();
   void CreateExternalImageIfNeeded();
 
   wr::MaybeExternalImageId mExternalImageId;
   Maybe<wr::ImageKey> mKey;
   RefPtr<ImageClient> mImageClient;
   Maybe<wr::PipelineId> mPipelineId;
   RefPtr<ImageContainer> mContainer;
-  uint32_t mGeneration;
+  bool mOwnsKey;
 };
 
 class WebRenderFallbackData : public WebRenderImageData
 {
 public:
   explicit WebRenderFallbackData(WebRenderLayerManager* aWRManager, nsDisplayItem* aItem);
   virtual ~WebRenderFallbackData();