SkDiscardablePixelRef should favor not decoding to YUV if they already decoded to RGB.

SkDiscardablePixelRef should favor not decoding to YUV if they already decoded to RGB.

BUG=skia:
BUG=459760
Committed: https://skia.googlesource.com/skia/+/278f86e4df6b835d0bcdb4ca14fa4dbbd4271cac

[vmiura, 2015-02-20 21:41:11]

vmiura@chromium.org changed reviewers:
+ bsalomon@google.com, sugoi@google.com

[vmiura, 2015-02-20 21:41:12]

PTAL.

If we have more than one raster thread (desktop only), image
pre-decoding in Chromium's compositor can make use of the
extra threads.

Currently this pre-decodes to RGB format with getPixels(),
and in the event that Ganesh uses YUV, then we decode the
images twice.  Favor not using YUV if SkDiscardablePixelRef
already has decoded data.

[sugoi1, 2015-02-20 21:51:08]

On 2015/02/20 21:41:12, vmiura wrote:
> PTAL.
> 
> If we have more than one raster thread (desktop only), image
> pre-decoding in Chromium's compositor can make use of the
> extra threads.
> 
> Currently this pre-decodes to RGB format with getPixels(),
> and in the event that Ganesh uses YUV, then we decode the
> images twice.  Favor not using YUV if SkDiscardablePixelRef
> already has decoded data.

LGTM, but in the event that SkDiscardablePixelRef::onNewLockPixels() knows that
the GPU path will be used, it could call onGetYUV8Planes() instead of
getPixels() and store the result using SkYUVPlanesCache::Add() instead of using
fDiscardableMemory. It shouldn't be very hard to do, except for the part where
you know that the GPU path will be used later on, this might be a bit tricky.

[vmiura, 2015-02-20 21:54:26]

On 2015/02/20 21:51:08, sugoi1 wrote:
> On 2015/02/20 21:41:12, vmiura wrote:
> > PTAL.
> > 
> > If we have more than one raster thread (desktop only), image
> > pre-decoding in Chromium's compositor can make use of the
> > extra threads.
> > 
> > Currently this pre-decodes to RGB format with getPixels(),
> > and in the event that Ganesh uses YUV, then we decode the
> > images twice.  Favor not using YUV if SkDiscardablePixelRef
> > already has decoded data.
> 
> LGTM, but in the event that SkDiscardablePixelRef::onNewLockPixels() knows
that
> the GPU path will be used, it could call onGetYUV8Planes() instead of
> getPixels() and store the result using SkYUVPlanesCache::Add() instead of
using
> fDiscardableMemory. It shouldn't be very hard to do, except for the part where
> you know that the GPU path will be used later on, this might be a bit tricky.

Yes, it might be nice to add that.  We're able to know that we should try
onGetYUV8Planes() from the Compositor side,
so we just need the caching to happen, and into the right GrGpu instance.

[bsalomon, 2015-02-20 22:24:44]

https://codereview.chromium.org/944823004/diff/1/src/lazy/SkDiscardablePixelR...
File src/lazy/SkDiscardablePixelRef.h (right):

https://codereview.chromium.org/944823004/diff/1/src/lazy/SkDiscardablePixelR...
src/lazy/SkDiscardablePixelRef.h:57: if (fDiscardableMemory)
tiny style nits, we always us {} and indents are (sadly imo) 4 spaces.

[vmiura, 2015-02-20 22:32:07]

New patchsets have been uploaded after l-g-t-m from sugoi@chromium.org

[vmiura, 2015-02-20 22:32:48]

On 2015/02/20 22:24:44, bsalomon wrote:
>
https://codereview.chromium.org/944823004/diff/1/src/lazy/SkDiscardablePixelR...
> File src/lazy/SkDiscardablePixelRef.h (right):
> 
>
https://codereview.chromium.org/944823004/diff/1/src/lazy/SkDiscardablePixelR...
> src/lazy/SkDiscardablePixelRef.h:57: if (fDiscardableMemory)
> tiny style nits, we always us {} and indents are (sadly imo) 4 spaces.

Done, PTAL.

[brian1, 2015-02-21 11:58:24]

brian@thesalomons.net changed reviewers:
+ brian@thesalomons.net

[brian1, 2015-02-21 11:58:25]

lgtm

[vmiura, 2015-02-23 18:48:49]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2015-02-23 18:49:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/944823004/20001

[commit-bot: I haz the power, 2015-02-23 18:59:48]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/278f86e4df6b835d0bcdb4ca14fa4dbbd4271cac