Respect colorSpace in DecodingImageGenerator::onGetPixels()

third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp

Index: third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
index caff5e4cca0f89744ffc0e7a61b000555e3b248f..a2e1169146b1665bbeff7258203413880fa1ab4f 100644
--- a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
@@ -36,20 +36,18 @@
 namespace blink {
 
 static bool compatibleInfo(const SkImageInfo& src, const SkImageInfo& dst) {
-  if (src == dst)
-    return true;
+  bool compatibleSize = src.dimensions() == dst.dimensions();
+  bool compatibleColorType = src.colorType() == dst.colorType();
 
-  // It is legal to write kOpaque_SkAlphaType pixels into a kPremul_SkAlphaType
-  // buffer. This can happen when DeferredImageDecoder allocates an
-  // kOpaque_SkAlphaType image generator based on cached frame info, while the
-  // ImageFrame-allocated dest bitmap stays kPremul_SkAlphaType.
-  if (src.alphaType() == kOpaque_SkAlphaType &&
-      dst.alphaType() == kPremul_SkAlphaType) {
-    const SkImageInfo& tmp = src.makeAlphaType(kPremul_SkAlphaType);
-    return tmp == dst;
-  }
+  // We should be lenient with alphaType.  Any image can be decoded to kUnpremul
+  // or kPremul.  Opaque images may be marked as kOpaque, but not until they
+  // are fully decoded.  This means that the buffer may be kPremul, but we are
+  // writing kOpaque pixels.
 
-  return false;
+  // The color spaces also do not need to match.  We will perform color space
+  // conversions when necessary.
+
+  return compatibleSize && compatibleColorType;
 }
 
 // Creates a SkPixelRef such that the memory for pixels is given by an external
@@ -103,10 +101,10 @@ static bool updateYUVComponentSizes(ImageDecoder* decoder,
   return true;
 }
 
-ImageFrameGenerator::ImageFrameGenerator(const SkISize& fullSize,
+ImageFrameGenerator::ImageFrameGenerator(const SkImageInfo& info,
                                          bool isMultiFrame,
                                          const ColorBehavior& colorBehavior)
-    : m_fullSize(fullSize),
+    : m_info(info),
       m_decoderColorBehavior(colorBehavior),
       m_isMultiFrame(isMultiFrame),
       m_decodeFailed(false),
@@ -120,7 +118,7 @@ ImageFrameGenerator::~ImageFrameGenerator() {
 bool ImageFrameGenerator::decodeAndScale(SegmentReader* data,
                                          bool allDataReceived,
                                          size_t index,
-                                         const SkImageInfo& info,
+                                         const SkImageInfo& dstInfo,
                                          void* pixels,
                                          size_t rowBytes) {
   if (m_decodeFailed)
@@ -130,16 +128,16 @@ bool ImageFrameGenerator::decodeAndScale(SegmentReader* data,
                static_cast<int>(index));
 
   // This implementation does not support scaling so check the requested size.
-  SkISize scaledSize = SkISize::Make(info.width(), info.height());
-  ASSERT(m_fullSize == scaledSize);
+  SkISize scaledSize = SkISize::Make(dstInfo.width(), dstInfo.height());
+  DCHECK(m_info.dimensions() == scaledSize);

[scroggo_chromium, 2017/04/07 18:06:06]

scaledSize looks to be never used again (or it doesn't have to be, see below),
so maybe make this

  DCHECK(m_info.dimensions() == dstInfo.dimensions())

?

I think there's an equality flavor of DCHECK, too:

  DCHECK_EQ(m_info.dimensions(), dstInfo.dimensions());

 
   // It is okay to allocate ref-counted ExternalMemoryAllocator on the stack,
   // because 1) it contains references to memory that will be invalid after
   // returning (i.e. a pointer to |pixels|) and therefore 2) should not live
   // longer than the call to the current method.
-  ExternalMemoryAllocator externalAllocator(info, pixels, rowBytes);
+  ExternalMemoryAllocator externalAllocator(dstInfo, pixels, rowBytes);
   SkBitmap bitmap = tryToResumeDecode(data, allDataReceived, index, scaledSize,
-                                      &externalAllocator);
+                                      &externalAllocator, dstInfo);
   DCHECK(externalAllocator.unique());  // Verify we have the only ref-count.
 
   if (bitmap.isNull())
@@ -151,7 +149,7 @@ bool ImageFrameGenerator::decodeAndScale(SegmentReader* data,
   ASSERT(bitmap.height() == scaledSize.height());
   SkAutoLockPixels bitmapLock(bitmap);
   if (bitmap.getPixels() != pixels)
-    return bitmap.copyPixelsTo(pixels, rowBytes * info.height(), rowBytes);
+    return bitmap.readPixels(bitmap.info(), pixels, rowBytes, 0, 0);

[scroggo_chromium, 2017/04/07 18:06:06]

Might bitmap.info() be different from dstInfo? I'm surprised this is not
dstInfo.

   return true;
 }
 
@@ -196,26 +194,38 @@ bool ImageFrameGenerator::decodeToYUV(SegmentReader* data,
   return false;
 }
 
-SkBitmap ImageFrameGenerator::tryToResumeDecode(
-    SegmentReader* data,
-    bool allDataReceived,
-    size_t index,
-    const SkISize& scaledSize,
-    SkBitmap::Allocator* allocator) {
+static inline bool needsColorXform(const SkImageInfo& src,
+                                   const SkImageInfo& dst) {
+  return src.colorSpace() && dst.colorSpace() &&
+         !SkColorSpace::Equals(src.colorSpace(), dst.colorSpace());
+}
+
+SkBitmap ImageFrameGenerator::tryToResumeDecode(SegmentReader* data,
+                                                bool allDataReceived,
+                                                size_t index,
+                                                const SkISize& scaledSize,
+                                                SkBitmap::Allocator* allocator,
+                                                const SkImageInfo& dstInfo) {
   TRACE_EVENT1("blink", "ImageFrameGenerator::tryToResumeDecode", "frame index",
                static_cast<int>(index));
 
   ImageDecoder* decoder = 0;
 
+  const bool needsXform = needsColorXform(m_info, dstInfo);
+  ImageDecoder::AlphaOption alphaOption = ImageDecoder::AlphaPremultiplied;
+  if (needsXform && !dstInfo.isOpaque()) {

[scroggo_chromium, 2017/04/07 18:06:06]

It might be worth noting that you check dstInfo because it may be more up to
date w.r.t. opacity than m_info.

On that note, should m_info be updated? (e.g. in setHasAlpha)

OTOH, why does the ImageFrameGenerator have an SkImageInfo (more specifically,
an SkAlphaType)? The ImageFrameGenerator itself is not natively Premul or
Unpremul, and opacity can vary by ImageFrame (as evidenced by setHasAlpha, which
has an <index> parameter).

[msarett1, 2017/04/10 14:42:46]

Acknowledged.

+    alphaOption = ImageDecoder::AlphaNotPremultiplied;
+  }
+
   // Lock the mutex, so only one thread can use the decoder at once.
   MutexLocker lock(m_decodeMutex);
-  const bool resumeDecoding =
-      ImageDecodingStore::instance().lockDecoder(this, m_fullSize, &decoder);
+  const bool resumeDecoding = ImageDecodingStore::instance().lockDecoder(
+      this, m_info.dimensions(), alphaOption, &decoder);
   ASSERT(!resumeDecoding || decoder);
 
   SkBitmap fullSizeImage;
-  bool complete =
-      decode(data, allDataReceived, index, &decoder, &fullSizeImage, allocator);
+  bool complete = decode(data, allDataReceived, index, &decoder, &fullSizeImage,
+                         allocator, alphaOption, dstInfo);
 
   if (!decoder)
     return SkBitmap();
@@ -280,10 +290,12 @@ bool ImageFrameGenerator::decode(SegmentReader* data,
                                  size_t index,
                                  ImageDecoder** decoder,
                                  SkBitmap* bitmap,
-                                 SkBitmap::Allocator* allocator) {
+                                 SkBitmap::Allocator* allocator,
+                                 ImageDecoder::AlphaOption alphaOption,
+                                 const SkImageInfo& dstInfo) {
   ASSERT(m_decodeMutex.locked());
-  TRACE_EVENT2("blink", "ImageFrameGenerator::decode", "width",
-               m_fullSize.width(), "height", m_fullSize.height());
+  TRACE_EVENT2("blink", "ImageFrameGenerator::decode", "width", m_info.width(),
+               "height", m_info.height());
 
   // Try to create an ImageDecoder if we are not given one.
   ASSERT(decoder);
@@ -295,8 +307,7 @@ bool ImageFrameGenerator::decode(SegmentReader* data,
       *decoder = m_imageDecoderFactory->create().release();
 
     if (!*decoder) {
-      *decoder = ImageDecoder::create(data, allDataReceived,
-                                      ImageDecoder::AlphaPremultiplied,
+      *decoder = ImageDecoder::create(data, allDataReceived, alphaOption,
                                       m_decoderColorBehavior)
                      .release();
       // The newly created decoder just grabbed the data.  No need to reset it.
@@ -307,7 +318,6 @@ bool ImageFrameGenerator::decode(SegmentReader* data,
       return false;
   }
 
-
   if (shouldCallSetData)
     (*decoder)->setData(data, allDataReceived);
 
@@ -350,11 +360,45 @@ bool ImageFrameGenerator::decode(SegmentReader* data,
 
   SkBitmap fullSizeBitmap = frame->bitmap();
   if (!fullSizeBitmap.isNull()) {
-    ASSERT(fullSizeBitmap.width() == m_fullSize.width() &&
-           fullSizeBitmap.height() == m_fullSize.height());
+    DCHECK(fullSizeBitmap.width() == m_info.width() &&

[scroggo_chromium, 2017/04/07 18:06:06]

So long as you're changing this, it could be:

  DCHECK_EQ(fullSizeBitmap.dimensions(), m_info.dimensions());

[msarett1, 2017/04/10 14:42:46]

Acknowledged.

+           fullSizeBitmap.height() == m_info.height());
     setHasAlpha(index, !fullSizeBitmap.isOpaque());
   }
 
+  if (needsColorXform(m_info, dstInfo)) {

[scroggo_chromium, 2017/04/07 18:06:06]

I assume you do not need to do this if fullSizeBitmap.isNull()?

(That said, I would expect that we won't get here if isNull() returns true. We
return early if the frame is ImageFrame::FrameEmpty. If it's not FrameEmpty, and
the bitmap isNull, one of them seems like a lie...)

[msarett1, 2017/04/10 14:42:46]

Acknowledged.

+    DCHECK(dstInfo.isOpaque() ||
+           ImageDecoder::AlphaNotPremultiplied == alphaOption);
+    std::unique_ptr<SkColorSpaceXform> xform =
+        SkColorSpaceXform::New(m_info.colorSpace(), dstInfo.colorSpace());
+
+    SkBitmap tmp(fullSizeBitmap);

[scroggo_chromium, 2017/04/07 18:06:06]

IIRC, this will make tmp share fullSizeBitmap's SkPixelRef. Is that what you
want? It seems like you might as well use fullSizeBitmap directly instead.

[msarett1, 2017/04/10 14:42:46]

Acknowledged.

+    tmp.lockPixels();
+    uint32_t* row = tmp.getAddr32(0, 0);
+    for (int y = 0; y < dstInfo.height(); y++) {

[scroggo_chromium, 2017/04/07 18:06:06]

This method is called while m_decodeMutex is locked, so we're now going to keep
it locked longer while we do the color correction. We certainly do not want to
let another thread access the SkBitmap (which is owned by the ImageDecoder)
while we're modifying it here, but this also means that if two threads attempt
to decode e.g. into different color spaces or different frames they cannot do it
simultaneously. (Not sure how often that will happen, though.)

Since we're (potentially) modifying the SkBitmap owned by the decoder, we could
run into an issue where we partially decode an image, convert it to one color
space, and then continue decoding from another DecodingImageGenerator which
needs a different color space.

Another issue here: I see that you convert every line of the bitmap, regardless
of what has already been decoded/converted:
- is it safe to convert the transparent lines left over from a partial
  decode?
- when resuming a decode, we'll re-transform the lines that were
  already transformed, which is probably not correct.

[msarett1, 2017/04/10 14:42:45]

Acknowledged.

+      SkColorSpaceXform::ColorFormat format =
+          SkColorSpaceXform::kRGBA_8888_ColorFormat;
+      if (kN32_SkColorType == kBGRA_8888_SkColorType) {
+        format = SkColorSpaceXform::kBGRA_8888_ColorFormat;
+      }
+      SkAlphaType alphaType =
+          dstInfo.isOpaque() ? kOpaque_SkAlphaType : kUnpremul_SkAlphaType;

[scroggo_chromium, 2017/04/07 18:06:06]

My first thought was that you could use dstInfo.alphaType() here. I guess you
cannot because the client may have requested kPremul, and this is the same
object the client provided? Instead, what if you at some point before here
switched to dstInfo.makeAlphaType(kUnpremul)? Does that make the code simpler?

Edit: I guess not, since down below you want to know the original alpha type.
NVM.

[msarett1, 2017/04/10 14:42:46]

Acknowledged.

+      bool xformed =
+          xform->apply(format, row, format, row, dstInfo.width(), alphaType);
+      DCHECK(xformed);
+
+      // To be compatible with dst space blending, premultiply in the dst space.
+      if (kPremul_SkAlphaType == dstInfo.alphaType()) {
+        for (int x = 0; x < dstInfo.width(); x++) {
+          row[x] =
+              SkPreMultiplyARGB(SkGetPackedA32(row[x]), SkGetPackedR32(row[x]),
+                                SkGetPackedG32(row[x]), SkGetPackedB32(row[x]));
+        }
+      }
+
+      row = (uint32_t*)(((uint8_t*)row) + fullSizeBitmap.rowBytes());

[scroggo_chromium, 2017/04/07 18:06:06]

I think tmp and fullSizeBitmap are essentially the same, but it would seem to be
more correct to offset by tmp.rowBytes().

Also, switch to C++ style casts?

[msarett1, 2017/04/10 14:42:46]

Acknowledged.

+    }
+  }
+
   *bitmap = fullSizeBitmap;
   return isDecodeComplete;
 }
@@ -369,7 +413,7 @@ bool ImageFrameGenerator::hasAlpha(size_t index) {
 bool ImageFrameGenerator::getYUVComponentSizes(SegmentReader* data,
                                                SkYUVSizeInfo* sizeInfo) {
   TRACE_EVENT2("blink", "ImageFrameGenerator::getYUVComponentSizes", "width",
-               m_fullSize.width(), "height", m_fullSize.height());
+               m_info.width(), "height", m_info.height());
 
   if (m_yuvDecodingFailed)
     return false;