Do not consolidate data in BMPImageDecoder

Do not consolidate data in BMPImageDecoder

In BMPImageDecoder, never call SharedBuffer::data(), which potentially
copies all the data. Instead, do the following:

- Read directly from the SharedBuffer when it is safe to do so. e.g.
  when only one byte is needed. We already know there is enough data,
  and getSomeData will return at least a byte if there is enough data.
- Use a FastSharedBufferReader when we may be on a segment boundary.

As a result, we make fewer copies when decoding BMPs. We never call
SharedBuffer::data(), which copies up to the entire encoded data
received so far. Occasionally we will make small copies when a two or
four byte number is on a segment boundary.

Add a setter to FastSharedBufferReader to update its SharedBuffer, so the
BMPImageReader can have one as a member, which it will reuse across calls.

Remove BIG_ENDIAN, and eliminate another memcpy that was there to support
it.

Using my modified version of ImageDecodeBench (see crrev.com/1145373003),
I see a slight speedup as compared to the old version which relied on
SharedBuffer::data(). (My data can be found here: [1].) This matches my
expectations, since the new version is copying less data.

Using a version of ImageDecodeBench with a premerged buffer (see
crrev.com/1318713005), I still see a speed improvement, although it is
very small. (See sheet 2 of the above spreadsheet).

[1] https://docs.google.com/a/google.com/spreadsheets/d/1GN2FDytKvkdZzV-WF4IEQaYQF65wa4bRxG67bNgpjAE/edit?usp=sharing

BUG=467772
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201811

[scroggo_chromium, 2015-08-31 19:07:00]

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

[scroggo_chromium, 2015-08-31 19:07:01]

Noel, I know we discussed updating ImageDecodeBench to have two modes:
pre-merged SharedBuffer (imitating the case where MemoryCache is used) and in
chunks (imitating network traffic). That is forthcoming. I had already started
collecting this data using the existing tests, which demonstrate the performance
improvement of this change.

https://chromiumcodereview.appspot.com/1259083003/diff/40001/Source/platform/...
File Source/platform/image-decoders/bmp/BMPImageReader.h (right):

https://chromiumcodereview.appspot.com/1259083003/diff/40001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:204: inline uint32_t
readCurrentPixel(FastSharedBufferReader* fastReader, int bytesPerPixel) const
This seems like an odd API to me - initially I created the
FastSharedBufferReader inside readCurrentPixel, but that seemed unfortunate to
me, since we would be unnecessarily creating a new one for each pixel on a line.

We could take it a step further, and keep a member variable of
FastSharedBufferReader. We would need to update it each time setData is called,
but that might be fine. Thoughts?

https://chromiumcodereview.appspot.com/1259083003/diff/40001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:256:
m_data->getSomeData(segment, offset);
I think there might be a tradeoff between speed and simplicity. This is
typically called multiple times in a row. We could stick with the same segment
until it is exhausted (rather than calling getSomeData over and over again), but
that would result in more complicated code. (And maybe that code would be no
faster than the checks getSomeData already does.)

The FastSharedBufferReader caches the last segment accessed - maybe that would
make this faster. We would need to pass the FastSharedBufferReader to this
function (which seems strange, since we don't take advantage of what I see as
its primary purpose - consolidating data across segment boundaries), unless we
made a FastSharedBufferReader a member of the class.

[Peter Kasting, 2015-08-31 19:53:42]

https://chromiumcodereview.appspot.com/1259083003/diff/40001/Source/platform/...
File Source/platform/image-decoders/bmp/BMPImageReader.cpp (right):

https://chromiumcodereview.appspot.com/1259083003/diff/40001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.cpp:280: if
(m_infoHeader.biSize >= 36) {
Nit: No {}

https://chromiumcodereview.appspot.com/1259083003/diff/40001/Source/platform/...
File Source/platform/image-decoders/bmp/BMPImageReader.h (right):

https://chromiumcodereview.appspot.com/1259083003/diff/40001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:37: #include <stdint.h>
Why was this header moved down?  I think it should stay at the top of the list.

https://chromiumcodereview.appspot.com/1259083003/diff/40001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:47: // endianness.
Do we actually compile on any big-endian targets?  Maybe MIPS?

I feel like much of the rest of Chromium is little-endian-only, and if so maybe
we can just simplify these and avoid some small static object allocations in
various places.

https://chromiumcodereview.appspot.com/1259083003/diff/40001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:204: inline uint32_t
readCurrentPixel(FastSharedBufferReader* fastReader, int bytesPerPixel) const
On 2015/08/31 19:07:00, scroggo_chromium wrote:
> This seems like an odd API to me - initially I created the
> FastSharedBufferReader inside readCurrentPixel, but that seemed unfortunate to
> me, since we would be unnecessarily creating a new one for each pixel on a
line.
> 
> We could take it a step further, and keep a member variable of
> FastSharedBufferReader. We would need to update it each time setData is
called,
> but that might be fine. Thoughts?

I think it would make sense to have a member FastSharedBufferReader either on
this class, or else on the owning decoder (BMP or ICO) which this class would
have access to somehow.  (The advantage of this latter idea is that the owning
decoder is what gets the setData update notifications first, so we could maybe
remove setData or even the whole concept of m_data from the reader; and once the
owning decoder has the FastSharedBufferReader, it can use it in the place you've
changed the BMPImageDecoder to create one, as well as in the ICO decoder where
you added your TODO.)

https://chromiumcodereview.appspot.com/1259083003/diff/40001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:254: unsigned bytes =
If you really want this assert, I'd do this:

#if ENABLE(ASSERT)
        unsigned bytes = m_data->getSomeData(segment, offset);
        ASSERT(bytes >= 1);
#else
        m_data->getSomeData(segment, offset);
#endif

I think this is a bit more readable and hard to accidentally break in the
future.

Otherwise, I would just remove the assert.  I don't know if it buys much.

https://chromiumcodereview.appspot.com/1259083003/diff/40001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:256:
m_data->getSomeData(segment, offset);
On 2015/08/31 19:07:00, scroggo_chromium wrote:
> I think there might be a tradeoff between speed and simplicity. This is
> typically called multiple times in a row. We could stick with the same segment
> until it is exhausted (rather than calling getSomeData over and over again),
but
> that would result in more complicated code. (And maybe that code would be no
> faster than the checks getSomeData already does.)
> 
> The FastSharedBufferReader caches the last segment accessed - maybe that would
> make this faster. We would need to pass the FastSharedBufferReader to this
> function (which seems strange, since we don't take advantage of what I see as
> its primary purpose - consolidating data across segment boundaries), unless we
> made a FastSharedBufferReader a member of the class.

It does kind of bug me that we have a method like this, since I feel like even
with caching, this is slower, and caller code already does something like what
we need by checking the length of m_data before reading.

That said, I think changing that calling code to try to do the right thing here
would actually be fairly tough to do without introducing unnecessary data
compaction.  So I don't know that we can practically do much better than what
you've written.  Therefore, I would probably just pursue the "member
FastSharedBufferReader" idea as discussed above.

[scroggo_chromium, 2015-09-01 22:50:30]

I've updated the test to have an option to decode in chunks (see
crrev.com/1318713005, which Peter has already reviewed). Running my version with
1357 byte chunks (as recommended by Noel, if my memory serves me well), my
version showed to be a little faster (page three of the spreadsheet).

For some reason reusing the same FastSharedBufferReader seemed to slow things
down a little, as noted inline. I have not figured out why, or whether the
difference is meaningful.

https://codereview.chromium.org/1259083003/diff/40001/Source/platform/image-d...
File Source/platform/image-decoders/bmp/BMPImageReader.cpp (right):

https://codereview.chromium.org/1259083003/diff/40001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.cpp:280: if
(m_infoHeader.biSize >= 36) {
On 2015/08/31 19:53:42, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/1259083003/diff/40001/Source/platform/image-d...
File Source/platform/image-decoders/bmp/BMPImageReader.h (right):

https://codereview.chromium.org/1259083003/diff/40001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.h:37: #include <stdint.h>
On 2015/08/31 19:53:42, Peter Kasting wrote:
> Why was this header moved down?  I think it should stay at the top of the
list.

The style guide disagrees
(https://www.chromium.org/blink/coding-style#TOC-include-Statements):
Includes of system headers must come after includes of other headers. This
happens to also be checked and ensured by alphabetical order, as < comes after
".

https://codereview.chromium.org/1259083003/diff/40001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.h:47: // endianness.
On 2015/08/31 19:53:42, Peter Kasting wrote:
> Do we actually compile on any big-endian targets?  Maybe MIPS?
> 
> I feel like much of the rest of Chromium is little-endian-only,

According to [1], you are correct!

From that same thread, it sounds like there at least was a planned big-endian
backend for V8, and Skia half-heartedly supports big-endian ([2]).

I thought maybe it should be a different CL, but I can make a small change here.
If we're wrong, or someone decides to add support, it should be easy to undo.

> and if so maybe
> we can just simplify these and avoid some small static object allocations in
> various places.

Are you talking about |result|? I would think this would be an automatic
allocation? Either way, removing the support for BIG_ENDIAN does eliminate some
allocations. It also removes some memcpys, which is the spirit of this CL, so it
seems like a fine change. (To be fair, this operation could be done without any
copying in the LITTLE_ENDIAN case, if we still wanted to support both.)

[1]
https://groups.google.com/a/google.com/forum/#!msg/chrome-team/vzRPeX7w6WQ/AS...
[2]
https://groups.google.com/a/google.com/forum/#!msg/skia-dev/EdzJnynHnzY/AoQ8U...

https://codereview.chromium.org/1259083003/diff/40001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.h:204: inline uint32_t
readCurrentPixel(FastSharedBufferReader* fastReader, int bytesPerPixel) const
On 2015/08/31 19:53:42, Peter Kasting wrote:
> On 2015/08/31 19:07:00, scroggo_chromium wrote:
> > This seems like an odd API to me - initially I created the
> > FastSharedBufferReader inside readCurrentPixel, but that seemed unfortunate
to
> > me, since we would be unnecessarily creating a new one for each pixel on a
> line.
> > 
> > We could take it a step further, and keep a member variable of
> > FastSharedBufferReader. We would need to update it each time setData is
> called,
> > but that might be fine. Thoughts?
> 
> I think it would make sense to have a member FastSharedBufferReader either on
> this class, or else on the owning decoder (BMP or ICO) which this class would
> have access to somehow.  (The advantage of this latter idea is that the owning
> decoder is what gets the setData update notifications first, so we could maybe
> remove setData or even the whole concept of m_data from the reader; and once
the
> owning decoder has the FastSharedBufferReader, it can use it in the place
you've
> changed the BMPImageDecoder to create one, as well as in the ICO decoder where
> you added your TODO.)

I've made the FastSharedBufferReader to the BMPImageReader. I did not
investigate moving it to the ImageDecoder. I tend to see the SharedBuffer as
belonging to the reader, rather than the ImageDecoder, which mostly does not use
it. Either way, I think that belongs in its own CL.

FWIW, I found the version with a single FastSharedBufferReader to be a little
bit slower than the version that puts it on the stack for decoding in 1357 byte
chunks. (It is still faster than the original version; see sheet 3 of the
spreadsheet in the description.) I do not know why it would be slower. It may be
noise, or maybe I've failed to consider something.

https://codereview.chromium.org/1259083003/diff/40001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.h:254: unsigned bytes =
On 2015/08/31 19:53:42, Peter Kasting wrote:
> If you really want this assert, I'd do this:
> 
> #if ENABLE(ASSERT)
>         unsigned bytes = m_data->getSomeData(segment, offset);
>         ASSERT(bytes >= 1);
> #else
>         m_data->getSomeData(segment, offset);
> #endif
> 
> I think this is a bit more readable and hard to accidentally break in the
> future.
> 
> Otherwise, I would just remove the assert.  I don't know if it buys much.

Removed.

https://codereview.chromium.org/1259083003/diff/60001/Source/platform/image-d...
File Source/platform/image-decoders/bmp/BMPImageReader.h (right):

https://codereview.chromium.org/1259083003/diff/60001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.h:122: inline uint16_t
readUint16(int offset)
Note that these are no longer const, since
FastSharedBufferReader::getConsecutiveData() is not const.

[Peter Kasting, 2015-09-02 22:08:55]

https://codereview.chromium.org/1259083003/diff/40001/Source/platform/image-d...
File Source/platform/image-decoders/bmp/BMPImageReader.h (right):

https://codereview.chromium.org/1259083003/diff/40001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.h:37: #include <stdint.h>
On 2015/09/01 22:50:30, scroggo_chromium wrote:
> On 2015/08/31 19:53:42, Peter Kasting wrote:
> > Why was this header moved down?  I think it should stay at the top of the
> list.
> 
> The style guide disagrees
> (https://www.chromium.org/blink/coding-style#TOC-include-Statements):
> Includes of system headers must come after includes of other headers. This
> happens to also be checked and ensured by alphabetical order, as < comes after
> ".

It is SO DUMB that Blink has its own style guide that flatly contradicts the
Chromium and Google ones.  Ugh.

https://codereview.chromium.org/1259083003/diff/60001/Source/platform/image-d...
File Source/platform/image-decoders/FastSharedBufferReader.h (right):

https://codereview.chromium.org/1259083003/diff/60001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReader.h:49: void
setData(SharedBuffer*);
Is it really correct that the constructor takes a PassRefPtr but this takes a
raw pointer?  I don't know Blink's pointer type semantics but that looks
strange.

https://codereview.chromium.org/1259083003/diff/60001/Source/platform/image-d...
File Source/platform/image-decoders/bmp/BMPImageReader.cpp (right):

https://codereview.chromium.org/1259083003/diff/60001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.cpp:435: 
Nit: I wouldn't add this blank line

https://codereview.chromium.org/1259083003/diff/60001/Source/platform/image-d...
File Source/platform/image-decoders/bmp/BMPImageReader.h (right):

https://codereview.chromium.org/1259083003/diff/60001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.h:122: inline uint16_t
readUint16(int offset)
On 2015/09/01 22:50:30, scroggo_chromium wrote:
> Note that these are no longer const, since
> FastSharedBufferReader::getConsecutiveData() is not const.

I wonder if FastSharedBufferReader's getters should be const and some of its
private members mutable.  It seems like maybe these APIs don't change any object
state that's caller-visible.

https://codereview.chromium.org/1259083003/diff/60001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.h:203: const char*
encodedPixel = m_fastReader.getConsecutiveData(m_decodedOffset + offset,
bytesPerPixel, buffer);
I'm not convinced this is right.  If we're only asking for 2 bytes per pixel, I
think it's possible for the data stream to not have 4 bytes in it here, which
would cause problems.

Instead I would leave everything in this function unchanged except that I would
convert case 3 to do a 3-byte call to getConsecutiveData and then memcpy out of
that.

https://codereview.chromium.org/1259083003/diff/60001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.h:210: // of the return value
here in little-endian mode, the caller
Nit: Might as well remove "in little-endian mode" and the CPU(BIG_ENDIAN) stuff
below.

https://codereview.chromium.org/1259083003/diff/60001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.h:243: // Retrieve one byte
from |m_data| at |offset|
Nit: Trailing period

https://codereview.chromium.org/1259083003/diff/60001/Source/platform/image-d...
Source/platform/image-decoders/bmp/BMPImageReader.h:246: const char* segment;
Why is this not just a call to m_fastReader.getOneByte(offset)?

[scroggo_chromium, 2015-09-02 23:01:08]

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
File Source/platform/image-decoders/FastSharedBufferReader.h (right):

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
Source/platform/image-decoders/FastSharedBufferReader.h:49: void
setData(SharedBuffer*);
On 2015/09/02 22:08:54, Peter Kasting wrote:
> Is it really correct that the constructor takes a PassRefPtr but this takes a
> raw pointer?  I don't know Blink's pointer type semantics

Me neither...

> but that looks
> strange.

Agreed. ImageDecoder::setData also takes a raw pointer, and this method is
called by that one. We could wrap that pointer...

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
File Source/platform/image-decoders/bmp/BMPImageReader.cpp (right):

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.cpp:435: 
On 2015/09/02 22:08:54, Peter Kasting wrote:
> Nit: I wouldn't add this blank line

Done.

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
File Source/platform/image-decoders/bmp/BMPImageReader.h (right):

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:122: inline uint16_t
readUint16(int offset)
On 2015/09/02 22:08:55, Peter Kasting wrote:
> On 2015/09/01 22:50:30, scroggo_chromium wrote:
> > Note that these are no longer const, since
> > FastSharedBufferReader::getConsecutiveData() is not const.
> 
> I wonder if FastSharedBufferReader's getters should be const and some of its
> private members mutable.  It seems like maybe these APIs don't change any
object
> state that's caller-visible.

That is the way I would have written; I was surprised that it wasn't written
that way! I'll see if I can figure out whether that was a deliberate decision...

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:203: const char*
encodedPixel = m_fastReader.getConsecutiveData(m_decodedOffset + offset,
bytesPerPixel, buffer);
On 2015/09/02 22:08:55, Peter Kasting wrote:
> I'm not convinced this is right.  If we're only asking for 2 bytes per pixel,
I
> think it's possible for the data stream to not have 4 bytes in it here, which
> would cause problems.

Wait, what's the problem? I originally called getConsecutiveData with a length
of 4, which yes, is a problem if we only need 2 bytes.

I am now calling getConsecutiveData with a length of bytesPerPixel, which means
that I only read the amount that is needed.

> 
> Instead I would leave everything in this function unchanged except that I
would
> convert case 3 to do a 3-byte call to getConsecutiveData and then memcpy out
of
> that.

That's essentially what is happening now.

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:210: // of the return value
here in little-endian mode, the caller
On 2015/09/02 22:08:55, Peter Kasting wrote:
> Nit: Might as well remove "in little-endian mode" and the CPU(BIG_ENDIAN)
stuff
> below.

Done.

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:243: // Retrieve one byte
from |m_data| at |offset|
On 2015/09/02 22:08:55, Peter Kasting wrote:
> Nit: Trailing period

Done.

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:246: const char* segment;
On 2015/09/02 22:08:55, Peter Kasting wrote:
> Why is this not just a call to m_fastReader.getOneByte(offset)?

Wrote this function before I had a fastReader. Done.

[Peter Kasting, 2015-09-02 23:48:44]

LGTM

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
File Source/platform/image-decoders/FastSharedBufferReader.h (right):

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
Source/platform/image-decoders/FastSharedBufferReader.h:49: void
setData(SharedBuffer*);
On 2015/09/02 23:01:07, scroggo_chromium wrote:
> On 2015/09/02 22:08:54, Peter Kasting wrote:
> > Is it really correct that the constructor takes a PassRefPtr but this takes
a
> > raw pointer?  I don't know Blink's pointer type semantics
> 
> Me neither...
> 
> > but that looks
> > strange.
> 
> Agreed. ImageDecoder::setData also takes a raw pointer, and this method is
> called by that one. We could wrap that pointer...

Definitely at least figure out precisely what the code is doing today and why. 
That way we'll know whether this difference is important.

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
File Source/platform/image-decoders/bmp/BMPImageReader.h (right):

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:203: const char*
encodedPixel = m_fastReader.getConsecutiveData(m_decodedOffset + offset,
bytesPerPixel, buffer);
On 2015/09/02 23:01:07, scroggo_chromium wrote:
> On 2015/09/02 22:08:55, Peter Kasting wrote:
> > I'm not convinced this is right.  If we're only asking for 2 bytes per
pixel,
> I
> > think it's possible for the data stream to not have 4 bytes in it here,
which
> > would cause problems.
> 
> Wait, what's the problem? I originally called getConsecutiveData with a length
> of 4, which yes, is a problem if we only need 2 bytes.
> 
> I am now calling getConsecutiveData with a length of bytesPerPixel, which
means
> that I only read the amount that is needed.
> 
> > 
> > Instead I would leave everything in this function unchanged except that I
> would
> > convert case 3 to do a 3-byte call to getConsecutiveData and then memcpy out
> of
> > that.
> 
> That's essentially what is happening now.

Ah, my bad, I didn't read the code correctly.

I still think the rework I suggest would be a little easier to understand as a
reader, since it saves the verbose getConsecutiveData() call for just one case,
but I don't care much.

https://chromiumcodereview.appspot.com/1259083003/diff/80001/Source/platform/...
File Source/platform/image-decoders/bmp/BMPImageReader.h (right):

https://chromiumcodereview.appspot.com/1259083003/diff/80001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:240: uint8_t getByte(size_t
offset)
Nit: Maybe this should be readUint8() and be up by readUint16() and
readUint32().

[scroggo_chromium, 2015-09-03 15:28:36]

https://chromiumcodereview.appspot.com/1259083003/diff/80001/Source/platform/...
File Source/platform/image-decoders/bmp/BMPImageReader.h (right):

https://chromiumcodereview.appspot.com/1259083003/diff/80001/Source/platform/...
Source/platform/image-decoders/bmp/BMPImageReader.h:240: uint8_t getByte(size_t
offset)
On 2015/09/02 23:48:44, Peter Kasting wrote:
> Nit: Maybe this should be readUint8() and be up by readUint16() and
> readUint32().

Good idea!

For consistency, it now adds m_decodedOffset as well.

[Peter Kasting, 2015-09-03 17:20:12]

Nice.  LGTM (again)

https://chromiumcodereview.appspot.com/1259083003/diff/120001/Source/platform...
File Source/platform/image-decoders/bmp/BMPImageReader.cpp (right):

https://chromiumcodereview.appspot.com/1259083003/diff/120001/Source/platform...
Source/platform/image-decoders/bmp/BMPImageReader.cpp:547: // Skip padding byte
(not present on OS/2 1.x).
Nit: This comment is less clear with the code change; how about: "On non-OS/2
1.x, an extra padding byte is present, which we need to skip."

[scroggo_chromium, 2015-09-03 22:04:19]

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

[scroggo_chromium, 2015-09-03 22:04:19]

Florin, would you mind taking a look at the PassRefPtr semantics in
FastSharedBufferReader (the difference between patch sets 7 and 8)?

I think we have it figured out (see my comments, below), but I'd appreciate the
verification.

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
File Source/platform/image-decoders/FastSharedBufferReader.h (right):

https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
Source/platform/image-decoders/FastSharedBufferReader.h:49: void
setData(SharedBuffer*);
On 2015/09/02 23:48:44, Peter Kasting wrote:
> On 2015/09/02 23:01:07, scroggo_chromium wrote:
> > On 2015/09/02 22:08:54, Peter Kasting wrote:
> > > Is it really correct that the constructor takes a PassRefPtr but this
takes
> a
> > > raw pointer?  I don't know Blink's pointer type semantics
> > 
> > Me neither...
> > 
> > > but that looks
> > > strange.
> > 
> > Agreed. ImageDecoder::setData also takes a raw pointer, and this method is
> > called by that one. We could wrap that pointer...
> 
> Definitely at least figure out precisely what the code is doing today and why.

> That way we'll know whether this difference is important.

I just talked this over with a couple of people, and here is my understanding:

m_data is a RefPtr<SharedBuffer>. As is written in this patch set, setData takes
a raw pointer. When I do the assignment to m_data, that increases the ref count
so the FastSharedBufferReader has a reference.

If I used a PassRefPtr<SharedBuffer> instead, passing the raw pointer (from
BMPImageReader::setData) to FastSharedBufferReader::setData implicitly creates a
PassRefPtr, which increases the ref count. When that gets assigned to m_data,
that does *not* increase the ref count, but it does tell the PassRefPtr not to
decrement it (which it would do otherwise - this is important, because it means
if data == m_data, so we do not do the assignment, the PassRefPtr *does*
decrement it).

So, both do essentially the same thing. The PassRefPtr approach increments the
pointer earlier (and therefore adds some unnecessary churn, in the case where it
is the same as m_data). That said, I think consistency is better, and I
understand we generally do not want to use raw pointers. The latest patch set
makes this parameter a PassRefPtr.

[f(malita), 2015-09-04 13:06:07]

On 2015/09/03 22:04:19, scroggo_chromium wrote:
> Florin, would you mind taking a look at the PassRefPtr semantics in
> FastSharedBufferReader (the difference between patch sets 7 and 8)?
> 
> I think we have it figured out (see my comments, below), but I'd appreciate
the
> verification.
> 
>
https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
> File Source/platform/image-decoders/FastSharedBufferReader.h (right):
> 
>
https://chromiumcodereview.appspot.com/1259083003/diff/60001/Source/platform/...
> Source/platform/image-decoders/FastSharedBufferReader.h:49: void
> setData(SharedBuffer*);
> On 2015/09/02 23:48:44, Peter Kasting wrote:
> > On 2015/09/02 23:01:07, scroggo_chromium wrote:
> > > On 2015/09/02 22:08:54, Peter Kasting wrote:
> > > > Is it really correct that the constructor takes a PassRefPtr but this
> takes
> > a
> > > > raw pointer?  I don't know Blink's pointer type semantics
> > > 
> > > Me neither...
> > > 
> > > > but that looks
> > > > strange.
> > > 
> > > Agreed. ImageDecoder::setData also takes a raw pointer, and this method is
> > > called by that one. We could wrap that pointer...
> > 
> > Definitely at least figure out precisely what the code is doing today and
why.
> 
> > That way we'll know whether this difference is important.
> 
> I just talked this over with a couple of people, and here is my understanding:
> 
> m_data is a RefPtr<SharedBuffer>. As is written in this patch set, setData
takes
> a raw pointer. When I do the assignment to m_data, that increases the ref
count
> so the FastSharedBufferReader has a reference.
> 
> If I used a PassRefPtr<SharedBuffer> instead, passing the raw pointer (from
> BMPImageReader::setData) to FastSharedBufferReader::setData implicitly creates
a
> PassRefPtr, which increases the ref count. When that gets assigned to m_data,
> that does *not* increase the ref count, but it does tell the PassRefPtr not to
> decrement it (which it would do otherwise - this is important, because it
means
> if data == m_data, so we do not do the assignment, the PassRefPtr *does*
> decrement it).
> 
> So, both do essentially the same thing.

That sounds right.


> That said, I think consistency is better, and I
> understand we generally do not want to use raw pointers. The latest patch set
> makes this parameter a PassRefPtr.


LGTM

[scroggo_chromium, 2015-09-04 16:04:04]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2015-09-04 16:04:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1259083003/#ps160001 (title: "Do not use
a raw pointer")

[commit-bot: I haz the power, 2015-09-04 16:04:12]

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

[commit-bot: I haz the power, 2015-09-04 18:10:12]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201811

[scroggo, 2015-09-14 13:30:10]

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

The reason for reverting is: crbug.com/530279

This comment [1] describes what needs to be done to reland.

[1] https://code.google.com/p/chromium/issues/detail?id=530279#c1.