Add whitelist parameter to refEncodedData

Add whitelist parameter to refEncodedData

Meant to allow the generator to preflight its encoded format, and if it
will not be supported, to return null instead of actually returning the
data. This is particularly important for a generator that does not have
contiguous data, so it would have to copy in order to return an SkData.

SkEncodedFormat.h:
- Move into core, where it can be referenced by SkImageGenerator.
- Remove unused kUnknown_SkEncodedFormat.

SkImageGenerator.h:
- Add RefEncodedWhitelist, and pass it (optionally) to refEncodedData.

SkCodecImageGenerator.cpp:
- Check the whitelist before returning fData.

SkImageCacherator:
- Add a whitelist for the GPU, and pass it to refEncodedData.

SkGr.cpp/SkGrPriv.h:
- Add a method to check to see if an SkEncodedFormat may be supported
  by the GPU

ImageGeneratorTest.cpp:
- Add a test

Actual benefit will be seen once we take advantage of it in Chromium,
and Chromium stops defining SK_SUPPORT_LEGACY_REFENCODEDDATA_NOCTX.

BUG=chromium:568016
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1862133002

[scroggo_chromium, 2016-04-06 15:25:32]

Description was changed from

==========
Add query parameter to refEncodedData

Meant to allow the generator to preflight its encoded format, and if it
will not be supported, to return null instead of actually returning the
data. This is particularly important for a generator that does not have
contiguous data, so it would have to copy in order to return an SkData.

Revival of reed@'s patch at crrev.com/1567623002

BUG=chromium:568016
==========

to

==========
Add query parameter to refEncodedData

Meant to allow the generator to preflight its encoded format, and if it
will not be supported, to return null instead of actually returning the
data. This is particularly important for a generator that does not have
contiguous data, so it would have to copy in order to return an SkData.

Revival of reed@'s patch at crrev.com/1567623002

BUG=chromium:568016
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[scroggo_chromium, 2016-04-06 17:42:07]

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

[scroggo_chromium, 2016-04-06 17:51:12]

scroggo@chromium.org changed reviewers:
+ aleksandar.stojiljkovic@intel.com, bsalomon@google.com,
robertphillips@google.com

[scroggo_chromium, 2016-04-06 17:51:13]

+ reviewers from Mike's original CL

[scroggo_chromium, 2016-04-06 20:52:34]

Description was changed from

==========
Add query parameter to refEncodedData

Meant to allow the generator to preflight its encoded format, and if it
will not be supported, to return null instead of actually returning the
data. This is particularly important for a generator that does not have
contiguous data, so it would have to copy in order to return an SkData.

Revival of reed@'s patch at crrev.com/1567623002

BUG=chromium:568016
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add query parameter to refEncodedData

Meant to allow the generator to preflight its encoded format, and if it
will not be supported, to return null instead of actually returning the
data. This is particularly important for a generator that does not have
contiguous data, so it would have to copy in order to return an SkData.

Revival of reed@'s patch at crrev.com/1567623002
Actual benefit will be seen once crrev.com/1869583002 lands in Chromium,
and Chromium stops defining SK_SUPPORT_LEGACY_REFENCODEDDATA_NOCTX.

BUG=chromium:568016
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[scroggo_chromium, 2016-04-06 20:52:50]

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

[commit-bot: I haz the power, 2016-04-06 20:52:55]

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

[commit-bot: I haz the power, 2016-04-06 20:52:57]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2016-04-07 02:52 UTC

[commit-bot: I haz the power, 2016-04-07 02:52:09]

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

[commit-bot: I haz the power, 2016-04-07 02:52:10]

Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer

[reed1, 2016-04-07 13:27:31]

I've forgotten, but why don't we just allow the caller to specify a whilte or
black list of encoded-formats when calling the generator?

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
File include/core/SkEncodedFormat.h (right):

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
include/core/SkEncodedFormat.h:15: kUnknown_SkEncodedFormat,
why do we have an unknown value. From the API, its not clear why we need it

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
include/core/SkEncodedFormat.h:29: class SkEncodedFormatQuery {
may need a block comment summarizing why we have this class

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
include/core/SkEncodedFormat.h:34: *  Returns true if the specified format is
supported.
... supported where/how?

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
include/core/SkEncodedFormat.h:47: virtual bool supportedFormatFromData(const
void* data, size_t length) = 0;
Not sure what supported means here. "I can encode into the same format as this
data is encoded as"?

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkImageGen...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkImageGen...
include/core/SkImageGenerator.h:82: *  it may ignore the bot and just return it.
However, if there is a performance or other
"bot"?

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkImageGen...
include/core/SkImageGenerator.h:83: *  constraint on returning the encoded data,
calling the bot allows the generator to discover
Slightly confusing dox, as the first paragraph says we only return it if its
"readily available", but now we're talking about a "performance constraint" for
some formats...

[scroggo_chromium, 2016-04-08 18:16:36]

It sounds like you want some better documentation. I haven't updated it, but
will once we agree on the approach.

> I've forgotten, but why don't we just allow the caller to
> specify a whilte or black list of encoded-formats when
> calling the generator?

That sounds like the first callback, supportedFormat(SkEncodedFormat). Are you
suggesting that we drop the second version (supportedFormatFromData)? That
should be enough; a DecodingImageGenerator is not created until the format is
known. (Or are you suggesting something else?)

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
File include/core/SkEncodedFormat.h (right):

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
include/core/SkEncodedFormat.h:15: kUnknown_SkEncodedFormat,
On 2016/04/07 13:27:31, reed1 wrote:
> why do we have an unknown value. From the API, its not clear why we need it

This is unchanged. But FWIW, it mirrors how we have unknown SkColorTypes etc. We
could perhaps get away without it - I just did a git grep and only found one
instance. I could imagine using it for something for which we don't yet have an
enum here, but maybe in that case we should just add another enum.

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
include/core/SkEncodedFormat.h:29: class SkEncodedFormatQuery {
On 2016/04/07 13:27:31, reed1 wrote:
> may need a block comment summarizing why we have this class

Acknowledged.

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
include/core/SkEncodedFormat.h:34: *  Returns true if the specified format is
supported.
On 2016/04/07 13:27:31, reed1 wrote:
> ... supported where/how?

Acknowledged.

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
include/core/SkEncodedFormat.h:47: virtual bool supportedFormatFromData(const
void* data, size_t length) = 0;
On 2016/04/07 13:27:31, reed1 wrote:
> Not sure what supported means here. "I can encode into the same format as this
> data is encoded as"?

Acknowledged.

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkImageGen...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkImageGen...
include/core/SkImageGenerator.h:82: *  it may ignore the bot and just return it.
However, if there is a performance or other
On 2016/04/07 13:27:31, reed1 wrote:
> "bot"?

Oops, I replaced the first instance of "bot" in your original CL with "query",
but missed the others...

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkImageGen...
include/core/SkImageGenerator.h:83: *  constraint on returning the encoded data,
calling the bot allows the generator to discover
On 2016/04/07 13:27:31, reed1 wrote:
> Slightly confusing dox, as the first paragraph says we only return it if its
> "readily available", but now we're talking about a "performance constraint"
for
> some formats...

Acknowledged.

[scroggo_chromium, 2016-04-12 14:18:53]

Description was changed from

==========
Add query parameter to refEncodedData

Meant to allow the generator to preflight its encoded format, and if it
will not be supported, to return null instead of actually returning the
data. This is particularly important for a generator that does not have
contiguous data, so it would have to copy in order to return an SkData.

Revival of reed@'s patch at crrev.com/1567623002
Actual benefit will be seen once crrev.com/1869583002 lands in Chromium,
and Chromium stops defining SK_SUPPORT_LEGACY_REFENCODEDDATA_NOCTX.

BUG=chromium:568016
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add whitelist parameter to refEncodedData

Meant to allow the generator to preflight its encoded format, and if it
will not be supported, to return null instead of actually returning the
data. This is particularly important for a generator that does not have
contiguous data, so it would have to copy in order to return an SkData.

SkEncodedFormat.h:
- Move into core, where it can be referenced by SkImageGenerator.
- Remove unused kUnknown_SkEncodedFormat.

SkImageGenerator.h:
- Add RefEncodedWhitelist, and pass it (optionally) to refEncodedData.

SkCodecImageGenerator.cpp:
- Check the whitelist before returning fData.

SkImageCacherator:
- Add a whitelist for the GPU, and pass it to refEncodedData.

SkGr.cpp/SkGrPriv.h:
- Add a method to check to see if an SkEncodedFormat may be supported
  by the GPU

ImageGeneratorTest.cpp:
- Add a test

Actual benefit will be seen once we take advantage of it in Chromium,
and Chromium stops defining SK_SUPPORT_LEGACY_REFENCODEDDATA_NOCTX.

BUG=chromium:568016
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[scroggo_chromium, 2016-04-12 14:41:19]

PTAL at patch set 9

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
File include/core/SkEncodedFormat.h (right):

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
include/core/SkEncodedFormat.h:15: kUnknown_SkEncodedFormat,
On 2016/04/08 18:16:36, scroggo_chromium wrote:
> On 2016/04/07 13:27:31, reed1 wrote:
> > why do we have an unknown value. From the API, its not clear why we need it
> 
> This is unchanged. But FWIW, it mirrors how we have unknown SkColorTypes etc.
We
> could perhaps get away without it - I just did a git grep and only found one
> instance. I could imagine using it for something for which we don't yet have
an
> enum here, but maybe in that case we should just add another enum.

Removed until we discover we need it.

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
include/core/SkEncodedFormat.h:29: class SkEncodedFormatQuery {
On 2016/04/08 18:16:36, scroggo_chromium wrote:
> On 2016/04/07 13:27:31, reed1 wrote:
> > may need a block comment summarizing why we have this class
> 
> Acknowledged.

I've moved this class into SkImageGenerator. I don't currently see other needs
for it, and it allows for a clearer block comment describing what it does.

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkEncodedF...
include/core/SkEncodedFormat.h:47: virtual bool supportedFormatFromData(const
void* data, size_t length) = 0;
On 2016/04/08 18:16:36, scroggo_chromium wrote:
> On 2016/04/07 13:27:31, reed1 wrote:
> > Not sure what supported means here. "I can encode into the same format as
this
> > data is encoded as"?
> 
> Acknowledged.

This version has been removed from the new class.

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkImageGen...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkImageGen...
include/core/SkImageGenerator.h:82: *  it may ignore the bot and just return it.
However, if there is a performance or other
On 2016/04/07 13:27:31, reed1 wrote:
> "bot"?

Removed.

https://codereview.chromium.org/1862133002/diff/80001/include/core/SkImageGen...
include/core/SkImageGenerator.h:83: *  constraint on returning the encoded data,
calling the bot allows the generator to discover
On 2016/04/07 13:27:31, reed1 wrote:
> Slightly confusing dox, as the first paragraph says we only return it if its
> "readily available", but now we're talking about a "performance constraint"
for
> some formats...

Agreed. I've updated the comment with an example (the case in Chrome).

This may bring up a larger question though - should DecodingImageGenerator
implement onRefEncodedData at all?

- None of the formats supported by DecodingImageGenerator today can be passed in
encoded form directly to the GPU (though perhaps this will change in the future)
- It does not necessarily have its data in contiguous form (though we could
check, by seeing if getSomeData() == size()). This goes against the original
intent of refEncodedData (which is still reflected in the first paragraph).
- If we're going to encode (the other client of refEncodedData), we definitely
want this data. Maybe there should be a different method which is more flexible.
It asks for the encoded data but does not require it to be contiguous. Both in
this case and for passing encoded pixels to the GPU we *could* call
refEncodedData first (return null for DecodingImageGenerator), then call the
more flexible version (which will consolidate it or use it directly, as
necessary, and can even bail early if the data is not desirable) and then fall
back if necessary (e.g. full decode). That approach is a bit more involved, but
maybe it's a better route?

[scroggo_chromium, 2016-05-02 17:53:09]

reed@, please take a look. This will help with crbug.com/568016 and potentially
crbug.com/604008

[scroggo_chromium, 2016-05-11 15:30:35]

On 2016/05/02 17:53:09, scroggo_chromium wrote:
> reed@, please take a look. This will help with crbug.com/568016 and
potentially
> crbug.com/604008

Fixed with https://codereview.chromium.org/1970773002/