Ganesh: images upload to GPU performance fix (skip copying encoded data)

GPU raster - images upload to GPU performance fix (skip copying encoded data).

For Bug 476531, improved FPS from 5 to 25 (forced GPU rasterization on Lenovo K3 Note).
There DecodingImageGenerator::onRefEncodedData() took 80 - 100 ms on ~65MB of animated
gif data.

This affects GPU texture upload for all image types when content is downloaded
(onRefEncoded returns 0 for partially downloaded content) since it removes 2 memory
allocations and copies from DecodingImageGenerator::onRefEncodedData() bellow:

When invoking e.g. drawImageRect code would trigger

GrTexture* SkImageCacherator::lockTexture...
// 3. Ask the generator to return a compressed form that the GPU might support
SkAutoTUnref<SkData> data(this->refEncoded());

leading to:

SkData* DecodingImageGenerator::onRefEncodedData() { ...
    m_frameGenerator->copyData(&buffer, &allDataReceived);
    if (buffer && allDataReceived)
        return SkData::NewWithCopy(buffer->data(), buffer->size());

Covered invalid use case, when ThreadSafeDataTransport::setData is called to append
data to already fully received SharedBuffer.

Thread safety ensured by holding the reference to ThreadSafeRefCounted
ThreadSafeDataTransport (that is again holding ref to SharedBuffer).

BUG=476531
Committed: https://crrev.com/03166f6a694d43dff7e71ae04fa2847317c51101
Cr-Commit-Position: refs/heads/master@{#363939}

[aleksandar.stojiljkovic, 2015-12-01 00:52:45]

Description was changed from

==========
GPU raster large images and animated gif performance fix.

For Bug 476531, improved FPS from 5 to 25 (forced GPU rasterization on Lenovo K3
Note).
There DecodingImageGenerator::onRefEncodedData() took 80 - 100 ms on ~65MB of
animated
gif data.

This affects GPU texture upload for other image types too when content is
downloaded
(onRefEncoded returns 0 for partially downloaded content) since it removes 2
memory
allocations and copies from DecodingImageGenerator::onRefEncodedData() bellow:

When invoking e.g. drawImageRect code would trigger

GrTexture* SkImageCacherator::lockTexture...
// 3. Ask the generator to return a compressed form that the GPU might support
SkAutoTUnref<SkData> data(this->refEncoded());

leading to:

SkData* DecodingImageGenerator::onRefEncodedData() { ...
    m_frameGenerator->copyData(&buffer, &allDataReceived);
    if (buffer && allDataReceived)
        return SkData::NewWithCopy(buffer->data(), buffer->size());

BUG=476531
==========

to

==========
GPU raster large images and animated gif performance fix.

FPS improves from 5 to 25 for example in Bug 476531.
(forced GPU rasterization on Lenovo K3 Note).

There, DecodingImageGenerator::onRefEncodedData() took 80 - 100 ms on ~65MB of
animated
gif data.

This affects GPU texture upload for other image types too when content is
downloaded
(onRefEncoded returns 0 for partially downloaded content) since it removes 2
memory
allocations and copies from DecodingImageGenerator::onRefEncodedData() bellow:

When invoking e.g. drawImageRect code would trigger

GrTexture* SkImageCacherator::lockTexture...
// 3. Ask the generator to return a compressed form that the GPU might support
SkAutoTUnref<SkData> data(this->refEncoded());

leading to:

SkData* DecodingImageGenerator::onRefEncodedData() { ...
    m_frameGenerator->copyData(&buffer, &allDataReceived);
    if (buffer && allDataReceived)
        return SkData::NewWithCopy(buffer->data(), buffer->size());

BUG=476531
==========

[aleksandar.stojiljkovic, 2015-12-01 00:52:45]

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

[reed1, 2015-12-01 03:42:45]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-12-01 03:42:45]

lgtm

[aleksandar.stojiljkovic, 2015-12-01 06:18:27]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-01 06:18:36]

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

[aleksandar.stojiljkovic, 2015-12-01 07:30:20]

The CQ bit was unchecked by aleksandar.stojiljkovic@intel.com

[aleksandar.stojiljkovic, 2015-12-01 07:30:25]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[commit-bot: I haz the power, 2015-12-01 07:30:55]

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

[aleksandar.stojiljkovic, 2015-12-01 07:38:23]

The CQ bit was unchecked by aleksandar.stojiljkovic@intel.com

[commit-bot: I haz the power, 2015-12-01 07:39:00]

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

[commit-bot: I haz the power, 2015-12-01 07:39:01]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[aleksandar.stojiljkovic, 2015-12-01 20:54:29]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ chrishtr@chromium.org, senorblanco@chromium.org

[aleksandar.stojiljkovic, 2015-12-01 20:54:29]

+chrishtr, senorblanko - owners approval required.

[chrishtr, 2015-12-01 21:56:45]

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

https://codereview.chromium.org/1484853003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:131:
m_data.data(&buffer, &allDataReceived);
Please change ThreadSafeDataTransport::data to return a PassRefPtr, and change
the name to something like getData(). That would make this CL easier to read and
more obviously correct.

[aleksandar.stojiljkovic, 2015-12-02 00:13:00]

On 2015/12/01 21:56:45, chrishtr wrote:
>
https://codereview.chromium.org/1484853003/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
> (right):
> 
>
https://codereview.chromium.org/1484853003/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:131:
> m_data.data(&buffer, &allDataReceived);
> Please change ThreadSafeDataTransport::data to return a PassRefPtr, and change
> the name to something like getData(). That would make this CL easier to read
and
> more obviously correct.

Did some background check and need your advise:

1) in original patch, hclam (reviewed by senorblanco
https://bugs.webkit.org/show_bug.cgi?id=103797) replaced original name
prepareData with m_data.data().
He also replaced RefPtr<SharedBuffer> by SharedBuffer* in effort to reduce
ref->unref reference count churn for path e.g. ImageFrameGenerator ->
JPEGImageReader::setData.

2) WebKit deprecated PassRefPtr: https://bugs.webkit.org/show_bug.cgi?id=144092
Replace PassRefPtr with Ref/RefPtr everywhere

I could add a method like prepareAndGetData to ThreadSafeDataTransport that
would pass ref ptr for this case, and maybe leave current
data(SharedBuffer**)->ImageDecoder::setData(SharedBuffer*...).
christr, what do you think? Thanks.

[chrishtr, 2015-12-02 01:02:55]

On 2015/12/02 at 00:13:00, aleksandar.stojiljkovic wrote:
> On 2015/12/01 21:56:45, chrishtr wrote:
> >
https://codereview.chromium.org/1484853003/diff/1/third_party/WebKit/Source/p...
> > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
> > (right):
> > 
> >
https://codereview.chromium.org/1484853003/diff/1/third_party/WebKit/Source/p...
> > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:131:
> > m_data.data(&buffer, &allDataReceived);
> > Please change ThreadSafeDataTransport::data to return a PassRefPtr, and
change
> > the name to something like getData(). That would make this CL easier to read
and
> > more obviously correct.
> 
> Did some background check and need your advise:
> 
> 1) in original patch, hclam (reviewed by senorblanco
https://bugs.webkit.org/show_bug.cgi?id=103797) replaced original name
prepareData with m_data.data().
> He also replaced RefPtr<SharedBuffer> by SharedBuffer* in effort to reduce
ref->unref reference count churn for path e.g. ImageFrameGenerator ->
JPEGImageReader::setData.
> 
> 2) WebKit deprecated PassRefPtr:
https://bugs.webkit.org/show_bug.cgi?id=144092 Replace PassRefPtr with
Ref/RefPtr everywhere
> 
> I could add a method like prepareAndGetData to ThreadSafeDataTransport that
would pass ref ptr for this case, and maybe leave current
data(SharedBuffer**)->ImageDecoder::setData(SharedBuffer*...).
> christr, what do you think? Thanks.

prepareAndGetData sounds fine. Please use PassRefPtr for now, we may get rid of
it but not today. senorblanco, do you agree?

[Stephen White, 2015-12-02 02:44:55]

reed@ or scroggo@ should probably look at this to validate the SkData semantics.

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

https://codereview.chromium.org/1484853003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:124: delete
pbuffer;
It might be clearer to pass a bare pointer though the callback, and use an
explicit ref() at the call site and derefIfNotNull() here, and forget about
allocating/deleting the RefPtr. Would that work? (I don't normally recommend
using bare ptrs in Blink, but this callback situation is unusual.)

[Stephen White, 2015-12-02 02:45:41]

On 2015/12/02 01:02:55, chrishtr wrote:
> On 2015/12/02 at 00:13:00, aleksandar.stojiljkovic wrote:
> > On 2015/12/01 21:56:45, chrishtr wrote:
> > >
>
https://codereview.chromium.org/1484853003/diff/1/third_party/WebKit/Source/p...
> > > File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
> > > (right):
> > > 
> > >
>
https://codereview.chromium.org/1484853003/diff/1/third_party/WebKit/Source/p...
> > > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:131:
> > > m_data.data(&buffer, &allDataReceived);
> > > Please change ThreadSafeDataTransport::data to return a PassRefPtr, and
> change
> > > the name to something like getData(). That would make this CL easier to
read
> and
> > > more obviously correct.
> > 
> > Did some background check and need your advise:
> > 
> > 1) in original patch, hclam (reviewed by senorblanco
> https://bugs.webkit.org/show_bug.cgi?id=103797) replaced original name
> prepareData with m_data.data().
> > He also replaced RefPtr<SharedBuffer> by SharedBuffer* in effort to reduce
> ref->unref reference count churn for path e.g. ImageFrameGenerator ->
> JPEGImageReader::setData.
> > 
> > 2) WebKit deprecated PassRefPtr:
> https://bugs.webkit.org/show_bug.cgi?id=144092 Replace PassRefPtr with
> Ref/RefPtr everywhere
> > 
> > I could add a method like prepareAndGetData to ThreadSafeDataTransport that
> would pass ref ptr for this case, and maybe leave current
> data(SharedBuffer**)->ImageDecoder::setData(SharedBuffer*...).
> > christr, what do you think? Thanks.
> 
> prepareAndGetData sounds fine. Please use PassRefPtr for now, we may get rid
of
> it but not today. senorblanco, do you agree?

SGTM

[Stephen White, 2015-12-02 02:50:39]

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

https://codereview.chromium.org/1484853003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:131:
m_data.data(&buffer, &allDataReceived);
On 2015/12/01 21:56:45, chrishtr wrote:
> Please change ThreadSafeDataTransport::data to return a PassRefPtr, and change
> the name to something like getData(). That would make this CL easier to read
and
> more obviously correct.

Even if we fix that, we still need something to pass through the callback. I've
recommended a bare pointer above, since I find this RefPtr* really odd, but
maybe that's going against your suggestion here.

[aleksandar.stojiljkovic, 2015-12-02 11:04:16]

1) Addressing readability issue with added comment:
    // Every SkData instance and SharedBuffer need to keep buffer->data()
refcounted.
    // Both SkData and m_data.m_readBuffer are having separate ref counting
implementations.
    //
    // SkData's SkData::NewWithProc is designed with similar use case in mind,
    // unmapping wrapped shmem data once all SkData instances are disposed.
    // in this case, sharedBufSkDataReleaseProc would just need to deref (since
m_data.m_readBuffer
    // might still hold a reference) and that is happening in
sharedBufSkDataReleaseProc.
    //
    // If m_data gets disposed, SkData would still need to hold the ref to
SharedBuffer,
    // and that is why SharedBuffer (accompanying returned SkData) is passed to
client of this
    // call - by contract they need to call SkData unref that would end up in
sharedBufSkDataReleaseProc
    // releasing the ref to SharedBuffer.
    //
    // A note about allDataReceived:
    // Client side use case is valid only for full image (encoded) data
downloaded,
    // but, incidentally, this is in line with current Chromium implementation,
where
    // DeferredImageDecoder::setData is called only once with allDataReceived
and after
    // that m_data.m_readBuffer.data() is not changed. For the sake of not
leaving loose ends,
    // ThreadSafeDataTransport::data is checking if there is new data added
after AllDataReceived

2) added check to invalid use case attempting to append
ThreadSafeDataTransport::setData when allData was already received. Handling
m_allDataReceived in thread safe way (although it wasn't an issue before).

chrishtr, I would prefer to send another follow up patch with refactoring
existing ThreadSafeDataTransport::data to all places - now that I got a grasp on
the code, plan to start with refactoring and documenting after GIF->Index8
decoding work. Just don't want some unplanned changes to creep in to critical
patch like this. What do you think?

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

https://codereview.chromium.org/1484853003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:131:
m_data.data(&buffer, &allDataReceived);
On 2015/12/02 02:50:39, Stephen White wrote:
> On 2015/12/01 21:56:45, chrishtr wrote:
> > Please change ThreadSafeDataTransport::data to return a PassRefPtr, and
change
> > the name to something like getData(). That would make this CL easier to read
> and
> > more obviously correct.
> 
> Even if we fix that, we still need something to pass through the callback.
I've
> recommended a bare pointer above, since I find this RefPtr* really odd, but
> maybe that's going against your suggestion here.

Thanks, both chrishtr and you have a point here about readability - looks
cleaner now without allocating RefPtr on heap to do deref.

[Noel Gordon, 2015-12-02 14:37:25]

"There, DecodingImageGenerator::onRefEncodedData() took 80 - 100 ms on ~65MB of
animated
gif data." 

So GrTexture* SkImageCacherator::lockTexture() tried to grab encoded GIF (and
likely WEBP / JPEG / PNG) data to upload to the GPU.  Currently, we can't GPU
decode such images from their encoded data.  No point trying to grab that data
as GrTexture.

Maybe SkImageCacherator could ask its cached SkImage if it's backed by a GIF /
WEBP / JPEG / PNG decoder to avoid these situations in future.

[aleksandar.stojiljkovic, 2015-12-02 15:23:59]

On 2015/12/02 14:37:25, noel gordon wrote:
> Maybe SkImageCacherator could ask its cached SkImage if it's backed by a GIF /
> WEBP / JPEG / PNG decoder to avoid these situations in future.
reed@ and scroggo@ would correct me, but my understanding is that because of
cheap discardable memory allocation in SkImageCacherator/SkImageGenerator (and
because 2 step decoding needed in blink decoders as we would need somewhere to
progress in decoding just to return information if something is supported)  it
is simpler/desired to have API contract like this: SkImageGenerator
onRefEncodedData, onGetYUVPlanes, onGetPixels. Otherwise we would have two step
contract like "is this supported, on get this". Methods in
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp need to
be fast and this patch is resolving it for onRefEncodedData.
Anyway, this is my take on this.

[Stephen White, 2015-12-02 16:15:14]

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

https://codereview.chromium.org/1484853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:125:
buffer->deref();
Are we 100% sure this is thread-safe? Does this callback always happen on the
same thread as the refSkData() below?

[reed1, 2015-12-02 16:26:41]

SkData's callback makes no promise about threads. Whenever (and where-ever) the
last owner calls unref(), the callback will be invoked.

[aleksandar.stojiljkovic, 2015-12-02 16:43:36]

On 2015/12/02 16:26:41, reed1 wrote:
> SkData's callback makes no promise about threads. Whenever (and where-ever)
the
> last owner calls unref(), the callback will be invoked.

Otherwise, while in ImageFrameGenerator access to decoder was protected using
ImageFrameGenerator::m_decodeMutex, read access to m_data (in previous
onRefEncodedData) wasn't. Didn't see it used in multiple threads in skia but
probably makes sense to protect. Need to think about how to do it.

[chrishtr, 2015-12-02 16:44:29]

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

https://codereview.chromium.org/1484853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:125:
buffer->deref();
On 2015/12/02 at 16:15:14, Stephen White wrote:
> Are we 100% sure this is thread-safe? Does this callback always happen on the
same thread as the refSkData() below?

It's not clear to me why removing the RefPtr for the callback in this patchset
is correct. The pointer points at an object which is ref-counted by
ThreadSafeDataTransport. If that object dies before this callback, the pointer
will be deleted at that time and the access here will be invalid, right?

https://codereview.chromium.org/1484853003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.h
(right):

https://codereview.chromium.org/1484853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.h:66: bool
m_newAllDataReceived;
How about m_allDataReceivedForNewBufferQueue. Long, but I think clearer.

[Stephen White, 2015-12-02 16:48:19]

On 2015/12/02 16:43:36, aleksandar.stojiljkovic wrote:
> On 2015/12/02 16:26:41, reed1 wrote:
> > SkData's callback makes no promise about threads. Whenever (and where-ever)
> the
> > last owner calls unref(), the callback will be invoked.
> 
> Otherwise, while in ImageFrameGenerator access to decoder was protected using
> ImageFrameGenerator::m_decodeMutex, read access to m_data (in previous
> onRefEncodedData) wasn't. Didn't see it used in multiple threads in skia but
> probably makes sense to protect. Need to think about how to do it.

Yes, I think the thread safety will have to be at the SharedBuffer level, since
we're extracting it out of the ThreadSafeDataTransport on the data() call, so
the latter's mutex no longer protects it.

It might be possible to fix it by making SharedBuffer derive from
ThreadSafeRefCounted
rather than RefCounted, but I have little experience with the former. It may
also have 
performance implications.

[aleksandar.stojiljkovic, 2015-12-02 16:58:24]

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

https://codereview.chromium.org/1484853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:156:
buffer->ref();
>If that object dies before this callback, the pointer will be deleted
I'll cover this with comment that the buffer->ref() is protecting it even m_data
get's disposed. Good point.

[aleksandar.stojiljkovic, 2015-12-02 17:02:29]

On 2015/12/02 16:48:19, Stephen White wrote:
> On 2015/12/02 16:43:36, aleksandar.stojiljkovic wrote:
> > On 2015/12/02 16:26:41, reed1 wrote:
> > > SkData's callback makes no promise about threads. Whenever (and
where-ever)
> > the
> > > last owner calls unref(), the callback will be invoked.
> > 
> > Otherwise, while in ImageFrameGenerator access to decoder was protected
using
> > ImageFrameGenerator::m_decodeMutex, read access to m_data (in previous
> > onRefEncodedData) wasn't. Didn't see it used in multiple threads in skia but
> > probably makes sense to protect. Need to think about how to do it.
> probably n
> Yes, I think the thread safety will have to be at the SharedBuffer level,
since
> we're extracting it out of the ThreadSafeDataTransport on the data() call, so
> the latter's mutex no longer protects it.
> 
> It might be possible to fix it by making SharedBuffer derive from
> ThreadSafeRefCounted
> rather than RefCounted, but I have little experience with the former. It may
> also have 
> performance implications.

Thanks. Sounds good - ThreadSafeRefCounted is needed in ThreadSafeDataTransport
and in callback (and need to check lifespan in decoders).

[Stephen White, 2015-12-02 18:25:48]

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

https://codereview.chromium.org/1484853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:125:
buffer->deref();
On 2015/12/02 16:44:28, chrishtr wrote:
> It's not clear to me why removing the RefPtr for the callback in this patchset
> is correct. The pointer points at an object which is ref-counted by
> ThreadSafeDataTransport. If that object dies before this callback, the pointer
> will be deleted at that time and the access here will be invalid, right?

There's an explicit call to ref() in refSkData() below, which will keep 
the object alive until this deref().

[chrishtr, 2015-12-02 18:26:44]

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

https://codereview.chromium.org/1484853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:125:
buffer->deref();
On 2015/12/02 at 18:25:47, Stephen White wrote:
> On 2015/12/02 16:44:28, chrishtr wrote:
> > It's not clear to me why removing the RefPtr for the callback in this
patchset
> > is correct. The pointer points at an object which is ref-counted by
> > ThreadSafeDataTransport. If that object dies before this callback, the
pointer
> > will be deleted at that time and the access here will be invalid, right?
> 
> There's an explicit call to ref() in refSkData() below, which will keep 
> the object alive until this deref().

Ah I see, thanks.

[aleksandar.stojiljkovic, 2015-12-02 18:49:23]

> It might be possible to fix it by making SharedBuffer derive from
> ThreadSafeRefCounted
> rather than RefCounted, but I have little experience with the former. It may
> also have 
> performance implications.
Found a way to fix it without performance impact on other cases - thread safe
reference counting only needed for creation and disposal, not on SharedBuffer
 passed to decoders, so did it on ThreadSafeDataTransport that is holding the
ref to shared buffer. Ownership of ThreadSafeDataTransport is then shared
between
ImageFrameGenerator and SkData.

[aleksandar.stojiljkovic, 2015-12-02 18:49:42]

Description was changed from

==========
GPU raster large images and animated gif performance fix.

FPS improves from 5 to 25 for example in Bug 476531.
(forced GPU rasterization on Lenovo K3 Note).

There, DecodingImageGenerator::onRefEncodedData() took 80 - 100 ms on ~65MB of
animated
gif data.

This affects GPU texture upload for other image types too when content is
downloaded
(onRefEncoded returns 0 for partially downloaded content) since it removes 2
memory
allocations and copies from DecodingImageGenerator::onRefEncodedData() bellow:

When invoking e.g. drawImageRect code would trigger

GrTexture* SkImageCacherator::lockTexture...
// 3. Ask the generator to return a compressed form that the GPU might support
SkAutoTUnref<SkData> data(this->refEncoded());

leading to:

SkData* DecodingImageGenerator::onRefEncodedData() { ...
    m_frameGenerator->copyData(&buffer, &allDataReceived);
    if (buffer && allDataReceived)
        return SkData::NewWithCopy(buffer->data(), buffer->size());

BUG=476531
==========

to

==========
GPU raster - images upload to GPU performance fix (skip copying encoded data).

For Bug 476531, improved FPS from 5 to 25 (forced GPU rasterization on Lenovo K3
Note).
There DecodingImageGenerator::onRefEncodedData() took 80 - 100 ms on ~65MB of
animated
gif data.

This affects GPU texture upload for all image types when content is downloaded
(onRefEncoded returns 0 for partially downloaded content) since it removes 2
memory
allocations and copies from DecodingImageGenerator::onRefEncodedData() bellow:

When invoking e.g. drawImageRect code would trigger

GrTexture* SkImageCacherator::lockTexture...
// 3. Ask the generator to return a compressed form that the GPU might support
SkAutoTUnref<SkData> data(this->refEncoded());

leading to:

SkData* DecodingImageGenerator::onRefEncodedData() { ...
    m_frameGenerator->copyData(&buffer, &allDataReceived);
    if (buffer && allDataReceived)
        return SkData::NewWithCopy(buffer->data(), buffer->size());

Covered invalid use case, when ThreadSafeDataTransport::setData is called to
append
data to already fully received SharedBuffer.

Thread safety ensured by holding the reference to ThreadSafeRefCounted
ThreadSafeDataTransport (that is again holding ref to SharedBuffer).

BUG=476531
==========

[aleksandar.stojiljkovic, 2015-12-02 18:50:08]

On 2015/12/02 18:49:23, aleksandar.stojiljkovic wrote:
> > It might be possible to fix it by making SharedBuffer derive from
> > ThreadSafeRefCounted
> > rather than RefCounted, but I have little experience with the former. It may
> > also have 
> > performance implications.
> Found a way to fix it without performance impact on other cases - thread safe
> reference counting only needed for creation and disposal, not on SharedBuffer
>  passed to decoders, so did it on ThreadSafeDataTransport that is holding the
> ref to shared buffer. Ownership of ThreadSafeDataTransport is then shared
> between
> ImageFrameGenerator and SkData.

Patch 3.

[scroggo_chromium, 2015-12-02 20:15:02]

> reed@ or scroggo@ should probably look at this to validate
> the SkData semantics.

SkData semantics look correct.

> "There, DecodingImageGenerator::onRefEncodedData() took 80
> - 100 ms on ~65MB of animated gif data."

Is this from a bug somewhere?

On 2015/12/02 15:23:59, aleksandar.stojiljkovic wrote:
> On 2015/12/02 14:37:25, noel gordon wrote:
> > Maybe SkImageCacherator could ask its cached SkImage if it's backed by a GIF
/
> > WEBP / JPEG / PNG decoder to avoid these situations in future.
> reed@ and scroggo@ would correct me, but my understanding is that because of
> cheap discardable memory allocation in SkImageCacherator/SkImageGenerator (and
> because 2 step decoding needed in blink decoders as we would need somewhere to
> progress in decoding just to return information if something is supported)  it
> is simpler/desired to have API contract like this: SkImageGenerator
> onRefEncodedData, onGetYUVPlanes, onGetPixels. Otherwise we would have two
step
> contract like "is this supported, on get this". Methods in
> third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp need to
> be fast and this patch is resolving it for onRefEncodedData.
> Anyway, this is my take on this.

We do have some instances of two-step contracts, e.g. computeScaledDimensions in
crrev.com/1396323007 (not yet landed), and I think they should be evaluated case
by case. I think it would make sense here, if the slow and often unnecessary
work in copyData is causing a problem (sounds like it is). But replacing it with
a faster function is a good approach, too. If onRefEncodedData is really cheap
(looks like it is here), two steps may be unnecessary.

https://codereview.chromium.org/1484853003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.h
(right):

https://codereview.chromium.org/1484853003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.h:66: bool
m_newAllDataReceived;
On 2015/12/02 16:44:29, chrishtr wrote:
> How about m_allDataReceivedForNewBufferQueue. Long, but I think clearer.

+1. I find m_newAllDataReceived confusing.

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

https://codereview.chromium.org/1484853003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:159: //
While SkData is holding reference to underlying data, prevent disposing m_data
and it's content.
its*

https://codereview.chromium.org/1484853003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:161: return
SkData::NewWithProc(buffer->data(), buffer->size(), sharedBufSkDataReleaseProc,
m_data.get());
What happens if the SharedBuffer's internal Vector needs to grow? Is it possible
that it will be reallocated, so the pointer held by the SkData is no longer
valid?

https://codereview.chromium.org/1484853003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right):

https://codereview.chromium.org/1484853003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:30: #include
"SkData.h"
Should this be forward-declared instead? (Blink style guide does not mention it,
but Chromium prefers it.)

https://codereview.chromium.org/1484853003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:80: //
SkImageGenerator::refEncodedData.
Should this be called refEncodedData also, since it behaves similarly?

[aleksandar.stojiljkovic, 2015-12-02 20:33:34]

On 2015/12/02 20:15:02, scroggo_chromium wrote:
> > reed@ or scroggo@ should probably look at this to validate
> > the SkData semantics.
> 
> SkData semantics look correct.
> 
> > "There, DecodingImageGenerator::onRefEncodedData() took 80
> > - 100 ms on ~65MB of animated gif data."
> 
> Is this from a bug somewhere?

Yes, https://code.google.com/p/chromium/issues/detail?id=476531#c14

Let me post update related to yours and chrishtr@'s comments...

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

https://codereview.chromium.org/1484853003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.cpp:71: if
(m_allDataReceived && newBufferQueue.size()) {
On 2015/12/02 20:15:02, scroggo_chromium wrote:
> What happens if the SharedBuffer's internal Vector needs to grow? Is it
possible
> that it will be reallocated, so the pointer held by the SkData is no longer
> valid?
Nice find. This was handled here, but after ref counting
ThreadSafeDataTransport, it is better to just disable appending data to already
fully downloaded content.

[aleksandar.stojiljkovic, 2015-12-02 21:01:11]

patch #4 addresses chrishtr@'s and scroggo@'s comments

[scroggo, 2015-12-03 14:59:58]

scroggo@google.com changed reviewers:
+ scroggo@google.com

[scroggo, 2015-12-03 14:59:59]

lgtm

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right):

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:82: SkData*
refEncodedData();
Can this be const?

[chrishtr, 2015-12-03 17:51:51]

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right):

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:110:
RefPtr<ThreadSafeDataTransport> m_data;
What does making this a RefPtr achieve?

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

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.cpp:57: if
(m_allDataReceived && newBufferQueue.size()) {
Is this code reachable in one of your testcases? Not sure why this is necessary.
Apologies if this has already been in the code review thread and I missed it.

[aleksandar.stojiljkovic, 2015-12-03 18:04:14]

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

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:141: // in
any thread, and SkData would still need to hold the ref to SharedBuffer if
ImageFrameGenerator
On 2015/12/03 17:51:51, chrishtr wrote:
> What does making this a RefPtr achieve?
I tried to cover it here, please help on reformulating if it is not good -
ThreadSafeDataTransport is referenced by SkData and ImageFrameGenerator - in
case ImageFrameGenerator get's deleted, SkData would hold the reference to
ThreadSafeDataTransport (and underlying data) until SkData gets deleted and
sharedBufSkDataReleaseProc called.

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

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.cpp:57: if
(m_allDataReceived && newBufferQueue.size()) {
On 2015/12/03 17:51:51, chrishtr wrote:
> Is this code reachable in one of your testcases? Not sure why this is
necessary.
> Apologies if this has already been in the code review thread and I missed it.

No, it is not reachable and it mustn't be. It wasn't checked here before though.
It is necessary as at this point SkData could have been returned and underlying
data shouldn't be changed (appending more data and calling data() could cause
change. Just wanted to prevent future changes in other parts triggering
erroneous situation - hence the ASSERT(false).

[chrishtr, 2015-12-03 18:32:00]

lgtm

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

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:141: // in
any thread, and SkData would still need to hold the ref to SharedBuffer if
ImageFrameGenerator
On 2015/12/03 at 18:04:14, aleksandar.stojiljkovic wrote:
> On 2015/12/03 17:51:51, chrishtr wrote:
> > What does making this a RefPtr achieve?
> I tried to cover it here, please help on reformulating if it is not good -
ThreadSafeDataTransport is referenced by SkData and ImageFrameGenerator - in
case ImageFrameGenerator get's deleted, SkData would hold the reference to
ThreadSafeDataTransport (and underlying data) until SkData gets deleted and
sharedBufSkDataReleaseProc called.

Oh I see now, thanks. I didn't notice the change to pass m_data to NewWithProc.

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

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.cpp:57: if
(m_allDataReceived && newBufferQueue.size()) {
On 2015/12/03 at 18:04:14, aleksandar.stojiljkovic wrote:
> On 2015/12/03 17:51:51, chrishtr wrote:
> > Is this code reachable in one of your testcases? Not sure why this is
necessary.
> > Apologies if this has already been in the code review thread and I missed
it.
> 
> No, it is not reachable and it mustn't be. It wasn't checked here before
though.
> It is necessary as at this point SkData could have been returned and
underlying data shouldn't be changed (appending more data and calling data()
could cause change. Just wanted to prevent future changes in other parts
triggering erroneous situation - hence the ASSERT(false).

If it is not something that can happen, just put in 
ASSERT(!m_allDataReceived || newBufferQueue.empty());

We should try to avoid adding defensive code.

If the call would be a security/crash bug then RELEASE_ASSERT may be
appropriate.

[chrishtr, 2015-12-03 18:32:34]

On 2015/12/03 at 18:32:00, chrishtr wrote:
> lgtm
> 
>
https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):
> 
>
https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:141: // in
any thread, and SkData would still need to hold the ref to SharedBuffer if
ImageFrameGenerator
> On 2015/12/03 at 18:04:14, aleksandar.stojiljkovic wrote:
> > On 2015/12/03 17:51:51, chrishtr wrote:
> > > What does making this a RefPtr achieve?
> > I tried to cover it here, please help on reformulating if it is not good -
ThreadSafeDataTransport is referenced by SkData and ImageFrameGenerator - in
case ImageFrameGenerator get's deleted, SkData would hold the reference to
ThreadSafeDataTransport (and underlying data) until SkData gets deleted and
sharedBufSkDataReleaseProc called.
> 
> Oh I see now, thanks. I didn't notice the change to pass m_data to
NewWithProc.
> 
>
https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.cpp
(right):
> 
>
https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.cpp:57: if
(m_allDataReceived && newBufferQueue.size()) {
> On 2015/12/03 at 18:04:14, aleksandar.stojiljkovic wrote:
> > On 2015/12/03 17:51:51, chrishtr wrote:
> > > Is this code reachable in one of your testcases? Not sure why this is
necessary.
> > > Apologies if this has already been in the code review thread and I missed
it.
> > 
> > No, it is not reachable and it mustn't be. It wasn't checked here before
though.
> > It is necessary as at this point SkData could have been returned and
underlying data shouldn't be changed (appending more data and calling data()
could cause change. Just wanted to prevent future changes in other parts
triggering erroneous situation - hence the ASSERT(false).
> 
> If it is not something that can happen, just put in 
> ASSERT(!m_allDataReceived || newBufferQueue.empty());
> 
> We should try to avoid adding defensive code.
> 
> If the call would be a security/crash bug then RELEASE_ASSERT may be
appropriate.

(Please do address all the comments before committing.)

[aleksandar.stojiljkovic, 2015-12-03 21:17:09]

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right):

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:82: SkData*
refEncodedData();
On 2015/12/03 14:59:59, scroggo wrote:
> Can this be const?

Done.

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:110:
RefPtr<ThreadSafeDataTransport> m_data;
On 2015/12/03 17:51:51, chrishtr wrote:
> What does making this a RefPtr achieve?

Done.

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

https://codereview.chromium.org/1484853003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.cpp:57: if
(m_allDataReceived && newBufferQueue.size()) {
On 2015/12/03 18:32:00, chrishtr wrote:
> On 2015/12/03 at 18:04:14, aleksandar.stojiljkovic wrote:
> > On 2015/12/03 17:51:51, chrishtr wrote:
> > > Is this code reachable in one of your testcases? Not sure why this is
> necessary.
> > > Apologies if this has already been in the code review thread and I missed
> it.
> > 
> > No, it is not reachable and it mustn't be. It wasn't checked here before
> though.
> > It is necessary as at this point SkData could have been returned and
> underlying data shouldn't be changed (appending more data and calling data()
> could cause change. Just wanted to prevent future changes in other parts
> triggering erroneous situation - hence the ASSERT(false).
> 
> If it is not something that can happen, just put in 
> ASSERT(!m_allDataReceived || newBufferQueue.empty());
> 
> We should try to avoid adding defensive code.
> 
> If the call would be a security/crash bug then RELEASE_ASSERT may be
> appropriate.

Done.

[aleksandar.stojiljkovic, 2015-12-03 21:17:29]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[aleksandar.stojiljkovic, 2015-12-03 21:17:29]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com,
chrishtr@chromium.org, scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1484853003/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-12-03 21:18:54]

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

[commit-bot: I haz the power, 2015-12-03 21:39:21]

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

[commit-bot: I haz the power, 2015-12-03 21:39:22]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aleksandar.stojiljkovic, 2015-12-03 23:23:23]

On 2015/12/03 21:39:22, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
>   linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
>   ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
>   mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
>   mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

tasak@, while rebasing this, hit the merge issue with
https://codereview.chromium.org/1497683002
Is it OK to remove DISALLOW_NEW from ThreadSafeDataTransport introduced here:
https://codereview.chromium.org/1497683002?
Thanks.

[aleksandar.stojiljkovic, 2015-12-03 23:30:59]

On 2015/12/03 23:23:23, aleksandar.stojiljkovic wrote:

> tasak@, while rebasing this, hit the merge issue with
> https://codereview.chromium.org/1497683002
> Is it OK to remove DISALLOW_NEW from ThreadSafeDataTransport introduced here:
> https://codereview.chromium.org/1497683002
> Thanks.

Anyway, providing updated patch for additional review after resolving rebase
conflict in ThreadSafeDataTransport..

[aleksandar.stojiljkovic, 2015-12-04 00:30:01]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-04 00:34:58]

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

[aleksandar.stojiljkovic, 2015-12-04 01:06:26]

On 2015/12/03 23:30:59, aleksandar.stojiljkovic wrote:
> On 2015/12/03 23:23:23, aleksandar.stojiljkovic wrote:
> 
> > tasak@, while rebasing this, hit the merge issue with
> > https://codereview.chromium.org/1497683002
> > Is it OK to remove DISALLOW_NEW from ThreadSafeDataTransport introduced
here:
> > https://codereview.chromium.org/1497683002
> > Thanks.
> 
> Anyway, providing updated patch for additional review after resolving rebase
> conflict in ThreadSafeDataTransport..

haraken@ provided instructions here:
https://codereview.chromium.org/1497683002/#msg11
"You can remove DISALLOW_NEW and add USING_FAST_MALLOC."

Done.

[commit-bot: I haz the power, 2015-12-04 02:43:01]

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

[commit-bot: I haz the power, 2015-12-04 02:43:02]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Noel Gordon, 2015-12-04 03:28:03]

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

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:105: ,
m_data(new ThreadSafeDataTransport())
Shouldn't this be using adoptRef?

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:124: static
void sharedBufSkDataReleaseProc(const void* addr, void* ctx)
What value does "Buf" add here?  sharedSkDataReleaseProc or even
sharedSkDataReleaseCallback since we do not abbrv in Blink code per style. 


Where is addr used in the function?  If it s not used, change the

"(const void* addr," -> (const void*,

ctx- > context.

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:126:
ThreadSafeDataTransport* someonesMData =
static_cast<ThreadSafeDataTransport*>(ctx);
someonesMData, is actually someone's dataTransport.  Call it dataTransport.

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:127:
ASSERT(someonesMData);
Is there a stronger assertion you could make here?  We don't appear to be
deref()ing any old data here, we are appear to be deref-ing our own m_data here.

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:150: //
Client side use case is valid only for full image (encoded) data downloaded,
What is "Client side use case"?  I have no clue what it means.

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right):

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:83: // Returns
pointer to SkData. Caller needs to unref it, according to contract in
How about ...

Return our encoded image data. Caller takes ownership and must unref the data
according to the contract SkImageGenerator::refEncodedData.

[aleksandar.stojiljkovic, 2015-12-04 11:11:43]

Thanks. Addressing concerns and checking if fast malloc in combination to
PLATFORM_EXPORT ThreadSafeDataTransport final : public
ThreadSafeRefCounted<ThreadSafeDataTransport>
is the reason for memory leak after merging- didn't see it elsewhere in code
used like that.

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

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:105: ,
m_data(new ThreadSafeDataTransport())
On 2015/12/04 03:28:03, noel gordon wrote:
> Shouldn't this be using adoptRef?

Done.

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:124: static
void sharedBufSkDataReleaseProc(const void* addr, void* ctx)
On 2015/12/04 03:28:03, noel gordon wrote:
> What value does "Buf" add here?  sharedSkDataReleaseProc or even
> sharedSkDataReleaseCallback since we do not abbrv in Blink code per style. 
> 
> 
> Where is addr used in the function?  If it s not used, change the
> 
> "(const void* addr," -> (const void*,
> 
> ctx- > context.

Using address now in assert bellow. abbrv resolved.

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:126:
ThreadSafeDataTransport* someonesMData =
static_cast<ThreadSafeDataTransport*>(ctx);
On 2015/12/04 03:28:03, noel gordon wrote:
> someonesMData, is actually someone's dataTransport.  Call it dataTransport.

Done.

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:127:
ASSERT(someonesMData);
On 2015/12/04 03:28:03, noel gordon wrote:
> Is there a stronger assertion you could make here?  We don't appear to be
> deref()ing any old data here, we are appear to be deref-ing our own m_data
here.

There is release_assert covering the situation in ThreadSafeDataTransport, added
assert here too. Thanks

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:150: //
Client side use case is valid only for full image (encoded) data downloaded,
On 2015/12/04 03:28:03, noel gordon wrote:
> What is "Client side use case"?  I have no clue what it means.

Done. It is irrelevant sentence since this code is returing SkData only when all
data is received.

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right):

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:83: // Returns
pointer to SkData. Caller needs to unref it, according to contract in
On 2015/12/04 03:28:03, noel gordon wrote:
> How about ...
> 
> Return our encoded image data. Caller takes ownership and must unref the data
> according to the contract SkImageGenerator::refEncodedData.

Done.

[aleksandar.stojiljkovic, 2015-12-04 11:12:01]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-04 11:12:40]

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

[Noel Gordon, 2015-12-04 11:20:11]

The CQ bit was unchecked by noel@chromium.org

[Noel Gordon, 2015-12-04 11:20:44]

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

[commit-bot: I haz the power, 2015-12-04 11:21:06]

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

[Noel Gordon, 2015-12-04 11:24:37]

On 2015/12/04 11:12:40, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1484853003/120001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1484853003/120001

Let's dry-run before going for submit the ASAN bot are happy, they were not at
patch #6 clearly.

Also, they might be more questions...

[Noel Gordon, 2015-12-04 11:25:20]

.. there might be more questions.

[Noel Gordon, 2015-12-04 11:26:55]

The CQ bit was unchecked by noel@chromium.org

[Noel Gordon, 2015-12-04 11:27:16]

The CQ bit was unchecked by noel@chromium.org

[aleksandar.stojiljkovic, 2015-12-04 11:27:16]

On 2015/12/04 11:24:37, noel gordon wrote:
> On 2015/12/04 11:12:40, commit-bot: I haz the power wrote:
> > Dry run: CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1484853003/120001
> > View timeline at
> >  https://chromium-cq-status.appspot.com/patch-timeline/1484853003/120001
> 
> Let's dry-run before going for submit the ASAN bot are happy, they were not at
> patch #6 clearly.
> 
> Also, they might be more questions...

Of course, in #62 I started "dry run".

[aleksandar.stojiljkovic, 2015-12-04 11:39:24]

On 2015/12/04 11:27:16, aleksandar.stojiljkovic wrote:
> On 2015/12/04 11:24:37, noel gordon wrote:
> > On 2015/12/04 11:12:40, commit-bot: I haz the power wrote:
> > > Dry run: CQ is trying da patch. Follow status at
> > >  https://chromium-cq-status.appspot.com/patch-status/1484853003/120001
> > > View timeline at
> > >  https://chromium-cq-status.appspot.com/patch-timeline/1484853003/120001
> > 
> > Let's dry-run before going for submit the ASAN bot are happy, they were not
at
> > patch #6 clearly.

Not sure yet though that ASAN bit are happy - linux_android_rel_ng,
linux_chromium_asan_rel_ng and linux_chromium_rel_ng didn't yet hit the place
they were failing with patch 6.
I'm waiting for dry run to finish (unchecked currently) - to ask haraken the
question about fast alloc and RefCounted here
:https://codereview.chromium.org/1497683002

> > 
> > Also, they might be more questions...
> 
> Of course, in #62 I started "dry run".

[Noel Gordon, 2015-12-04 11:41:58]

Ok good. There was a memory leak detected on linux ASAN, so we can wait to see
if it's fixed.

I asked you on bug where did the copy come from, but I'm not sure you provided
one, or if you even investigated.  Leon mentioned on bug that the change the
introduced the copy was for testing.  Indeed, his reviewer asked that the he
copy the data: there were no performance concerns [1].

 
https://chromium.googlesource.com/chromium/src/+/327d8895fbbe6cbb5a4b88961b61...

So the error on the current bug was introduced later.  Was that when the ETC1
texture support was added?

[aleksandar.stojiljkovic, 2015-12-04 11:58:59]

On 2015/12/04 11:41:58, noel gordon wrote:
> Ok good. There was a memory leak detected on linux ASAN, so we can wait to see
> if it's fixed.
> 
> I asked you on bug where did the copy come from, but I'm not sure you provided
> one, or if you even investigated.  Leon mentioned on bug that the change the
> introduced the copy was for testing.  Indeed, his reviewer asked that the he
> copy the data: there were no performance concerns [1].

Of course; it was for test.
His answer here http://code.google.com/p/chromium/issues/detail?id=476531#c26
clarified it way better than the info I fetched from git blame analysis.

> 
>  
>
https://chromium.googlesource.com/chromium/src/+/327d8895fbbe6cbb5a4b88961b61...
> 
> So the error on the current bug was introduced later.  Was that when the ETC1
> texture support was added?

[aleksandar.stojiljkovic, 2015-12-04 12:21:03]

On 2015/12/04 02:43:02, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no
build
> URL)

Memory leak that was result of merging RefEncoded and fast alloc, resolved by
removing fast alloc.

Asked haraken@ question here about it:
https://codereview.chromium.org/1497683002/#msg12

Processed multiple good points made by Noel.
Since the patch is changed after merging to latest master and processing the
comments, I'm waiting for new comments or confirmation that LGTM (also LGTMs
were conditional) is still valid.

[Noel Gordon, 2015-12-04 12:38:59]

On 2015/12/04 11:58:59, aleksandar.stojiljkovic wrote:
> On 2015/12/04 11:41:58, noel gordon wrote:
> > I asked you on bug where did the copy come from, but I'm not sure you
provided
> > one, or if you even investigated.  Leon mentioned on bug that the change the
> > introduced the copy was for testing.  Indeed, his reviewer asked that the he
> > copy the data: there were no performance concerns [1].
> 
> Of course; it was for test.
> His answer here http://code.google.com/p/chromium/issues/detail?id=476531#c26

Yes I know that.

> clarified it way better than the info I fetched from git blame analysis.

I just read the webkit patch and review comments.

> > So the error on the current bug was introduced later.  Was that when the
ETC1
> > texture support was added?

So will you answering this part?

[Noel Gordon, 2015-12-04 12:40:30]

Some more review comments.

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

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:150: //
Client side use case is valid only for full image (encoded) data downloaded,
On 2015/12/04 11:11:43, aleksandar.stojiljkovic wrote:
> On 2015/12/04 03:28:03, noel gordon wrote:
> > What is "Client side use case"?  I have no clue what it means.
> 
> Done. It is irrelevant sentence since this code is returing SkData only when
all
> data is received.

Thanks, and feel free to remove any other "irrelevant sentences" here too.

Then check the result for accuracy.

So for example, in patch #59 (the lastest) I noted this comment contains
"sharedBufSkDataReleaseProc", but you changed the name of that routine. 
Comments are a burden on our project, especially if they are hard to keep brief,
accurate, and precise.

I find the comment hard to follow, frankly.  I hope others understand what it is
trying to say.

https://codereview.chromium.org/1484853003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (left):

https://codereview.chromium.org/1484853003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:83: 
Noticed one review question about this "Could this be const?".  I read that as a
request for

  const SkData* refEncodedData() const;

since we probably don't want callers changing our encoded data.

[aleksandar.stojiljkovic, 2015-12-04 14:14:17]

On 2015/12/04 12:38:59, noel gordon wrote:
> On 2015/12/04 11:58:59, aleksandar.stojiljkovic wrote:
> > On 2015/12/04 11:41:58, noel gordon wrote:
> > > I asked you on bug where did the copy come from, but I'm not sure you
> provided
> > > one, or if you even investigated.  Leon mentioned on bug that the change
the
> > > introduced the copy was for testing.  Indeed, his reviewer asked that the
he
> > > copy the data: there were no performance concerns [1].
> > 
> > Of course; it was for test.
> > His answer here
http://code.google.com/p/chromium/issues/detail?id=476531#c26
> 
> Yes I know that.
> 
> > clarified it way better than the info I fetched from git blame analysis.
> 
> I just read the webkit patch and review comments.
Me too. chromium side patch and skia code so was able to understand the use
cases:
http://code.google.com/p/chromium/issues/detail?id=476531#c19


> 
> > > So the error on the current bug was introduced later.  Was that when the
> ETC1
> > > texture support was added?
> 
> So will you answering this part?
Of course. I sense I might not be able to cover the question completely - cannot
guess. 
Git blame and code search don't tell me more than Scroggo's answer: testing code
promoted later to cover multiple use cases to "production" code.
About my understanding of texture compression use case, cannot guess - covered
it here http://code.google.com/p/chromium/issues/detail?id=476531#c33.
Other use cases where onRefEncoded data is needed:
http://code.google.com/p/chromium/issues/detail?id=476531#c19

[haraken, 2015-12-04 14:22:37]

(Moving the discussion from https://codereview.chromium.org/1497683002/)

>
https://codereview.chromium.org/1497683002/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/platform/graphics/ThreadSafeDataTransport.h:49:
> > > DISALLOW_NEW();
> > > tasak@, while rebasing this, hit the merge issue with this.
> > > Usage of this class changed, it is now like:
> > > class PLATFORM_EXPORT ThreadSafeDataTransport final : public
> > > ThreadSafeRefCounted<ThreadSafeDataTransport> {
> > >     WTF_MAKE_NONCOPYABLE(ThreadSafeDataTransport);
> > > 
> > > https://codereview.chromium.org/1484853003/
> > > 
> > > Could you please comment if you're fine with removing DISALLOW_NEW from
> > > ThreadSafeDataTransport?
> > > Thanks.
> > 
> > You can remove DISALLOW_NEW and add USING_FAST_MALLOC.
> 
> @haraken, good morning.
> 
> Sorry if misusing this review for another issue - looks related:
> 
> This combination is causing memory leak (more info in the bot log):
> class PLATFORM_EXPORT ThreadSafeDataTransport final : public
> ThreadSafeRefCounted<ThreadSafeDataTransport> {
>     WTF_MAKE_NONCOPYABLE(ThreadSafeDataTransport);
>     USING_FAST_MALLOC(ThreadSafeDataTransport);
> 
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> 
> After I removed USING_FAST_MALLOC(ThreadSafeDataTransport), asan build seems
> fine:
> 
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> 
> Also, noticed that USING_FAST_MALLOC is not on other places combined with
> RefCounted or ThreadSafeRefCounted (a lot of those - maybe there are I just
> wasn't able to see/grep them).

You're right that you don't need to add USING_FAST_MALLOC to
ThreadSafeDataTransport because RefCounted already has USING_FAST_MALLOC.

However, it looks strange that the leak happens only when you add
USING_FAST_MALLOC to ThreadSafeDataTransport. It shouldn't change LSan results.
I'm suspecting that the CL is actually leaking ThreadSafeDataTransport but the
leak is not detected without having the USING_FAST_MALLOC for some reason.

Can you run the DeferredImageDecoderTest.decodedSize manually and printf the
number of ThreadSafeDataTransport at the end of the test? That way you can
confirm if it's leaking or not.

[aleksandar.stojiljkovic, 2015-12-04 14:31:15]

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

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:150: //
Client side use case is valid only for full image (encoded) data downloaded,
On 2015/12/04 12:40:30, noel gordon wrote:
> On 2015/12/04 11:11:43, aleksandar.stojiljkovic wrote:
> > On 2015/12/04 03:28:03, noel gordon wrote:
> > > What is "Client side use case"?  I have no clue what it means.
> > 
> > Done. It is irrelevant sentence since this code is returing SkData only when
> all
> > data is received.
> 
> Thanks, and feel free to remove any other "irrelevant sentences" here too.
> 
> Then check the result for accuracy.
> 
> So for example, in patch #59 (the lastest) I noted this comment contains
> "sharedBufSkDataReleaseProc", but you changed the name of that routine. 
> Comments are a burden on our project, especially if they are hard to keep
brief,
> accurate, and precise.
> 
> I find the comment hard to follow, frankly.  I hope others understand what it
is
> trying to say.

Done.

https://codereview.chromium.org/1484853003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (left):

https://codereview.chromium.org/1484853003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:83: 
On 2015/12/04 12:40:30, noel gordon wrote:
> Noticed one review question about this "Could this be const?".  I read that as
a
> request for
> 
>   const SkData* refEncodedData() const;
> 
> since we probably don't want callers changing our encoded data.

That is handled by
const void* SkData::data() const { return fPtr; } - so client cannot alter our
data.

Otherwise, it cannot be like this "const SkData* refEncodedData() const;"
because it is non const through the call stack. Signatures:

virtual SkData* SkImageGenerator::onRefEncodedData();
overriden here: SkData* DecodingImageGenerator::onRefEncodedData();

calling:

SkData* DecodingImageGenerator::onRefEncodedData();
SkData* ImageFrameGenerator::refEncodedData() const;

[aleksandar.stojiljkovic, 2015-12-04 15:07:37]

On 2015/12/04 14:14:17, aleksandar.stojiljkovic wrote:
> On 2015/12/04 12:38:59, noel gordon wrote:
> > On 2015/12/04 11:58:59, aleksandar.stojiljkovic wrote:
> > > On 2015/12/04 11:41:58, noel gordon wrote:
> > > > I asked you on bug where did the copy come from, but I'm not sure you
> > provided
> > > > one, or if you even investigated.  Leon mentioned on bug that the change
> the
> > > > introduced the copy was for testing.  Indeed, his reviewer asked that
the
> he
> > > > copy the data: there were no performance concerns [1].
> > > 
> > > Of course; it was for test.
> > > His answer here
> http://code.google.com/p/chromium/issues/detail?id=476531#c26
> > 
> > Yes I know that.
> > 
> > > clarified it way better than the info I fetched from git blame analysis.
> > 
> > I just read the webkit patch and review comments.
> Me too. chromium side patch and skia code so was able to understand the use
> cases:
> http://code.google.com/p/chromium/issues/detail?id=476531#c19
> 
> 
> > 
> > > > So the error on the current bug was introduced later.  Was that when the
> > ETC1
> > > > texture support was added?
> > 
> > So will you answering this part?
> Of course. I sense I might not be able to cover the question completely -
cannot
> guess. 
> Git blame and code search don't tell me more than Scroggo's answer: testing
code
> promoted later to cover multiple use cases to "production" code.
> About my understanding of texture compression use case, cannot guess - covered
> it here http://code.google.com/p/chromium/issues/detail?id=476531#c33.
> Other use cases where onRefEncoded data is needed:
> http://code.google.com/p/chromium/issues/detail?id=476531#c19

Noel, what are you after? is it about figuring out the way to benchmark this -
to prevent regression in future? I cuold check it.

[aleksandar.stojiljkovic, 2015-12-04 15:37:00]

updated comment.

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

https://codereview.chromium.org/1484853003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:105: ,
m_data(adoptRef(new ThreadSafeDataTransport()))
haraken@
> Can you run the DeferredImageDecoderTest.decodedSize manually and printf the
> number of ThreadSafeDataTransport at the end of the test? That way you can
> confirm if it's leaking or not.

The problem wasn't in fast alloc - it was in adoptRef

previous code had m_data(new ThreadSafeDataTransport()) (without adoptRef)

Monitored the test you asked:

1) with    adoptRef, with or without fast alloc - no leak
2) without adoptRef, with or without fast alloc - leak

Noel, thanks.

[scroggo_chromium, 2015-12-04 16:15:25]

https://codereview.chromium.org/1484853003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (left):

https://codereview.chromium.org/1484853003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:83: 
On 2015/12/04 14:31:15, aleksandar.stojiljkovic wrote:
> On 2015/12/04 12:40:30, noel gordon wrote:
> > Noticed one review question about this "Could this be const?".  I read that
as
> a
> > request for
> > 
> >   const SkData* refEncodedData() const;
> > 
> > since we probably don't want callers changing our encoded data.
> 
> That is handled by
> const void* SkData::data() const { return fPtr; } - so client cannot alter our
> data.

SkData also has writable_data()
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...).

which allows modifying its data. FWIW, its documentation recommends using with
caution, and it asserts that there is only one owner.

> 
> Otherwise, it cannot be like this "const SkData* refEncodedData() const;"
> because it is non const through the call stack. Signatures:
> 
> virtual SkData* SkImageGenerator::onRefEncodedData();
> overriden here: SkData* DecodingImageGenerator::onRefEncodedData();
> 
> calling:
> 
> SkData* DecodingImageGenerator::onRefEncodedData();
> SkData* ImageFrameGenerator::refEncodedData() const;

When SkImageGenerator::onRefEncodedData was added (back in May 2014 - see
https://codereview.chromium.org/300263005), SkData was always assumed to be
const (it had no non-const methods, which we can verify with
https://skia.googlesource.com/skia/+/00f8d6c75d22ce8f95f932c5b101354b196fa0df...).
writable_data() was added more recently (September 2014:
https://codereview.chromium.org/560653004). I think it makes sense to make these
all return a const SkData.

On a related note, one could argue that it's incorrect to label the method
const, since it modifies its ThreadSafeDataTransport's SharedBuffer (by
collapsing the data). That also seems like it might conflict with my efforts in
crbug.com/467772 to stop collapsing the data. I think that's okay though - here
we only collapse the data when the image is complete, so we only do it once
(rather than the worst case scenario of doing it several times during load).

But it will still be unnecessary, in the case where the image is (for example) a
GIF file that will not be decoded on the GPU. It's much better than the existing
case - where we create a new copy each time the method is called, even if the
image is not complete, but I think we could combine this with a check to
determine whether the format is one that can be GPU decoded to save even more
wasted work.

[aleksandar.stojiljkovic, 2015-12-04 16:31:56]

On 2015/12/04 16:15:25, scroggo_chromium wrote:

> But it will still be unnecessary, in the case where the image is (for example)
a
> GIF file that will not be decoded on the GPU. It's much better than the
existing
> case - where we create a new copy each time the method is called, even if the
> image is not complete, but I think we could combine this with a check to
> determine whether the format is one that can be GPU decoded to save even more
> wasted work.

Access to compressed texture happens from SkImageCacherator::lockTexture and
there it is clear why refEncodedData is called (GL compression).
Another part is through SkPixelRef (SkDiscardablePixelRef) and there would also
be need to cover the reason for calling.

[scroggo_chromium, 2015-12-04 16:53:06]

https://codereview.chromium.org/1484853003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right):

https://codereview.chromium.org/1484853003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:84: //
according to the contract SkImageGenerator::refEncodedData.
I think this should also state that it returns null if the image is not
complete.

[aleksandar.stojiljkovic, 2015-12-04 21:50:10]

Simplifying the logic, added unit tests and made SkData unique() to be false -
so not writeable.


> SkData also has writable_data()
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...).
> 
> which allows modifying its data. FWIW, its documentation recommends using with
> caution, and it asserts that there is only one owner.

Implemented it this way and simplifies the logic. Thanks for the pointer.

Now SkData instance and ThreadSafeDataTransfer are deleted in
ImageFrameDestructor or later, if SkData lives longer than ImageFrameGenerator.
Added unit test to verify the later case.



> On a related note, one could argue that it's incorrect to label the method
> const, since it modifies its ThreadSafeDataTransport's SharedBuffer (by

It is not const anynmore, as it is instantiating m_encodedData - single
referenced instance by ImageFrameGenerator.
Probably no need to have ThreadSafeDataTransfer ThreadSafeRefCounted - could
just be RefCounted.

[aleksandar.stojiljkovic, 2015-12-07 19:32:08]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-07 19:34:45]

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

[commit-bot: I haz the power, 2015-12-07 19:46:27]

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

[commit-bot: I haz the power, 2015-12-07 19:46:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[aleksandar.stojiljkovic, 2015-12-07 20:34:38]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-07 20:40:52]

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

[aleksandar.stojiljkovic, 2015-12-07 21:13:21]

The CQ bit was unchecked by aleksandar.stojiljkovic@intel.com

[aleksandar.stojiljkovic, 2015-12-07 21:30:31]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-07 21:31:19]

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

[aleksandar.stojiljkovic, 2015-12-07 21:46:08]

On 2015/12/04 21:50:10, aleksandar.stojiljkovic wrote:
> Simplifying the logic, added unit tests and made SkData unique() to be false -
> so not writeable.
> 
> 
> > SkData also has writable_data()
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...).
> > 
> > which allows modifying its data. FWIW, its documentation recommends using
with
> > caution, and it asserts that there is only one owner.
> 
> Implemented it this way and simplifies the logic. Thanks for the pointer.
> 
> Now SkData instance and ThreadSafeDataTransfer are deleted in
> ImageFrameDestructor or later, if SkData lives longer than
ImageFrameGenerator.
> Added unit test to verify the later case.
> 
> 
> 
> > On a related note, one could argue that it's incorrect to label the method
> > const, since it modifies its ThreadSafeDataTransport's SharedBuffer (by
> 
> It is not const anynmore, as it is instantiating m_encodedData - single
> referenced instance by ImageFrameGenerator.
> Probably no need to have ThreadSafeDataTransfer ThreadSafeRefCounted - could
> just be RefCounted.

chrishtr@, scroggo@
I have added unit tests and simplified it a bit - long time since you put LGTM;
don't think those LGTMs are still valid. There seems to be no other issues open,
but could you please recheck if it is OK to submit. Thanks.

[chrishtr, 2015-12-07 23:06:29]

lgtm

[aleksandar.stojiljkovic, 2015-12-07 23:13:48]

On 2015/12/07 23:06:29, chrishtr wrote:
> lgtm

Thanks. I'll wait for Noel tomorrow morning (he has added traces and work
related to early-out on YUV which seems related [1]) before submitting.

[1]
https://codereview.chromium.org/1508683002/

[commit-bot: I haz the power, 2015-12-08 00:41:10]

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

[commit-bot: I haz the power, 2015-12-08 00:41:11]

Dry run: This issue passed the CQ dry run.

[Noel Gordon, 2015-12-08 13:39:51]

On 2015/12/02 15:23:59, aleksandar.stojiljkovic wrote:
> On 2015/12/02 14:37:25, noel gordon wrote:
> > Maybe SkImageCacherator could ask its cached SkImage if it's backed by a GIF
/
> > WEBP / JPEG / PNG decoder to avoid these situations in future.

> reed@ and scroggo@ would correct me, but my understanding is that because of
> cheap discardable memory allocation in SkImageCacherator/SkImageGenerator (and
> because 2 step decoding needed in blink decoders as we would need somewhere to
> progress in decoding just to return information if something is supported)

There is no 2-step decoding needed here, btw.  DecodingImageGenerator: is an
image (frame) from an image decoder, is a Blink class backed by one of the
decoders Blink provides (GIF/PNG/JPEG/WEPB/ICO/BMP).  No decoding is needed to
work out it is an image decoder. That fact is known when it was created and does
not change.

> it is simpler/desired to have API contract like this: SkImageGenerator
> onRefEncodedData, onGetYUVPlanes, onGetPixels.

Simple contract yes, but it requires that onRefEncodedData be called, on an
image decoder frame here, to work out it the frame is ETC1/KTX (compressed
texture) by using data sniffing in Ganesh as we seen on the bug.

Seems to me that SkImageCacherator could just ask its SkImageGenerator if was an
image decoder, and avoid the ETC1/KTX data sniff entirely.  The image generator
frame might be from a video decoder and you probably do not want to sniff those
frames for ETC1/KTX data if want to good performance.

Seems that what's inside the SkImageGenerator is currently sealed off from
SkImageCacherator and so it has no choice but to data sniff ...

[Noel Gordon, 2015-12-08 13:53:40]

On 2015/12/04 11:11:43, aleksandar.stojiljkovic wrote:

https://codereview.chromium.org/1484853003/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:150: //
> Client side use case is valid only for full image (encoded) data downloaded,
> On 2015/12/04 03:28:03, noel gordon wrote:
> > What is "Client side use case"?  I have no clue what it means.
> 
> Done. It is irrelevant sentence since this code is returning SkData only when
all
> data is received.

Right, once all the data is received, we will return the encoded data in SkData.

[Noel Gordon, 2015-12-08 14:18:57]

On 2015/12/04 16:15:25, scroggo_chromium wrote:
>
https://codereview.chromium.org/1484853003/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (left):
> 
>
https://codereview.chromium.org/1484853003/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:83: 
> On 2015/12/04 14:31:15, aleksandar.stojiljkovic wrote:
> > On 2015/12/04 12:40:30, noel gordon wrote:
> > > Noticed one review question about this "Could this be const?".  I read
> > > that as a request for
> > > 
> > >   const SkData* refEncodedData() const;
> > > 
> > > since we probably don't want callers changing our encoded data.
> > 
> > That is handled by
> > const void* SkData::data() const { return fPtr; } - so client cannot
> > alter our data.
> 
> SkData also has writable_data()
> which allows modifying its data. FWIW, its documentation recommends using
> with caution, and it asserts that there is only one owner.

Right thanks, I had forgotten that SkData::data() is const.

Aleksandar noted it above: clients cannot alter the data.  Clients do expect it
to be a contiguous block of memory containing the encoded data.

[Noel Gordon, 2015-12-08 14:48:42]

On 2015/12/04 16:15:25, scroggo_chromium wrote:

> On a related note, one could argue that it's incorrect to label the method
> const, since it modifies its ThreadSafeDataTransport's SharedBuffer (by
> collapsing the data).

Once only, as you went on say, but good to know, since clients require
contiguous memory.

But the collapsing could be avoided ...

> But it will still be unnecessary, in the case where the image is (for example)
a
> GIF file that will not be decoded on the GPU. It's much better than the
existing
> case - where we create a new copy each time the method is called, even if the
> image is not complete, but I think we could combine this with a check to
> determine whether the format is one that can be GPU decoded to save even more
> wasted work.

 ... agree. No need to call refEncodedData() because there's no need to sniff
when we know it's an image decoder.  How could we get to that fine-sounding,
more performant place, I wonder?

[aleksandar.stojiljkovic, 2015-12-08 15:23:56]

On 2015/12/08 14:48:42, noel gordon wrote:
> On 2015/12/04 16:15:25, scroggo_chromium wrote:
> 
> > On a related note, one could argue that it's incorrect to label the method
> > const, since it modifies its ThreadSafeDataTransport's SharedBuffer (by
> > collapsing the data).
> 
> Once only, as you went on say, but good to know, since clients require
> contiguous memory.
> 
> But the collapsing could be avoided ...
> 
> > But it will still be unnecessary, in the case where the image is (for
example)
> a
> > GIF file that will not be decoded on the GPU. It's much better than the
> existing
> > case - where we create a new copy each time the method is called, even if
the
> > image is not complete, but I think we could combine this with a check to
> > determine whether the format is one that can be GPU decoded to save even
more
> > wasted work.
> 
>  ... agree. No need to call refEncodedData() because there's no need to sniff
> when we know it's an image decoder.  How could we get to that fine-sounding,
> more performant place, I wonder?

Thanks Noel. Thanks also to other in clarifying the requirements (multi threaded
and no writable access).
All good then with the patch.
Let me summarize follow up actions:

1) try to add Skia API like isGPUCompressed to skImageGenerator to prevent call
to onRefEncoded in SkImageCacherator. Since sniffing for texture compression
goes usually through first 6 bytes of header (ETC1/2, ASTC start with signature
and content starts with headers of up 16 bytes) it is good to measure tie
consumed. Noel added traces. We could add traces in skia to to cover other other
places where onRefEncoded data is called, e.g. [1].
Let me open the issue (bug) to track the requirements/discussion where
onRefEncodedData should be called and where it shouldn't.

2) refactor the ThreadSafeDataTransport::data() to something like
prepareAndGetData() as chrishtr suggested.

I'll start working on both.

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

[Noel Gordon, 2015-12-08 15:41:59]

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

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:54:
return m_frameGenerator->refEncodedData();
Space before this line, as done elsewhere in this file.

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

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:140:
ASSERT(allDataReceived && buffer && buffer->data() == address);
Interesting, a little too much on looking, sorry for asking.  Let's write all
this the way you did before I asked: one-line

    ASSERT(dataTransport);

and be done with it.

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:148: // A
note about allDataReceived:
Remove this line.

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:149: //
SkData is returned only when full image (encoded) data is downloaded. This is
important
downloaded -> received

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:158: 
Prefer the early return: do this case first

    if (m_encodedData) {
        m_encodedData->ref();
        return m_encodedData;
    }

and then create it below.

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:161:
MutexLocker lock(m_decodeMutex);
Hmmm ... none of your other patches had this lock.  Can you explain to me why
you added it here?

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

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:83: // Returns
null if image is not complete.
"Returns null if image is not complete", not quite true.

It's not about the image being complete: it's something to do with all the
encoded data having arrived, right?

[aleksandar_stojiljkovic, 2015-12-08 16:03:58]

astojilj@gmail.com changed reviewers:
+ astojilj@gmail.com

[aleksandar_stojiljkovic, 2015-12-08 16:04:00]

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

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:161:
MutexLocker lock(m_decodeMutex);
On 2015/12/08 15:41:59, noel gordon wrote:
> Hmmm ... none of your other patches had this lock.  Can you explain to me why
> you added it here?
>  

The comment two lines above - for multithreaded rasterizer this is critical
section

[Noel Gordon, 2015-12-08 16:05:02]

On 2015/12/08 15:23:56, aleksandar.stojiljkovic wrote:
> On 2015/12/08 14:48:42, noel gordon wrote:
> > On 2015/12/04 16:15:25, scroggo_chromium wrote:
> > 
> > > On a related note, one could argue that it's incorrect to label the method
> > > const, since it modifies its ThreadSafeDataTransport's SharedBuffer (by
> > > collapsing the data).
> > 
> > Once only, as you went on say, but good to know, since clients require
> > contiguous memory.
> > 
> > But the collapsing could be avoided ...
> > 
> > > But it will still be unnecessary, in the case where the image is (for
> example)
> > a
> > > GIF file that will not be decoded on the GPU. It's much better than the
> > existing
> > > case - where we create a new copy each time the method is called, even if
> the
> > > image is not complete, but I think we could combine this with a check to
> > > determine whether the format is one that can be GPU decoded to save even
> more
> > > wasted work.
> > 
> >  ... agree. No need to call refEncodedData() because there's no need to
sniff
> > when we know it's an image decoder.  How could we get to that fine-sounding,
> > more performant place, I wonder?
> 
> Thanks Noel. Thanks also to other in clarifying the requirements (multi
threaded
> and no writable access).

No worries.

> 1) try to add Skia API like isGPUCompressed to skImageGenerator to prevent
call
> to onRefEncoded in SkImageCacherator. Since sniffing for texture compression
> goes usually through first 6 bytes of header (ETC1/2, ASTC start with
signature
> and content starts with headers of up 16 bytes) it is good to measure tie
> consumed. Noel added traces. We could add traces in skia to to cover other
other
> places where onRefEncoded data is called, e.g. [1].

Yeah, open a bug about it. It should be !easy to skip the sniff entirely [1]

[1] https://gist.github.com/anonymous/9ca292f1911df7a86500

[aleksandar.stojiljkovic, 2015-12-08 20:28:14]

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

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp:54:
return m_frameGenerator->refEncodedData();
On 2015/12/08 15:41:58, noel gordon wrote:
> Space before this line, as done elsewhere in this file.

Done.

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

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:140:
ASSERT(allDataReceived && buffer && buffer->data() == address);
On 2015/12/08 15:41:58, noel gordon wrote:
> Interesting, a little too much on looking, sorry for asking.  Let's write all
> this the way you did before I asked: one-line
> 
>     ASSERT(dataTransport);
> 
> and be done with it.

Acknowledged. Prefer to keep it since it is very useful check. It will be anyway
refactored to singleliner after changing ThreadSafeTransferData.

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:148: // A
note about allDataReceived:
On 2015/12/08 15:41:59, noel gordon wrote:
> Remove this line.

Done.

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:149: //
SkData is returned only when full image (encoded) data is downloaded. This is
important
On 2015/12/08 15:41:59, noel gordon wrote:
> downloaded -> received

Done.

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:158: 
On 2015/12/08 15:41:59, noel gordon wrote:
> Prefer the early return: do this case first
> 
>     if (m_encodedData) {
>         m_encodedData->ref();
>         return m_encodedData;
>     }
> 
> and then create it below.

Done.

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

https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:83: // Returns
null if image is not complete.
On 2015/12/08 15:41:59, noel gordon wrote:
> "Returns null if image is not complete", not quite true.
> 
> It's not about the image being complete: it's something to do with all the
> encoded data having arrived, right?

Done. "Fully received" used on other place.

[aleksandar.stojiljkovic, 2015-12-08 20:28:35]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[aleksandar.stojiljkovic, 2015-12-08 20:28:35]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
reed@google.com, chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1484853003/#ps240001
(title: "process comment #101 and rebase to latest")

[commit-bot: I haz the power, 2015-12-08 20:30:39]

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

[scroggo_chromium, 2015-12-08 21:45:00]

lgtm

https://codereview.chromium.org/1484853003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right):

https://codereview.chromium.org/1484853003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:114: // In
case that ImageFrameGenerator get's deleted before m_encodedData,
get*

[commit-bot: I haz the power, 2015-12-09 02:40:54]

Description was changed from

==========
GPU raster - images upload to GPU performance fix (skip copying encoded data).

For Bug 476531, improved FPS from 5 to 25 (forced GPU rasterization on Lenovo K3
Note).
There DecodingImageGenerator::onRefEncodedData() took 80 - 100 ms on ~65MB of
animated
gif data.

This affects GPU texture upload for all image types when content is downloaded
(onRefEncoded returns 0 for partially downloaded content) since it removes 2
memory
allocations and copies from DecodingImageGenerator::onRefEncodedData() bellow:

When invoking e.g. drawImageRect code would trigger

GrTexture* SkImageCacherator::lockTexture...
// 3. Ask the generator to return a compressed form that the GPU might support
SkAutoTUnref<SkData> data(this->refEncoded());

leading to:

SkData* DecodingImageGenerator::onRefEncodedData() { ...
    m_frameGenerator->copyData(&buffer, &allDataReceived);
    if (buffer && allDataReceived)
        return SkData::NewWithCopy(buffer->data(), buffer->size());

Covered invalid use case, when ThreadSafeDataTransport::setData is called to
append
data to already fully received SharedBuffer.

Thread safety ensured by holding the reference to ThreadSafeRefCounted
ThreadSafeDataTransport (that is again holding ref to SharedBuffer).

BUG=476531
==========

to

==========
GPU raster - images upload to GPU performance fix (skip copying encoded data).

For Bug 476531, improved FPS from 5 to 25 (forced GPU rasterization on Lenovo K3
Note).
There DecodingImageGenerator::onRefEncodedData() took 80 - 100 ms on ~65MB of
animated
gif data.

This affects GPU texture upload for all image types when content is downloaded
(onRefEncoded returns 0 for partially downloaded content) since it removes 2
memory
allocations and copies from DecodingImageGenerator::onRefEncodedData() bellow:

When invoking e.g. drawImageRect code would trigger

GrTexture* SkImageCacherator::lockTexture...
// 3. Ask the generator to return a compressed form that the GPU might support
SkAutoTUnref<SkData> data(this->refEncoded());

leading to:

SkData* DecodingImageGenerator::onRefEncodedData() { ...
    m_frameGenerator->copyData(&buffer, &allDataReceived);
    if (buffer && allDataReceived)
        return SkData::NewWithCopy(buffer->data(), buffer->size());

Covered invalid use case, when ThreadSafeDataTransport::setData is called to
append
data to already fully received SharedBuffer.

Thread safety ensured by holding the reference to ThreadSafeRefCounted
ThreadSafeDataTransport (that is again holding ref to SharedBuffer).

BUG=476531
==========

[commit-bot: I haz the power, 2015-12-09 02:40:56]

Committed patchset #13 (id:240001)

[Noel Gordon, 2015-12-09 02:42:02]

Description was changed from

==========
GPU raster - images upload to GPU performance fix (skip copying encoded data).

For Bug 476531, improved FPS from 5 to 25 (forced GPU rasterization on Lenovo K3
Note).
There DecodingImageGenerator::onRefEncodedData() took 80 - 100 ms on ~65MB of
animated
gif data.

This affects GPU texture upload for all image types when content is downloaded
(onRefEncoded returns 0 for partially downloaded content) since it removes 2
memory
allocations and copies from DecodingImageGenerator::onRefEncodedData() bellow:

When invoking e.g. drawImageRect code would trigger

GrTexture* SkImageCacherator::lockTexture...
// 3. Ask the generator to return a compressed form that the GPU might support
SkAutoTUnref<SkData> data(this->refEncoded());

leading to:

SkData* DecodingImageGenerator::onRefEncodedData() { ...
    m_frameGenerator->copyData(&buffer, &allDataReceived);
    if (buffer && allDataReceived)
        return SkData::NewWithCopy(buffer->data(), buffer->size());

Covered invalid use case, when ThreadSafeDataTransport::setData is called to
append
data to already fully received SharedBuffer.

Thread safety ensured by holding the reference to ThreadSafeRefCounted
ThreadSafeDataTransport (that is again holding ref to SharedBuffer).

BUG=476531
==========

to

==========
Ganesh: images upload to GPU performance fix (skip copying encoded data)

Improved FPS from 5 to 25 (forced GPU rasterization on, on Lenovo K3 Note).
There DecodingImageGenerator::onRefEncodedData() was taking 80-100ms on
~65MB of animated gif data.

This patch improves GPU texture upload for all image types when the encoded
data has been downloaded (onRefEncoded returns 0 for partially downloaded
data) since it removes 2 memory allocations and copies from the Blink-side
DecodingImageGenerator::onRefEncodedData().

For example, when invoking drawImageRect on a GIF image frame, the existing
code would trigger:

GrTexture* SkImageCacherator::lockTexture...
// 3. Ask the generator to return a compressed form that the GPU might support
SkAutoTUnref<SkData> data(this->refEncoded());

which caused an encoded data copy in Blink:

SkData* DecodingImageGenerator::onRefEncodedData() { ...
    m_frameGenerator->copyData(&buffer, &allDataReceived);
    if (buffer && allDataReceived)
        return SkData::NewWithCopy(buffer->data(), buffer->size());

Covered invalid use case, when ThreadSafeDataTransport::setData is called to
append data to already fully received SharedBuffer.

Thread safety ensured by holding the reference to ThreadSafeRefCounted
ThreadSafeDataTransport (that is again holding ref to SharedBuffer).

BUG=476531
==========

[commit-bot: I haz the power, 2015-12-09 02:42:08]

Description was changed from

==========
Ganesh: images upload to GPU performance fix (skip copying encoded data)

Improved FPS from 5 to 25 (forced GPU rasterization on, on Lenovo K3 Note).
There DecodingImageGenerator::onRefEncodedData() was taking 80-100ms on
~65MB of animated gif data.

This patch improves GPU texture upload for all image types when the encoded
data has been downloaded (onRefEncoded returns 0 for partially downloaded
data) since it removes 2 memory allocations and copies from the Blink-side
DecodingImageGenerator::onRefEncodedData().

For example, when invoking drawImageRect on a GIF image frame, the existing
code would trigger:

GrTexture* SkImageCacherator::lockTexture...
// 3. Ask the generator to return a compressed form that the GPU might support
SkAutoTUnref<SkData> data(this->refEncoded());

which caused an encoded data copy in Blink:

SkData* DecodingImageGenerator::onRefEncodedData() { ...
    m_frameGenerator->copyData(&buffer, &allDataReceived);
    if (buffer && allDataReceived)
        return SkData::NewWithCopy(buffer->data(), buffer->size());

Covered invalid use case, when ThreadSafeDataTransport::setData is called to
append data to already fully received SharedBuffer.

Thread safety ensured by holding the reference to ThreadSafeRefCounted
ThreadSafeDataTransport (that is again holding ref to SharedBuffer).

BUG=476531
==========

to

==========
GPU raster - images upload to GPU performance fix (skip copying encoded data).

For Bug 476531, improved FPS from 5 to 25 (forced GPU rasterization on Lenovo K3
Note).
There DecodingImageGenerator::onRefEncodedData() took 80 - 100 ms on ~65MB of
animated
gif data.

This affects GPU texture upload for all image types when content is downloaded
(onRefEncoded returns 0 for partially downloaded content) since it removes 2
memory
allocations and copies from DecodingImageGenerator::onRefEncodedData() bellow:

When invoking e.g. drawImageRect code would trigger

GrTexture* SkImageCacherator::lockTexture...
// 3. Ask the generator to return a compressed form that the GPU might support
SkAutoTUnref<SkData> data(this->refEncoded());

leading to:

SkData* DecodingImageGenerator::onRefEncodedData() { ...
    m_frameGenerator->copyData(&buffer, &allDataReceived);
    if (buffer && allDataReceived)
        return SkData::NewWithCopy(buffer->data(), buffer->size());

Covered invalid use case, when ThreadSafeDataTransport::setData is called to
append
data to already fully received SharedBuffer.

Thread safety ensured by holding the reference to ThreadSafeRefCounted
ThreadSafeDataTransport (that is again holding ref to SharedBuffer).

BUG=476531

Committed: https://crrev.com/03166f6a694d43dff7e71ae04fa2847317c51101
Cr-Commit-Position: refs/heads/master@{#363939}
==========

[commit-bot: I haz the power, 2015-12-09 02:42:10]

Patchset 13 (id:??) landed as
https://crrev.com/03166f6a694d43dff7e71ae04fa2847317c51101
Cr-Commit-Position: refs/heads/master@{#363939}

[Noel Gordon, 2015-12-09 02:50:02]

On 2015/12/08 16:04:00, aleksandar_stojiljkovic wrote:
>
https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:161:
> MutexLocker lock(m_decodeMutex);
> On 2015/12/08 15:41:59, noel gordon wrote:
> > Hmmm ... none of your other patches had this lock.  Can you explain to me
why
> > you added it here?
> >  
> 
> The comment two lines above - for multithreaded rasterizer this is critical
> section

Maybe, but you've blocked the entire decoder now just the read the encoded data.
 Perhaps that'll be fixed when you file the bugs about work needed for
ThreadSafeDataTransport.

[aleksandar.stojiljkovic, 2015-12-09 08:21:40]

On 2015/12/09 02:50:02, noel gordon wrote:
> On 2015/12/08 16:04:00, aleksandar_stojiljkovic wrote:
> >
>
https://codereview.chromium.org/1484853003/diff/220001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:161:
> > MutexLocker lock(m_decodeMutex);
> > On 2015/12/08 15:41:59, noel gordon wrote:
> > > Hmmm ... none of your other patches had this lock.  Can you explain to me
> why
> > > you added it here?
> > >  
> > 
> > The comment two lines above - for multithreaded rasterizer this is critical
> > section
> 
> Maybe, but you've blocked the entire decoder now just the read the encoded
data.
>  Perhaps that'll be fixed when you file the bugs about work needed for
> ThreadSafeDataTransport.

Issue 568016 added.
Mutex seems necessary there to cover SkData instantiation (a minor note, that
part should be fast as there is no copy). One could argue that we dont need 2
mutexes (ThreadSafeDataTransport::data() called in that method already has a
mutex). Yes, let me address it during refactoring.