Respect colorSpace in DecodingImageGenerator::onGetPixels()

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

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 18 matching lines @@
 #include "platform/graphics/ImageDecodingStore.h"
 #include "platform/image-decoders/ImageDecoder.h"
 #include "platform/instrumentation/tracing/TraceEvent.h"
 #include "third_party/skia/include/core/SkYUVSizeInfo.h"
 #include "wtf/PtrUtil.h"
 #include <memory>
 
 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
 // body. This is used to write directly to the memory given by Skia during
 // decoding.
 class ExternalMemoryAllocator final : public SkBitmap::Allocator {
   USING_FAST_MALLOC(ExternalMemoryAllocator);
   WTF_MAKE_NONCOPYABLE(ExternalMemoryAllocator);
 
  public:
@@ skipping 33 matching lines @@
   componentWidthBytes[0] = decoder->decodedYUVWidthBytes(0);
   size = decoder->decodedYUVSize(1);
   componentSizes[1].set(size.width(), size.height());
   componentWidthBytes[1] = decoder->decodedYUVWidthBytes(1);
   size = decoder->decodedYUVSize(2);
   componentSizes[2].set(size.width(), size.height());
   componentWidthBytes[2] = decoder->decodedYUVWidthBytes(2);
   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),
       m_yuvDecodingFailed(false),
       m_frameCount(0) {}
 
 ImageFrameGenerator::~ImageFrameGenerator() {
   ImageDecodingStore::instance().removeCacheIndexedByGenerator(this);
 }
 
 bool ImageFrameGenerator::decodeAndScale(SegmentReader* data,
                                          bool allDataReceived,
                                          size_t index,
-                                         const SkImageInfo& info,
+                                         const SkImageInfo& dstInfo,
                                          void* pixels,
                                          size_t rowBytes) {
   if (m_decodeFailed)
     return false;
 
   TRACE_EVENT1("blink", "ImageFrameGenerator::decodeAndScale", "frame index",
                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())
     return false;
 
   // Check to see if the decoder has written directly to the pixel memory
   // provided. If not, make a copy.
   ASSERT(bitmap.width() == scaledSize.width());

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

If you drop scaledSize, this could be:

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

OTOH, I've already advised elsewhere that you don't change code you don't have
to. Take your pick.

   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;
 }
 
 bool ImageFrameGenerator::decodeToYUV(SegmentReader* data,
                                       size_t index,
                                       const SkISize componentSizes[3],
                                       void* planes[3],
                                       const size_t rowBytes[3]) {
   // TODO (scroggo): The only interesting thing this uses from the
   // ImageFrameGenerator is m_decodeFailed. Move this into
@@ skipping 24 matching lines @@
   if (decoder->decodeToYUV()) {
     setHasAlpha(0, false);  // YUV is always opaque
     return true;
   }
 
   ASSERT(decoder->failed());
   m_yuvDecodingFailed = true;
   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();
 
   // If we are not resuming decoding that means the decoder is freshly
   // created and we have ownership. If we are resuming decoding then
   // the decoder is owned by ImageDecodingStore.
   std::unique_ptr<ImageDecoder> decoderContainer;
   if (!resumeDecoding)
     decoderContainer = WTF::wrapUnique(decoder);
@@ skipping 44 matching lines @@
       m_hasAlpha[i] = true;
   }
   m_hasAlpha[index] = hasAlpha;
 }
 
 bool ImageFrameGenerator::decode(SegmentReader* data,
                                  bool allDataReceived,
                                  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);
   bool newDecoder = false;
   bool shouldCallSetData = true;
   if (!*decoder) {
     newDecoder = true;
     if (m_imageDecoderFactory)
       *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.
       shouldCallSetData = false;
     }
 
     if (!*decoder)
       return false;
   }
 
-
   if (shouldCallSetData)
     (*decoder)->setData(data, allDataReceived);
 
   bool usingExternalAllocator = false;
 
   // For multi-frame image decoders, we need to know how many frames are
   // in that image in order to release the decoder when all frames are
   // decoded. frameCount() is reliable only if all data is received and set in
   // decoder, particularly with GIF.
   if (allDataReceived) {
@@ skipping 22 matching lines @@
     return false;
 
   // A cache object is considered complete if we can decode a complete frame.
   // Or we have received all data. The image might not be fully decoded in
   // the latter case.
   const bool isDecodeComplete =
       frame->getStatus() == ImageFrame::FrameComplete || allDataReceived;
 
   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;
 }
 
 bool ImageFrameGenerator::hasAlpha(size_t index) {
   MutexLocker lock(m_alphaMutex);
   if (index < m_hasAlpha.size())
     return m_hasAlpha[index];
   return true;
 }
 
 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;
 
   std::unique_ptr<ImageDecoder> decoder = ImageDecoder::create(
       data, true, ImageDecoder::AlphaPremultiplied, m_decoderColorBehavior);
   if (!decoder)
     return false;
 
   // Setting a dummy ImagePlanes object signals to the decoder that we want to
   // do YUV decoding.
   std::unique_ptr<ImagePlanes> dummyImagePlanes =
       WTF::wrapUnique(new ImagePlanes);
   decoder->setImagePlanes(std::move(dummyImagePlanes));
 
   return updateYUVComponentSizes(decoder.get(), sizeInfo->fSizes,
                                  sizeInfo->fWidthBytes);
 }
 
 }  // namespace blink