Use demux API in SkWebpCodec

Use demux API in SkWebpCodec

* Necessary to read ICC profiles
* Will be necessary to implement animation
* Requires consolidated data with length

Doesn't affect decode performance (thought I believe
our performance tests only cover SkStreams with memory
bases).

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2311793004

Committed: https://skia.googlesource.com/skia/+/ff2a6c8a70d5f7e3be99ef41c75412878785edcb

[msarett, 2016-09-06 22:59:22]

Description was changed from

==========
Use demux API in SkWebpCodec

* Necessary to read ICC profiles
* Will be necessary to implement animation
* Requires consolidated data with length

BUG=skia:
==========

to

==========
Use demux API in SkWebpCodec

* Necessary to read ICC profiles
* Will be necessary to implement animation
* Requires consolidated data with length

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2311793004
==========

[msarett, 2016-09-06 23:36:54]

Description was changed from

==========
Use demux API in SkWebpCodec

* Necessary to read ICC profiles
* Will be necessary to implement animation
* Requires consolidated data with length

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2311793004
==========

to

==========
Use demux API in SkWebpCodec

* Necessary to read ICC profiles
* Will be necessary to implement animation
* Requires consolidated data with length

Doesn't affect decode performance (thought I believe
our performance tests only cover SkStreams with memory
bases).

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2311793004
==========

[msarett, 2016-09-06 23:47:10]

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

[msarett, 2016-09-06 23:47:11]

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:286: switch (WebPIUpdate(idec, frame.fragment.bytes,
frame.fragment.size)) {
It's kind of ugly to create the WebPIDecoder with no data and then immediately
update it...

But I can't see that WebPIDecode() gives us back the status in any way.

[scroggo, 2016-09-07 14:20:52]

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:41: if (stream->getMemoryBase()) {
I think we have some common code to handle this case that comes up a lot. Maybe
in SkStreamPriv or something like that.

Maybe you can't use that because you sometimes want to delete the stream?

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:42: data =
SkData::MakeWithoutCopy(stream->getMemoryBase(), stream->getLength());
Maybe add a comment here that this is safe because we hang onto the stream?

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:65: SkAutoTCallVProc<WebPDemuxer, WebPDemuxDelete>
demux(WebPDemuxPartial(&webpData, nullptr));
I think this is OK (in fact I think it's what chrome does), but it's a little
weird that the demux will outlive the webpData (though it needs those pointers
to remain valid!)

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:83: if (!WebPDemuxGetFrame(demux, 1, &frame)) {
I forget, does this decode the frame? I guess not, since you haven't set the
output yet. This just gets info?

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:140: streamDeleter.release(), demux.release(),
std::move(data));
Sometimes streamDeleter will hold null. I forgot, is that OK? I.e it doesn't
cause problems for rewinding or anything?

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:286: switch (WebPIUpdate(idec, frame.fragment.bytes,
frame.fragment.size)) {
On 2016/09/06 23:47:11, msarett wrote:
> It's kind of ugly to create the WebPIDecoder with no data and then immediately
> update it...

It's the same as we did before though, right?

> 
> But I can't see that WebPIDecode() gives us back the status in any way.

[msarett, 2016-09-07 16:47:10]

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:41: if (stream->getMemoryBase()) {
On 2016/09/07 14:20:51, scroggo_BACK_FROM_PATERNITY wrote:
> I think we have some common code to handle this case that comes up a lot.
Maybe
> in SkStreamPriv or something like that.
> 
> Maybe you can't use that because you sometimes want to delete the stream? 

Ahh yes.  The common code always performs a copy, so I'll keep the
getMemoryBase() path.

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:42: data =
SkData::MakeWithoutCopy(stream->getMemoryBase(), stream->getLength());
On 2016/09/07 14:20:51, scroggo_BACK_FROM_PATERNITY wrote:
> Maybe add a comment here that this is safe because we hang onto the stream?

SGTM

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:65: SkAutoTCallVProc<WebPDemuxer, WebPDemuxDelete>
demux(WebPDemuxPartial(&webpData, nullptr));
On 2016/09/07 14:20:51, scroggo_BACK_FROM_PATERNITY wrote:
> I think this is OK (in fact I think it's what chrome does), but it's a little
> weird that the demux will outlive the webpData (though it needs those pointers
> to remain valid!)

Agree that this is weird...  Don't think it's nicer to preserve our copy of the
WebPData though.

I've added a comment.

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:83: if (!WebPDemuxGetFrame(demux, 1, &frame)) {
On 2016/09/07 14:20:51, scroggo_BACK_FROM_PATERNITY wrote:
> I forget, does this decode the frame? I guess not, since you haven't set the
> output yet. This just gets info?

Yes, this won't decode the image - just sets us up with pointers to the first
frame.  And then we parse the header to get the features.  Though the WebpDemux
has already parsed the header of all the frame... it just doesn't expose all of
the information we want.

I think this redundancy is fine while our decoder is still in flux (need to add
incremental support, animation) - but we may want to request an API change at
some point (once we know what we really want).  I've put some comments in the
code.

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:140: streamDeleter.release(), demux.release(),
std::move(data));
On 2016/09/07 14:20:51, scroggo_BACK_FROM_PATERNITY wrote:
> Sometimes streamDeleter will hold null. I forgot, is that OK? I.e it doesn't
> cause problems for rewinding or anything?

I hadn't thought about this, but now that I look closer, it will be fine.

There's no need to do anything to rewind SkWebpCodec, and we are already doing
nothing.  I've modified a comment in SkCodec.cpp.

https://codereview.chromium.org/2311793004/diff/20001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:286: switch (WebPIUpdate(idec, frame.fragment.bytes,
frame.fragment.size)) {
On 2016/09/07 14:20:51, scroggo_BACK_FROM_PATERNITY wrote:
> On 2016/09/06 23:47:11, msarett wrote:
> > It's kind of ugly to create the WebPIDecoder with no data and then
immediately
> > update it...
> 
> It's the same as we did before though, right?
> 
> > 
> > But I can't see that WebPIDecode() gives us back the status in any way.
> 

Yes, true.  Before it seemed to make a little more sense because we were
appending data in a loop (rather than just once).

I think this is fine though.

[scroggo, 2016-09-07 17:33:04]

lgtm

Doesn't affect decode performance (thought I believe
our performance tests only cover SkStreams with memory
bases

Seems like we should fix that, given that we often have other streams in
practice.

https://codereview.chromium.org/2311793004/diff/60001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/2311793004/diff/60001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:145: // Some codecs do not have a stream.  They may hold
onto their own data or another codec.
So when we pass the stream from webp, we're not doing it because the codec needs
it, but so we can hold onto it? 

We only keep it for a memory stream, so we'll unnecessarily rewind it, but it
will succeed.

That should all be fine then.

[msarett, 2016-09-07 17:36:30]

https://codereview.chromium.org/2311793004/diff/60001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/2311793004/diff/60001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:145: // Some codecs do not have a stream.  They may hold
onto their own data or another codec.
On 2016/09/07 17:33:04, scroggo_BACK_FROM_PATERNITY wrote:
> So when we pass the stream from webp, we're not doing it because the codec
needs
> it, but so we can hold onto it? 
> 

True.

> We only keep it for a memory stream, so we'll unnecessarily rewind it, but it
> will succeed.
> 
> That should all be fine then.

Agreed.  Do you think there's a need to say more in the comments?

[msarett, 2016-09-07 17:36:51]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 17:37:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msarett, 2016-09-07 17:45:42]

On 2016/09/07 17:33:04, scroggo_BACK_FROM_PATERNITY wrote:
> lgtm
> 
> Doesn't affect decode performance (thought I believe
> our performance tests only cover SkStreams with memory
> bases
> 
> Seems like we should fix that, given that we often have other streams in
> practice.
> 

Yes, I think so.

> https://codereview.chromium.org/2311793004/diff/60001/src/codec/SkCodec.cpp
> File src/codec/SkCodec.cpp (right):
> 
>
https://codereview.chromium.org/2311793004/diff/60001/src/codec/SkCodec.cpp#n...
> src/codec/SkCodec.cpp:145: // Some codecs do not have a stream.  They may hold
> onto their own data or another codec.
> So when we pass the stream from webp, we're not doing it because the codec
needs
> it, but so we can hold onto it? 
> 
> We only keep it for a memory stream, so we'll unnecessarily rewind it, but it
> will succeed.
> 
> That should all be fine then.

[commit-bot: I haz the power, 2016-09-07 17:59:32]

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

[commit-bot: I haz the power, 2016-09-07 17:59:32]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-09-07 18:20:14]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-09-07 18:20:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 18:23:30]

Description was changed from

==========
Use demux API in SkWebpCodec

* Necessary to read ICC profiles
* Will be necessary to implement animation
* Requires consolidated data with length

Doesn't affect decode performance (thought I believe
our performance tests only cover SkStreams with memory
bases).

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2311793004
==========

to

==========
Use demux API in SkWebpCodec

* Necessary to read ICC profiles
* Will be necessary to implement animation
* Requires consolidated data with length

Doesn't affect decode performance (thought I believe
our performance tests only cover SkStreams with memory
bases).

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2311793004

Committed:
https://skia.googlesource.com/skia/+/ff2a6c8a70d5f7e3be99ef41c75412878785edcb
==========

[commit-bot: I haz the power, 2016-09-07 18:23:30]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/ff2a6c8a70d5f7e3be99ef41c75412878785edcb