Bug 1201796 (Part 4) - Add downscale-during-decode support for the ICO decoder. r=tn

image/ImageFactory.cpp

@@ -38,6 +38,7 @@ ShouldDownscaleDuringDecode(const nsCString& aMimeType)
   DecoderType type = DecoderFactory::GetDecoderType(aMimeType.get());
   return type == DecoderType::JPEG ||
          type == DecoderType::ICON ||
+         type == DecoderType::ICO ||
          type == DecoderType::PNG ||
          type == DecoderType::BMP ||
          type == DecoderType::GIF;

image/decoders/nsICODecoder.cpp

@@ -14,13 +14,14 @@
 
 #include "RasterImage.h"
 
+using namespace mozilla::gfx;
+
 namespace mozilla {
 namespace image {
 
 // Constants.
 static const uint32_t ICOHEADERSIZE = 6;
 static const uint32_t BITMAPINFOSIZE = 40;
-static const uint32_t PREFICONSIZE = 16;
 
 // ----------------------------------------
 // Actual Data Processing
@@ -60,6 +61,7 @@ nsICODecoder::GetNumColors()
 nsICODecoder::nsICODecoder(RasterImage* aImage)
   : Decoder(aImage)
   , mLexer(Transition::To(ICOState::HEADER, ICOHEADERSIZE))
+  , mBiggestResourceColorDepth(0)
   , mBestResourceDelta(INT_MIN)
   , mBestResourceColorDepth(0)
   , mNumIcons(0)
@@ -69,6 +71,20 @@ nsICODecoder::nsICODecoder(RasterImage* aImage)
   , mCurrMaskLine(0)
 { }
 
+nsresult
+nsICODecoder::SetTargetSize(const nsIntSize& aSize)
+{
+  // Make sure the size is reasonable.
+  if (MOZ_UNLIKELY(aSize.width <= 0 || aSize.height <= 0)) {
+    return NS_ERROR_FAILURE;
+  }
+
+  // Create a downscaler that we'll filter our output through.
+  mDownscaler.emplace(aSize);
+
+  return NS_OK;
+}
+
 void
 nsICODecoder::FinishInternal()
 {
@@ -222,16 +238,6 @@ nsICODecoder::ReadBIHSize(const char* aBIH)
   return headerSize;
 }
 
-void
-nsICODecoder::SetHotSpotIfCursor()
-{
-  if (!mIsCursor) {
-    return;
-  }
-
-  mImageMetadata.SetHotspot(mDirEntry.mXHotspot, mDirEntry.mYHotspot);
-}
-
 LexerTransition<ICOState>
 nsICODecoder::ReadHeader(const char* aData)
 {
@@ -248,10 +254,13 @@ nsICODecoder::ReadHeader(const char* aData)
     return Transition::Terminate(ICOState::SUCCESS); // Nothing to do.
   }
 
-  // If we didn't get a #-moz-resolution, default to PREFICONSIZE.
-  if (mResolution.width == 0 && mResolution.height == 0) {
-    mResolution.SizeTo(PREFICONSIZE, PREFICONSIZE);
-  }
+  // Downscale-during-decode can end up decoding different resources in the ICO
+  // file depending on the target size. Since the resources are not necessarily
+  // scaled versions of the same image, some may be transparent and some may not
+  // be. We could be precise about transparency if we decoded the metadata of
+  // every resource, but for now we don't and it's safest to assume that
+  // transparency could be present.
+  PostHasTransparency();
 
   return Transition::To(ICOState::DIR_ENTRY, ICODIRENTRYSIZE);
 }
@@ -288,35 +297,69 @@ nsICODecoder::ReadDirEntry(const char* aData)
   memcpy(&e.mImageOffset, aData + 12, sizeof(e.mImageOffset));
   e.mImageOffset = LittleEndian::readUint32(&e.mImageOffset);
 
-  // Calculate the delta between this image's size and the desired size, so we
-  // can see if it is better than our current-best option.  In the case of
-  // several equally-good images, we use the last one. "Better" in this case is
-  // determined by |delta|, a measure of the difference in size between the
-  // entry we've found and the requested size. We will choose the smallest image
-  // that is >= requested size (i.e. we assume it's better to downscale a larger
-  // icon than to upscale a smaller one).
-  int32_t delta = GetRealWidth(e) - mResolution.width +
-                  GetRealHeight(e) - mResolution.height;
+  // Determine if this is the biggest resource we've seen so far. We always use
+  // the biggest resource for the intrinsic size, and if we're not downscaling,
+  // we select it as the best resource as well.
+  IntSize entrySize(GetRealWidth(e), GetRealHeight(e));
+  if (e.mBitCount >= mBiggestResourceColorDepth &&
+      entrySize.width * entrySize.height >=
+        mBiggestResourceSize.width * mBiggestResourceSize.height) {
+    mBiggestResourceSize = entrySize;
+    mBiggestResourceColorDepth = e.mBitCount;
+    mBiggestResourceHotSpot = IntSize(e.mXHotspot, e.mYHotspot);
+
+    if (!mDownscaler) {
+      mDirEntry = e;
+    }
+  }
+
+  if (mDownscaler) {
+    // Calculate the delta between this resource's size and the desired size, so
+    // we can see if it is better than our current-best option.  In the case of
+    // several equally-good resources, we use the last one. "Better" in this
+    // case is determined by |delta|, a measure of the difference in size
+    // between the entry we've found and the downscaler's target size. We will
+    // choose the smallest resource that is >= the target size (i.e. we assume
+    // it's better to downscale a larger icon than to upscale a smaller one).
+    IntSize desiredSize = mDownscaler->TargetSize();
+    int32_t delta = entrySize.width - desiredSize.width +
+                    entrySize.height - desiredSize.height;
     if (e.mBitCount >= mBestResourceColorDepth &&
         ((mBestResourceDelta < 0 && delta >= mBestResourceDelta) ||
          (delta >= 0 && delta <= mBestResourceDelta))) {
       mBestResourceDelta = delta;
-
-    // Ensure mImageOffset is >= size of the direntry headers (bug #245631).
-    if (e.mImageOffset < FirstResourceOffset()) {
-      return Transition::Terminate(ICOState::FAILURE);
-    }
-
       mBestResourceColorDepth = e.mBitCount;
       mDirEntry = e;
     }
+  }
 
   if (mCurrIcon == mNumIcons) {
-    PostSize(GetRealWidth(mDirEntry), GetRealHeight(mDirEntry));
+    // Ensure the resource we selected has an offset past the ICO headers.
+    if (mDirEntry.mImageOffset < FirstResourceOffset()) {
+      return Transition::Terminate(ICOState::FAILURE);
+    }
+
+    // If this is a cursor, set the hotspot. We use the hotspot from the biggest
+    // resource since we also use that resource for the intrinsic size.
+    if (mIsCursor) {
+      mImageMetadata.SetHotspot(mBiggestResourceHotSpot.width,
+                                mBiggestResourceHotSpot.height);
+    }
+
+    // We always report the biggest resource's size as the intrinsic size; this
+    // is necessary for downscale-during-decode to work since we won't even
+    // attempt to *upscale* while decoding.
+    PostSize(mBiggestResourceSize.width, mBiggestResourceSize.height);
     if (IsMetadataDecode()) {
       return Transition::Terminate(ICOState::SUCCESS);
     }
 
+    // If the resource we selected matches the downscaler's target size
+    // perfectly, we don't need to do any downscaling.
+    if (mDownscaler && GetRealSize() == mDownscaler->TargetSize()) {
+      mDownscaler.reset();
+    }
+
     size_t offsetToResource = mDirEntry.mImageOffset - FirstResourceOffset();
     return Transition::ToUnbuffered(ICOState::FOUND_RESOURCE,
                                     ICOState::SKIP_TO_RESOURCE,
@@ -339,6 +382,9 @@ nsICODecoder::SniffResource(const char* aData)
     mContainedDecoder->SetMetadataDecode(IsMetadataDecode());
     mContainedDecoder->SetDecoderFlags(GetDecoderFlags());
     mContainedDecoder->SetSurfaceFlags(GetSurfaceFlags());
+    if (mDownscaler) {
+      mContainedDecoder->SetTargetSize(mDownscaler->TargetSize());
+    }
     mContainedDecoder->Init();
 
     if (!WriteToContainedDecoder(aData, PNGSIGNATURESIZE)) {
@@ -363,6 +409,9 @@ nsICODecoder::SniffResource(const char* aData)
     mContainedDecoder->SetMetadataDecode(IsMetadataDecode());
     mContainedDecoder->SetDecoderFlags(GetDecoderFlags());
     mContainedDecoder->SetSurfaceFlags(GetSurfaceFlags());
+    if (mDownscaler) {
+      mContainedDecoder->SetTargetSize(mDownscaler->TargetSize());
+    }
     mContainedDecoder->Init();
 
     // Make sure we have a sane size for the bitmap information header.
@@ -389,8 +438,7 @@ nsICODecoder::ReadPNG(const char* aData, uint32_t aLen)
 
   // Raymond Chen says that 32bpp only are valid PNG ICOs
   // http://blogs.msdn.com/b/oldnewthing/archive/2010/10/22/10079192.aspx
-  if (!IsMetadataDecode() &&
-      !static_cast<nsPNGDecoder*>(mContainedDecoder.get())->IsValidICO()) {
+  if (!static_cast<nsPNGDecoder*>(mContainedDecoder.get())->IsValidICO()) {
     return Transition::Terminate(ICOState::FAILURE);
   }
 
@@ -420,9 +468,6 @@ nsICODecoder::ReadBIH(const char* aData)
     return Transition::Terminate(ICOState::FAILURE);
   }
 
-  // Set up the cursor hot spot if one is present.
-  SetHotSpotIfCursor();
-
   // Fix the ICO height from the BIH. It needs to be halved so our BMP decoder
   // will understand, because the BMP decoder doesn't expect the alpha mask that
   // follows the BMP data in an ICO.
@@ -567,9 +612,8 @@ nsICODecoder::FinishResource()
 {
   // Make sure the actual size of the resource matches the size in the directory
   // entry. If not, we consider the image corrupt.
-  IntSize expectedSize(GetRealWidth(mDirEntry), GetRealHeight(mDirEntry));
   if (mContainedDecoder->HasSize() &&
-      mContainedDecoder->GetSize() != expectedSize) {
+      mContainedDecoder->GetSize() != GetRealSize()) {
     return Transition::Terminate(ICOState::FAILURE);
   }
 

image/decoders/nsICODecoder.h

@@ -43,6 +43,8 @@ class nsICODecoder : public Decoder
 public:
   virtual ~nsICODecoder() { }
 
+  nsresult SetTargetSize(const nsIntSize& aSize) override;
+
   /// @return the width of the icon directory entry @aEntry.
   static uint32_t GetRealWidth(const IconDirEntry& aEntry)
   {
@@ -61,9 +63,10 @@ public:
   /// @return the height of the selected directory entry (mDirEntry).
   uint32_t GetRealHeight() const { return GetRealHeight(mDirEntry); }
 
-  virtual void SetResolution(const gfx::IntSize& aResolution) override
+  /// @return the size of the selected directory entry (mDirEntry).
+  gfx::IntSize GetRealSize() const
   {
-    mResolution = aResolution;
+    return gfx::IntSize(GetRealWidth(), GetRealHeight());
   }
 
   /// @return The offset from the beginning of the ICO to the first resource.
@@ -86,8 +89,6 @@ private:
   // Gets decoder state from the contained decoder so it's visible externally.
   void GetFinalStateFromContainedDecoder();
 
-  // Sets the hotspot property of if we have a cursor
-  void SetHotSpotIfCursor();
   // Creates a bitmap file header buffer, returns true if successful
   bool FillBitmapFileHeaderBuffer(int8_t* bfh);
   // Fixes the ICO height to match that of the BIH.
@@ -118,10 +119,13 @@ private:
   LexerTransition<ICOState> FinishResource();
 
   StreamingLexer<ICOState, 32> mLexer; // The lexer.
+  Maybe<Downscaler> mDownscaler;       // Our downscaler, if we're downscaling.
   nsRefPtr<Decoder> mContainedDecoder; // Either a BMP or PNG decoder.
-  gfx::IntSize mResolution;            // The requested -moz-resolution.
   char mBIHraw[40];                    // The bitmap information header.
   IconDirEntry mDirEntry;              // The dir entry for the selected resource.
+  IntSize mBiggestResourceSize;        // Used to select the intrinsic size.
+  IntSize mBiggestResourceHotSpot;     // Used to select the intrinsic size.
+  uint16_t mBiggestResourceColorDepth; // Used to select the intrinsic size.
   int32_t mBestResourceDelta;          // Used to select the best resource.
   uint16_t mBestResourceColorDepth;    // Used to select the best resource.
   uint16_t mNumIcons; // Stores the number of icons in the ICO file.

image/test/mochitest/test_has_transparency.html

@@ -57,8 +57,9 @@ function testFiles() {
   yield ["opaque.bmp", false];
 
   // ICO files which contain BMPs have an additional type of transparency - the
-  // AND mask - that warrants separate testing.
-  yield ["ico-bmp-opaque.ico", false];
+  // AND mask - that warrants separate testing. (Although, after bug 1201796,
+  // all ICOs are considered transparent.)
+  yield ["ico-bmp-opaque.ico", true];
   yield ["ico-bmp-transparent.ico", true];
 
   // SVGs are always transparent.

image/test/reftest/ico/ico-mixed/reftest.list

@@ -1,14 +1,3 @@
 # ICO BMP and PNG mixed tests
 
-== mixed-bmp-png.ico mixed-bmp-png.png
-
-# Using media fragments to select different resolutions
-
-== mixed-bmp-png.ico#-moz-resolution=8,8 mixed-bmp-png.png
-== mixed-bmp-png.ico#test=true&-moz-resolution=8,8&other mixed-bmp-png.png
-== mixed-bmp-png.ico#-moz-resolution=32,32 mixed-bmp-png32.png
-== mixed-bmp-png.ico#-moz-resolution=39,39 mixed-bmp-png48.png
-== mixed-bmp-png.ico#-moz-resolution=40,40 mixed-bmp-png48.png
-== mixed-bmp-png.ico#-moz-resolution=48,48 mixed-bmp-png48.png
-== mixed-bmp-png.ico#-moz-resolution=64,64 mixed-bmp-png48.png
-== mixed-bmp-png.ico#-moz-resolution=64 mixed-bmp-png.png # Bad syntax will fall back to lowest resolution
+== mixed-bmp-png.ico mixed-bmp-png48.png

image/test/unit/test_imgtools.js

@@ -691,8 +691,10 @@ var errsrc = "none";
 try {
   container = imgTools.decodeImage(istream, inMimeType);
 
-  // We should never hit this - decodeImage throws an assertion because the
-  // image decoded doesn't have enough frames.
+  // We expect to hit an error during encoding because the ICO header of the
+  // image is fine, but the actual resources are corrupt. Since decodeImage()
+  // only performs a metadata decode, it doesn't decode far enough to realize
+  // this, but we'll find out when we do a full decode during encodeImage().
   try {
       istream = imgTools.encodeImage(container, "image/png");
   } catch (e) {
@@ -704,7 +706,7 @@ try {
   errsrc = "decode";
 }
 
-do_check_eq(errsrc, "decode");
+do_check_eq(errsrc, "encode");
 checkExpectedError(/NS_ERROR_FAILURE/, err);