Eliminate copies of encoded image data

Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhSUw/edit#heading=h.hvbfr8rfyj9k
Benchmark results can be found at https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7OQOqEo/

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data. (Note - this still requires a mutex for using
the SkROBuffer::Iter. But in practice, there is no contention, unlike
the old implementation, which had to lock in order to copy into the
reader thread.)
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369
BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created.

e.g

DeferredImageDecoder did = ...
did.setData(...);

SkImage image = did.createFrameAtIndex(0);
did.setData(...); // More data

image will not attempt to decode the extra data provided in the last
call to setData.

However, it could use a decoder that had previously been used to
decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

imageA *may* look like imageB, even though it had less data.

Committed: https://crrev.com/d2234904faee943bd987bd38d620096db808efca
Cr-Commit-Position: refs/heads/master@{#387344}

[scroggo_chromium, 2016-03-22 17:27:57]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data.

<Insert more explanation here>

BUG=467772
BUG=483369

This may also help with BUG=335694. The DecodingImageGenerator will no
longer attempt to read beyond the data that had been received when it
was created. However, it theoretically could use a decoder that had
previously been used to decode more data than it was created with. e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice, but it is an
improvement.
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data into an SkRWBuffer, which
allows creating read only snapshots that can be safely read in another
thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread)
ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data
SkDataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().

Tests/WebImage.cpp/PictureSnapshot.cpp/WebGLImageConversion.cpp:
Mostly wrap SharedBuffers/SkData in SegmentReaders.

ImageDecodeBench:
Wrap SharedBuffer, plus update the gyp file so it builds.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice, but it is an
improvement.
==========

[scroggo_chromium, 2016-03-22 17:28:27]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data into an SkRWBuffer, which
allows creating read only snapshots that can be safely read in another
thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread)
ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data
SkDataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().

Tests/WebImage.cpp/PictureSnapshot.cpp/WebGLImageConversion.cpp:
Mostly wrap SharedBuffers/SkData in SegmentReaders.

ImageDecodeBench:
Wrap SharedBuffer, plus update the gyp file so it builds.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice, but it is an
improvement.
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread)
ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data
SkDataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().

Tests/WebImage.cpp/PictureSnapshot.cpp/WebGLImageConversion.cpp:
Mostly wrap SharedBuffers/SkData in SegmentReaders.

ImageDecodeBench:
Wrap SharedBuffer, plus update the gyp file so it builds.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice, but it is an
improvement.
==========

[scroggo_chromium, 2016-03-22 17:31:00]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread)
ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data
SkDataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().

Tests/WebImage.cpp/PictureSnapshot.cpp/WebGLImageConversion.cpp:
Mostly wrap SharedBuffers/SkData in SegmentReaders.

ImageDecodeBench:
Wrap SharedBuffer, plus update the gyp file so it builds.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice, but it is an
improvement.
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread)
ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data
SkDataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().

Tests/WebImage.cpp/PictureSnapshot.cpp/WebGLImageConversion.cpp:
Mostly wrap SharedBuffers/SkData in SegmentReaders.

ImageDecodeBench:
Wrap SharedBuffer, plus update the gyp file so it builds.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

[scroggo_chromium, 2016-03-22 20:10:52]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread)
ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data
SkDataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().

Tests/WebImage.cpp/PictureSnapshot.cpp/WebGLImageConversion.cpp:
Mostly wrap SharedBuffers/SkData in SegmentReaders.

ImageDecodeBench:
Wrap SharedBuffer, plus update the gyp file so it builds.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread)
ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data
SkDataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().

Tests/WebImage.cpp/PictureSnapshot.cpp/WebGLImageConversion.cpp:
Mostly wrap SharedBuffers/SkData in SegmentReaders.

ImageDecodeBench:
Wrap SharedBuffer, plus update the gyp file so it builds.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

[scroggo_chromium, 2016-03-22 20:12:33]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread)
ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data
SkDataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().

Tests/WebImage.cpp/PictureSnapshot.cpp/WebGLImageConversion.cpp:
Mostly wrap SharedBuffers/SkData in SegmentReaders.

ImageDecodeBench:
Wrap SharedBuffer, plus update the gyp file so it builds.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- SkDataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().

Tests/WebImage.cpp/PictureSnapshot.cpp/WebGLImageConversion.cpp:
Mostly wrap SharedBuffers/SkData in SegmentReaders.

ImageDecodeBench:
Wrap SharedBuffer, plus update the gyp file so it builds.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

[scroggo_chromium, 2016-03-22 20:18:40]

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

[scroggo_chromium, 2016-03-22 20:18:42]

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:61:
return m_data->getAsSkData().leakRef();
When combined with crrev.com/1567623002 (which first checks the first segment to
see if it can support it), this will mean we never copy the data here unless the
client knows it can support the contiguous buffer.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h:31:
#include "skia/ext/refptr.h"
No longer needed. Will remove.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:132: while
(size_t length = data.getSomeData(segment, m_rwBuffer->size())) {
This copy is analogous to the code in ThreadSafeDataTransport's setData (which
has been removed).

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:280:
m_actualDecoder.clear();
We hang on to the SkRWBuffer here, unlike the old code and its SharedBuffer,
since we may get more calls to createFrameAtIndex, which will take a snapshot
from the SkRWBuffer.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:295:
skia::RefPtr<SkROBuffer> roBuffer =
skia::AdoptRef(m_rwBuffer->newRBufferSnapshot());
Some of the Blink code that uses a Skia object wraps it in a skia::RefPtr, and
some uses a RefPtr. I'm not sure which is right (or if we should be using an
sk_sp, which did not exist when I started writing this code). Preferences?

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:41: #include
"platform/image-decoders/SkDataSegmentReader.h"
I'm not sure the proper way to name this, since it is not a Skia class, but it
logically made sense to me to include the name of the Skia class in the name,
resulting in it starting with "Sk". I took the opposite route with
ROBufferSegmentReader, but they should be the same (or use some different naming
convention). Suggestions welcome.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReader.h:79:
// method clears that cache in case the SegmentReader has been modified
This should still say SharedBuffer

[reed1, 2016-03-22 20:22:55]

reed@google.com changed reviewers:
+ fmalita@chromium.org

[Peter Kasting, 2016-03-22 20:37:24]

There are a lot of reviewers here.  Who's reviewing what?

[scroggo_chromium, 2016-03-22 20:55:33]

On 2016/03/22 20:37:24, Peter Kasting wrote:
> There are a lot of reviewers here.  Who's reviewing what?

pkasting@ and noel@: ImageDecoder changes (though they're pretty simple).
DeferredImageDecoder (and related) changes (which basically covers the whole CL)
- if you're the right ones to review? Alpha left, so I assumed you two were the
best to review, but if someone else is (and you know who they are), please let
me know!

reed@: Mainly the new path for refEncodedData (from DecodingImageGenerator). He
has a change on hold (crrev.com/1567623002) that will benefit from this change.

fmalita@: reed@ added fmalita, I think to review the platform/graphics code.

[Peter Kasting, 2016-03-23 02:42:59]

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/blink_platform.gypi (right):

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/blink_platform.gypi:742:
'image-decoders/ROBufferSegmentReader.h',
Nit: I think it would be nicer to spell out "ReadOnly" in the file name instead
of saving a few characters with "RO".

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:61:
return m_data->getAsSkData().leakRef();
On 2016/03/22 20:18:41, scroggo_chromium wrote:
> When combined with crrev.com/1567623002 (which first checks the first segment
to
> see if it can support it), this will mean we never copy the data here unless
the
> client knows it can support the contiguous buffer.

Can we pass this out as a smart object and thus avoid the leakRef() call?

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:87: if
(!m_canYUVDecode || !m_allDataReceived)
Why is it important to not YUV decode when we don't have all the data?  I assume
JPEG is the only format we have that uses YUV; does it store e.g. the full
image's Y data, then the full U, then the full V?  (Even then I would imagine we
could partially decode once the V started to appear?)

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:110:
return 0;
Is there a particular reason this explicit check needed to be added?

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:120:
RefPtr<SegmentReader> segmentReader = adoptRef(new
SharedBufferSegmentReader(RefPtr<SharedBuffer>(data)));
Nit: Move down one line

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:122:
m_actualDecoder->setData(segmentReader.get(), allDataReceived);
This pattern (create a local SharedBufferSegmentReader, pass to setData())
happens so many places in this CL that I think it may be worth adding a wrapper
function to ImageDecoder that takes a SharedBuffer directly.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:127: if
(!m_rwBuffer) {
Nit: No {} (anywhere we have a one-line body)

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:243:
m_frameGenerator =
ImageFrameGenerator::create(SkISize::Make(m_actualDecoder->decodedSize().width(),
m_actualDecoder->decodedSize().height()),
Nit: Either put everything on one line, or linebreak in such a way that the args
start at the same horizontal position each line.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:280:
m_actualDecoder.clear();
On 2016/03/22 20:18:42, scroggo_chromium wrote:
> We hang on to the SkRWBuffer here, unlike the old code and its SharedBuffer,
> since we may get more calls to createFrameAtIndex, which will take a snapshot
> from the SkRWBuffer.

Might want to write a comment about that.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:295:
skia::RefPtr<SkROBuffer> roBuffer =
skia::AdoptRef(m_rwBuffer->newRBufferSnapshot());
On 2016/03/22 20:18:42, scroggo_chromium wrote:
> Some of the Blink code that uses a Skia object wraps it in a skia::RefPtr, and
> some uses a RefPtr. I'm not sure which is right (or if we should be using an
> sk_sp, which did not exist when I started writing this code). Preferences?

Use skia::RefPtr for now, I think.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.h:93: // Allows
snapping readonly readers off to use in another thread.
"snapping"?

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:143: //
Should we move this into DecodingImageGenerator, which is the only class that
calls it?
I don't know.  I don't know much about YUV decoding.  I would likely make the
decision based on which route wound up with simpler code.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:182: // Lock
the mutex, so only one thread can use the decoder at once.
Do we have to lock for the whole rest of this function?  That's a lot of code.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:76: // Must
not be called unless getYUVCompenentSizes has been called and returned true.
Nit: Component

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:100: // FIXME:
Change the parameter to take an SkBitmap::Allocator, and no need for declaring
this class.
Are you planning to do that in this CL?

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:103: // This
method is called while m_decodeMutex is locked.
Nit: Unclear what this means: can be called while, or should only be called
while?

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGeneratorTest.cpp:192: //
Delete generator
Nit: Period

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:41: #include
"platform/image-decoders/SkDataSegmentReader.h"
On 2016/03/22 20:18:42, scroggo_chromium wrote:
> I'm not sure the proper way to name this, since it is not a Skia class, but it
> logically made sense to me to include the name of the Skia class in the name,
> resulting in it starting with "Sk". I took the opposite route with
> ROBufferSegmentReader, but they should be the same (or use some different
naming
> convention). Suggestions welcome.

I think DataSegmentReader is still readable enough, but I'm not categorically
opposed to using "Sk".

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:63:
SkDataSegmentReader segmentReader(skdata.get());
I think this needs to be refcounted if it's passed to setData() below.

This is where my suggestion in ImageDecoder.h that that function take a
non-raw-pointer would help to prevent errors.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:58:
FastSharedBufferReader reader(segmentReader);
This pattern happening repeatedly makes me wonder if FastSharedBufferReader
should take |data| directly.  But maybe that would screw up other callers that
aren't creating a temporary SharedBufferSegmentReader just for this.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:77: // No
implementation of SegmentReader will return less than 14 bytes from a read from
the beginning, unless there is no more data.
Does this comment matter?  It looks like we check the returned length below
anyway, so it doesn't seem like there's a problem even if this weren't true.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:108: void
setData(SegmentReader* data, bool allDataReceived)
This takes ownership from the caller.  It should either be passed via PassRefPtr
or by value with the caller doing std::move(), whichever way common WebKit style
is these days.  (Chromium style would be the latter.)

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:38:
// position is in this block.
Nit: Either capitalize Position or wrap it in ||.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:47:
if (!m_iter.next()) {
Hmm.  If this returns false, should we have incremented m_positionOfBlock yet? 
I think no?

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:55:
auto roBuffer = static_cast<const SkROBuffer*>(context);
Nit: Just inline into next statement

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:66:
const bool multipleBlocks = iter.next();
Nit: Maybe there should be a hasNext() method (or similar)?

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:71:
void* dst = data->writable_data();
Nit: I think there would be less casting if this was a char* and you only cast
here.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:82:
return adoptRef(SkData::NewWithProc(iter.data(), iter.size(), &unrefRobuffer,
Nit: Begin all lines of args at the same position

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.h:32: virtual
size_t getSomeData(const char*& data, size_t position) const = 0;
Maybe for clarity at the callers this should take char** instead of char*&?

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/SharedBufferSegmentReader.h:17:
// Interface for ImageDecoder to read a SharedBuffer
Nit: Period

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/SkDataSegmentReader.cpp:22:
data = reinterpret_cast<const char*>(m_data->bytes() + position);
I'm sort of tempted to suggest using uint8_t* everywhere here like SkData
already does but I think that would probably touch a huge number of places.

Bring on std::byte in C++17...

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/SkDataSegmentReader.h:16: //
Interface for ImageDecoder to read an SkData
Nit: Period

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:205:
m_encodedData = skia::AdoptRef(m_data->getAsSkData().leakRef());
Is AdoptRef(leakRef()) really the only way to do this?  I don't know these
interfaces that well but it would sure be nice to ditch both those...

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebImageDecoder.cpp (right):

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebImageDecoder.cpp:69: RefPtr<SharedBuffer>
buffer = PassRefPtr<SharedBuffer>(data).get();
Nit: Can't we just do something like RefPtr<SharedBuffer> buffer(data); ?

[f(malita), 2016-03-23 16:41:58]

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:61:
return m_data->getAsSkData().leakRef();
On 2016/03/23 02:42:57, Peter Kasting wrote:
> Can we pass this out as a smart object and thus avoid the leakRef() call?

This is part of the SkImageGenerator interface (called from Skia), and ATM only
exists in the raw ptr flavor.

As we're working on converting Skia to smart ptr based interfaces, I expect
we'll update it.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:107:
SkImageGenerator* DecodingImageGenerator::create(SkData* data)
Not new to this CL, but I think this factory is transferring SkData ownership,
so it should ideally use a smart ptr.  Maybe we can look into that separately (I
think it'll also involve updating some Skia interfaces).

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:130:
return new DecodingImageGenerator(frame, info, segmentReader, true, 0);
segmentReader.release()

(to minimize refcount churn, since we're done with the local)

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:47: // it
does not modify it.
Since SharedBufferSegmentReader only calls const SharedBuffer methods, can it be
updated to take/store a const SharedBuffer (and then get rid of this cast)?

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:295:
skia::RefPtr<SkROBuffer> roBuffer =
skia::AdoptRef(m_rwBuffer->newRBufferSnapshot());
On 2016/03/22 20:18:42, scroggo_chromium wrote:
> Some of the Blink code that uses a Skia object wraps it in a skia::RefPtr, and
> some uses a RefPtr. I'm not sure which is right (or if we should be using an
> sk_sp, which did not exist when I started writing this code). Preferences?

skia::RefPtr is only used in Chromium code AFAIK.  In Blink we stick to native
RefPtr/PassRefptr/adoptRef which are compatible with SkRefCnt-derived objects.

My preference would be RefPtr<SkROBuffer> ... = adoptRef(...);

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:41: #include
"platform/image-decoders/SkDataSegmentReader.h"
On 2016/03/23 02:42:58, Peter Kasting wrote:
> On 2016/03/22 20:18:42, scroggo_chromium wrote:
> > I'm not sure the proper way to name this, since it is not a Skia class, but
it
> > logically made sense to me to include the name of the Skia class in the
name,
> > resulting in it starting with "Sk". I took the opposite route with
> > ROBufferSegmentReader, but they should be the same (or use some different
> naming
> > convention). Suggestions welcome.
> 
> I think DataSegmentReader is still readable enough, but I'm not categorically
> opposed to using "Sk".

I would normally use SkiaDataSegmentReader to clarify this is not a native Skia
("Sk") class, but in this case I also vote for DataSegmentReader.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:62:
skia::RefPtr<SkData> skdata = skia::AdoptRef(SkData::NewWithoutCopy(data,
length));
RefPtr<SkData> skData = adoptRef(...);

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:58:
FastSharedBufferReader reader(segmentReader);
nit: segmentReader.release()

(here and below)

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:108: void
setData(SegmentReader* data, bool allDataReceived)
On 2016/03/23 02:42:59, Peter Kasting wrote:
> This takes ownership from the caller.  It should either be passed via
PassRefPtr
> or by value with the caller doing std::move(), whichever way common WebKit
style
> is these days.  (Chromium style would be the latter.)

+1

Not new to this CL, but setData should take a PassRefPtr<SegmentReader>.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:47:
decoder->setData(segmentReader.get(), true);
If we update setData to take a PassRefPtr, this should turn into
segmentReader.release().

Or inlined: setData(adoptRef(...)).

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp:56:
decoder->setData(segmentReader.get(), true);
ditto

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:85:
const_cast<SkROBuffer*>(m_roBuffer.get())));
SkROBuffer is intrinsically const (I think :), so it should be ok to relax
m_roBuffer's constness (as I suggested above).

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.h:9:
#include "skia/ext/refptr.h"
Replace with wtf/RefPtr.h, wtf/PassRefPtr.h

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.h:18:
ROBufferSegmentReader(const skia::RefPtr<SkROBuffer>&);
PassRefPtr<SkROBuffer>

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.h:25:
skia::RefPtr<const SkROBuffer> m_roBuffer;
RefPtr<SkROBuffer>

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageDecoder.h:46: void
onSetData(SegmentReader*) override;
Should likely be PassRefPtr<SegmentReader>.

Here and all other decoders... hmm, I expect this interface change will be
noisy, so maybe it should be a separate CL.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:205:
m_encodedData = skia::AdoptRef(m_data->getAsSkData().leakRef());
If we switch to RefPtr, this should simplify to m_encodedData =
m_data->getAsSkData();

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.h:33:
#include "skia/ext/refptr.h"
wtf/RefPtr.h

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.h:97:
skia::RefPtr<SkData> m_encodedData;
RefPtr<SkData>

[scroggo_chromium, 2016-03-24 13:58:33]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- SkDataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().

Tests/WebImage.cpp/PictureSnapshot.cpp/WebGLImageConversion.cpp:
Mostly wrap SharedBuffers/SkData in SegmentReaders.

ImageDecodeBench:
Wrap SharedBuffer, plus update the gyp file so it builds.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

WebImage.cpp:
Reduce ref count churn. (Ultimately unrelated.)

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

ImageDecodeBench:
Update the gyp file so it builds. (Independent from rest of change.)

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

[scroggo_chromium, 2016-03-24 13:59:46]

PTAL.

Thank you, Peter, for suggesting a version of setData that takes a (PassRefPtr)
SharedBuffer - that removed the necessity for a lot of changes, including entire
files.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/blink_platform.gypi (right):

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/blink_platform.gypi:742:
'image-decoders/ROBufferSegmentReader.h',
On 2016/03/23 02:42:57, Peter Kasting wrote:
> Nit: I think it would be nicer to spell out "ReadOnly" in the file name
instead
> of saving a few characters with "RO".

This reads from an object called "SkROBuffer", so I think this name makes more
sense, unless we change the name of the Skia class.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:87: if
(!m_canYUVDecode || !m_allDataReceived)
On 2016/03/23 02:42:58, Peter Kasting wrote:
> Why is it important to not YUV decode when we don't have all the data?  I
assume
> JPEG is the only format we have that uses YUV; does it store e.g. the full
> image's Y data, then the full U, then the full V?  (Even then I would imagine
we
> could partially decode once the V started to appear?)

I'm not very familiar with the YUV code, but there has been a FIXME [1] since it
was introduced [2]. It may be that we should support partial decoding, but that
would be outside the scope of this change.

The commit message does not provide any details, but I can hazard a guess why we
wouldn't support partial YUV - we support partial images by decoding into
ImageFrames. We would need to keep track of partial planes to pass for each
call. In addition, we'd have to have a way to tell the GPU to ignore the rows we
are not using yet.

So we just need to do more work to make it function.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[2]
https://chromium.googlesource.com/chromium/src/+/b9a911935caf1c633ce1517209ae...

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:107:
SkImageGenerator* DecodingImageGenerator::create(SkData* data)
On 2016/03/23 16:41:57, f(malita) wrote:
> Not new to this CL, but I think this factory is transferring SkData ownership,
> so it should ideally use a smart ptr.  Maybe we can look into that separately
(I
> think it'll also involve updating some Skia interfaces).

That's correct. This method gets passed to SetImageGeneratorFromEncodedFactory,
which currently takes a raw pointer.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:110:
return 0;
On 2016/03/23 02:42:57, Peter Kasting wrote:
> Is there a particular reason this explicit check needed to be added?

I have not added any calls that might pass null, so it is not currently needed.
Removed.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:130:
return new DecodingImageGenerator(frame, info, segmentReader, true, 0);
On 2016/03/23 16:41:57, f(malita) wrote:
> segmentReader.release()
> 
> (to minimize refcount churn, since we're done with the local)

Done.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.h:31:
#include "skia/ext/refptr.h"
On 2016/03/22 20:18:41, scroggo_chromium wrote:
> No longer needed. Will remove.

Done.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:47: // it
does not modify it.
On 2016/03/23 16:41:57, f(malita) wrote:
> Since SharedBufferSegmentReader only calls const SharedBuffer methods, can it
be
> updated to take/store a const SharedBuffer (and then get rid of this cast)?

I don't think so. ref() and deref() are not marked const, but it's kind of silly
that this even needs a SegmentReader, when it could easily use a SharedBuffer
directly. Updated so this is unnecessary.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:120:
RefPtr<SegmentReader> segmentReader = adoptRef(new
SharedBufferSegmentReader(RefPtr<SharedBuffer>(data)));
On 2016/03/23 02:42:58, Peter Kasting wrote:
> Nit: Move down one line

Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:122:
m_actualDecoder->setData(segmentReader.get(), allDataReceived);
On 2016/03/23 02:42:58, Peter Kasting wrote:
> This pattern (create a local SharedBufferSegmentReader, pass to setData())
> happens so many places in this CL that I think it may be worth adding a
wrapper
> function to ImageDecoder that takes a SharedBuffer directly.

Agreed. Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:127: if
(!m_rwBuffer) {
On 2016/03/23 02:42:58, Peter Kasting wrote:
> Nit: No {} (anywhere we have a one-line body)

Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:243:
m_frameGenerator =
ImageFrameGenerator::create(SkISize::Make(m_actualDecoder->decodedSize().width(),
m_actualDecoder->decodedSize().height()),
On 2016/03/23 02:42:58, Peter Kasting wrote:
> Nit: Either put everything on one line, or linebreak in such a way that the
args
> start at the same horizontal position each line.

I created a temporary variable, as is done with isSingleFrame, which I think
makes this more readable. (Blink's style guide does not mention anything about
lining up parameters, and I think I tried that, but got a presubmit error. I
think it suggested lining them all up 4 spaces from the first line.)

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:280:
m_actualDecoder.clear();
On 2016/03/23 02:42:58, Peter Kasting wrote:
> On 2016/03/22 20:18:42, scroggo_chromium wrote:
> > We hang on to the SkRWBuffer here, unlike the old code and its SharedBuffer,
> > since we may get more calls to createFrameAtIndex, which will take a
snapshot
> > from the SkRWBuffer.
> 
> Might want to write a comment about that.

Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:295:
skia::RefPtr<SkROBuffer> roBuffer =
skia::AdoptRef(m_rwBuffer->newRBufferSnapshot());
On 2016/03/23 16:41:58, f(malita) wrote:
> On 2016/03/22 20:18:42, scroggo_chromium wrote:
> > Some of the Blink code that uses a Skia object wraps it in a skia::RefPtr,
and
> > some uses a RefPtr. I'm not sure which is right (or if we should be using an
> > sk_sp, which did not exist when I started writing this code). Preferences?
> 
> skia::RefPtr is only used in Chromium code AFAIK.  In Blink we stick to native
> RefPtr/PassRefptr/adoptRef which are compatible with SkRefCnt-derived objects.
> 
> My preference would be RefPtr<SkROBuffer> ... = adoptRef(...);

Yes, it does appear skia::RefPtr is only used in Chromium. Done.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.h:93: // Allows
snapping readonly readers off to use in another thread.
On 2016/03/23 02:42:58, Peter Kasting wrote:
> "snapping"?

I'll update the comment. FWIW, I meant "snapping" as in "snapshot" - the reader
can only read up to the data that had been received when it was created.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:143: //
Should we move this into DecodingImageGenerator, which is the only class that
calls it?
On 2016/03/23 02:42:58, Peter Kasting wrote:
> I don't know.  I don't know much about YUV decoding.  I would likely make the
> decision based on which route wound up with simpler code.

I think moving it into DecodingImageGenerator is simpler. I made this a TODO and
plan to follow up in another CL.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:182: // Lock
the mutex, so only one thread can use the decoder at once.
On 2016/03/23 02:42:58, Peter Kasting wrote:
> Do we have to lock for the whole rest of this function?  That's a lot of code.

I agree that this is a lot of code to keep the mutex locked. We used to lock for
longer, though. In the old code, we locked in decodeAndScale, which calls this
method and then potentially copies the entire bitmap, both while the mutex is
locked.

I actually considered whether we could avoid locking the mutex here. The
ImageDecoderStore, which provides an ImageDecoder in lockDecoder(), expects that
we'll only use that decoder in one thread at a time, and this ensures that. We
could rewrite ImageDecodingStore to detach the decoder and require us to put it
back when we're done, so another call will get a new ImageDecoder if we're still
using the first one. This would remove the need for this mutex.

But I'm not sure that's the right thing to do. Using the same ImageDecoder
(which happens now if we leave the mutex lock where it was before my CL, or
where I've move it to here) means that the second user of the same ImageDecoder
will not need to do any/as much work, since the ImageDecoder already decoded
some or all of the image (into its cache, which will be reused the second time).

There is an exception - if both calls are decoding directly into the output
memory we might as well use two different decoders, since that skips the
ImageDecoder's internal cache. We do not know that yet, though. That will only
happen if allDataReceived is true and lockDecoder() does not have a decoder. So
we *could* unlock the mutex after calling lockDecoder() if it does not give us a
decoder. (If we pursue this, we should isolate it to a follow up CL.) On the
other hand, that still means that we are wasting work, since we could've just
waited and used the same SkImage. vmpstr is making changes in the compositor so
that multiple tiles using the same SkImage will all wait until the image is
decoded, so this shouldn't be a problem.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:76: // Must
not be called unless getYUVCompenentSizes has been called and returned true.
On 2016/03/23 02:42:58, Peter Kasting wrote:
> Nit: Component

Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:100: // FIXME:
Change the parameter to take an SkBitmap::Allocator, and no need for declaring
this class.
On 2016/03/23 02:42:58, Peter Kasting wrote:
> Are you planning to do that in this CL?

Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:103: // This
method is called while m_decodeMutex is locked.
On 2016/03/23 02:42:58, Peter Kasting wrote:
> Nit: Unclear what this means: can be called while, or should only be called
> while?

Should. Done.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGeneratorTest.cpp:192: //
Delete generator
On 2016/03/23 02:42:58, Peter Kasting wrote:
> Nit: Period

Done.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:41: #include
"platform/image-decoders/SkDataSegmentReader.h"
On 2016/03/23 16:41:58, f(malita) wrote:
> On 2016/03/23 02:42:58, Peter Kasting wrote:
> > On 2016/03/22 20:18:42, scroggo_chromium wrote:
> > > I'm not sure the proper way to name this, since it is not a Skia class,
but
> it
> > > logically made sense to me to include the name of the Skia class in the
> name,
> > > resulting in it starting with "Sk". I took the opposite route with
> > > ROBufferSegmentReader, but they should be the same (or use some different
> > naming
> > > convention). Suggestions welcome.
> > 
> > I think DataSegmentReader is still readable enough, but I'm not
categorically
> > opposed to using "Sk".
> 
> I would normally use SkiaDataSegmentReader to clarify this is not a native
Skia
> ("Sk") class, but in this case I also vote for DataSegmentReader.

How did you choose between DataSegmentReader and SkiaDataSegmentReader? I'm
happy to use DataSegmentReader (done), but it would be helpful to know for the
future why I would choose one over the other.

I just had an idea - we could make them all hidden classes and just have an
interface with factories:

class SegmentReader {
public:
  // methods...

  // Leaving out smart pointer semantics for simplicity here...
  static SegmentReader* Create(SharedBuffer*);
  static SegmentReader* Create(SkROBuffer*);
  static SegmentReader* Create(SkData*);
};

(Maybe this is independent of the names, but if we go this route the names can
be isolated to a single .cpp file) A downside to this is that if/when we move
ImageDecoder into Skia, we will need to fix it so that it can still support
SharedBuffer. But that may not be a good enough reason not to do it now, if this
is cleaner.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:62:
skia::RefPtr<SkData> skdata = skia::AdoptRef(SkData::NewWithoutCopy(data,
length));
On 2016/03/23 16:41:58, f(malita) wrote:
> RefPtr<SkData> skData = adoptRef(...);

Now that DataSegmentReader takes a PassRefPtr, I just inlined this.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:63:
SkDataSegmentReader segmentReader(skdata.get());
On 2016/03/23 02:42:58, Peter Kasting wrote:
> I think this needs to be refcounted if it's passed to setData() below.
> 
> This is where my suggestion in ImageDecoder.h that that function take a
> non-raw-pointer would help to prevent errors.

I think this is safe since the ImageDecoder will ref it and then be deleted and
unref it when it gets deleted (before the SkDataSegmentReader).

OTOH its more future-proof to change over.

Done.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReader.h:79:
// method clears that cache in case the SegmentReader has been modified
On 2016/03/22 20:18:42, scroggo_chromium wrote:
> This should still say SharedBuffer

Done.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:58:
FastSharedBufferReader reader(segmentReader);
On 2016/03/23 02:42:58, Peter Kasting wrote:
> This pattern happening repeatedly makes me wonder if FastSharedBufferReader
> should take |data| directly.  But maybe that would screw up other callers that
> aren't creating a temporary SharedBufferSegmentReader just for this.

The typical caller (outside of tests) already has a SegmentReader. We could add
a second constructor that takes a SharedBuffer, though (similar to your proposal
for ImageDecoder). I'm not sure that's appropriate for a call that's only used
in tests, though.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:58:
FastSharedBufferReader reader(segmentReader);
On 2016/03/23 16:41:58, f(malita) wrote:
> nit: segmentReader.release()
> 
> (here and below)

Done.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:77: // No
implementation of SegmentReader will return less than 14 bytes from a read from
the beginning, unless there is no more data.
On 2016/03/23 02:42:58, Peter Kasting wrote:
> Does this comment matter?  It looks like we check the returned length below
> anyway, so it doesn't seem like there's a problem even if this weren't true.

Removed.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:108: void
setData(SegmentReader* data, bool allDataReceived)
On 2016/03/23 16:41:58, f(malita) wrote:
> On 2016/03/23 02:42:59, Peter Kasting wrote:
> > This takes ownership from the caller.  It should either be passed via
> PassRefPtr
> > or by value with the caller doing std::move(), whichever way common WebKit
> style
> > is these days.  (Chromium style would be the latter.)
> 
> +1
> 
> Not new to this CL, but setData should take a PassRefPtr<SegmentReader>.

Since my CL did not introduce this pattern, do you mind if I leave that until a
follow-on?

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:38:
// position is in this block.
On 2016/03/23 02:42:59, Peter Kasting wrote:
> Nit: Either capitalize Position or wrap it in ||.

Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:47:
if (!m_iter.next()) {
On 2016/03/23 02:42:59, Peter Kasting wrote:
> Hmm.  If this returns false, should we have incremented m_positionOfBlock yet?

> I think no?

Good catch - this is a bug! But the fix is slightly more than that - we need to
reset our state, so the next call can succeed. Will add a test.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:55:
auto roBuffer = static_cast<const SkROBuffer*>(context);
On 2016/03/23 02:42:59, Peter Kasting wrote:
> Nit: Just inline into next statement

Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:66:
const bool multipleBlocks = iter.next();
On 2016/03/23 02:42:59, Peter Kasting wrote:
> Nit: Maybe there should be a hasNext() method (or similar)?

That seems reasonable. It would require landing something like
crrev.com/1822413002 in Skia.

But resetting the iterator is fairly cheap, so I don't think there's any reason
to hold this up.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:71:
void* dst = data->writable_data();
On 2016/03/23 02:42:59, Peter Kasting wrote:
> Nit: I think there would be less casting if this was a char* and you only cast
> here.

Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:82:
return adoptRef(SkData::NewWithProc(iter.data(), iter.size(), &unrefRobuffer,
On 2016/03/23 02:42:59, Peter Kasting wrote:
> Nit: Begin all lines of args at the same position

This is where the presubmit hooks complained (I tried doing that), but thanks to
Florin's comments below, I've made this a (cleaner) one line with no comments
needed.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:85:
const_cast<SkROBuffer*>(m_roBuffer.get())));
On 2016/03/23 16:41:58, f(malita) wrote:
> SkROBuffer is intrinsically const (I think :), so it should be ok to relax
> m_roBuffer's constness (as I suggested above).

I missed your comment above, but you're right! I've removed all this nonsense
about whether or not the SkROBuffer is const, making this a lot cleaner :)

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.h:9:
#include "skia/ext/refptr.h"
On 2016/03/23 16:41:58, f(malita) wrote:
> Replace with wtf/RefPtr.h, wtf/PassRefPtr.h

Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.h:18:
ROBufferSegmentReader(const skia::RefPtr<SkROBuffer>&);
On 2016/03/23 16:41:58, f(malita) wrote:
> PassRefPtr<SkROBuffer>

Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.h:25:
skia::RefPtr<const SkROBuffer> m_roBuffer;
On 2016/03/23 16:41:58, f(malita) wrote:
> RefPtr<SkROBuffer>

Done.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.h:32: virtual
size_t getSomeData(const char*& data, size_t position) const = 0;
On 2016/03/23 02:42:59, Peter Kasting wrote:
> Maybe for clarity at the callers this should take char** instead of char*&?

I'm not opposed to that in theory, but a goal is to have the same API as
SharedBuffer, which *& matches.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/SharedBufferSegmentReader.h:17:
// Interface for ImageDecoder to read a SharedBuffer
On 2016/03/23 02:42:59, Peter Kasting wrote:
> Nit: Period

Done.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/SkDataSegmentReader.cpp:22:
data = reinterpret_cast<const char*>(m_data->bytes() + position);
On 2016/03/23 02:42:59, Peter Kasting wrote:
> I'm sort of tempted to suggest using uint8_t* everywhere here like SkData
> already does but I think that would probably touch a huge number of places.
> 
> Bring on std::byte in C++17...

Haha +1!

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageDecoder.h:46: void
onSetData(SegmentReader*) override;
On 2016/03/23 16:41:58, f(malita) wrote:
> Should likely be PassRefPtr<SegmentReader>.
> 
> Here and all other decoders... hmm, I expect this interface change will be
> noisy, so maybe it should be a separate CL.
> 

+1 for making this a separate CL :)

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:205:
m_encodedData = skia::AdoptRef(m_data->getAsSkData().leakRef());
On 2016/03/23 16:41:58, f(malita) wrote:
> If we switch to RefPtr, this should simplify to m_encodedData =
> m_data->getAsSkData();

Done.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.h:33:
#include "skia/ext/refptr.h"
On 2016/03/23 16:41:58, f(malita) wrote:
> wtf/RefPtr.h

Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.h:97:
skia::RefPtr<SkData> m_encodedData;
On 2016/03/23 16:41:58, f(malita) wrote:
> RefPtr<SkData>

Done.

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebImageDecoder.cpp (right):

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebImageDecoder.cpp:69: RefPtr<SharedBuffer>
buffer = PassRefPtr<SharedBuffer>(data).get();
On 2016/03/23 02:42:59, Peter Kasting wrote:
> Nit: Can't we just do something like RefPtr<SharedBuffer> buffer(data); ?

I don't think so. WebData has a "operator PassRefPtr<SharedBuffer>() const" that
allows this conversion. I tried exactly what you proposed, and it did not work,
but I cannot promise that there is not some other incantation that will.

https://codereview.chromium.org/1812273003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp (right):

https://codereview.chromium.org/1812273003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:66:
RefPtr<DataSegmentReader> segmentReader = new
DataSegmentReader(adoptRef(SkData::NewWithoutCopy(data, length)));
Oops, I think the DataSegmentReader needs to be adopted. Fixed in next patch
set.

https://codereview.chromium.org/1812273003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1812273003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:117:
m_actualDecoder->setData(RefPtr<SharedBuffer>(data).release(), allDataReceived);
I think this can be simplified. We should probably go upstack to change the
parameter to a PassRefPtr, making this even simpler.

[f(malita), 2016-03-24 16:48:38]

Looks like you need to rebase this - would be nice to sic the bots on it :)

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h:108: void
setData(SegmentReader* data, bool allDataReceived)
On 2016/03/24 13:59:45, scroggo_chromium wrote:
> On 2016/03/23 16:41:58, f(malita) wrote:
> > On 2016/03/23 02:42:59, Peter Kasting wrote:
> > > This takes ownership from the caller.  It should either be passed via
> > PassRefPtr
> > > or by value with the caller doing std::move(), whichever way common WebKit
> > style
> > > is these days.  (Chromium style would be the latter.)
> > 
> > +1
> > 
> > Not new to this CL, but setData should take a PassRefPtr<SegmentReader>.
> 
> Since my CL did not introduce this pattern, do you mind if I leave that until
a
> follow-on?

Yeah, we should do that separately.

https://codereview.chromium.org/1812273003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
(right):

https://codereview.chromium.org/1812273003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:35:
#include "third_party/skia/src/core/SkRWBuffer.h"
Is this include needed?

https://codereview.chromium.org/1812273003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1812273003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:120:
ExternalMemoryAllocator externalAllocator(info, pixels, rowBytes);
I think this needs to stay heap-allocated: it appears to be plumbed all the way
down to ImageFrame::setMemoryAllocator(), which grabs a ref => cannot be
stack-allocated.

(which also begs for all the intermediate APIs to use
PassRefPtr<SkBitmap::Allocator> - can you add a TODO?)

[scroggo_chromium, 2016-03-24 19:04:33]

On 2016/03/24 16:48:38, f(malita) wrote:
> Looks like you need to rebase this - would be nice to sic
> the bots on it :)

Patch set 6 addresses your comments, and then I'll follow up with a rebase.

https://codereview.chromium.org/1812273003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
(right):

https://codereview.chromium.org/1812273003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:35:
#include "third_party/skia/src/core/SkRWBuffer.h"
On 2016/03/24 16:48:38, f(malita) wrote:
> Is this include needed?

Nope. Removed.

https://codereview.chromium.org/1812273003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1812273003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:120:
ExternalMemoryAllocator externalAllocator(info, pixels, rowBytes);
On 2016/03/24 16:48:38, f(malita) wrote:
> I think this needs to stay heap-allocated: it appears to be plumbed all the
way
> down to ImageFrame::setMemoryAllocator(), which grabs a ref => cannot be
> stack-allocated.
> 
> (which also begs for all the intermediate APIs to use
> PassRefPtr<SkBitmap::Allocator> - can you add a TODO?)

Done.

[scroggo_chromium, 2016-03-24 20:12:47]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

WebImage.cpp:
Reduce ref count churn. (Ultimately unrelated.)

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

ImageDecodeBench:
Update the gyp file so it builds. (Independent from rest of change.)

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

WebImage.cpp:
Reduce ref count churn. (Ultimately unrelated.)

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

[scroggo_chromium, 2016-03-24 20:16:28]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

WebImage.cpp:
Reduce ref count churn. (Ultimately unrelated.)

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

[scroggo_chromium, 2016-03-24 20:34:49]

Rebased. PTAL

[Peter Kasting, 2016-03-24 22:05:43]

Have not re-reviewed, just replying

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/blink_platform.gypi (right):

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/blink_platform.gypi:742:
'image-decoders/ROBufferSegmentReader.h',
On 2016/03/24 13:59:44, scroggo_chromium wrote:
> On 2016/03/23 02:42:57, Peter Kasting wrote:
> > Nit: I think it would be nicer to spell out "ReadOnly" in the file name
> instead
> > of saving a few characters with "RO".
> 
> This reads from an object called "SkROBuffer", so I think this name makes more
> sense, unless we change the name of the Skia class.

It's a judgment call.  I would try to apply the same logic here as for
DataSegmentReader -- do we want to closely match the underlying Skia type name
(in which case SkDataSegmentReader might be better) or are we trying to write a
name that's readable without reference to the SKia typename, or somewhere in the
middle?

I would probably prefer SkROBufferSegmentReader and ReadOnlyBufferSegmentReader
to ROBufferSegmentReader, but all have their downsides, so as long as you're
consistent in the end go with what you think is best.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:87: if
(!m_canYUVDecode || !m_allDataReceived)
On 2016/03/24 13:59:44, scroggo_chromium wrote:
> On 2016/03/23 02:42:58, Peter Kasting wrote:
> > Why is it important to not YUV decode when we don't have all the data?  I
> assume
> > JPEG is the only format we have that uses YUV; does it store e.g. the full
> > image's Y data, then the full U, then the full V?  (Even then I would
imagine
> we
> > could partially decode once the V started to appear?)
> 
> I'm not very familiar with the YUV code, but there has been a FIXME [1] since
it
> was introduced [2]. It may be that we should support partial decoding, but
that
> would be outside the scope of this change.

Ah.  Maybe it makes sense to have a comment here saying something like "For now
YUV doesn't support progressive decoding, see comment in ...", and then perhaps
beef up the comment in the other place with a little more rationale on why this
isn't supported yet.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:41: #include
"platform/image-decoders/SkDataSegmentReader.h"
On 2016/03/24 13:59:45, scroggo_chromium wrote:
> On 2016/03/23 16:41:58, f(malita) wrote:
> > On 2016/03/23 02:42:58, Peter Kasting wrote:
> > > On 2016/03/22 20:18:42, scroggo_chromium wrote:
> > > > I'm not sure the proper way to name this, since it is not a Skia class,
> but
> > it
> > > > logically made sense to me to include the name of the Skia class in the
> > name,
> > > > resulting in it starting with "Sk". I took the opposite route with
> > > > ROBufferSegmentReader, but they should be the same (or use some
different
> > > naming
> > > > convention). Suggestions welcome.
> > > 
> > > I think DataSegmentReader is still readable enough, but I'm not
> categorically
> > > opposed to using "Sk".
> > 
> > I would normally use SkiaDataSegmentReader to clarify this is not a native
> Skia
> > ("Sk") class, but in this case I also vote for DataSegmentReader.
> 
> How did you choose between DataSegmentReader and SkiaDataSegmentReader? I'm
> happy to use DataSegmentReader (done), but it would be helpful to know for the
> future why I would choose one over the other.
> 
> I just had an idea - we could make them all hidden classes and just have an
> interface with factories:
> 
> class SegmentReader {
> public:
>   // methods...
> 
>   // Leaving out smart pointer semantics for simplicity here...
>   static SegmentReader* Create(SharedBuffer*);
>   static SegmentReader* Create(SkROBuffer*);
>   static SegmentReader* Create(SkData*);
> };
> 
> (Maybe this is independent of the names, but if we go this route the names can
> be isolated to a single .cpp file) A downside to this is that if/when we move
> ImageDecoder into Skia, we will need to fix it so that it can still support
> SharedBuffer. But that may not be a good enough reason not to do it now, if
this
> is cleaner.

It sounds like that would be a bit cleaner, yes; mostly in the form of reducing
the per-file boilerplate overhead of defining these in several files.  But not
exposing these different classes externally is also somewhat semantically nice
in that it keeps the callers seeing this as a black box; if we were to end up
collapsing the implementations of these into fewer individual classes
internally, no external users would need to change.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:58:
FastSharedBufferReader reader(segmentReader);
On 2016/03/24 13:59:45, scroggo_chromium wrote:
> On 2016/03/23 02:42:58, Peter Kasting wrote:
> > This pattern happening repeatedly makes me wonder if FastSharedBufferReader
> > should take |data| directly.  But maybe that would screw up other callers
that
> > aren't creating a temporary SharedBufferSegmentReader just for this.
> 
> The typical caller (outside of tests) already has a SegmentReader. We could
add
> a second constructor that takes a SharedBuffer, though (similar to your
proposal
> for ImageDecoder). I'm not sure that's appropriate for a call that's only used
> in tests, though.

Up to you.  I'm not opposed to another constructor with a "// Used by tests."
sort of comment on it, especially if we minimize added code by having one
constructor delegate to the other (yay C++11).  Seems like a reasonably small
cost to simplify these calls, but we could live without it as well.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.h:32: virtual
size_t getSomeData(const char*& data, size_t position) const = 0;
On 2016/03/24 13:59:46, scroggo_chromium wrote:
> On 2016/03/23 02:42:59, Peter Kasting wrote:
> > Maybe for clarity at the callers this should take char** instead of char*&?
> 
> I'm not opposed to that in theory, but a goal is to have the same API as
> SharedBuffer, which *& matches.

Yeah, that seems sane.  Up to you whether you think this would be worth making a
larger change for, in which case I'd do it separately (although possibly in
advance, so this code can then land in its final form).

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebImageDecoder.cpp (right):

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebImageDecoder.cpp:69: RefPtr<SharedBuffer>
buffer = PassRefPtr<SharedBuffer>(data).get();
On 2016/03/24 13:59:46, scroggo_chromium wrote:
> On 2016/03/23 02:42:59, Peter Kasting wrote:
> > Nit: Can't we just do something like RefPtr<SharedBuffer> buffer(data); ?
> 
> I don't think so. WebData has a "operator PassRefPtr<SharedBuffer>() const"
that
> allows this conversion. I tried exactly what you proposed, and it did not
work,
> but I cannot promise that there is not some other incantation that will.

Yeah, I had been hoping the compiler could figure out that there was a RefPtr
constructor taking a PassRefPtr, and then implicitly invoke that operator.  But
if we have to be explicit about it, so be it.

[scroggo_chromium, 2016-03-24 22:16:29]

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:47:
if (!m_iter.next()) {
On 2016/03/24 13:59:45, scroggo_chromium wrote:
> On 2016/03/23 02:42:59, Peter Kasting wrote:
> > Hmm.  If this returns false, should we have incremented m_positionOfBlock
yet?
> 
> > I think no?
> 
> Good catch - this is a bug! But the fix is slightly more than that - we need
to
> reset our state, so the next call can succeed. Will add a test.

I was wrong. This was not a bug. m_roBuffer is const (will never get more data),
so moving m_positionOfBlock before calling iter.next() is okay - it means that
any valid calls will reset the iter because a valid position will be <
m_positionOfBlock. That said, the old implementation may have been too tricky
for its own good, since it wasn't obvious to you, and I had forgotten its
cleverness.

Either way, I added tests in the latest patch set.

[scroggo_chromium, 2016-03-24 23:13:47]

The CQ bit was checked by scroggo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-24 23:14:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1812273003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1812273003/180001

[commit-bot: I haz the power, 2016-03-24 23:14:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-24 23:14:22]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[scroggo_chromium, 2016-03-25 01:05:10]

PTAL.

Also, I am not yet a committer. Will someone who is please trigger some trybot
runs? (I requested try access, but have not yet received a response.)

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/blink_platform.gypi (right):

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/blink_platform.gypi:742:
'image-decoders/ROBufferSegmentReader.h',
On 2016/03/24 22:05:43, Peter Kasting wrote:
> On 2016/03/24 13:59:44, scroggo_chromium wrote:
> > On 2016/03/23 02:42:57, Peter Kasting wrote:
> > > Nit: I think it would be nicer to spell out "ReadOnly" in the file name
> > instead
> > > of saving a few characters with "RO".
> > 
> > This reads from an object called "SkROBuffer", so I think this name makes
more
> > sense, unless we change the name of the Skia class.
> 
> It's a judgment call.  I would try to apply the same logic here as for
> DataSegmentReader -- do we want to closely match the underlying Skia type name
> (in which case SkDataSegmentReader might be better) or are we trying to write
a
> name that's readable without reference to the SKia typename, or somewhere in
the
> middle?
> 
> I would probably prefer SkROBufferSegmentReader and
ReadOnlyBufferSegmentReader
> to ROBufferSegmentReader, but all have their downsides, so as long as you're
> consistent in the end go with what you think is best.

I've stuck with DataSegmentReader and ROBufferSegmentReader, which both mirror
their Skia types minus the Sk. I also moved them into factories, so if we don't
like the names we can change them in few places.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:87: if
(!m_canYUVDecode || !m_allDataReceived)
On 2016/03/24 22:05:43, Peter Kasting wrote:
> On 2016/03/24 13:59:44, scroggo_chromium wrote:
> > On 2016/03/23 02:42:58, Peter Kasting wrote:
> > > Why is it important to not YUV decode when we don't have all the data?  I
> > assume
> > > JPEG is the only format we have that uses YUV; does it store e.g. the full
> > > image's Y data, then the full U, then the full V?  (Even then I would
> imagine
> > we
> > > could partially decode once the V started to appear?)
> > 
> > I'm not very familiar with the YUV code, but there has been a FIXME [1]
since
> it
> > was introduced [2]. It may be that we should support partial decoding, but
> that
> > would be outside the scope of this change.
> 
> Ah.  Maybe it makes sense to have a comment here saying something like "For
now
> YUV doesn't support progressive decoding, see comment in ...", and then
perhaps
> beef up the comment in the other place with a little more rationale on why
this
> isn't supported yet.

Done.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/PictureSnapshot.cpp:41: #include
"platform/image-decoders/SkDataSegmentReader.h"
On 2016/03/24 22:05:43, Peter Kasting wrote:
> On 2016/03/24 13:59:45, scroggo_chromium wrote:
> > On 2016/03/23 16:41:58, f(malita) wrote:
> > > On 2016/03/23 02:42:58, Peter Kasting wrote:
> > > > On 2016/03/22 20:18:42, scroggo_chromium wrote:
> > > > > I'm not sure the proper way to name this, since it is not a Skia
class,
> > but
> > > it
> > > > > logically made sense to me to include the name of the Skia class in
the
> > > name,
> > > > > resulting in it starting with "Sk". I took the opposite route with
> > > > > ROBufferSegmentReader, but they should be the same (or use some
> different
> > > > naming
> > > > > convention). Suggestions welcome.
> > > > 
> > > > I think DataSegmentReader is still readable enough, but I'm not
> > categorically
> > > > opposed to using "Sk".
> > > 
> > > I would normally use SkiaDataSegmentReader to clarify this is not a native
> > Skia
> > > ("Sk") class, but in this case I also vote for DataSegmentReader.
> > 
> > How did you choose between DataSegmentReader and SkiaDataSegmentReader? I'm
> > happy to use DataSegmentReader (done), but it would be helpful to know for
the
> > future why I would choose one over the other.
> > 
> > I just had an idea - we could make them all hidden classes and just have an
> > interface with factories:
> > 
> > class SegmentReader {
> > public:
> >   // methods...
> > 
> >   // Leaving out smart pointer semantics for simplicity here...
> >   static SegmentReader* Create(SharedBuffer*);
> >   static SegmentReader* Create(SkROBuffer*);
> >   static SegmentReader* Create(SkData*);
> > };
> > 
> > (Maybe this is independent of the names, but if we go this route the names
can
> > be isolated to a single .cpp file) A downside to this is that if/when we
move
> > ImageDecoder into Skia, we will need to fix it so that it can still support
> > SharedBuffer. But that may not be a good enough reason not to do it now, if
> this
> > is cleaner.
> 
> It sounds like that would be a bit cleaner, yes; mostly in the form of
reducing
> the per-file boilerplate overhead of defining these in several files.  But not
> exposing these different classes externally is also somewhat semantically nice
> in that it keeps the callers seeing this as a black box; if we were to end up
> collapsing the implementations of these into fewer individual classes
> internally, no external users would need to change.

I've left the names alone and switched to using factories. No other files
(besides build files) will need to worry about the naming convention, and if we
want to change the names they are isolated.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:58:
FastSharedBufferReader reader(segmentReader);
On 2016/03/24 22:05:43, Peter Kasting wrote:
> On 2016/03/24 13:59:45, scroggo_chromium wrote:
> > On 2016/03/23 02:42:58, Peter Kasting wrote:
> > > This pattern happening repeatedly makes me wonder if
FastSharedBufferReader
> > > should take |data| directly.  But maybe that would screw up other callers
> that
> > > aren't creating a temporary SharedBufferSegmentReader just for this.
> > 
> > The typical caller (outside of tests) already has a SegmentReader. We could
> add
> > a second constructor that takes a SharedBuffer, though (similar to your
> proposal
> > for ImageDecoder). I'm not sure that's appropriate for a call that's only
used
> > in tests, though.
> 
> Up to you.  I'm not opposed to another constructor with a "// Used by tests."
> sort of comment on it, especially if we minimize added code by having one
> constructor delegate to the other (yay C++11).  Seems like a reasonably small
> cost to simplify these calls, but we could live without it as well.

Acknowledged. Left out the constructor for now.

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

https://codereview.chromium.org/1812273003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.h:32: virtual
size_t getSomeData(const char*& data, size_t position) const = 0;
On 2016/03/24 22:05:43, Peter Kasting wrote:
> On 2016/03/24 13:59:46, scroggo_chromium wrote:
> > On 2016/03/23 02:42:59, Peter Kasting wrote:
> > > Maybe for clarity at the callers this should take char** instead of
char*&?
> > 
> > I'm not opposed to that in theory, but a goal is to have the same API as
> > SharedBuffer, which *& matches.
> 
> Yeah, that seems sane.  Up to you whether you think this would be worth making
a
> larger change for, in which case I'd do it separately (although possibly in
> advance, so this code can then land in its final form).

Acknowledged.

https://codereview.chromium.org/1812273003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/DataSegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/DataSegmentReader.cpp:23:
RefPtr<SkData> m_data;
The last patch set had a straggling skia::RefPtr. I think I got the last one
when I moved this into the impl file.

[Peter Kasting, 2016-03-25 06:24:37]

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:60:
return m_data->getAsSkData().leakRef();
Nit: Shorter:

    return m_allDataReceived ? m_data->getAsSkData().leakRef() : nullptr;

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:86: //
YUV decoding does not currently support progressive decoding. See comment in
ImageFrameGenerator.h
Nit: Trailing period (2 places)

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:125: while
(size_t length = data.getSomeData(segment, m_rwBuffer->size()))
Nit: Using side-effect statements in conditionals can trigger warnings in some
cases in MSVC, and usually I just direct people to avoid them.

Unfortunately none of the alternative options in this case are quite as compact.
 The shortest one is probably a for() loop:

        for (size_t length = data.getSomeData(segment, m_rwBuffer->size());
             length; length = data.getSomeData(segment, m_rwBuffer->size())) {
            ...

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:273: //
Hold on to the SkRWBuffer, which is still needed by createFrameAtIndex.
Nit: the SkRWBuffer -> m_rwBuffer

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:130: //
instead of a bare pointer.
Nit: Why bother word-wrapping the comment when you're going to wrap so much
later than 80 columns?

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:255: {
Nit: Maybe ASSERT(m_decodeMutex.locked())?

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/DataSegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/DataSegmentReader.cpp:14: //
Interface for ImageDecoder to read an SkData
Nit: Trailing period

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/DataSegmentReader.cpp:32: :
m_data(data) {}
Nit: Indent 4

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:69:
void createReaders(SegmentReaders* s, PassRefPtr<SharedBuffer> input)
Nit: Why not use a constructor for SegmentReaders (that takes |input|) instead?

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:162:
const unsigned dataSize = SharedBuffer::kSegmentSize;
Should this test test a buffer with more than one segment in it?

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:173:
EXPECT_EQ(length, 0u);
Nit: (expected, actual) (2 places)

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:175:
length = segmentReader->getSomeData(contents, static_cast<size_t>(0));
Nit: Shouldn't need to static_cast here.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:104: const
size_t length = data.getSomeData(contents, static_cast<size_t>(0));
Nit: Should not need static_cast

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:21:
size_t size() const override { return m_roBuffer ? m_roBuffer->size() : 0; }
Nit: Why define this inline (and nothing else)?

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:54:
while (true) {
Nit: Could write this as

    for (size_t sizeOfBlock = m_iter.size(); sizeOfBlock != 0; m_positionOfBlock
+= sizeOfBlock) {
        ...
    }
    return 0;

This would not only be shorter (as it would eliminate several lines of code at
the beginning and end of the loop body), it would be less likely to confuse dumb
compilers (or dumb readers) as to why there's no return statement at the end of
a non-void function.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:81:
static void unrefRobuffer(const void* ptr, void* context)
Nit: ROBuffer?

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:107:
// Contiguous data. No need to copy.
Nit: It's a little weird to see:

    // Check to see if the data is already contiguous.
    <brief check>

    <whole bunch of other code>

    // Contiguous data. No need to copy.
    ...

It might read more clearly to reverse the conditional above so the "contiguous
data" case happens right after the check for it?  (This also results in
indenting fewer lines of code.)

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/SegmentReader.h (right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.h:39: virtual
PassRefPtr<SkData> getAsSkData() const = 0;
Given this interface, I think it would be a little less confusing to read to
merge the three .cpp files you have (for the three different implementations of
this) into one SegmentReader.cpp.

Then within that file, you can first define each of the classes, with dividers,
and then finally define the createXXX functions for SegmentReader in a bottom
section:

// SharedBufferSegmentReader ---------------------------------------------------

class SharedBufferSegmentReader : public SegmentReader {
...
};


// DataSegmentReader -----------------------------------------------------------

...

...


// SegmentReader ---------------------------------------------------------------

// static
PassRefPtr<SegmentReader>
SegmentReader::createFromSharedBuffer(PassRefPtr<SharedBuffer>) {
  ...
}

...

This ensures the implementations of concrete SegmentReader functions are all
together and in the place one would most likely look; there aren't .cpp files
with no corresponding .h files to confuse people looking at directory file
lists; makes it clear all these other classes are implementation details of this
particular public class; and puts those other classes near each other so readers
can see the parallelism in their implementations.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/image-decoders/SharedBufferSegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SharedBufferSegmentReader.cpp:33:
: m_sharedBuffer(buffer) {}
Nit: Indent 4

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:198:
m_encodedData = m_data->getAsSkData();
We never reset this anywhere; won't this mean we now hang onto the last data we
looked at even after decoding has finished (succeeded or failed)?

[scroggo, 2016-03-25 15:49:50]

scroggo@google.com changed reviewers:
+ scroggo@google.com, urvang@chromium.org

[scroggo, 2016-03-25 15:49:53]

urvang@, would you please take a look at the WEBPImageDecoder changes?

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:60:
return m_data->getAsSkData().leakRef();
On 2016/03/25 06:24:36, Peter Kasting wrote:
> Nit: Shorter:
> 
>     return m_allDataReceived ? m_data->getAsSkData().leakRef() : nullptr;

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:86: //
YUV decoding does not currently support progressive decoding. See comment in
ImageFrameGenerator.h
On 2016/03/25 06:24:36, Peter Kasting wrote:
> Nit: Trailing period (2 places)

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:125: while
(size_t length = data.getSomeData(segment, m_rwBuffer->size()))
On 2016/03/25 06:24:36, Peter Kasting wrote:
> Nit: Using side-effect statements in conditionals can trigger warnings in some
> cases in MSVC, and usually I just direct people to avoid them.
> 
> Unfortunately none of the alternative options in this case are quite as
compact.
>  The shortest one is probably a for() loop:
> 
>         for (size_t length = data.getSomeData(segment, m_rwBuffer->size());
>              length; length = data.getSomeData(segment, m_rwBuffer->size())) {
>             ...

:(

Done.

I considered using a do while loop:

const char* segment 
size_t length = 0;
do {
  m_rwBuffer->append(segment, length); // this call will safely do nothing the
first time.
  length = data.getSomeData(segment, m_rwBuffer->size());
} while (length > 0);

This option eliminates the duplicate code (which, admittedly, happens to line up
well so if they get out of sync it will be obvious), but is a little awkward,
because the first call to append is useless.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:273: //
Hold on to the SkRWBuffer, which is still needed by createFrameAtIndex.
On 2016/03/25 06:24:36, Peter Kasting wrote:
> Nit: the SkRWBuffer -> m_rwBuffer

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:130: //
instead of a bare pointer.
On 2016/03/25 06:24:36, Peter Kasting wrote:
> Nit: Why bother word-wrapping the comment when you're going to wrap so much
> later than 80 columns?

What would you prefer? Blink seems to prefer *not* wrapping at all, but I find
it hard to look at. I think 80 columns is a reasonable standard, although it
doesn't really map to monitors anymore (and doesn't match surrounding code). I
chose to wrap here based on a subconscious aesthetic that said "this looks
ugly", probably because it goes beyond the line of code below.

I wrapped to 80 columns.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:255: {
On 2016/03/25 06:24:36, Peter Kasting wrote:
> Nit: Maybe ASSERT(m_decodeMutex.locked())?

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/DataSegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/DataSegmentReader.cpp:14: //
Interface for ImageDecoder to read an SkData
On 2016/03/25 06:24:36, Peter Kasting wrote:
> Nit: Trailing period

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/DataSegmentReader.cpp:32: :
m_data(data) {}
On 2016/03/25 06:24:36, Peter Kasting wrote:
> Nit: Indent 4

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:69:
void createReaders(SegmentReaders* s, PassRefPtr<SharedBuffer> input)
On 2016/03/25 06:24:36, Peter Kasting wrote:
> Nit: Why not use a constructor for SegmentReaders (that takes |input|)
instead?

Agreed, that is much better.

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:162:
const unsigned dataSize = SharedBuffer::kSegmentSize;
On 2016/03/25 06:24:37, Peter Kasting wrote:
> Should this test test a buffer with more than one segment in it?

My goal in this test was to test reading past the end, which is independent of
the number of segments. But I'm fine with using more than one segment.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:173:
EXPECT_EQ(length, 0u);
On 2016/03/25 06:24:36, Peter Kasting wrote:
> Nit: (expected, actual) (2 places)

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:175:
length = segmentReader->getSomeData(contents, static_cast<size_t>(0));
On 2016/03/25 06:24:36, Peter Kasting wrote:
> Nit: Shouldn't need to static_cast here.

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:104: const
size_t length = data.getSomeData(contents, static_cast<size_t>(0));
On 2016/03/25 06:24:37, Peter Kasting wrote:
> Nit: Should not need static_cast

I was surprised by this one, too, but position is a STRICTLY_TYPED_ARG so I have
to use this cast. I've actually seen this elsewhere as a solution for using a
literal [1]. It seems like something isn't working though; it looks to have a
default argument, but calling data.getSomeData(contents) does not compile.

[1] e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:21:
size_t size() const override { return m_roBuffer ? m_roBuffer->size() : 0; }
On 2016/03/25 06:24:37, Peter Kasting wrote:
> Nit: Why define this inline (and nothing else)?

Because it's short. What is your preference?

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:54:
while (true) {
On 2016/03/25 06:24:37, Peter Kasting wrote:
> Nit: Could write this as
> 
>     for (size_t sizeOfBlock = m_iter.size(); sizeOfBlock != 0;
m_positionOfBlock
> += sizeOfBlock) {
>         ...
>     }
>     return 0;
> 
> This would not only be shorter (as it would eliminate several lines of code at
> the beginning and end of the loop body), it would be less likely to confuse
dumb
> compilers (or dumb readers) as to why there's no return statement at the end
of
> a non-void function.

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:81:
static void unrefRobuffer(const void* ptr, void* context)
On 2016/03/25 06:24:37, Peter Kasting wrote:
> Nit: ROBuffer?

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:107:
// Contiguous data. No need to copy.
On 2016/03/25 06:24:37, Peter Kasting wrote:
> Nit: It's a little weird to see:
> 
>     // Check to see if the data is already contiguous.
>     <brief check>
> 
>     <whole bunch of other code>
> 
>     // Contiguous data. No need to copy.
>     ...
> 
> It might read more clearly to reverse the conditional above so the "contiguous
> data" case happens right after the check for it?  (This also results in
> indenting fewer lines of code.)

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/SegmentReader.h (right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.h:39: virtual
PassRefPtr<SkData> getAsSkData() const = 0;
On 2016/03/25 06:24:37, Peter Kasting wrote:
> Given this interface, I think it would be a little less confusing to read to
> merge the three .cpp files you have (for the three different implementations
of
> this) into one SegmentReader.cpp.
> 
> Then within that file, you can first define each of the classes, with
dividers,
> and then finally define the createXXX functions for SegmentReader in a bottom
> section:
> 
> // SharedBufferSegmentReader
---------------------------------------------------
> 
> class SharedBufferSegmentReader : public SegmentReader {
> ...
> };
> 
> 
> // DataSegmentReader
-----------------------------------------------------------
> 
> ...
> 
> ...
> 
> 
> // SegmentReader
---------------------------------------------------------------
> 
> // static
> PassRefPtr<SegmentReader>
> SegmentReader::createFromSharedBuffer(PassRefPtr<SharedBuffer>) {
>   ...
> }
> 
> ...
> 
> This ensures the implementations of concrete SegmentReader functions are all
> together and in the place one would most likely look; there aren't .cpp files
> with no corresponding .h files to confuse people looking at directory file
> lists; makes it clear all these other classes are implementation details of
this
> particular public class; and puts those other classes near each other so
readers
> can see the parallelism in their implementations.

Done in patch set 15.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/image-decoders/SharedBufferSegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SharedBufferSegmentReader.cpp:33:
: m_sharedBuffer(buffer) {}
On 2016/03/25 06:24:37, Peter Kasting wrote:
> Nit: Indent 4

Done.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:198:
m_encodedData = m_data->getAsSkData();
On 2016/03/25 06:24:37, Peter Kasting wrote:
> We never reset this anywhere; won't this mean we now hang onto the last data
we
> looked at even after decoding has finished (succeeded or failed)?

That looks correct. I've updated the code to clear m_encodedData any time we
clear m_demux. Will add urvang@ to look over changes here.

https://codereview.chromium.org/1812273003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:98:
SkData* data = SkData::NewUninitialized(m_roBuffer->size());
Updating this to a RefPtr...

https://codereview.chromium.org/1812273003/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
(right):

https://codereview.chromium.org/1812273003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:204:
m_consolidatedData.clear();
It seems like we could get rid of m_demux here or other places we call
setFailed, but that might be worth a separate change.

[scroggo_chromium, 2016-03-25 17:24:06]

The CQ bit was checked by scroggo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-25 17:24:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1812273003/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1812273003/300001

[commit-bot: I haz the power, 2016-03-25 17:39:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-25 17:39:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[f(malita), 2016-03-25 18:47:53]

Thanks Leon. LGTM w/ a couple of nits, but deferring to decoder experts for
final approval.

https://codereview.chromium.org/1812273003/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1812273003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:135:
externalAllocator.clear();
Nit: since externalAllocator is now local, I think it's fine to let it go out of
scope - no need to clear explicitly.

https://codereview.chromium.org/1812273003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:291:
(*decoder)->setData(PassRefPtr<SegmentReader>(nullptr), false); // Unref
SegmentReader from ImageDecoder.
nit: would straight nullptr not work here? (we have implicit nullptr ->
PassRefPtr conversion)

[scroggo_chromium, 2016-03-25 18:56:21]

https://codereview.chromium.org/1812273003/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1812273003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:135:
externalAllocator.clear();
On 2016/03/25 18:47:53, f(malita) wrote:
> Nit: since externalAllocator is now local, I think it's fine to let it go out
of
> scope - no need to clear explicitly.

Done.

https://codereview.chromium.org/1812273003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:291:
(*decoder)->setData(PassRefPtr<SegmentReader>(nullptr), false); // Unref
SegmentReader from ImageDecoder.
On 2016/03/25 18:47:53, f(malita) wrote:
> nit: would straight nullptr not work here? (we have implicit nullptr ->
> PassRefPtr conversion)

It would, except that this method is overloaded - that nullptr could either be a
PassRefPtr<SegmentReader> or a PassRefPtr<SharedBuffer>

[Peter Kasting, 2016-03-25 21:15:56]

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:130: //
instead of a bare pointer.
On 2016/03/25 15:49:52, scroggo wrote:
> On 2016/03/25 06:24:36, Peter Kasting wrote:
> > Nit: Why bother word-wrapping the comment when you're going to wrap so much
> > later than 80 columns?
> 
> What would you prefer? Blink seems to prefer *not* wrapping at all, but I find
> it hard to look at. I think 80 columns is a reasonable standard, although it
> doesn't really map to monitors anymore (and doesn't match surrounding code). I
> chose to wrap here based on a subconscious aesthetic that said "this looks
> ugly", probably because it goes beyond the line of code below.
> 
> I wrapped to 80 columns.

FWIW, Blink style is soon going to be transitioning to be more like Chromium
style, and at that point I think they're adopting the 80-column rule, so
wrapping to 80 now seems good to me.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:104: const
size_t length = data.getSomeData(contents, static_cast<size_t>(0));
On 2016/03/25 15:49:53, scroggo wrote:
> On 2016/03/25 06:24:37, Peter Kasting wrote:
> > Nit: Should not need static_cast
> 
> I was surprised by this one, too, but position is a STRICTLY_TYPED_ARG so I
have
> to use this cast. I've actually seen this elsewhere as a solution for using a
> literal [1]. It seems like something isn't working though; it looks to have a
> default argument, but calling data.getSomeData(contents) does not compile.

I think the one-arg version might compile if you do
getSomeData<size_t>(contents), since the macros expand to a templatized function
like this.

Also, I think if you change the body of getSomeData() to use
ALLOW_NUMERIC_ARG_TYPES_PROMOTABLE_TO() where it uses STRICT_ARG_TYPE(), you
could pass 0U here (maybe just 0, but I'm not sure).  Consider doing this as a
separate (possible precursor?) change and possibly also changing the other uses
of that macro in that file similarly.

Although, since this seems to be the only class in the codebase which uses these
macros, I think it would be even better to just rip these out entirely.  I doubt
they buy significant safety and it's not compelling to have them for just one of
the many classes which can take buffer/byte sizes.  This would also probably
eliminate the weirdness where the default arg doesn't work.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:21:
size_t size() const override { return m_roBuffer ? m_roBuffer->size() : 0; }
On 2016/03/25 15:49:53, scroggo wrote:
> On 2016/03/25 06:24:37, Peter Kasting wrote:
> > Nit: Why define this inline (and nothing else)?
> 
> Because it's short. What is your preference?

I generally avoid inlining virtuals (see "stop inlining virtuals" on
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts ), but it
doesn't matter much here as this is in a .cpp file and not a .h file.  Do it
however you like.

[scroggo_chromium, 2016-03-25 21:31:59]

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:130: //
instead of a bare pointer.
On 2016/03/25 21:15:56, Peter Kasting wrote:
> On 2016/03/25 15:49:52, scroggo wrote:
> > On 2016/03/25 06:24:36, Peter Kasting wrote:
> > > Nit: Why bother word-wrapping the comment when you're going to wrap so
much
> > > later than 80 columns?
> > 
> > What would you prefer? Blink seems to prefer *not* wrapping at all, but I
find
> > it hard to look at. I think 80 columns is a reasonable standard, although it
> > doesn't really map to monitors anymore (and doesn't match surrounding code).
I
> > chose to wrap here based on a subconscious aesthetic that said "this looks
> > ugly", probably because it goes beyond the line of code below.
> > 
> > I wrapped to 80 columns.
> 
> FWIW, Blink style is soon going to be transitioning to be more like Chromium
> style, and at that point I think they're adopting the 80-column rule, so
> wrapping to 80 now seems good to me.

Yay!

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:104: const
size_t length = data.getSomeData(contents, static_cast<size_t>(0));
On 2016/03/25 21:15:56, Peter Kasting wrote:
> On 2016/03/25 15:49:53, scroggo wrote:
> > On 2016/03/25 06:24:37, Peter Kasting wrote:
> > > Nit: Should not need static_cast
> > 
> > I was surprised by this one, too, but position is a STRICTLY_TYPED_ARG so I
> have
> > to use this cast. I've actually seen this elsewhere as a solution for using
a
> > literal [1]. It seems like something isn't working though; it looks to have
a
> > default argument, but calling data.getSomeData(contents) does not compile.
> 
> I think the one-arg version might compile if you do
> getSomeData<size_t>(contents), since the macros expand to a templatized
function
> like this.
> 
> Also, I think if you change the body of getSomeData() to use
> ALLOW_NUMERIC_ARG_TYPES_PROMOTABLE_TO() where it uses STRICT_ARG_TYPE(), you
> could pass 0U here (maybe just 0, but I'm not sure).  Consider doing this as a
> separate (possible precursor?) change and possibly also changing the other
uses
> of that macro in that file similarly.
> 
> Although, since this seems to be the only class in the codebase which uses
these
> macros, I think it would be even better to just rip these out entirely.  I
doubt
> they buy significant safety and it's not compelling to have them for just one
of
> the many classes which can take buffer/byte sizes.  This would also probably
> eliminate the weirdness where the default arg doesn't work.

FWIW, this macro was introduced fairly recently, in crrev.com/1571233003. In
that commit message junov states that it would be a giant patch to use it
everywhere, but this started where there was a problem.

If it's alright with you, I'll specify the template for now and not expand the
scope of this CL.

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ROBufferSegmentReader.cpp:21:
size_t size() const override { return m_roBuffer ? m_roBuffer->size() : 0; }
On 2016/03/25 21:15:56, Peter Kasting wrote:
> On 2016/03/25 15:49:53, scroggo wrote:
> > On 2016/03/25 06:24:37, Peter Kasting wrote:
> > > Nit: Why define this inline (and nothing else)?
> > 
> > Because it's short. What is your preference?
> 
> I generally avoid inlining virtuals (see "stop inlining virtuals" on
> http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts ), but it
> doesn't matter much here as this is in a .cpp file and not a .h file.  Do it
> however you like.

Good enough reason to me not to inline, for consistency. Done.

[Peter Kasting, 2016-03-26 01:53:04]

LGTM

https://codereview.chromium.org/1812273003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:120: //
beginning.
Nit: No need to linebreak comment

https://codereview.chromium.org/1812273003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:142: 
Nit: Extra blank line

[urvang, 2016-03-28 21:29:32]

A couple of comments on WebP

https://codereview.chromium.org/1812273003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
(right):

https://codereview.chromium.org/1812273003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:199:
m_consolidatedData = m_data->getAsSkData();
You need to clear existing data in m_consolidatedData before assigning new data
to it? Otherwise, you'll leak data on consecutive updateDemuxer() calls, IIUC.
[Note how the line above deletes m_demux object too before creating a new
m_demux object, similarly].

https://codereview.chromium.org/1812273003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:204:
m_consolidatedData.clear();
Why this special clear() call? Wouldn't this automatically happen in the
WebPIMageDecoder destructor anyway?

[scroggo_chromium, 2016-04-04 13:59:47]

Sorry for the long delay - just got back from "vacation"/moving.

I've responded to the comments in patch set 19, and next I'll upload a rebase if
one is necessary.

https://codereview.chromium.org/1812273003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:120: //
beginning.
On 2016/03/26 01:53:03, Peter Kasting wrote:
> Nit: No need to linebreak comment

Done.

https://codereview.chromium.org/1812273003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:142: 
On 2016/03/26 01:53:03, Peter Kasting wrote:
> Nit: Extra blank line

Done.

https://codereview.chromium.org/1812273003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
(right):

https://codereview.chromium.org/1812273003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:199:
m_consolidatedData = m_data->getAsSkData();
On 2016/03/28 21:29:32, urvang wrote:
> You need to clear existing data in m_consolidatedData before assigning new
data
> to it? Otherwise, you'll leak data on consecutive updateDemuxer() calls, IIUC.
> [Note how the line above deletes m_demux object too before creating a new
> m_demux object, similarly].

m_demux is a raw pointer, so we definitely need to delete it before reassigning
to it. m_consolidatedData is a RefPtr<SkData>. As I understand it, assigning a
PassRefPtr (which is returned by getAsSkData) to it will clear the old value.
(To be more precise, based on RefPtr's operator=, which takes a RefPtr
parameter, the PassRefPtr will be implicitly converted to a RefPtr, which will
be swapped with the existing value. The anonymous RefPtr's destructor will unref
the previous SkData.) Am I missing something?

https://codereview.chromium.org/1812273003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp:204:
m_consolidatedData.clear();
On 2016/03/28 21:29:32, urvang wrote:
> Why this special clear() call? Wouldn't this automatically happen in the
> WebPIMageDecoder destructor anyway?

This was to tie the lifetime of m_consolidatedData to the lifetime of m_demux.
Here we failed to create m_demux, so I cleared m_consolidatedData. Yes,
m_consolidatedData will get cleared in the destructor, although as far as I can
tell, we will not necessarily call the destructor soon, just because the decoder
has failed. For example, it looks like the ImageFrameGenerator leaves a failed
decoder in the ImageDecodingStore [1]. I think the intent here is so that the
next call to decode still knows that it failed before. But it does not need
m_consolidatedData anymore (or m_demux, for that matter, but it only gets
cleared in the destructor or in one of the places where we call setFailed). As I
said, I tied the lifetime to the demuxer, but it seems like neither is needed
after failure. I'd be interested to delete m_demux when it's no longer needed,
but I think that should be an isolated change.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[scroggo_chromium, 2016-04-04 15:16:38]

The CQ bit was checked by scroggo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-04 15:16:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1812273003/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1812273003/400001

[commit-bot: I haz the power, 2016-04-04 16:18:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-04 16:18:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[urvang, 2016-04-04 18:28:15]

lgtm

LGTM for WebP

[scroggo_chromium, 2016-04-05 20:15:15]

The CQ bit was checked by scroggo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-05 20:15:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1812273003/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1812273003/420001

[scroggo_chromium, 2016-04-05 20:43:04]

https://codereview.chromium.org/1812273003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:113:
SkROBuffer::Iter iter(m_roBuffer.get());
Along with the switch to ThreadSafeRefCounted, this makes the
ROBufferSegmentReader thread-safe. I now use a local Iter instead of keeping a
mutable one in the class. In theory this could slow down decodes, since we have
to iterate from the beginning in each call to getSomeData, but I did not see a
significant  difference. If it did cause a slowdown, we could try another way to
make this safe (without having to start the iterator from the beginning), e.g.
locking a mutex.

https://codereview.chromium.org/1812273003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:114: for
(size_t sizeOfBlock = iter.size(), positionOfBlock = 0; sizeOfBlock != 0;
positionOfBlock += sizeOfBlock, sizeOfBlock = iter.size()) {
When I converted to a for loop, I left out updating sizeOfBlock. This often
worked out okay, but I eventually noticed artifacts from reading the wrong data.
This patch set updates sizeOfBlock correctly.

https://codereview.chromium.org/1812273003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/SegmentReader.h (right):

https://codereview.chromium.org/1812273003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.h:28: class
PLATFORM_EXPORT SegmentReader : public ThreadSafeRefCounted<SegmentReader> {
This fixed a bug where I sometimes hit an assert in
ThreadRestrictionVerifier::checkSafeToUse() [1]

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Peter Kasting, 2016-04-05 22:26:57]

Segment reader still LGTM.

https://codereview.chromium.org/1812273003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:114: for
(size_t sizeOfBlock = iter.size(), positionOfBlock = 0; sizeOfBlock != 0;
positionOfBlock += sizeOfBlock, sizeOfBlock = iter.size()) {
On 2016/04/05 20:43:04, scroggo_chromium wrote:
> When I converted to a for loop, I left out updating sizeOfBlock. This often
> worked out okay, but I eventually noticed artifacts from reading the wrong
data.
> This patch set updates sizeOfBlock correctly.

Good catch.  Any way to set up a test that would verify this (by having blocks
of different sizes)?

[commit-bot: I haz the power, 2016-04-05 22:35:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-05 22:35:41]

Dry run: This issue passed the CQ dry run.

[scroggo_chromium, 2016-04-06 14:23:45]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2016-04-06 14:23:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmalita@chromium.org, urvang@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1812273003/#ps440001
(title: "Add test for ROBufferSegmentReader::getSomeData with variable lengths")

[commit-bot: I haz the power, 2016-04-06 14:24:05]

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

[commit-bot: I haz the power, 2016-04-06 15:52:16]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

[commit-bot: I haz the power, 2016-04-06 15:52:18]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2016-04-06 15:55:24]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.

Committed: https://crrev.com/33cfb9bd4aec6d9ae36231057436c78b47de8585
Cr-Commit-Position: refs/heads/master@{#385470}
==========

[commit-bot: I haz the power, 2016-04-06 15:55:25]

Patchset 23 (id:??) landed as
https://crrev.com/33cfb9bd4aec6d9ae36231057436c78b47de8585
Cr-Commit-Position: refs/heads/master@{#385470}

[scroggo_chromium, 2016-04-06 19:20:23]

https://codereview.chromium.org/1812273003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:114: for
(size_t sizeOfBlock = iter.size(), positionOfBlock = 0; sizeOfBlock != 0;
positionOfBlock += sizeOfBlock, sizeOfBlock = iter.size()) {
On 2016/04/05 22:26:57, Peter Kasting wrote:
> On 2016/04/05 20:43:04, scroggo_chromium wrote:
> > When I converted to a for loop, I left out updating sizeOfBlock. This often
> > worked out okay, but I eventually noticed artifacts from reading the wrong
> data.
> > This patch set updates sizeOfBlock correctly.
> 
> Good catch.  Any way to set up a test that would verify this (by having blocks
> of different sizes)?

Done.

[scroggo_chromium, 2016-04-07 20:16:01]

A revert of this CL (patchset #23 id:440001) has been created in
https://codereview.chromium.org/1866243003/ by scroggo@chromium.org.

The reason for reverting is: Speculative fix for crbug.com/601416.

[scroggo_chromium, 2016-04-08 19:23:27]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created. However, it theoretically could use a decoder that
had previously been used to decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

I do not know whether this can happen in practice.

Committed: https://crrev.com/33cfb9bd4aec6d9ae36231057436c78b47de8585
Cr-Commit-Position: refs/heads/master@{#385470}
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created.

e.g

DeferredImageDecoder did = ...
did.setData(...);

SkImage image = did.createFrameAtIndex(0);
did.setData(...); // More data

image will not attempt to decode the extra data provided in the last
to setData.

However, it could use a decoder that had previously been used to
decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

imageA will look like imageB, even though it had less data. 

Committed: https://crrev.com/33cfb9bd4aec6d9ae36231057436c78b47de8585
Cr-Commit-Position: refs/heads/master@{#385470}
==========

[scroggo_chromium, 2016-04-11 20:27:19]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data.
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created.

e.g

DeferredImageDecoder did = ...
did.setData(...);

SkImage image = did.createFrameAtIndex(0);
did.setData(...); // More data

image will not attempt to decode the extra data provided in the last
to setData.

However, it could use a decoder that had previously been used to
decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

imageA will look like imageB, even though it had less data. 

Committed: https://crrev.com/33cfb9bd4aec6d9ae36231057436c78b47de8585
Cr-Commit-Position: refs/heads/master@{#385470}
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data. (Note - this still requires a mutex for using
the SkROBuffer::Iter. But in practice, there is no contention, unlike
the old implementation, which had to lock in order to copy into the
reader thread.)
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created.

e.g

DeferredImageDecoder did = ...
did.setData(...);

SkImage image = did.createFrameAtIndex(0);
did.setData(...); // More data

image will not attempt to decode the extra data provided in the last
call to setData.

However, it could use a decoder that had previously been used to
decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

imageA *may* look like imageB, even though it had less data.
==========

[scroggo_chromium, 2016-04-11 20:56:57]

On 2016/04/07 20:16:01, scroggo_chromium wrote:
> A revert of this CL (patchset #23 id:440001) has been created in
> https://codereview.chromium.org/1866243003/ by mailto:scroggo@chromium.org.
> 
> The reason for reverting is: Speculative fix for crbug.com/601416.

It turns out that this was causing crbug.com/601416. The problem was the
specific case I stated in the commit message that I was not sure could happen.
It does. I have fixed the problem and added tests to ensure that we do not crash
in that case. (I'll follow up in crbug.com/335694 with more details.)

There was also a performance regression (crbug.com/601799), which I believe to
be related to using a new iterator each time (see
https://codereview.chromium.org/1812273003/diff/420001/third_party/WebKit/Sou...).
This has also been fixed.

[Peter Kasting, 2016-04-12 22:56:26]

LGTM

https://codereview.chromium.org/1812273003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp
(right):

https://codereview.chromium.org/1812273003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:412: //
FIXME (scroggo): combine this with ImageDecoderTestHelpers?
Nit: We prefer TODO to FIXME, I believe.

https://codereview.chromium.org/1812273003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:98: //
Protects access to mutable fields
Nit: Trailing period

https://codereview.chromium.org/1812273003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp:390: if
(m_bytesRead >= m_data->size()) {
Add a comment describing the sequence that can result in this.

[scroggo_chromium, 2016-04-13 12:16:43]

https://codereview.chromium.org/1812273003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp
(right):

https://codereview.chromium.org/1812273003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp:412: //
FIXME (scroggo): combine this with ImageDecoderTestHelpers?
On 2016/04/12 22:56:25, Peter Kasting wrote:
> Nit: We prefer TODO to FIXME, I believe.

Done.

https://codereview.chromium.org/1812273003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/SegmentReader.cpp:98: //
Protects access to mutable fields
On 2016/04/12 22:56:25, Peter Kasting wrote:
> Nit: Trailing period

Done.

https://codereview.chromium.org/1812273003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
(right):

https://codereview.chromium.org/1812273003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp:390: if
(m_bytesRead >= m_data->size()) {
On 2016/04/12 22:56:25, Peter Kasting wrote:
> Add a comment describing the sequence that can result in this.

Done.

[scroggo_chromium, 2016-04-13 12:16:54]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2016-04-13 12:16:55]

The patchset sent to the CQ was uploaded after l-g-t-m from urvang@chromium.org,
fmalita@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1812273003/#ps560001
(title: "Clarify comment further")

[commit-bot: I haz the power, 2016-04-13 12:17:07]

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

[scroggo, 2016-04-13 12:30:23]

The CQ bit was unchecked by scroggo@google.com

[Noel Gordon, 2016-04-13 13:28:27]

LGTM, less data copying, less mutex holding, less work all round, and
measurement that suggests about a 15% win?  Not sure why we saw a regression on
the first try, but let's land and check perf again.

[scroggo_chromium, 2016-04-13 17:40:36]

I'm baffled. linux_chromium_asan_rel_ng [1] is consistently failing with my
patch. It is only failing my tests (which pass everywhere else), but the
interesting part is where it's failing. It fails to create a
DeferredImageDecoder from a SharedBuffer, which is known to not be null.
Investigating...

[1]
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

[scroggo_chromium, 2016-04-13 18:34:04]

On 2016/04/13 17:40:36, scroggo_chromium wrote:
> I'm baffled. linux_chromium_asan_rel_ng [1] is consistently failing with my
> patch. It is only failing my tests (which pass everywhere else), but the
> interesting part is where it's failing. It fails to create a
> DeferredImageDecoder from a SharedBuffer, which is known to not be null.
> Investigating...
> 
> [1]
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

I just uploaded the test (by itself - no other changes) to crrev.com/1887733002.
It failed without the rest of this change.

I also tried following the instructions to build asan locally
(http://www.chromium.org/developers/testing/addresssanitizer) and building with
the command that is used by the bot (from  [2]:

--brave-new-test-launcher --test-launcher-bot-mode
--test-launcher-print-test-stdio=always --test-launcher-batch-limit=1
--test-launcher-summary-output=/tmp/out1IHawf/output.json --no-sandbox

[2]
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

I have not been able to repro locally. So there is a problem, but it's not clear
there is a problem with my patch.

[scroggo_chromium, 2016-04-13 18:35:08]

The CQ bit was checked by scroggo@chromium.org

[commit-bot: I haz the power, 2016-04-13 18:35:42]

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

[commit-bot: I haz the power, 2016-04-13 19:24:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-13 19:24:50]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scroggo_chromium, 2016-04-13 20:05:47]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2016-04-13 20:05:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, urvang@chromium.org, fmalita@chromium.org,
noel@chromium.org
Link to the patchset: https://codereview.chromium.org/1812273003/#ps580001
(title: "Switch to ASSERT_TRUE(decoder.get())")

[commit-bot: I haz the power, 2016-04-13 20:06:08]

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

[commit-bot: I haz the power, 2016-04-13 20:18:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-13 20:18:19]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[f(malita), 2016-04-14 13:16:24]

On 2016/04/13 18:34:04, scroggo_chromium wrote:
> On 2016/04/13 17:40:36, scroggo_chromium wrote:
> > I'm baffled. linux_chromium_asan_rel_ng [1] is consistently failing with my
> > patch. It is only failing my tests (which pass everywhere else), but the
> > interesting part is where it's failing. It fails to create a
> > DeferredImageDecoder from a SharedBuffer, which is known to not be null.
> > Investigating...
> > 
> > [1]
> >
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> 
> I just uploaded the test (by itself - no other changes) to
crrev.com/1887733002.
> It failed without the rest of this change.
> 
> I also tried following the instructions to build asan locally
> (http://www.chromium.org/developers/testing/addresssanitizer) and building
with
> the command that is used by the bot (from  [2]:
> 
> --brave-new-test-launcher --test-launcher-bot-mode
> --test-launcher-print-test-stdio=always --test-launcher-batch-limit=1
> --test-launcher-summary-output=/tmp/out1IHawf/output.json --no-sandbox
> 
> [2]
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> 
> I have not been able to repro locally. So there is a problem, but it's not
clear
> there is a problem with my patch.

Confirmed that local ASAN builds don't have this problem.

As an experiment, can you try inlining the image data (rather than reading from
external file) - or maybe assert that readFile actually reads something?

[scroggo_chromium, 2016-04-14 13:37:29]

On 2016/04/14 13:16:24, f(malita) wrote:
> On 2016/04/13 18:34:04, scroggo_chromium wrote:
> > On 2016/04/13 17:40:36, scroggo_chromium wrote:
> > > I'm baffled. linux_chromium_asan_rel_ng [1] is consistently failing with
my
> > > patch. It is only failing my tests (which pass everywhere else), but the
> > > interesting part is where it's failing. It fails to create a
> > > DeferredImageDecoder from a SharedBuffer, which is known to not be null.
> > > Investigating...
> > > 
> > > [1]
> > >
> >
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> > 
> > I just uploaded the test (by itself - no other changes) to
> crrev.com/1887733002.
> > It failed without the rest of this change.
> > 
> > I also tried following the instructions to build asan locally
> > (http://www.chromium.org/developers/testing/addresssanitizer) and building
> with
> > the command that is used by the bot (from  [2]:
> > 
> > --brave-new-test-launcher --test-launcher-bot-mode
> > --test-launcher-print-test-stdio=always --test-launcher-batch-limit=1
> > --test-launcher-summary-output=/tmp/out1IHawf/output.json --no-sandbox
> > 
> > [2]
> >
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> > 
> > I have not been able to repro locally. So there is a problem, but it's not
> clear
> > there is a problem with my patch.
> 
> Confirmed that local ASAN builds don't have this problem.
> 
> As an experiment, can you try inlining the image data (rather than reading
from
> external file)

That should work. There are a couple of inlined images already in this file, and
no other tests fail.

> - or maybe assert that readFile actually reads something?

I tried uploading a CL with just the test, and adding some print statements (see
crrev.com/1887733002), and as you suspect, the files are empty. (To my surprise,
readFile returned an empty SharedBuffer, rather than a nullptr.)

I was thinking this was an ASAN problem, but I'm not sure any other trybots ran
webkit_unit_tests (the ones I've clicked on did not, but I'm not positive I dug
into them all). So maybe it would be a problem on other bots, too. I just kicked
off some more tries that I guessed might run webkit_unit_tests, to see if they
also fail. Many of our ImageDecoder tests, which are also part of
webkit_unit_tests, also attempt to read files. So those should also fail if e.g.
the file is missing or cannot be read. (The ImageDecoder tests did not get run
on this failing bot, which says 24 tests are disabled from webkit_unit_tests. I
wonder if the ImageDecoderTests are disabled due to missing files? I have not
been able to figure that out.)

[scroggo_chromium, 2016-04-14 13:56:51]

On 2016/04/14 13:37:29, scroggo_chromium wrote:
> On 2016/04/14 13:16:24, f(malita) wrote:
> > On 2016/04/13 18:34:04, scroggo_chromium wrote:
> > > On 2016/04/13 17:40:36, scroggo_chromium wrote:
> > > > I'm baffled. linux_chromium_asan_rel_ng [1] is consistently failing with
> my
> > > > patch. It is only failing my tests (which pass everywhere else), but the
> > > > interesting part is where it's failing. It fails to create a
> > > > DeferredImageDecoder from a SharedBuffer, which is known to not be null.
> > > > Investigating...
> > > > 
> > > > [1]
> > > >
> > >
> >
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> > > 
> > > I just uploaded the test (by itself - no other changes) to
> > crrev.com/1887733002.
> > > It failed without the rest of this change.
> > > 
> > > I also tried following the instructions to build asan locally
> > > (http://www.chromium.org/developers/testing/addresssanitizer) and building
> > with
> > > the command that is used by the bot (from  [2]:
> > > 
> > > --brave-new-test-launcher --test-launcher-bot-mode
> > > --test-launcher-print-test-stdio=always --test-launcher-batch-limit=1
> > > --test-launcher-summary-output=/tmp/out1IHawf/output.json --no-sandbox
> > > 
> > > [2]
> > >
> >
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> > > 
> > > I have not been able to repro locally. So there is a problem, but it's not
> > clear
> > > there is a problem with my patch.
> > 
> > Confirmed that local ASAN builds don't have this problem.
> > 
> > As an experiment, can you try inlining the image data (rather than reading
> from
> > external file)
> 
> That should work. There are a couple of inlined images already in this file,
and
> no other tests fail.
> 
> > - or maybe assert that readFile actually reads something?
> 
> I tried uploading a CL with just the test, and adding some print statements
(see
> crrev.com/1887733002), and as you suspect, the files are empty. (To my
surprise,
> readFile returned an empty SharedBuffer, rather than a nullptr.)
> 
> I was thinking this was an ASAN problem, but I'm not sure any other trybots
ran
> webkit_unit_tests (the ones I've clicked on did not, but I'm not positive I
dug
> into them all). So maybe it would be a problem on other bots, too. I just
kicked
> off some more tries that I guessed might run webkit_unit_tests, to see if they
> also fail. Many of our ImageDecoder tests, which are also part of
> webkit_unit_tests, also attempt to read files. So those should also fail if
e.g.
> the file is missing or cannot be read. (The ImageDecoder tests did not get run
> on this failing bot, which says 24 tests are disabled from webkit_unit_tests.
I
> wonder if the ImageDecoderTests are disabled due to missing files? I have not
> been able to figure that out.)

Latest try confirms that:
- another bot (not ASAN) had the same failure
- JPEGImageDecoderTest was not run on that bot either (again, some tests were
disabled...)

Using an inline image seems like a good idea.

[f(malita), 2016-04-14 14:41:17]

On 2016/04/14 13:56:51, scroggo_chromium wrote:
> Latest try confirms that:
> - another bot (not ASAN) had the same failure
> - JPEGImageDecoderTest was not run on that bot either (again, some tests were
> disabled...)
> 
> Using an inline image seems like a good idea.

I noticed that tests which make use of testing::blinkRootDir()/readFromFile()
are under a different target: blink_platform_unittests (rather than
webkit_unit_tests).  I'm guessing this is a limitation of the target.

Based on comments in blink_platform.gypi, it seems we should avoid
webkit_unit_tests if possible:

    # NOTE: These are legacy unit tests and tests that require a Platform
    # object. Do not add more unless the test requires a Platform object.
    # These tests are a part of the web:webkit_unit_tests binary.

Maybe you can keep the data external, but have your new tests live under
blink_platform_unittests:DeferredImageDecoderPlatformTest instead?  That should
also give you access to ImageDecoderTestHelpers and remove some duplication.

[f(malita), 2016-04-14 15:03:05]

On 2016/04/14 14:41:17, f(malita) wrote:
> On 2016/04/14 13:56:51, scroggo_chromium wrote:
> > Latest try confirms that:
> > - another bot (not ASAN) had the same failure
> > - JPEGImageDecoderTest was not run on that bot either (again, some tests
were
> > disabled...)
> > 
> > Using an inline image seems like a good idea.
> 
> I noticed that tests which make use of testing::blinkRootDir()/readFromFile()
> are under a different target: blink_platform_unittests (rather than
> webkit_unit_tests).  I'm guessing this is a limitation of the target.
> 
> Based on comments in blink_platform.gypi, it seems we should avoid
> webkit_unit_tests if possible:
> 
>     # NOTE: These are legacy unit tests and tests that require a Platform
>     # object. Do not add more unless the test requires a Platform object.
>     # These tests are a part of the web:webkit_unit_tests binary.
> 
> Maybe you can keep the data external, but have your new tests live under
> blink_platform_unittests:DeferredImageDecoderPlatformTest instead?  That
should
> also give you access to ImageDecoderTestHelpers and remove some duplication.

(I'm also fine with landing inline data and looking at relocating later -
whatever is easier for you to move this forward at this point)

[scroggo_chromium, 2016-04-14 15:49:42]

The CQ bit was checked by scroggo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-14 15:49:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1812273003/620001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1812273003/620001

[scroggo_chromium, 2016-04-14 15:52:08]

On 2016/04/14 14:41:17, f(malita) wrote:
> On 2016/04/14 13:56:51, scroggo_chromium wrote:
> > Latest try confirms that:
> > - another bot (not ASAN) had the same failure
> > - JPEGImageDecoderTest was not run on that bot either (again, some tests
were
> > disabled...)
> > 
> > Using an inline image seems like a good idea.
> 
> I noticed that tests which make use of testing::blinkRootDir()/readFromFile()
> are under a different target: blink_platform_unittests (rather than
> webkit_unit_tests).  I'm guessing this is a limitation of the target.
> 
> Based on comments in blink_platform.gypi, it seems we should avoid
> webkit_unit_tests if possible:
> 
>     # NOTE: These are legacy unit tests and tests that require a Platform
>     # object. Do not add more unless the test requires a Platform object.
>     # These tests are a part of the web:webkit_unit_tests binary.
> 
> Maybe you can keep the data external, but have your new tests live under
> blink_platform_unittests:DeferredImageDecoderPlatformTest instead?  That
should
> also give you access to ImageDecoderTestHelpers and remove some duplication.

Done. I called it DeferredImageDecoderWoPlatform instead, since the interesting
difference is that it does not require a Platform object. (Some of the other
tests could be moved, too, but I left that for a later exercise. I think the
only ones that still require it (in that file) need it for threading, but
perhaps there's a way to do that in blink_platform_unittests.) I am not attached
to the name, though.

[f(malita), 2016-04-14 16:24:23]

On 2016/04/14 15:52:08, scroggo_chromium wrote:
> On 2016/04/14 14:41:17, f(malita) wrote:
> > On 2016/04/14 13:56:51, scroggo_chromium wrote:
> > > Latest try confirms that:
> > > - another bot (not ASAN) had the same failure
> > > - JPEGImageDecoderTest was not run on that bot either (again, some tests
> were
> > > disabled...)
> > > 
> > > Using an inline image seems like a good idea.
> > 
> > I noticed that tests which make use of
testing::blinkRootDir()/readFromFile()
> > are under a different target: blink_platform_unittests (rather than
> > webkit_unit_tests).  I'm guessing this is a limitation of the target.
> > 
> > Based on comments in blink_platform.gypi, it seems we should avoid
> > webkit_unit_tests if possible:
> > 
> >     # NOTE: These are legacy unit tests and tests that require a Platform
> >     # object. Do not add more unless the test requires a Platform object.
> >     # These tests are a part of the web:webkit_unit_tests binary.
> > 
> > Maybe you can keep the data external, but have your new tests live under
> > blink_platform_unittests:DeferredImageDecoderPlatformTest instead?  That
> should
> > also give you access to ImageDecoderTestHelpers and remove some duplication.
> 
> Done. I called it DeferredImageDecoderWoPlatform instead, since the
interesting
> difference is that it does not require a Platform object. (Some of the other
> tests could be moved, too, but I left that for a later exercise. I think the
> only ones that still require it (in that file) need it for threading, but
> perhaps there's a way to do that in blink_platform_unittests.) I am not
attached
> to the name, though.

Thanks Leon.  LGTM w/ name bikeshedding.

Blink's unit tests generally use a *Test sufix; we also tend to avoid
abbreviations - so maybe DeferredImageDecoderWithoutPlatformTest or
DeferredImageDecoderNoPlatformTest?


(Another approach would be to name the test files based on the target/binary
they belong to:

  blink_platform_unittests -> DeferredImageDecoderPlatformTest
  webkit_unit_tests        -> DeferredImageDecoderWebkitTest

We can look at this + moving some of the other tests in a follow-up)

[scroggo_chromium, 2016-04-14 16:36:05]

The CQ bit was unchecked by scroggo@chromium.org

[scroggo_chromium, 2016-04-14 16:36:06]

The CQ bit was checked by scroggo@chromium.org

[scroggo_chromium, 2016-04-14 16:36:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, urvang@chromium.org, noel@chromium.org
Link to the patchset: https://codereview.chromium.org/1812273003/#ps620001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-14 16:36:30]

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

[commit-bot: I haz the power, 2016-04-14 17:04:31]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data. (Note - this still requires a mutex for using
the SkROBuffer::Iter. But in practice, there is no contention, unlike
the old implementation, which had to lock in order to copy into the
reader thread.)
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created.

e.g

DeferredImageDecoder did = ...
did.setData(...);

SkImage image = did.createFrameAtIndex(0);
did.setData(...); // More data

image will not attempt to decode the extra data provided in the last
call to setData.

However, it could use a decoder that had previously been used to
decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

imageA *may* look like imageB, even though it had less data.
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data. (Note - this still requires a mutex for using
the SkROBuffer::Iter. But in practice, there is no contention, unlike
the old implementation, which had to lock in order to copy into the
reader thread.)
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created.

e.g

DeferredImageDecoder did = ...
did.setData(...);

SkImage image = did.createFrameAtIndex(0);
did.setData(...); // More data

image will not attempt to decode the extra data provided in the last
call to setData.

However, it could use a decoder that had previously been used to
decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

imageA *may* look like imageB, even though it had less data.
==========

[commit-bot: I haz the power, 2016-04-14 17:04:33]

Committed patchset #32 (id:620001)

[commit-bot: I haz the power, 2016-04-14 17:05:21]

Description was changed from

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data. (Note - this still requires a mutex for using
the SkROBuffer::Iter. But in practice, there is no contention, unlike
the old implementation, which had to lock in order to copy into the
reader thread.)
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created.

e.g

DeferredImageDecoder did = ...
did.setData(...);

SkImage image = did.createFrameAtIndex(0);
did.setData(...); // More data

image will not attempt to decode the extra data provided in the last
call to setData.

However, it could use a decoder that had previously been used to
decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

imageA *may* look like imageB, even though it had less data.
==========

to

==========
Eliminate copies of encoded image data

Remove the ThreadSafeDataTransport, which required making two copies of
the encoded data. Instead, copy the data (once) into an SkRWBuffer,
which allows creating read only snapshots that can be safely read in
another thread.

Design doc can be found at
https://docs.google.com/document/d/1aNmMaWfjkSSkdnVkAJoBUN7mNwdCXT1wLghrhDhhS...
Benchmark results can be found at
https://docs.google.com/spreadsheets/d/1Y_9UbzBvyg1HRs2uieeV2Twb7seJFKZulRVg7...

DeferredImageDecoder:
Copy into an SkRWBuffer, and create snapshots to pass to each
DecodingImageGenerator.

DecodingImageGenerator:
Store a snapshot of the data, along with whether the snapshot contains
all of the data. Pass this to the ImageFrameGenerator when it is time
to decode. Since it already knows whether it has received all the data,
never attempt to YUV decode if not.
When created from an SkData, skip the copy into a SharedBuffer. Instead,
wrap in a SegmentReader.

ImageFrameGenerator:
Its decoding APIs now take the encoded data and whether it's complete as
parameters. Does not own the encoded data, and no longer has the
implementation of onRefEncodedData.
Make the object thread-safe where possible, so we can only lock the
mutex during operations that truly need it.

SegmentReader:
Add a new interface that looks like a read only SharedBuffer. Add three
implementations:
- SharedBufferSegmentReader:
Used when we already have a SharedBuffer (e.g. in the main thread).
- ROBufferSegmentReader:
Facilitates decoding in a worker thread without requiring mutex locks/
an extra copy of the data. (Note - this still requires a mutex for using
the SkROBuffer::Iter. But in practice, there is no contention, unlike
the old implementation, which had to lock in order to copy into the
reader thread.)
- DataSegmentReader:
Allows using an SkData directly without copying.

ImageDecoders/FastSharedBufferReader:
Read data from a SegmentReader instead of directly from a SharedBuffer.
(Add an ImageDecoder helper that takes a SharedBuffer parameter, reducing
the need to change code that already has a SharedBuffer.)
Since the API is the same, the changes here are small, except for
WEBPImageDecoder, which no longer calls SharedBuffer::data(), and
instead calls SegmentReader::refAsSkData().
Use PassRefPtr instead of raw pointers for parameter passing in setData.

Tests:
Adapt where a SharedBuffer is no longer used.

PictureSnapshot.cpp
Wrap SkData in SegmentReader.

BUG=467772
BUG=483369

BUG=335694
This may also help with crrbug.com/335694. The DecodingImageGenerator
will no longer attempt to read beyond the data that had been received
when it was created.

e.g

DeferredImageDecoder did = ...
did.setData(...);

SkImage image = did.createFrameAtIndex(0);
did.setData(...); // More data

image will not attempt to decode the extra data provided in the last
call to setData.

However, it could use a decoder that had previously been used to
decode more data than it was created with.

e.g.

DeferredImageDecoder did = ...
did.setData(...);
SkImage imageA = did.createFrameAtIndex(0); // DecodingImageGenerator

did.setData(...);   // More data
SkImage imageB = did.createFrameAtIndex(0);

// draw imageB - this decodes to the end of data provided in the second
//               call to setData

// draw imageA - this can use the same ImageDecoder retrieved from the
//               ImageDecodingStore, meaning it will have already
//               decoded beyond the data supplied to imageA

imageA *may* look like imageB, even though it had less data.

Committed: https://crrev.com/d2234904faee943bd987bd38d620096db808efca
Cr-Commit-Position: refs/heads/master@{#387344}
==========

[commit-bot: I haz the power, 2016-04-14 17:05:24]

Patchset 32 (id:??) landed as
https://crrev.com/d2234904faee943bd987bd38d620096db808efca
Cr-Commit-Position: refs/heads/master@{#387344}