JPEGImageDecoder RGB565 and downsample support and related Skia imagegenerator changes

third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp

Index: third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
index 3b75b8c146cb4f2e29ea3f220fac0f9797a9cdfd..68768ae22dfc6f37251bd6fde548591c29b70650 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp
@@ -46,7 +46,6 @@ ImageFrame& ImageFrame::operator=(const ImageFrame& other)
 {
     if (this == &other)
         return *this;
-

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

nit: Why did this line get removed?

     m_bitmap = other.m_bitmap;
     // Keep the pixels locked since we will be writing directly into the
     // bitmap throughout this object's lifetime.
@@ -94,14 +93,19 @@ bool ImageFrame::copyBitmapData(const ImageFrame& other)
     return other.m_bitmap.copyTo(&m_bitmap, other.m_bitmap.colorType());
 }
 
-bool ImageFrame::setSize(int newWidth, int newHeight)
+bool ImageFrame::setSize(int newWidth, int newHeight, ColorType bitmapType)
 {
-    // setSize() should only be called once, it leaks memory otherwise.

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

Was this removed because you intend to call it more than once?

I don't see where you call it more than once, but it does seem like you need to
e.g.:

1. receive partial data
2. decode to 8888 (so we can have transparent pixels)
3. receive full data

If we want to decode to 565 (which seems wasteful, since we've already done some
of the work to decode to 8888 and allocated the memory for it), we would need to
reinitialize the bitmap.

(FWIW, I think that comment is out of date. Calling setInfo and tryAllocPixels
will free the memory that was being used and allocate a new block. It is
wasteful, though, if we don't need to call it again...)

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

In current usage, allocation happens in Skia on SkOneShotDiscardablePixelRef -
and there it is often disposed after unpinning it from shared memory.
So, SkResourceCache is reallocating new memory for new image in skia, and that
chunk of memory is here used as external allocator - as it is only called here
from DecodingImageGenerator::onGetPixels
when all data is received and this called from skia
(DecodingImageGenerator::onGetPixels) bitmap dat is already disposed by
discardable cache, new pixel data already allocated within skia (for another
SkBitmap) and this SkBitmap serves just to write to skia's one
(SkOneShotDiscardable) pixel ref.

Yes, comment was out of date - lot of changes meanwhile on this code once the
discardable memory and no copy pixels was introduced. I could nmake another
patch later to update comments and documentation.

-    ASSERT(!width() && !height());
+    if (bitmapType == RGBA8888) {
+        m_bitmap.setInfo(SkImageInfo::MakeN32Premul(newWidth, newHeight));
+    } else if (bitmapType == RGB565) {
+        m_bitmap.setInfo(SkImageInfo::Make(newWidth, newHeight, kRGB_565_SkColorType, kOpaque_SkAlphaType));
+    } else {
+        ASSERT(false);
+    }
 
-    m_bitmap.setInfo(SkImageInfo::MakeN32Premul(newWidth, newHeight));
-    if (!m_bitmap.tryAllocPixels(m_allocator, 0))
+    if (!m_bitmap.tryAllocPixels(m_allocator, 0)) {

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

I prefer this style personally, but AFAIK braces are discouraged for one line
bodies. (At least, I've been asked not to use/add them. From
https://www.chromium.org/blink/coding-style#TOC-Braces, it appears they are
allowed but not required.)

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

Done.

         return false;
+    }
 
     zeroFillPixelData();
     return true;