JPEGImageDecoder RGB565 and downsample support and related Skia imagegenerator changes

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 b332fd9d54f678d45843a7c906dc1d3ff6805cb7..3b29a47ee3e6abb65d0afd9fc57f98248844b922 100644
--- a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
@@ -143,7 +143,7 @@ bool ImageFrameGenerator::decodeAndScale(const SkImageInfo& info, size_t index,
 
     m_externalAllocator = adoptPtr(new ExternalMemoryAllocator(info, pixels, rowBytes));
 
-    SkBitmap bitmap = tryToResumeDecode(scaledSize, index);
+    SkBitmap bitmap = tryToResumeDecode(scaledSize, index, info.colorType());
     if (bitmap.isNull())
         return false;
 
@@ -205,7 +205,7 @@ bool ImageFrameGenerator::decodeToYUV(SkISize componentSizes[3], void* planes[3]
     return yuvDecoded;
 }
 
-SkBitmap ImageFrameGenerator::tryToResumeDecode(const SkISize& scaledSize, size_t index)
+SkBitmap ImageFrameGenerator::tryToResumeDecode(const SkISize& scaledSize, size_t index, SkColorType outType)
 {
     TRACE_EVENT1("blink", "ImageFrameGenerator::tryToResumeDecodeAndScale", "index", static_cast<int>(index));
 
@@ -214,7 +214,7 @@ SkBitmap ImageFrameGenerator::tryToResumeDecode(const SkISize& scaledSize, size_
     ASSERT(!resumeDecoding || decoder);
 
     SkBitmap fullSizeImage;
-    bool complete = decode(index, &decoder, &fullSizeImage);
+    bool complete = decode(index, &decoder, &fullSizeImage, outType);
 
     if (!decoder)
         return SkBitmap();
@@ -280,19 +280,17 @@ void ImageFrameGenerator::setHasAlpha(size_t index, bool hasAlpha)
     m_hasAlpha[index] = hasAlpha;
 }
 
-bool ImageFrameGenerator::decode(size_t index, ImageDecoder** decoder, SkBitmap* bitmap)
+bool ImageFrameGenerator::decode(size_t index, ImageDecoder** decoder, SkBitmap* bitmap, SkColorType outType)
 {
     TRACE_EVENT2("blink", "ImageFrameGenerator::decode", "width", m_fullSize.width(), "height", m_fullSize.height());
 
     ASSERT(decoder);
     SharedBuffer* data = 0;
     bool allDataReceived = false;
-    bool newDecoder = false;
     m_data.data(&data, &allDataReceived);
 
     // Try to create an ImageDecoder if we are not given one.
     if (!*decoder) {
-        newDecoder = true;
         if (m_imageDecoderFactory)
             *decoder = m_imageDecoderFactory->create().leakPtr();
 
@@ -303,15 +301,23 @@ bool ImageFrameGenerator::decode(size_t index, ImageDecoder** decoder, SkBitmap*
             return false;
     }
 
-    if (!m_isMultiFrame && newDecoder && allDataReceived) {
+    if (!m_isMultiFrame && allDataReceived) {

[scroggo_chromium, 2015/10/19 20:41:36]

It seems like removing newDecoder is a separate behavior change?

[aleksandar.stojiljkovic, 2015/10/20 09:51:12]

Yes.
That code was originally contributed here:
https://codereview.chromium.org/141483004/diff/480001/Source/platform/graphic...
Target back then was apparently to prevent switching allocators in ImageFrame
between SkBitmap pixels and discardable memory allocator.

What it does:
for single frame decoding, it just end up setting ImageFrame::m_allocator, used
only in ImageFrame::setSize (calling m_bitmap.tryAllocPixels(m_allocator, 0))
when decoder wants to write decoded content.

There is no problem with setting SkBitmap to different SkImageInfo and calling
multiple times tryAllocPixels (pixel ref unref called).

Note that few lines bellow call to
(*decoder)->setMemoryAllocator(m_externalAllocator.get()); there is a call
(*decoder)->setMemoryAllocator(0);



Another thing I didn't mention:
There is a related change in skia I used when testing this, planning to send for
review later today in 
bool SkResourceCacheDiscardableAllocator::allocPixelRef... allowing also rgb565

    // can we relax this?
    if (kN32_SkColorType != bitmap->colorType() && kRGB_565_SkColorType !=
bitmap->colorType()) {

[scroggo_chromium, 2015/10/20 15:06:41]

Does it need to be tied to the change to support 565?
Agreed. But here's where I do not follow:

- AFAICT, setSize is *not* called a second time. (Am I missing something?) So
while it may be fine to set this new allocator, it seems that it does not do
anything (in the case of a decoder where setSize has already been called).
- If setSize were to be called again, we would need to start the decode over
again. Again, I do not see where that happens.

FWIW, it's been suggested that we should write directly to the discardable
memory always i.e. use the external allocator (e.g.
https://groups.google.com/a/chromium.org/d/msg/scheduler-dev/HksAXxoT6M0/8-9s...),
but this means we also need to tell the decoders to start over if the memory is
no longer available.
I don't remember why that only supports kN32. It didn't need to support 565, but
it's possible that it also interacts with code that assumes N32, so they need to
be updated together.

[aleksandar.stojiljkovic, 2015/10/20 18:28:41]

It makes this use case possible: images originally decoded to 8888, pixel refs
kept in discardable. "Low discardable" memory situation detected - and same
bitmaps from SkBitmapCache get decoded through SkImageCacherator to 565 instead.
It is not necessary - just the way I'm implementing it now in skia. And,
understand it is difficult to review this without skia patch, so I target to
submit it for review too later today.

[aleksandar.stojiljkovic, 2015/10/20 18:57:56]

It is also required for decode to display resolution (downsample on decode) work
I'm working on - on different zooms, different size would be decoded for same
ImageFrame.

         // If we're using an external memory allocator that means we're decoding
         // directly into the output memory and we can save one memcpy.
         ASSERT(m_externalAllocator.get());
         (*decoder)->setMemoryAllocator(m_externalAllocator.get());
     }
     (*decoder)->setData(data, allDataReceived);
-
+    if (outType == kRGB_565_SkColorType) {
+        if (!(*decoder)->setDecodeRGB565Enabled(true)) {
+            ASSERT(false);
+            return false;
+        }
+    }
     ImageFrame* frame = (*decoder)->frameBufferAtIndex(index);
+    // restore back to default
+    (*decoder)->setDecodeRGB565Enabled(false);

[scroggo_chromium, 2015/10/19 20:41:36]

What happens if the caller tries to switch back and forth between decoding to
565 and 8888?

I think we need to either clear the ImageFrame or fail, and I have not found how
that happens. (I would expect this call to either fail or destroy whatever data
is in the ImageFrame you just retrieved from the decoder.)

[aleksandar.stojiljkovic, 2015/10/20 09:51:12]

clear the image frame happens in call to ImageFrame::setSize (decoder calling
it)

Yes, this is the purpose of this change:
1) switching 8888 -> 565 (while testing I was using that use case) 
2) switching between different sizes (downsampled [1-7] / 8 for JPEG)
(performance comparison explaining benefit of downsampled decoding I put here:
https://code.google.com/p/chromium/issues/detail?id=438323#c38)

newDecoder was added to prevent someone calling different types of allocators
set, but then, it is not a problem either, nor there is memory leak coming if
calling multiple times allocPixelRef...

[scroggo_chromium, 2015/10/20 15:06:41]

Agreed. But ImageDecoder only calls setSize if it is decoding from the beginning
(e.g. m_state is JPEG_HEADER). Where do we tell the ImageDecoder to start over?

[aleksandar.stojiljkovic, 2015/10/20 18:28:41]

It is something I've seen happening every time on this path:
SkImage_Generator::getROPixels -> SkBitmapCache has entry but it is evicted ->
SkImageCacherator -> DecodingImageGenerator::onGetPixels ->
JPEGImageDecoder::decode(bool false) and there . 
Took me some time to check my branch isn't that stale. On the top of
JPEGImageDecoder::decode(bool false), m_reader is created (initializing state to
JPEG_HEADER) and on the end of the same method, m_reader cleared when full
decode happened. so every next time full decode is asked, it starts from
JPEG_HEADER state.  

+
     // 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
@@ -379,4 +385,27 @@ bool ImageFrameGenerator::getYUVComponentSizes(SkISize componentSizes[3])
     return updateYUVComponentSizes(decoder.get(), componentSizes, ImageDecoder::SizeForMemoryAllocation);
 }
 
+bool ImageFrameGenerator::canDecodeToRGB565()
+{
+    // as the same buffer could be used to store consecutive frames, avoid the complexity
+    // since some of consecutive frames could have alpha
+    if (m_isMultiFrame)
+        return false;
+
+    // FIXME check how much this cost (decoder creation) and, also for getYUVComponentSizes,
+    // cache it in ImageDecodingStore for the real decode
+    SharedBuffer* data = 0;
+    bool allDataReceived = false;
+    m_data.data(&data, &allDataReceived);
+
+    // partially received data needs to have alpha
+    if (!allDataReceived)
+        return false;
+
+    OwnPtr<ImageDecoder> decoder = ImageDecoder::create(*data, ImageDecoder::AlphaPremultiplied, ImageDecoder::GammaAndColorProfileApplied);
+    if (!decoder)
+        return false;
+    return decoder->canDecodeToRGB565();
+}
+
 } // namespace blink