SkScaledCodec should implement onRewind()

SkScaledCodec should implement onRewind()

This is a bug fix.  I'm also adding a test.

BUG=skia:

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

[msarett, 2015-10-02 20:27:29]

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

[msarett, 2015-10-02 20:27:29]

Thoughts on making onRewind() a pure virtual?
Only ICO and WEBP use the default implementation now.

[scroggo, 2015-10-02 20:56:06]

lgtm

On 2015/10/02 20:27:29, msarett wrote:
> Thoughts on making onRewind() a pure virtual?
> Only ICO and WEBP use the default implementation now.

Is the goal to ensure that any new classes we write override it? I guess we
could write new ones in the future (e.g. ktx, astc, for which we have
SkImageDecoder versions that are not used), and it would ensure we override it.

I think it would be fine to make it pure virtual.

https://codereview.chromium.org/1385703002/diff/1/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1385703002/diff/1/tests/CodexTest.cpp#newcode242
tests/CodexTest.cpp:242: if (SkEncodedFormat::kICO_SkEncodedFormat !=
codec->getEncodedFormat()) {
Another way you could do this - not sure if it's better:

You could run this whole block only if (supportsScanlineDecoding), which would
eliminate ICO (until we update ICO)... The awkward part is that you need to also
consider webp, so it becomes

if (supportsScanlineDecoding || supportsSubsetDecoding)

https://codereview.chromium.org/1385703002/diff/1/tests/CodexTest.cpp#newcode247
tests/CodexTest.cpp:247: codec->getPixels(info, bm.getPixels(), bm.rowBytes(),
nullptr, nullptr, nullptr);
nit: This is copied and pasted, so I know it's the same as the other code, but
this could all fit on one line if you call the other version of getPixels and
leave out the three nullptrs.

On that note, maybe this should be factored out into another method? (I
considered it to share with test_info, but wasn't sure it would be better.

Actually, wait, shouldn't this look identical to the non SkScaledCodec version?
This whole block can just be:

(Note: "digest" here means the digest you shadowed, which you do not need to
shadow - just use the same one since it should match the original.)

test_info(r, codec, info, SkCodec::kSuccess, &digest);

// Verify that re-decoding (and thus rewinding) gives the same result
test_info(r, codec, info, SkCodec::kSuccess, &digest);

[msarett, 2015-10-05 14:26:51]

"Is the goal to ensure that any new classes we write override it?"

Yes, I will hold off for now though.

https://codereview.chromium.org/1385703002/diff/1/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1385703002/diff/1/tests/CodexTest.cpp#newcode242
tests/CodexTest.cpp:242: if (SkEncodedFormat::kICO_SkEncodedFormat !=
codec->getEncodedFormat()) {
On 2015/10/02 20:56:06, scroggo wrote:
> Another way you could do this - not sure if it's better:
> 
> You could run this whole block only if (supportsScanlineDecoding), which would
> eliminate ICO (until we update ICO)... The awkward part is that you need to
also
> consider webp, so it becomes
> 
> if (supportsScanlineDecoding || supportsSubsetDecoding)

Done.

https://codereview.chromium.org/1385703002/diff/1/tests/CodexTest.cpp#newcode247
tests/CodexTest.cpp:247: codec->getPixels(info, bm.getPixels(), bm.rowBytes(),
nullptr, nullptr, nullptr);
On 2015/10/02 20:56:06, scroggo wrote:
> nit: This is copied and pasted, so I know it's the same as the other code, but
> this could all fit on one line if you call the other version of getPixels and
> leave out the three nullptrs.
> 
> On that note, maybe this should be factored out into another method? (I
> considered it to share with test_info, but wasn't sure it would be better.
> 
> Actually, wait, shouldn't this look identical to the non SkScaledCodec
version?
> This whole block can just be:
> 
> (Note: "digest" here means the digest you shadowed, which you do not need to
> shadow - just use the same one since it should match the original.)
> 
> test_info(r, codec, info, SkCodec::kSuccess, &digest);
> 
> // Verify that re-decoding (and thus rewinding) gives the same result
> test_info(r, codec, info, SkCodec::kSuccess, &digest);

I will refactor into a different method and check that SkScaledCodec digests
match SkCodec digests.

[scroggo, 2015-10-05 14:42:49]

lgtm

[msarett, 2015-10-05 14:44:27]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-10-05 14:44:38]

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

[commit-bot: I haz the power, 2015-10-05 14:45:39]

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

[commit-bot: I haz the power, 2015-10-05 14:45:39]

Try jobs failed on following builders:
  skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[msarett, 2015-10-05 14:50:23]

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

[msarett, 2015-10-05 14:50:23]

Derek, can you take a look at the API on this?

[msarett, 2015-10-05 21:04:49]

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

[reed1, 2015-10-05 21:10:04]

api lgtm

[msarett, 2015-10-05 21:10:57]

The CQ bit was checked by msarett@google.com

[msarett, 2015-10-05 21:10:58]

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

[commit-bot: I haz the power, 2015-10-05 21:11:16]

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

[commit-bot: I haz the power, 2015-10-05 21:20:30]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/cc7f305c696144455ece3e55082d28f0316bbdc1

[scroggo, 2015-10-20 17:49:22]

On 2015/10/05 21:20:30, commit-bot: I haz the power wrote:
> Committed patchset #3 (id:40001) as
> https://skia.googlesource.com/skia/+/cc7f305c696144455ece3e55082d28f0316bbdc1

I don't think this actually fixes anything.

SkScaledCodec does not have a stream, so we early exit out of rewindIfNeeded
without ever calling onRewind.

I tried removing the override (see crrev.com/1411203005), and the tests added in
this CL still pass.