Respect colorSpace in DecodingImageGenerator::onGetPixels()

third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h

Index: third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h
diff --git a/third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h b/third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h
index fadf40f878dda3658e4242618a976ea09cee49c5..567f792071fc9a711df4b4af43a9297ceadf7bbb 100644
--- a/third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h
+++ b/third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h
@@ -78,6 +78,7 @@ class PLATFORM_EXPORT ImageDecodingStore final {
   // found.
   bool lockDecoder(const ImageFrameGenerator*,
                    const SkISize& scaledSize,
+                   ImageDecoder::AlphaOption,
                    ImageDecoder**);
   void unlockDecoder(const ImageFrameGenerator*, const ImageDecoder*);
   void insertDecoder(const ImageFrameGenerator*, std::unique_ptr<ImageDecoder>);
@@ -96,7 +97,9 @@ class PLATFORM_EXPORT ImageDecodingStore final {
   // Decoder cache entry is identified by:
   // 1. Pointer to ImageFrameGenerator.
   // 2. Size of the image.
-  typedef std::pair<const ImageFrameGenerator*, SkISize> DecoderCacheKey;
+  // 3. Decoder alpha option
+  typedef std::pair<const ImageFrameGenerator*, std::pair<SkISize, uint32_t>>

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

Why not make this a tuple? (And why do you convert AlphaOption to a uint32_t?)

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

I'm trying to make use of the existing hash functions in "wtf/HashSet.h".

There is no support for tuples - but there probably should be.  I'll look at
adding that.

Also there is no support for AlphaOption.  But it makes sense to hash this as if
it is an int (rather than add an explicit specialization for AlphaOption). 
Actually, probably better to use uint8_t.

[scroggo_chromium, 2017/04/10 17:31:23]

sgtm

+      DecoderCacheKey;
 
   // Base class for all cache entries.
   class CacheEntry : public DoublyLinkedListNode<CacheEntry> {
@@ -141,17 +144,20 @@ class PLATFORM_EXPORT ImageDecodingStore final {
     static std::unique_ptr<DecoderCacheEntry> create(
         const ImageFrameGenerator* generator,
         std::unique_ptr<ImageDecoder> decoder) {
+      ImageDecoder::AlphaOption alphaOption = decoder->alphaOption();
       return WTF::wrapUnique(
-          new DecoderCacheEntry(generator, 0, std::move(decoder)));
+          new DecoderCacheEntry(generator, 0, std::move(decoder), alphaOption));
     }
 
     DecoderCacheEntry(const ImageFrameGenerator* generator,
                       int count,
-                      std::unique_ptr<ImageDecoder> decoder)
+                      std::unique_ptr<ImageDecoder> decoder,
+                      ImageDecoder::AlphaOption alphaOption)
         : CacheEntry(generator, count),
           m_cachedDecoder(std::move(decoder)),
           m_size(SkISize::Make(m_cachedDecoder->decodedSize().width(),
-                               m_cachedDecoder->decodedSize().height())) {}
+                               m_cachedDecoder->decodedSize().height())),
+          m_alphaOption(alphaOption) {}

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

Can you move the call to decoder->alphaOption() here? I'm assuming that decoder
should never be null. If it were, there wouldn't be a point to this Entry.

Which makes me wonder a little more - why is this constructor public? It looks
like it's only called from the public create(); it seems like that method could
check for null (if necessary) and this constructor could be private.

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

|decoder| is deleted by std::move(), so we would end up with code that depends
on the order of evaluation of the arguments.
I'll make it private.

[scroggo_chromium, 2017/04/10 17:31:23]

We already call m_cachedDecoder->decodedSize() - doesn't that also depend on the
order of the evaluation of the arguments?

I think this ordering is well defined though - it matches the order that the
members are declared in. (In fact, you'll get a warning if the order here does
not match the order of declaration.) So you can safely change this to

  m_alphaOption(m_cachedDecoder->alphaOption())

(You would be in trouble if you instead had something like the following:

  return new DecoderCacheEntry(gen, count, std::move(decoder),
decoder->alphaOption());

That ordering is not defined.)

 
     size_t memoryUsageInBytes() const override {
       return m_size.width() * m_size.height() * 4;
@@ -159,23 +165,27 @@ class PLATFORM_EXPORT ImageDecodingStore final {
     CacheType type() const override { return TypeDecoder; }
 
     static DecoderCacheKey makeCacheKey(const ImageFrameGenerator* generator,
-                                        const SkISize& size) {
-      return std::make_pair(generator, size);
+                                        const SkISize& size,
+                                        ImageDecoder::AlphaOption alphaOption) {
+      return std::make_pair(generator,
+                            std::make_pair(size, (uint32_t)alphaOption));
     }
     static DecoderCacheKey makeCacheKey(const ImageFrameGenerator* generator,
                                         const ImageDecoder* decoder) {
-      return std::make_pair(generator,
-                            SkISize::Make(decoder->decodedSize().width(),
-                                          decoder->decodedSize().height()));
+      return makeCacheKey(generator,
+                          SkISize::Make(decoder->decodedSize().width(),
+                                        decoder->decodedSize().height()),
+                          decoder->alphaOption());
     }
     DecoderCacheKey cacheKey() const {
-      return makeCacheKey(m_generator, m_size);
+      return makeCacheKey(m_generator, m_size, m_alphaOption);
     }
     ImageDecoder* cachedDecoder() const { return m_cachedDecoder.get(); }
 
    private:
     std::unique_ptr<ImageDecoder> m_cachedDecoder;
     SkISize m_size;
+    ImageDecoder::AlphaOption m_alphaOption;
   };
 
   ImageDecodingStore();