Bug 1462355 - Part 8. Avoid allocating on the heap in DrawableFrameRef. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Tue, 29 May 2018 08:36:13 -0400
changeset 420221 8d412f560489b690870257077e15f1b1ad3d7b75
parent 420220 baeab7da1768fb4741c1ee8942a04e70340fc3b4
child 420222 9b516954e1031202b00b924accfb0861a973986f
push id103751
push useraosmond@gmail.com
push dateTue, 29 May 2018 12:36:34 +0000
treeherdermozilla-inbound@9b516954e103 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1462355
milestone62.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 1462355 - Part 8. Avoid allocating on the heap in DrawableFrameRef. r=tnikkel We can easily use Maybe<DataSourceSurface::ScopedMap> instead of allocated the map on the heap. This does require some minor changes to ScopedMap to properly support moves, but should be much more efficient.
gfx/2d/2D.h
image/imgFrame.h
image/test/gtest/TestDecoders.cpp
image/test/gtest/TestMetadata.cpp
--- a/gfx/2d/2D.h
+++ b/gfx/2d/2D.h
@@ -468,23 +468,45 @@ public:
 
   /**
    * This is a scoped version of Map(). Map() is called in the constructor and
    * Unmap() in the destructor. Use this for automatic unmapping of your data
    * surfaces.
    *
    * Use IsMapped() to verify whether Map() succeeded or not.
    */
-  class ScopedMap {
+  class ScopedMap final {
   public:
     explicit ScopedMap(DataSourceSurface* aSurface, MapType aType)
       : mSurface(aSurface)
       , mIsMapped(aSurface->Map(aType, &mMap)) {}
 
-    virtual ~ScopedMap()
+    ScopedMap(ScopedMap&& aOther)
+      : mSurface(Move(aOther.mSurface))
+      , mMap(aOther.mMap)
+      , mIsMapped(aOther.mIsMapped)
+    {
+      aOther.mMap.mData = nullptr;
+      aOther.mIsMapped = false;
+    }
+
+    ScopedMap& operator=(ScopedMap&& aOther)
+    {
+      if (mIsMapped) {
+        mSurface->Unmap();
+      }
+      mSurface = Move(aOther.mSurface);
+      mMap = aOther.mMap;
+      mIsMapped = aOther.mIsMapped;
+      aOther.mMap.mData = nullptr;
+      aOther.mIsMapped = false;
+      return *this;
+    }
+
+    ~ScopedMap()
     {
       if (mIsMapped) {
         mSurface->Unmap();
       }
     }
 
     uint8_t* GetData() const
     {
@@ -502,16 +524,19 @@ public:
     {
       MOZ_ASSERT(mIsMapped);
       return &mMap;
     }
 
     bool IsMapped() const { return mIsMapped; }
 
   private:
+    ScopedMap(const ScopedMap& aOther) = delete;
+    ScopedMap& operator=(const ScopedMap& aOther) = delete;
+
     RefPtr<DataSourceSurface> mSurface;
     MappedSurface mMap;
     bool mIsMapped;
   };
 
   virtual SurfaceType GetType() const override { return SurfaceType::DATA; }
   /** @deprecated
    * Get the raw bitmap data of the surface.
--- a/image/imgFrame.h
+++ b/image/imgFrame.h
@@ -10,17 +10,16 @@
 #include "mozilla/Maybe.h"
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/Monitor.h"
 #include "mozilla/Move.h"
 #include "AnimationParams.h"
 #include "gfxDrawable.h"
 #include "imgIContainer.h"
 #include "MainThreadUtils.h"
-#include "nsAutoPtr.h"
 
 namespace mozilla {
 namespace image {
 
 class ImageRegion;
 class DrawableFrameRef;
 class RawAccessFrameRef;
 
@@ -333,30 +332,26 @@ class DrawableFrameRef final
 public:
   DrawableFrameRef() { }
 
   explicit DrawableFrameRef(imgFrame* aFrame)
     : mFrame(aFrame)
   {
     MOZ_ASSERT(aFrame);
     MonitorAutoLock lock(aFrame->mMonitor);
+    MOZ_ASSERT(!aFrame->GetIsPaletted(), "Paletted must use RawAccessFrameRef");
 
-    // Paletted images won't have a surface so there is no strong reference
-    // to hold on to. Since Draw() and GetSourceSurface() calls will not work
-    // in that case, we should be using RawAccessFrameRef exclusively instead.
-    // See FrameAnimator::GetRawFrame for an example of this behaviour.
     if (aFrame->mRawSurface) {
-      mRef = new DataSourceSurface::ScopedMap(aFrame->mRawSurface,
-                                              DataSourceSurface::READ_WRITE);
+      mRef.emplace(aFrame->mRawSurface, DataSourceSurface::READ);
       if (!mRef->IsMapped()) {
         mFrame = nullptr;
-        mRef = nullptr;
+        mRef.reset();
       }
     } else {
-      MOZ_ASSERT(aFrame->mOptSurface || aFrame->GetIsPaletted());
+      MOZ_ASSERT(aFrame->mOptSurface);
     }
   }
 
   DrawableFrameRef(DrawableFrameRef&& aOther)
     : mFrame(aOther.mFrame.forget())
     , mRef(Move(aOther.mRef))
   { }
 
@@ -383,24 +378,25 @@ public:
   }
 
   imgFrame* get() { return mFrame; }
   const imgFrame* get() const { return mFrame; }
 
   void reset()
   {
     mFrame = nullptr;
-    mRef = nullptr;
+    mRef.reset();
   }
 
 private:
   DrawableFrameRef(const DrawableFrameRef& aOther) = delete;
+  DrawableFrameRef& operator=(const DrawableFrameRef& aOther) = delete;
 
   RefPtr<imgFrame> mFrame;
-  nsAutoPtr<DataSourceSurface::ScopedMap> mRef;
+  Maybe<DataSourceSurface::ScopedMap> mRef;
 };
 
 /**
  * A reference to an imgFrame that holds the imgFrame's surface in memory in a
  * format appropriate for access as raw data. If you have a RawAccessFrameRef
  * |ref| and |if (ref)| is true, then calls to GetImageData() and
  * GetPaletteData() are guaranteed to succeed. This guarantee is stronger than
  * DrawableFrameRef, so everything that a valid DrawableFrameRef guarantees is
--- a/image/test/gtest/TestDecoders.cpp
+++ b/image/test/gtest/TestDecoders.cpp
@@ -462,18 +462,18 @@ TEST_F(ImageDecoders, AnimatedGIFWithFRA
                            RasterSurfaceKey(imageSize,
                                             DefaultSurfaceFlags(),
                                             PlaybackType::eAnimated));
     ASSERT_EQ(MatchType::EXACT, result.Type());
 
     EXPECT_TRUE(NS_SUCCEEDED(result.Surface().Seek(0)));
     EXPECT_TRUE(bool(result.Surface()));
 
-    EXPECT_TRUE(NS_SUCCEEDED(result.Surface().Seek(1)));
-    EXPECT_TRUE(bool(result.Surface()));
+    RawAccessFrameRef partialFrame = result.Surface().RawAccessRef(1);
+    EXPECT_TRUE(bool(partialFrame));
   }
 
   // Ensure that the static version is still around.
   {
     LookupResult result =
       SurfaceCache::Lookup(ImageKey(image.get()),
                            RasterSurfaceKey(imageSize,
                                             DefaultSurfaceFlags(),
@@ -548,18 +548,18 @@ TEST_F(ImageDecoders, AnimatedGIFWithFRA
                            RasterSurfaceKey(imageSize,
                                             DefaultSurfaceFlags(),
                                             PlaybackType::eAnimated));
     ASSERT_EQ(MatchType::EXACT, result.Type());
 
     EXPECT_TRUE(NS_SUCCEEDED(result.Surface().Seek(0)));
     EXPECT_TRUE(bool(result.Surface()));
 
-    EXPECT_TRUE(NS_SUCCEEDED(result.Surface().Seek(1)));
-    EXPECT_TRUE(bool(result.Surface()));
+    RawAccessFrameRef partialFrame = result.Surface().RawAccessRef(1);
+    EXPECT_TRUE(bool(partialFrame));
   }
 
   // Ensure that we didn't decode the static version of the image.
   {
     LookupResult result =
       SurfaceCache::Lookup(ImageKey(image.get()),
                            RasterSurfaceKey(imageSize,
                                             DefaultSurfaceFlags(),
@@ -591,18 +591,18 @@ TEST_F(ImageDecoders, AnimatedGIFWithFRA
                            RasterSurfaceKey(imageSize,
                                             DefaultSurfaceFlags(),
                                             PlaybackType::eAnimated));
     ASSERT_EQ(MatchType::EXACT, result.Type());
 
     EXPECT_TRUE(NS_SUCCEEDED(result.Surface().Seek(0)));
     EXPECT_TRUE(bool(result.Surface()));
 
-    EXPECT_TRUE(NS_SUCCEEDED(result.Surface().Seek(1)));
-    EXPECT_TRUE(bool(result.Surface()));
+    RawAccessFrameRef partialFrame = result.Surface().RawAccessRef(1);
+    EXPECT_TRUE(bool(partialFrame));
   }
 }
 
 TEST_F(ImageDecoders, AnimatedGIFWithExtraImageSubBlocks)
 {
   ImageTestCase testCase = ExtraImageSubBlocksAnimatedGIFTestCase();
 
   // Verify that we can decode this test case and get two frames, even though
@@ -660,18 +660,18 @@ TEST_F(ImageDecoders, AnimatedGIFWithExt
                          RasterSurfaceKey(imageSize,
                                           DefaultSurfaceFlags(),
                                           PlaybackType::eAnimated));
   ASSERT_EQ(MatchType::EXACT, result.Type());
 
   EXPECT_TRUE(NS_SUCCEEDED(result.Surface().Seek(0)));
   EXPECT_TRUE(bool(result.Surface()));
 
-  EXPECT_TRUE(NS_SUCCEEDED(result.Surface().Seek(1)));
-  EXPECT_TRUE(bool(result.Surface()));
+  RawAccessFrameRef partialFrame = result.Surface().RawAccessRef(1);
+  EXPECT_TRUE(bool(partialFrame));
 }
 
 TEST_F(ImageDecoders, TruncatedSmallGIFSingleChunk)
 {
   CheckDecoderSingleChunk(TruncatedSmallGIFTestCase());
 }
 
 TEST_F(ImageDecoders, LargeICOWithBMPSingleChunk)
--- a/image/test/gtest/TestMetadata.cpp
+++ b/image/test/gtest/TestMetadata.cpp
@@ -250,11 +250,11 @@ TEST_F(ImageDecoderMetadata, NoFrameDela
                          RasterSurfaceKey(imageSize,
                                           DefaultSurfaceFlags(),
                                           PlaybackType::eAnimated));
   ASSERT_EQ(MatchType::EXACT, result.Type());
 
   EXPECT_TRUE(NS_SUCCEEDED(result.Surface().Seek(0)));
   EXPECT_TRUE(bool(result.Surface()));
 
-  EXPECT_TRUE(NS_SUCCEEDED(result.Surface().Seek(1)));
-  EXPECT_TRUE(bool(result.Surface()));
+  RawAccessFrameRef partialFrame = result.Surface().RawAccessRef(1);
+  EXPECT_TRUE(bool(partialFrame));
 }