JPEGImageDecoder RGB565 and downsample support and related Skia imagegenerator changes

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

 /*
  * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
  * Copyright (C) 2008, 2009 Google, Inc.
  *
  * 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
@@ skipping 28 matching lines @@
     , m_premultiplyAlpha(true)
     , m_pixelsChanged(false)
     , m_requiredPreviousFrameIndex(kNotFound)
 {
 }
 
 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.
     m_bitmap.lockPixels();
     // Be sure to assign this before calling setStatus(), since setStatus() may
     // call notifyBitmapIfPixelsChanged().
     m_pixelsChanged = other.m_pixelsChanged;
     setMemoryAllocator(other.allocator());
     setOriginalFrameRect(other.originalFrameRect());
     setStatus(other.status());
@@ skipping 27 matching lines @@
 bool ImageFrame::copyBitmapData(const ImageFrame& other)
 {
     if (this == &other)
         return true;
 
     m_hasAlpha = other.m_hasAlpha;
     m_bitmap.reset();
     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;
 }
 
 const SkBitmap& ImageFrame::bitmap() const
 {
     return m_bitmap;
 }
 
@@ skipping 29 matching lines @@
 void ImageFrame::zeroFillFrameRect(const IntRect& rect)
 {
     if (rect.isEmpty())
         return;
 
     m_bitmap.eraseArea(rect, SkColorSetARGB(0, 0, 0, 0));
     setHasAlpha(true);
 }
 
 } // namespace blink