Bug 1291071 (Part 4) - Clean up Decoder::SpeedHistogram() and related code. r=edwin
authorSeth Fowler <mark.seth.fowler@gmail.com>
Tue, 02 Aug 2016 16:45:22 -0700
changeset 349757 eef2029cae9d33943011306ac593906492368d2b
parent 349756 9b9a6dca288397b794f31e52c8a21b1262c72d47
child 349758 55a26e91a828df3cd54b432a03014ed4801dc20f
push id6570
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:26:13 +0000
treeherdermozilla-beta@f455459b2ae5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersedwin
bugs1291071
milestone51.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 1291071 (Part 4) - Clean up Decoder::SpeedHistogram() and related code. r=edwin
image/Decoder.cpp
image/Decoder.h
image/RasterImage.cpp
image/decoders/nsGIFDecoder2.cpp
image/decoders/nsGIFDecoder2.h
image/decoders/nsJPEGDecoder.cpp
image/decoders/nsJPEGDecoder.h
image/decoders/nsPNGDecoder.cpp
image/decoders/nsPNGDecoder.h
--- a/image/Decoder.cpp
+++ b/image/Decoder.cpp
@@ -263,17 +263,17 @@ Maybe<uint32_t>
 Decoder::TakeCompleteFrameCount()
 {
   const bool finishedNewFrame = mFinishedNewFrame;
   mFinishedNewFrame = false;
   return finishedNewFrame ? Some(GetCompleteFrameCount()) : Nothing();
 }
 
 DecoderTelemetry
-Decoder::Telemetry()
+Decoder::Telemetry() const
 {
   MOZ_ASSERT(mIterator);
   return DecoderTelemetry(SpeedHistogram(),
                           mIterator->ByteCount(),
                           mIterator->ChunkCount(),
                           mDecodeTime);
 }
 
@@ -538,17 +538,10 @@ Decoder::PostError()
 {
   mError = true;
 
   if (mInFrame && mCurrentFrame) {
     mCurrentFrame->Abort();
   }
 }
 
-Telemetry::ID
-Decoder::SpeedHistogram()
-{
-  // Use HistogramCount as an invalid Histogram ID.
-  return Telemetry::HistogramCount;
-}
-
 } // namespace image
 } // namespace mozilla
--- a/image/Decoder.h
+++ b/image/Decoder.h
@@ -25,17 +25,17 @@ namespace mozilla {
 namespace Telemetry {
   enum ID : uint32_t;
 } // namespace Telemetry
 
 namespace image {
 
 struct DecoderTelemetry final
 {
-  DecoderTelemetry(Telemetry::ID aSpeedHistogram,
+  DecoderTelemetry(Maybe<Telemetry::ID> aSpeedHistogram,
                    size_t aBytesDecoded,
                    uint32_t aChunkCount,
                    TimeDuration aDecodeTime)
     : mSpeedHistogram(aSpeedHistogram)
     , mBytesDecoded(aBytesDecoded)
     , mChunkCount(aChunkCount)
     , mDecodeTime(aDecodeTime)
   { }
@@ -44,18 +44,19 @@ struct DecoderTelemetry final
   int32_t Speed() const
   {
     return mBytesDecoded / (1024 * mDecodeTime.ToSeconds());
   }
 
   /// @return our decoder's decode time, in microseconds.
   int32_t DecodeTimeMicros() { return mDecodeTime.ToMicroseconds(); }
 
-  /// The per-image-format telemetry ID for recording our decoder's speed.
-  const Telemetry::ID mSpeedHistogram;
+  /// The per-image-format telemetry ID for recording our decoder's speed, or
+  /// Nothing() if we don't record speed telemetry for this kind of decoder.
+  const Maybe<Telemetry::ID> mSpeedHistogram;
 
   /// The number of bytes of input our decoder processed.
   const size_t mBytesDecoded;
 
   /// The number of chunks our decoder's input was divided into.
   const uint32_t mChunkCount;
 
   /// The amount of time our decoder spent inside DoDecode().
@@ -331,23 +332,21 @@ public:
    *
    * Illegal to call if HasSize() returns false.
    */
   gfx::IntRect FullOutputFrame() const
   {
     return gfx::IntRect(gfx::IntPoint(), OutputSize());
   }
 
-  virtual Telemetry::ID SpeedHistogram();
-
   /// @return the metadata we collected about this image while decoding.
   const ImageMetadata& GetImageMetadata() { return mImageMetadata; }
 
   /// @return performance telemetry we collected while decoding.
-  DecoderTelemetry Telemetry();
+  DecoderTelemetry Telemetry() const;
 
   /**
    * @return a weak pointer to the image associated with this decoder. Illegal
    * to call if this decoder is not associated with an image.
    */
   NotNull<RasterImage*> GetImage() const { return WrapNotNull(mImage.get()); }
 
   /**
@@ -382,16 +381,24 @@ protected:
    */
   virtual nsresult InitInternal();
   virtual LexerResult DoDecode(SourceBufferIterator& aIterator,
                                IResumable* aOnResume) = 0;
   virtual nsresult BeforeFinishInternal();
   virtual nsresult FinishInternal();
   virtual nsresult FinishWithErrorInternal();
 
+  /**
+   * @return the per-image-format telemetry ID for recording this decoder's
+   * speed, or Nothing() if we don't record speed telemetry for this kind of
+   * decoder.
+   */
+  virtual Maybe<Telemetry::ID> SpeedHistogram() const { return Nothing(); }
+
+
   /*
    * Progress notifications.
    */
 
   // Called by decoders when they determine the size of the image. Informs
   // the image of its size and sends notifications.
   void PostSize(int32_t aWidth,
                 int32_t aHeight,
--- a/image/RasterImage.cpp
+++ b/image/RasterImage.cpp
@@ -1614,21 +1614,18 @@ RasterImage::FinalizeDecoder(Decoder* aD
   }
 
   if (done) {
     // Do some telemetry if this isn't a metadata decode.
     if (!wasMetadata) {
       Telemetry::Accumulate(Telemetry::IMAGE_DECODE_TIME,
                             int32_t(aTelemetry.mDecodeTime.ToMicroseconds()));
 
-      // We record the speed for only some decoders. The rest have
-      // SpeedHistogram return HistogramCount.
-      Telemetry::ID id = aTelemetry.mSpeedHistogram;
-      if (id < Telemetry::HistogramCount) {
-        Telemetry::Accumulate(id, aTelemetry.Speed());
+      if (aTelemetry.mSpeedHistogram) {
+        Telemetry::Accumulate(*aTelemetry.mSpeedHistogram, aTelemetry.Speed());
       }
     }
 
     // Detect errors.
     if (aDecoder->HasError() && !aDecoder->WasAborted()) {
       DoError();
     } else if (wasMetadata && !mHasSize) {
       DoError();
--- a/image/decoders/nsGIFDecoder2.cpp
+++ b/image/decoders/nsGIFDecoder2.cpp
@@ -1068,16 +1068,16 @@ nsGIFDecoder2::SkipSubBlocks(const char*
   }
 
   // Skip to the next sub-block length value.
   return Transition::ToUnbuffered(State::FINISHED_SKIPPING_DATA,
                                   State::SKIP_DATA_THEN_SKIP_SUB_BLOCKS,
                                   nextSubBlockLength);
 }
 
-Telemetry::ID
-nsGIFDecoder2::SpeedHistogram()
+Maybe<Telemetry::ID>
+nsGIFDecoder2::SpeedHistogram() const
 {
-  return Telemetry::IMAGE_DECODE_SPEED_GIF;
+  return Some(Telemetry::IMAGE_DECODE_SPEED_GIF);
 }
 
 } // namespace image
 } // namespace mozilla
--- a/image/decoders/nsGIFDecoder2.h
+++ b/image/decoders/nsGIFDecoder2.h
@@ -19,20 +19,22 @@ class RasterImage;
 //////////////////////////////////////////////////////////////////////
 // nsGIFDecoder2 Definition
 
 class nsGIFDecoder2 : public Decoder
 {
 public:
   ~nsGIFDecoder2();
 
+protected:
   LexerResult DoDecode(SourceBufferIterator& aIterator,
                        IResumable* aOnResume) override;
   nsresult FinishInternal() override;
-  virtual Telemetry::ID SpeedHistogram() override;
+
+  Maybe<Telemetry::ID> SpeedHistogram() const override;
 
 private:
   friend class DecoderFactory;
 
   // Decoders should only be instantiated via DecoderFactory.
   explicit nsGIFDecoder2(RasterImage* aImage);
 
   /// Called when we begin decoding the image.
--- a/image/decoders/nsJPEGDecoder.cpp
+++ b/image/decoders/nsJPEGDecoder.cpp
@@ -118,20 +118,20 @@ nsJPEGDecoder::~nsJPEGDecoder()
     qcms_profile_release(mInProfile);
   }
 
   MOZ_LOG(sJPEGDecoderAccountingLog, LogLevel::Debug,
          ("nsJPEGDecoder::~nsJPEGDecoder: Destroying JPEG decoder %p",
           this));
 }
 
-Telemetry::ID
-nsJPEGDecoder::SpeedHistogram()
+Maybe<Telemetry::ID>
+nsJPEGDecoder::SpeedHistogram() const
 {
-  return Telemetry::IMAGE_DECODE_SPEED_JPEG;
+  return Some(Telemetry::IMAGE_DECODE_SPEED_JPEG);
 }
 
 nsresult
 nsJPEGDecoder::InitInternal()
 {
   mCMSMode = gfxPlatform::GetCMSMode();
   if (GetSurfaceFlags() & SurfaceFlags::NO_COLORSPACE_CONVERSION) {
     mCMSMode = eCMSMode_Off;
--- a/image/decoders/nsJPEGDecoder.h
+++ b/image/decoders/nsJPEGDecoder.h
@@ -52,23 +52,25 @@ class nsJPEGDecoder : public Decoder
 public:
   virtual ~nsJPEGDecoder();
 
   virtual void SetSampleSize(int aSampleSize) override
   {
     mSampleSize = aSampleSize;
   }
 
+  void NotifyDone();
+
+protected:
   nsresult InitInternal() override;
   LexerResult DoDecode(SourceBufferIterator& aIterator,
                        IResumable* aOnResume) override;
   nsresult FinishInternal() override;
 
-  virtual Telemetry::ID SpeedHistogram() override;
-  void NotifyDone();
+  Maybe<Telemetry::ID> SpeedHistogram() const override;
 
 protected:
   Orientation ReadOrientationFromEXIF();
   void OutputScanlines(bool* suspend);
 
 private:
   friend class DecoderFactory;
 
--- a/image/decoders/nsPNGDecoder.cpp
+++ b/image/decoders/nsPNGDecoder.cpp
@@ -1029,20 +1029,20 @@ nsPNGDecoder::error_callback(png_structp
 
 
 void
 nsPNGDecoder::warning_callback(png_structp png_ptr, png_const_charp warning_msg)
 {
   MOZ_LOG(sPNGLog, LogLevel::Warning, ("libpng warning: %s\n", warning_msg));
 }
 
-Telemetry::ID
-nsPNGDecoder::SpeedHistogram()
+Maybe<Telemetry::ID>
+nsPNGDecoder::SpeedHistogram() const
 {
-  return Telemetry::IMAGE_DECODE_SPEED_PNG;
+  return Some(Telemetry::IMAGE_DECODE_SPEED_PNG);
 }
 
 bool
 nsPNGDecoder::IsValidICO() const
 {
   // Only 32-bit RGBA PNGs are valid ICO resources; see here:
   //   http://blogs.msdn.com/b/oldnewthing/archive/2010/10/22/10079192.aspx
 
--- a/image/decoders/nsPNGDecoder.h
+++ b/image/decoders/nsPNGDecoder.h
@@ -17,23 +17,25 @@ namespace mozilla {
 namespace image {
 class RasterImage;
 
 class nsPNGDecoder : public Decoder
 {
 public:
   virtual ~nsPNGDecoder();
 
+  /// @return true if this PNG is a valid ICO resource.
+  bool IsValidICO() const;
+
+protected:
   nsresult InitInternal() override;
   LexerResult DoDecode(SourceBufferIterator& aIterator,
                        IResumable* aOnResume) override;
-  virtual Telemetry::ID SpeedHistogram() override;
 
-  /// @return true if this PNG is a valid ICO resource.
-  bool IsValidICO() const;
+  Maybe<Telemetry::ID> SpeedHistogram() const override;
 
 private:
   friend class DecoderFactory;
 
   // Decoders should only be instantiated via DecoderFactory.
   explicit nsPNGDecoder(RasterImage* aImage);
 
   /// The information necessary to create a frame.