Never consolidate data in ICO decoder

Never consolidate data in ICO decoder

Follows the same approach as BMPImageDecoder: Use a
FastSharedBufferReader to avoid consolidation.

Add an optional offset to PNGImageDecoder for ICOImageDecoder, so that
PNGImageDecoder can read a PNG subimage in an ICO without copying the
data. This eliminates another call to SharedBuffer::data(), as well as
a copy to create the separate SharedBuffer for PNG.

BUG=467772
BUG=528625
Committed: https://crrev.com/0bad3920ae6c1274d35c95281afcfbab8cc33a78
git-svn-id: svn://svn.chromium.org/blink/trunk@201981 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[scroggo_chromium, 2015-09-08 17:53:20]

scroggo@chromium.org changed reviewers:
+ noel@chromium.org, pkasting@chromium.org

[scroggo_chromium, 2015-09-08 17:53:20]

This tacks on to crrev.com/1259083003. One thing I did not consider when I made
that change is that BMPImageReader now relies on the fact that no one is calling
SharedBuffer::data() (in its FastSharedBufferReader, which caches the segment).
That is true for BMPImageDecoder, but *not* for ICOImageDecoder.

This fixes the problem in ICOImageDecoder - it will no longer make calls to
consolidate the data, so BMPImageReader's assumption is now fine.

I have not yet done performance tests on this change; this does not change
anything interestingly different from the BMP change, so I would be surprised to
see a different outcome, but I'll be happy to benchmark, if you like (we do not
appear to have very many valid .ico's checked in to src-internal, but there are
a few in the LayoutTests dir (still not very many)).

[Peter Kasting, 2015-09-08 20:03:13]

LGTM

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
File Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right):

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/ico/ICOImageDecoder.cpp:188:
GammaAndColorProfileOption colorOptions = m_ignoreGammaAndColorProfile ?
GammaAndColorProfileIgnored : GammaAndColorProfileApplied;
Nit: I'd name this "option" instead of "options" to match the singularity of the
type name

(Although I might omit these and just reformat like this:)

        m_pngDecoders[index] = adoptPtr(new PNGImageDecoder(
            m_premultiplyAlpha ? AlphaPremultiplied : AlphaNotPremultiplied,
            m_ignoreGammaAndColorProfile ? GammaAndColorProfileIgnored :
GammaAndColorProfileApplied,
            m_maxDecodedBytes,
            dirEntry.m_imageOffset));

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/ico/ICOImageDecoder.cpp:190: //
dirEntry.m_imageOffset is known to be correct, since this function is never
called unless decodeDirectory succeeds.
Nit: This comment is probably unnecessary, we assume similar ordering
requirements many places.

I would probably remove the blank lines above and below here too.

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/ico/ICOImageDecoder.cpp:258: // The following
calls to readUint8 return an uint8_t, which is appropriate
Nit: "...readUint8() return a uint8_t..." (add parent, an -> a)

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
File Source/platform/image-decoders/ico/ICOImageDecoder.h (right):

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/ico/ICOImageDecoder.h:87: // TODO (scroggo):
These functions are identical to functions in BMPImageReader. Share code?
Yeah, we should share the code.

This seems like another argument for making the buffer reader object part of
ImageDecoder.  Then we could make these functions protected members of
ImageDecoder as well.  We could also avoid having to customize setData() to
update the buffer reader in each decoder.

[scroggo_chromium, 2015-09-08 20:52:52]

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
File Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right):

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/ico/ICOImageDecoder.cpp:188:
GammaAndColorProfileOption colorOptions = m_ignoreGammaAndColorProfile ?
GammaAndColorProfileIgnored : GammaAndColorProfileApplied;
On 2015/09/08 20:03:13, Peter Kasting wrote:
> Nit: I'd name this "option" instead of "options" to match the singularity of
the
> type name

This name was suggested to me by noel@ a refactoring CL, and it matches the
other uses of this type.

> 
> (Although I might omit these and just reformat like this:)
> 
>         m_pngDecoders[index] = adoptPtr(new PNGImageDecoder(
>             m_premultiplyAlpha ? AlphaPremultiplied : AlphaNotPremultiplied,
>             m_ignoreGammaAndColorProfile ? GammaAndColorProfileIgnored :
> GammaAndColorProfileApplied,
>             m_maxDecodedBytes,
>             dirEntry.m_imageOffset));

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/ico/ICOImageDecoder.cpp:190: //
dirEntry.m_imageOffset is known to be correct, since this function is never
called unless decodeDirectory succeeds.
On 2015/09/08 20:03:13, Peter Kasting wrote:
> Nit: This comment is probably unnecessary, we assume similar ordering
> requirements many places.
> 
> I would probably remove the blank lines above and below here too.

Done.

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/ico/ICOImageDecoder.cpp:258: // The following
calls to readUint8 return an uint8_t, which is appropriate
On 2015/09/08 20:03:13, Peter Kasting wrote:
> Nit: "...readUint8() return a uint8_t..." (add parent, an -> a)

Done.

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
File Source/platform/image-decoders/ico/ICOImageDecoder.h (right):

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/ico/ICOImageDecoder.h:87: // TODO (scroggo):
These functions are identical to functions in BMPImageReader. Share code?
On 2015/09/08 20:03:13, Peter Kasting wrote:
> Yeah, we should share the code.
> 
> This seems like another argument for making the buffer reader object part of
> ImageDecoder.  Then we could make these functions protected members of
> ImageDecoder as well.  We could also avoid having to customize setData() to
> update the buffer reader in each decoder.

On the other hand, not all of the classes need a FastSharedBufferReader (PNG and
JPEG do not need one; I have not looked at WEBP yet.)

[Peter Kasting, 2015-09-08 21:01:05]

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
File Source/platform/image-decoders/ico/ICOImageDecoder.h (right):

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/ico/ICOImageDecoder.h:87: // TODO (scroggo):
These functions are identical to functions in BMPImageReader. Share code?
On 2015/09/08 20:52:51, scroggo_chromium wrote:
> On 2015/09/08 20:03:13, Peter Kasting wrote:
> > Yeah, we should share the code.
> > 
> > This seems like another argument for making the buffer reader object part of
> > ImageDecoder.  Then we could make these functions protected members of
> > ImageDecoder as well.  We could also avoid having to customize setData() to
> > update the buffer reader in each decoder.
> 
> On the other hand, not all of the classes need a FastSharedBufferReader (PNG
and
> JPEG do not need one; I have not looked at WEBP yet.)

Well, with PNG and JPEG, we rely on an external decoding library.  Doesn't that
library require consolidating the data passed to it?  So it's not so much "don't
need" as "as currently implemented these just unconditionally coalesce".

[scroggo_chromium, 2015-09-09 14:32:23]

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
File Source/platform/image-decoders/ico/ICOImageDecoder.h (right):

https://codereview.chromium.org/1316203008/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/ico/ICOImageDecoder.h:87: // TODO (scroggo):
These functions are identical to functions in BMPImageReader. Share code?
On 2015/09/08 21:01:05, Peter Kasting wrote:
> On 2015/09/08 20:52:51, scroggo_chromium wrote:
> > On 2015/09/08 20:03:13, Peter Kasting wrote:
> > > Yeah, we should share the code.
> > > 
> > > This seems like another argument for making the buffer reader object part
of
> > > ImageDecoder.  Then we could make these functions protected members of
> > > ImageDecoder as well.  We could also avoid having to customize setData()
to
> > > update the buffer reader in each decoder.
> > 
> > On the other hand, not all of the classes need a FastSharedBufferReader (PNG
> and
> > JPEG do not need one; I have not looked at WEBP yet.)
> 
> Well, with PNG and JPEG, we rely on an external decoding library.  Doesn't
that
> library require consolidating the data passed to it?  So it's not so much
"don't
> need" as "as currently implemented these just unconditionally coalesce".

Actually, no. JPEG does use an external library, but with my recent changes to
JPEGImageDecoder, libjpeg directly uses the data we give to it. So it will not
do any coalescing.

I haven't dug into PNG very much, but presumably it copies the data when we
supply it. It still doesn't need a FastSharedBufferReader. Maybe it would be
fine for it to have an unused one (it is only four fields, plus the RefPtr; it
also would mean some unnecessary calls to FastSharedBufferReader::setData, but
maybe this is all negligible).

[scroggo_chromium, 2015-09-09 14:32:27]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2015-09-09 14:32:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1316203008/#ps40001
(title: "Respond to pkasting@'s comments in patch set 2")

[commit-bot: I haz the power, 2015-09-09 14:32:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1316203008/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1316203008/40001

[commit-bot: I haz the power, 2015-09-09 15:52:09]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201981

[Noel Gordon, 2015-09-09 22:48:34]

LGTM ftr

[scroggo, 2015-09-14 13:27:24]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1338293002/ by scroggo@google.com.

The reason for reverting is: Causing crbug.com/530279. Will reland once I have
fixed the problem.

Will next revert crrev.com/1259083003 (otherwise crbug.com/528625 will show back
up).

I will reland both CLs together as one CL that fixes the problems..

[commit-bot: I haz the power, 2015-09-23 12:00:11]

Patchset 3 (id:??) landed as
https://crrev.com/0bad3920ae6c1274d35c95281afcfbab8cc33a78