Call rewindIfNeeded in SkCodec

Call rewindIfNeeded in SkCodec

Rather than calling it in each subclass, call it once in the base
class. Call it first, since other steps may modify internal structures
which would be replaced by a call to onRewind.

BUG=skia:4284
Committed: https://skia.googlesource.com/skia/+/3a7701c0101386ba05acdde6f911be0c2696f317

[scroggo, 2015-09-29 17:47:15]

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

[msarett, 2015-09-30 12:19:37]

Rereading the bug description in 4284, it looks like this will not solve that
problem.

Is it still a TODO to check scaling before rewinding?

Or have we decided that we can't check scaling before rewinding because it
doesn't work for JPEG?  I'm assuming that you're referring to JPEG when you say
that other checks modify the internal structures that are set by onRewind().

On second look, I agree that calling rewindIfNeeded() first makes sense.  In
that case, I think we need to rewrite SkScaledCodec to not rely on a call to
start to check for a valid scale.

Another option would be to rewrite the checks in JPEG to not modify the internal
structure until after rewind() is called.  I think this would be doable but a
bit tricky, and probably would make it harder to pull out all of this common
code.

[scroggo, 2015-09-30 12:41:16]

On 2015/09/30 12:19:37, msarett wrote:
> Rereading the bug description in 4284, it looks like this will not solve that
> problem.
> 
> Is it still a TODO to check scaling before rewinding?

Yes. (This CL depends on crrev.com/1365313002, which adds a FIXME to
SkScaledCodec.) I have some changes in progress on my machine to fix that, but
it is a very large change, so I wanted to break out any pieces that are
separable. This is a prerequisite for that change, and I think it also stands on
its own as a good change.
> 
> Or have we decided that we can't check scaling before rewinding because it
> doesn't work for JPEG?  I'm assuming that you're referring to JPEG when you
say
> that other checks modify the internal structures that are set by onRewind().

In reverse:
- Yes, I am referring to JPEG. Maybe I should be more explicit?
- No, we can and should. I just think that's a larger change. This doesn't break
anything (just leaves a piece broken), and gets us closer to the solution to
that problem.

> 
> On second look, I agree that calling rewindIfNeeded() first makes sense.  In
> that case, I think we need to rewrite SkScaledCodec to not rely on a call to
> start to check for a valid scale.

Exactly. In my local changes I have rewritten SkScaledCodec to make a call to
see if the dimensions are supported. If not, it calls start() with the default
dimensions.

> 
> Another option would be to rewrite the checks in JPEG to not modify the
internal
> structure until after rewind() is called.  I think this would be doable but a
> bit tricky, and probably would make it harder to pull out all of this common
> code.

Agreed. I think calling rewind first (and doing a check for the scale first,
eventually) is a more elegant approach.

[msarett, 2015-09-30 12:44:19]

Thanks for your clarifications.  I definitely agree that this stands on its own
as a good change.

lgtm once your other CL lands

[djsollen, 2015-09-30 12:57:41]

public api lgtm

[scroggo, 2015-09-30 16:08:04]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-09-30 16:08:05]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1381483002/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-30 16:08:14]

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

[commit-bot: I haz the power, 2015-09-30 16:15:18]

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