Respect colorSpace in DecodingImageGenerator::onGetPixels()

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

 /*
  * 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 60 matching lines @@
   }
   ~ImageDecodingStore();
 
   static ImageDecodingStore& instance();
 
   // Accesses a cached decoder object. A decoder is indexed by origin
   // (ImageFrameGenerator) and scaled size. Returns true if the cached object is
   // 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>);
   void removeDecoder(const ImageFrameGenerator*, const ImageDecoder*);
 
   // Remove all cache entries indexed by ImageFrameGenerator.
   void removeCacheIndexedByGenerator(const ImageFrameGenerator*);
 
   void clear();
   void setCacheLimitInBytes(size_t);
   size_t memoryUsageInBytes();
   int cacheEntries();
   int decoderCacheEntries();
 
  private:
   // 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> {
     USING_FAST_MALLOC(CacheEntry);
     WTF_MAKE_NONCOPYABLE(CacheEntry);
     friend class WTF::DoublyLinkedListNode<CacheEntry>;
 
    public:
     enum CacheType {
       TypeDecoder,
@@ skipping 24 matching lines @@
    private:
     CacheEntry* m_prev;
     CacheEntry* m_next;
   };
 
   class DecoderCacheEntry final : public CacheEntry {
    public:
     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;
     }
     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();
 
   void prune();
 
   // These helper methods are called while m_mutex is locked.
   template <class T, class U, class V>
   void insertCacheInternal(std::unique_ptr<T> cacheEntry,
                            U* cacheMap,
@@ skipping 57 matching lines @@
   //   m_heapLimitInBytes
   //   m_heapMemoryUsageInBytes
   // This mutex also protects calls to underlying skBitmap's
   // lockPixels()/unlockPixels() as they are not threadsafe.
   Mutex m_mutex;
 };
 
 }  // namespace blink
 
 #endif