new API for retrieving YUV data

new API for retrieving YUV data

BUG=skia:

[reed1, 2015-01-21 22:20:46]

reed@google.com changed reviewers:
+ robertphillips@google.com, sugoi@google.com

[reed1, 2015-01-21 22:20:46]

just want to converge on the API semantics/naming first.

[sugoi1, 2015-01-22 15:01:36]

sugoi@chromium.org changed reviewers:
+ sugoi@chromium.org

[sugoi1, 2015-01-22 15:01:37]

https://codereview.chromium.org/863053002/diff/1/include/core/SkImageGenerator.h
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/863053002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:132: *  If this returns false, the value of the
(optional) sizes parameters is undefined.
What if the decoder failed completely and the logical sizes were also not set?
(maybe the image's header isn't fully downloaded yet because of a crappy
internet connection) I don't know if that's possible, just wondering.

[reed1, 2015-01-22 16:01:36]

https://codereview.chromium.org/863053002/diff/1/include/core/SkImageGenerator.h
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/863053002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:132: *  If this returns false, the value of the
(optional) sizes parameters is undefined.
On 2015/01/22 15:01:37, sugoi1 wrote:
> What if the decoder failed completely and the logical sizes were also not set?
> (maybe the image's header isn't fully downloaded yet because of a crappy
> internet connection) I don't know if that's possible, just wondering.

To get to the stage of being an imagegenerator at all, we needed to have decoded
enough to know the "master" W/H. However, if we fail to get the next step of
querying the U/V plane sizes, then I think this should return false.

[reed1, 2015-01-22 17:01:05]

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

[reed1, 2015-01-22 17:01:05]

Leon, seems like we need to coordinate w/ your plans for future Decoder API
changes (esp. around YUV)

[scroggo, 2015-01-22 19:47:47]

https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
include/core/SkImageGenerator.h:138: bool queryYUV8(SkISize logicalSizes[3],
SkISize optimalAllocationSizes[3]);
It's interesting to me that optimalAllocationSizes is a set of SkISizes. What
are the units? bytes? Pixels? Would it be reasonable to make it a set of
size_ts?

https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
include/core/SkImageGenerator.h:145: *  sizes for the 3 planes. For getYUV8(),
tt is legal to pass the logical sizes
it*

https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
include/core/SkImageGenerator.h:155: bool getYUV8(const SkISize
allocationSizes[3], void* planes[3], SkYUVColorSpace* colorSpace);
Can the color space be known in queryYUV8?

https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
include/core/SkImageGenerator.h:170: virtual bool onQueryYUV8(SkISize
logicalSizes[3], SkISize optimalAllocationSizes[3]);
Should we have documentation for implementors that the parameters will not be
NULL when calling the virtual functions?

https://codereview.chromium.org/863053002/diff/60001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/863053002/diff/60001/src/gpu/SkGr.cpp#newcode262
src/gpu/SkGr.cpp:262: // Get the YUV planes and update plane sizes to actual
image size
Does this comment still apply?

https://codereview.chromium.org/863053002/diff/60001/src/images/SkDecodingIma...
File src/images/SkDecodingImageGenerator.cpp (right):

https://codereview.chromium.org/863053002/diff/60001/src/images/SkDecodingIma...
src/images/SkDecodingImageGenerator.cpp:48: bool onGetYUV8(const SkISize
sizes[3], void* planes[3], SkYUVColorSpace*) SK_OVERRIDE;
#ifdef SK_SUPPORT_LEGACY_IMAGEGENERATOR_YUV_API ?

[scroggo, 2015-01-27 18:35:49]

On 2015/01/22 17:01:05, reed1 wrote:
> Leon, seems like we need to coordinate w/ your plans for future Decoder API
> changes (esp. around YUV)

This change seems to be compatible with the new Decoder API.

https://codereview.chromium.org/863053002/diff/1/include/core/SkImageGenerator.h
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/863053002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:132: *  If this returns false, the value of the
(optional) sizes parameters is undefined.
On 2015/01/22 16:01:36, reed1 wrote:
> On 2015/01/22 15:01:37, sugoi1 wrote:
> > What if the decoder failed completely and the logical sizes were also not
set?
> > (maybe the image's header isn't fully downloaded yet because of a crappy
> > internet connection) I don't know if that's possible, just wondering.
> 
> To get to the stage of being an imagegenerator at all, we needed to have
decoded
> enough to know the "master" W/H. However, if we fail to get the next step of
> querying the U/V plane sizes, then I think this should return false.

In general, I would suggest that if it returns false, all output parameters
should be assumed to be undefined. (I think this matches our behavior
elsewhere.)

I'm also not sure why the logical sizes would be known by the decoder and not
the optimalAllocationSizes. All the information comes from the cinfo, which was
initilized when reading the header.

[reed1, 2015-01-27 18:42:40]

https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
include/core/SkImageGenerator.h:138: bool queryYUV8(SkISize logicalSizes[3],
SkISize optimalAllocationSizes[3]);
On 2015/01/22 19:47:47, scroggo wrote:
> It's interesting to me that optimalAllocationSizes is a set of SkISizes. What
> are the units? bytes? Pixels? Would it be reasonable to make it a set of
> size_ts?

width/height in pixels -- will document.

https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
include/core/SkImageGenerator.h:145: *  sizes for the 3 planes. For getYUV8(),
tt is legal to pass the logical sizes
On 2015/01/22 19:47:46, scroggo wrote:
> it*

Done.

https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
include/core/SkImageGenerator.h:155: bool getYUV8(const SkISize
allocationSizes[3], void* planes[3], SkYUVColorSpace* colorSpace);
On 2015/01/22 19:47:46, scroggo wrote:
> Can the color space be known in queryYUV8?

Yea, good question. Seems like its a slightly more onerous constraint on the
codec, so I was going to leave it off until someone made a case for knowing it
sooner. Do you think we should push for it to be at the query step?

https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
include/core/SkImageGenerator.h:170: virtual bool onQueryYUV8(SkISize
logicalSizes[3], SkISize optimalAllocationSizes[3]);
On 2015/01/22 19:47:47, scroggo wrote:
> Should we have documentation for implementors that the parameters will not be
> NULL when calling the virtual functions?

Done.

[reed1, 2015-01-27 18:44:04]

https://codereview.chromium.org/863053002/diff/1/include/core/SkImageGenerator.h
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/863053002/diff/1/include/core/SkImageGenerato...
include/core/SkImageGenerator.h:132: *  If this returns false, the value of the
(optional) sizes parameters is undefined.
On 2015/01/27 18:35:49, scroggo wrote:
> On 2015/01/22 16:01:36, reed1 wrote:
> > On 2015/01/22 15:01:37, sugoi1 wrote:
> > > What if the decoder failed completely and the logical sizes were also not
> set?
> > > (maybe the image's header isn't fully downloaded yet because of a crappy
> > > internet connection) I don't know if that's possible, just wondering.
> > 
> > To get to the stage of being an imagegenerator at all, we needed to have
> decoded
> > enough to know the "master" W/H. However, if we fail to get the next step of
> > querying the U/V plane sizes, then I think this should return false.
> 
> In general, I would suggest that if it returns false, all output parameters
> should be assumed to be undefined. (I think this matches our behavior
> elsewhere.)

Agreed, will make that change.

> 
> I'm also not sure why the logical sizes would be known by the decoder and not
> the optimalAllocationSizes. All the information comes from the cinfo, which
was
> initilized when reading the header.

Agreed.

[scroggo, 2015-01-27 18:59:18]

https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
include/core/SkImageGenerator.h:155: bool getYUV8(const SkISize
allocationSizes[3], void* planes[3], SkYUVColorSpace* colorSpace);
On 2015/01/27 18:42:39, reed1 wrote:
> On 2015/01/22 19:47:46, scroggo wrote:
> > Can the color space be known in queryYUV8?
> 
> Yea, good question. Seems like its a slightly more onerous constraint on the
> codec, so I was going to leave it off until someone made a case for knowing it
> sooner. Do you think we should push for it to be at the query step?

It would make sense to me if it was known at the query step, but I don't know
enough about JPEG to know when we could know that.

Our jpeg decoder only uses one color space, so our implementation could easily
specify it at the query step.

VideoImageGenerator in Chromium checks a VideoFrame's ::format() to determine
the colorspace
(https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/skca...).
It appears that the format is constant
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_f...).

So at least for the existing implementations, putting it at the query step is
not a problem.

That said, if we were ever to support the other SkYUVColorSpace,
kRec601_SkYUVColorSpace, I don't know how soon we can know the color space.
sugoi?

[sugoi1, 2015-02-04 19:04:18]

On 2015/01/27 18:59:18, scroggo wrote:
>
https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
> File include/core/SkImageGenerator.h (right):
> 
>
https://codereview.chromium.org/863053002/diff/60001/include/core/SkImageGene...
> include/core/SkImageGenerator.h:155: bool getYUV8(const SkISize
> allocationSizes[3], void* planes[3], SkYUVColorSpace* colorSpace);
> On 2015/01/27 18:42:39, reed1 wrote:
> > On 2015/01/22 19:47:46, scroggo wrote:
> > > Can the color space be known in queryYUV8?
> > 
> > Yea, good question. Seems like its a slightly more onerous constraint on the
> > codec, so I was going to leave it off until someone made a case for knowing
it
> > sooner. Do you think we should push for it to be at the query step?
> 
> It would make sense to me if it was known at the query step, but I don't know
> enough about JPEG to know when we could know that.
> 
> Our jpeg decoder only uses one color space, so our implementation could easily
> specify it at the query step.
> 
> VideoImageGenerator in Chromium checks a VideoFrame's ::format() to determine
> the colorspace
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/skca...).
> It appears that the format is constant
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_f...).
> 
> So at least for the existing implementations, putting it at the query step is
> not a problem.
> 
> That said, if we were ever to support the other SkYUVColorSpace,
> kRec601_SkYUVColorSpace, I don't know how soon we can know the color space.
> sugoi?

(Sorry for the long delay) AFAIK, if we know the size, we should also have the
colorspace information, so the query step should be able to get the colorspace.

There's currently a memory usage issue that we're trying to fix here:
https://codereview.chromium.org/901573004/

It seems like the VideoFrame already contains the YUV planes data in this case
and our API forces a memcpy where it may not be necessary (it's currently not
doing a memcpy, but that's a misuse of this API to simply overwrite the YUV
planes pointers). I don't know if this API should be able to reflect being able
to receive data from already allocated memory (from an outside source,
basically). Although it isn't the desired API, it does cost some time, so any
idea to fix the VideoImageGenerator case efficiently is welcome.

[reed1, 2015-02-04 19:22:17]

Gotcha.

1. I think we should try to land a version of this CL, as it clarifies the
existing protocol.

2. Lets think about how to expose to Generators the ability to return their data
already-in-hand... Or maybe they're not a generate as much as a bitmap itself. I
think one tricky part either way is life-time management: how long is the client
allowed to treat that memory as valid? (e.g. can I embed those ptrs in a
picture?)

[sugoi1, 2015-02-04 19:36:00]

On 2015/02/04 19:22:17, reed1 wrote:
> Gotcha.
> 
> 1. I think we should try to land a version of this CL, as it clarifies the
> existing protocol.

Ok, sgtm. Could you update to ToT? SkGr.cpp changed a little bit today.

> 2. Lets think about how to expose to Generators the ability to return their
data
> already-in-hand... Or maybe they're not a generate as much as a bitmap itself.
I
> think one tricky part either way is life-time management: how long is the
client
> allowed to treat that memory as valid? (e.g. can I embed those ptrs in a
> picture?)

Right, this can get pretty hairy if the client can deallocate that memory while
we're using it.
For me, any API that allows for maximum performance is fine, but I don't have a
perfect solution right now.

[scroggo, 2015-02-09 20:08:53]

On 2015/02/04 19:22:17, reed1 wrote:
> Gotcha.
> 
> 1. I think we should try to land a version of this CL, as it clarifies the
> existing protocol.
> 
> 2. Lets think about how to expose to Generators the ability to return their
data
> already-in-hand... Or maybe they're not a generate as much as a bitmap itself.
I
> think one tricky part either way is life-time management: how long is the
client
> allowed to treat that memory as valid? (e.g. can I embed those ptrs in a
> picture?)

IIUC, the motivation behind an SkImageGenerator is to be an abstraction for
regenerating in the case where the client had thrown away the generated image.
In the case of returning a bitmap or memory supplied by the generator, does it
still make sense to be an SkImageGenerator? Or is there some other abstraction
that makes more sense? (If it should *not* be an SkImageGenerator, we need to
rethink merging SkCodec and SkImageGenerator (as suggested in
https://codereview.chromium.org/822083002/).)

As for how long the memory is valid, I would say as long as the SkImageGenerator
(or other abstraction) is still around. So if you used
SkImage::NewFromGenerator, which returns a new SkImage that holds on to the
SkImageGenerator, there should not be any problems. When drawing to an
SkPicture, I assume drawImage stores a reference to the SkImage, so that should
be fine, too. I would say that embedding those pointers directly into an
SkPicture (or hanging on to them directly in general) would be a bad idea.