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 397564 eef2029cae9d33943011306ac593906492368d2b
parent 397563 9b9a6dca288397b794f31e52c8a21b1262c72d47
child 397565 55a26e91a828df3cd54b432a03014ed4801dc20f
push id25332
push usermaglione.k@gmail.com
push dateSat, 06 Aug 2016 21:21:51 +0000
reviewersedwin
bugs1291071
milestone51.0a1
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.