JPEGImageDecoder RGB565 and downsample support and related Skia imagegenerator changes

JPEGImageDecoder RGB565&downsample support and related Skia imagegenerator changes

This is part of effort related to better handling large res images:
 decoding bitmaps to displayed resolution (not to fullsize as now),
 using other than RGBA8888 where acceptable...
See also: Multiple large (~2800x2000) jpegs on page evict each other
 from discardable cache and  deteriorate performance (re-decoding) -
 Issues 438323, 472630

Related Skia changes:
 Handling large jpegs in CPU raster - downsample to display res and 565
 https://codereview.chromium.org/1412423008/  
 Proposal for SkImageGenerator::canDecodeAndScale API
 https://codereview.chromium.org/1404923004/

This code path is going to be activated from Skia,... it is compromise between
 quality (image affinity loss) vs. SkResourceCache behavior. Tweaking and
 measurement before activation.

BUG=438323, 472630

[aleksandar.stojiljkovic, 2015-10-19 07:44:45]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ fmalita@chromium.org, noel@google.com, reed@google.com, scroggo@google.com

[reed1, 2015-10-19 17:18:55]

Is this spec'd to dither down to 565?

[aleksandar.stojiljkovic, 2015-10-19 18:17:10]

reed1, about dither:

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:197: *dest =
SkDitherPack888ToRGB16(r, g, b);
Dither: Don't know if it makes sense to use ordered dithering here.
Floyd-Steinberg was set for all decoding (so this too).
For clarification, this path is executed when there is color profile (e.g. when
QCMS is in use and then output is set to rgb), so when output is RGB or CMYK.

This path is not executed when jpeg output is JCS_RGB565 - scanlines are
directly outputted to SkBitmap's rows.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
(right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:565:
m_info.dither_mode = JDITHER_FS;
Dither: Floyd-Steinberg set here for all decoding.

[scroggo_chromium, 2015-10-19 20:41:35]

scroggo@chromium.org changed reviewers:
+ scroggo@chromium.org

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

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
(right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:128: bool
DecodingImageGenerator::onCanDecodeAndScale(const SkColorType targetType, const
SkScalar scale, SkISize *availableSize, SkISize *lowerSize)
nit: I believe the "*" should go next to the type, rather than the name. (I do
not see this explicitly in the Blink style guide, though its examples use that
style, and Chromium's style guide requires it. It also matches the surrounding
code.)

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:133:
return DecodingImageGenerator::onCanDecodeAndScale(targetType, scale,
availableSize, lowerSize);
This looks like infinite recursion, unless I'm missing something? Did you mean
to call SkImageGenerator::onCanDecodeAndScale?

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:304: if
(!m_isMultiFrame && allDataReceived) {
It seems like removing newDecoder is a separate behavior change?

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:319:
(*decoder)->setDecodeRGB565Enabled(false);
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.)

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:103: SkBitmap
tryToResumeDecode(const SkISize& scaledSize, size_t index, SkColorType outType);
Maybe this should use an SkImageInfo, and combine the color type and size? Or
would that be confusing since we ignore the other pieces (profile and alpha)?

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (left):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:49: 
nit: Why did this line get removed?

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:99: //
setSize() should only be called once, it leaks memory otherwise.
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...)

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:106: if
(!m_bitmap.tryAllocPixels(m_allocator, 0)) {
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.)

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:119: // called
from decoder before start writing pixel data
I don't see how this says anything different from "Must be called before any
pixels are written", above.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
(right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:481:
m_info.out_color_space = JCS_RGB;
Isn't that the same as rgbOutputColorSpace when JCS_ALPHA_EXTENSIONS is not
defined? So isn't this line unnecessary?

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:491:
// fell through
nit: Other places in this file say "FALL THROUGH". I think this should be
consistent.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:608:

Why the extra line here?

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:865:
// Calling isSizeAvailable() ensures the reader is created and the output
I do not follow how this comment applies.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:877:
m_decodeToRGB565 = enable;
Should m_decodeToRGB565 only be defined if JCS_ALPHA_EXTENSIONS is defined?
Otherwise it seems like we do not need it.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:880:
if (!enable)
Can this just be:

  return !enable

and then drop the extra return at the end?

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:1080:
// for RGB565 this would just fail, having bitmap opaque during decoding
So doesn't this need to say something like if (!565)?

[aleksandar.stojiljkovic, 2015-10-20 09:51:13]

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
(right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:128: bool
DecodingImageGenerator::onCanDecodeAndScale(const SkColorType targetType, const
SkScalar scale, SkISize *availableSize, SkISize *lowerSize)
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> nit: I believe the "*" should go next to the type, rather than the name. (I do
> not see this explicitly in the Blink style guide, though its examples use that
> style, and Chromium's style guide requires it. It also matches the surrounding
> code.)

Done.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:133:
return DecodingImageGenerator::onCanDecodeAndScale(targetType, scale,
availableSize, lowerSize);
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> This looks like infinite recursion, unless I'm missing something? Did you mean
> to call SkImageGenerator::onCanDecodeAndScale?

Done.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:304: if
(!m_isMultiFrame && allDataReceived) {
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> It seems like removing newDecoder is a separate behavior change?

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()) {

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:319:
(*decoder)->setDecodeRGB565Enabled(false);
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> 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.)

>clear the ImageFrame
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...

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:103: SkBitmap
tryToResumeDecode(const SkISize& scaledSize, size_t index, SkColorType outType);
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> Maybe this should use an SkImageInfo, and combine the color type and size? Or
> would that be confusing since we ignore the other pieces (profile and alpha)?

This is private code for this class.
It comes back to proposal about SkImageGenerator::canDecodeAndScale API (to put
SkImageInfo there) and if Alpha should be part specified or implicitly covered
by RGB565.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (left):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:99: //
setSize() should only be called once, it leaks memory otherwise.
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> 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...)
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


>we would need to reinitialize the bitmap.
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.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:106: if
(!m_bitmap.tryAllocPixels(m_allocator, 0)) {
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> 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.)

Done.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.h (right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.h:119: // called
from decoder before start writing pixel data
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> I don't see how this says anything different from "Must be called before any
> pixels are written", above.

Updating it, "Must only be called once" is not needed.
My intention was to document usage in current code - that it is called from
decoder.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
(right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:481:
m_info.out_color_space = JCS_RGB;
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> Isn't that the same as rgbOutputColorSpace when JCS_ALPHA_EXTENSIONS is not
> defined? So isn't this line unnecessary?

Correct, not needed to set to same.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:491:
// fell through
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> nit: Other places in this file say "FALL THROUGH". I think this should be
> consistent.

Done.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:865:
// Calling isSizeAvailable() ensures the reader is created and the output
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> I do not follow how this comment applies.
Didn't do cleanup after copy paste.
Also decoding works without #if defined(JCS_ALPHA_EXTENSIONS). I'm
over-complicating it, here and bellow. 
Done.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:877:
m_decodeToRGB565 = enable;
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> Should m_decodeToRGB565 only be defined if JCS_ALPHA_EXTENSIONS is defined?
> Otherwise it seems like we do not need it.

Done.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:880:
if (!enable)
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> Can this just be:
> 
>   return !enable
> 
> and then drop the extra return at the end?

Done.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:1080:
// for RGB565 this would just fail, having bitmap opaque during decoding
On 2015/10/19 20:41:36, scroggo_chromium wrote:
> So doesn't this need to say something like if (!565)?
Done.

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

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:304: if
(!m_isMultiFrame && allDataReceived) {
On 2015/10/20 09:51:12, aleksandar.stojiljkovic wrote:
> On 2015/10/19 20:41:36, scroggo_chromium wrote:
> > It seems like removing newDecoder is a separate behavior change?
> 
> Yes.

Does it need to be tied to the change to support 565?

> 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).

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.

> 
> 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()) {

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.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:319:
(*decoder)->setDecodeRGB565Enabled(false);
On 2015/10/20 09:51:12, aleksandar.stojiljkovic wrote:
> On 2015/10/19 20:41:36, scroggo_chromium wrote:
> > 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.)
> 
> >clear the ImageFrame
> clear the image frame happens in call to ImageFrame::setSize (decoder calling
> it)

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?

> 
> 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...

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

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:304: if
(!m_isMultiFrame && allDataReceived) {
On 2015/10/20 15:06:41, scroggo_chromium wrote:
> On 2015/10/20 09:51:12, aleksandar.stojiljkovic wrote:
> > On 2015/10/19 20:41:36, scroggo_chromium wrote:
> > > It seems like removing newDecoder is a separate behavior change?
> > 
> > Yes.
> 
> Does it need to be tied to the change to support 565?
> 
> > 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).
> 
> 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.
> 
> > 
> > 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()) {
> 
> 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.
> 

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.

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:319:
(*decoder)->setDecodeRGB565Enabled(false);
> 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?
> 
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.

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

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1403393004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:304: if
(!m_isMultiFrame && allDataReceived) {
On 2015/10/20 18:28:41, aleksandar.stojiljkovic wrote:
> On 2015/10/20 15:06:41, scroggo_chromium wrote:
> > On 2015/10/20 09:51:12, aleksandar.stojiljkovic wrote:
> > > On 2015/10/19 20:41:36, scroggo_chromium wrote:
> > > > It seems like removing newDecoder is a separate behavior change?
> > > 
> > > Yes.
> > 
> > Does it need to be tied to the change to support 565?
> > 
> > > 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).
> > 
> > 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.
> > 
> > > 
> > > 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()) {
> > 
> > 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.
> > 
> 
> 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.

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.

[aleksandar.stojiljkovic, 2015-10-23 09:14:08]

Description was changed from

==========
JPEGImageDecoder RGB565 support and related Skia imagegenerator changes

This is part of effort related to better handling large res images:
 decoding bitmaps to displayed resolution (not to fullsize as now),
 using other than RGBA8888 where acceptable...
See also: Multiple large (~2800x2000) jpegs on page evict each other
 from discardable cache and  deteriorate performance (re-decoding) -
 Issues 438323, 472630

Related Skia API change (overriden in this CL):
 Proposal for SkImageGenerator::canDecodeAndScale API
 https://codereview.chromium.org/1404923004/

This code path is going to be activated from Skia,... it is compromise between
 quality (image affinity loss) vs. SkResourceCache behavior. Tweaking and
 measurement before activation.

BUGS=438323,472630
BUG=
==========

to

==========
JPEGImageDecoder RGB565&downsample support and related Skia imagegenerator
changes

This is part of effort related to better handling large res images:
 decoding bitmaps to displayed resolution (not to fullsize as now),
 using other than RGBA8888 where acceptable...
See also: Multiple large (~2800x2000) jpegs on page evict each other
 from discardable cache and  deteriorate performance (re-decoding) -
 Issues 438323, 472630

Related Skia changes:
 Handling large jpegs in CPU raster - downsample to display res and 565
 https://codereview.chromium.org/1412423008/  
 Proposal for SkImageGenerator::canDecodeAndScale API
 https://codereview.chromium.org/1404923004/

This code path is going to be activated from Skia,... it is compromise between
 quality (image affinity loss) vs. SkResourceCache behavior. Tweaking and
 measurement before activation.

BUG=438323,472630
==========

[aleksandar.stojiljkovic, 2015-10-23 09:19:10]

Combining 565 with downsampling, in the same review, since it didn't
significantly increase the code size for review and it fixes the issues. 
Scroggo, Reed, apologize for extending the scope to downsample too, during
ongoing review.

[scroggo_chromium, 2015-10-28 19:08:47]

scroggo@chromium.org changed reviewers:
+ msarett@chromium.org

[scroggo_chromium, 2015-10-28 19:08:48]

https://codereview.chromium.org/1403393004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/1403393004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:111:
zeroFillPixelData();
This will erase the background to black, in the case of 565. If the entire image
is available, we'll write over it, but if the image is only partially available,
we'll end up with black at the bottom instead of transparent. (I know we skip
565 in this CL if allDataReceived is not true, but I assume it's possible that
all the data was received but the image itself was incomplete. If that happens,
we'll report that the image decoder has failed, which prevents it from doing any
more work, but does not appear to prevent any callers from drawing that bitmap.)
I don't know whether such a change is good, bad, or indifferent, but we should
be aware of it.

In addition, this will set m_hasAlpha to true, which I think you do not want to
do (maybe it gets set back to false later, so it does not matter).

[aleksandar.stojiljkovic, 2015-10-29 13:06:43]

https://codereview.chromium.org/1403393004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/1403393004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:111:
zeroFillPixelData();
On 2015/10/28 19:08:47, scroggo_chromium wrote:
> This will erase the background to black, in the case of 565. If the entire
image
> is available, we'll write over it, but if the image is only partially
available,
> we'll end up with black at the bottom instead of transparent. (I know we skip
> 565 in this CL if allDataReceived is not true, but I assume it's possible that
> all the data was received but the image itself was incomplete. If that
happens,
The initial idea was to let image generators decode partial to 8888 until it is
successful. By that time, we would know:
- if image is complete
- for png, if it really has alpha so that it can be decoded to 565 (next time
there is a request to decode it).
Not handling here the case that there once was complete data and for some reason
it is not complete anymore. Can it happen?

> we'll report that the image decoder has failed, which prevents it from doing
any
> more work, but does not appear to prevent any callers from drawing that
bitmap.)
> I don't know whether such a change is good, bad, or indifferent, but we should
> be aware of it.
> 
> In addition, this will set m_hasAlpha to true, which I think you do not want
to
> do (maybe it gets set back to false later, so it does not matter).

Will clean this part about alpha. Thanks.