Create a scanline decoder without creating a codec

Create a scanline decoder without creating a codec

Prior to this CL, if a client wanted to decode scanlines, they had to
create an SkCodec in order to get an SkScanlineDecoder. This introduces
complications if input data is not easily shared between the two
objects.

Instead, add methods to SkScanlineDecoder for creating a new one from
input data, and remove the creation functions from SkCodec.

Update DM and tests.

Committed: https://skia.googlesource.com/skia/+/1c005e4a38e29d648ecebada25d3a718155043a3

[scroggo_chromium, 2015-07-29 21:41:43]

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

[scroggo_chromium, 2015-07-29 21:41:44]

https://codereview.chromium.org/1267583002/diff/20001/include/codec/SkCodec.h
File include/codec/SkCodec.h (left):

https://codereview.chromium.org/1267583002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:226: SkScanlineDecoder* getScanlineDecoder(const
SkImageInfo& dstInfo, const Options* options,
We could keep these methods around, but it's not really any easier than creating
an SkScanlineDecoder from scratch.

[emmaleer, 2015-07-29 22:40:48]

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:603: SkCodec::Result onReset(const SkImageInfo&
dstInfo,
Is reset required to be called, for the ScanlineDecoder to work?

Could we call move what's in this function to NewSDFromStream, so then the
ScanlineDecoder would work right away, and reset would only be needed if the
user wanted to update the dstInfo to something else.
This would require NewSDFromStream to take extra parameters.

I think if reset is required, it's going to have to be called right after
NewSDFromStream anyways, so we might as well make them one function. 
Or is there a different reason we don't want to pass the extra parameters to
NewSDFromStream?

If we want to keep the reset function, maybe we should rename is start.. (sorry
I know I changed your mind earlier from start to reset in the first place, but I
didn't know reset would be required to be called)

[msarett, 2015-07-30 12:58:46]

I'm happy with this landing + a few minor comments from Emmalee and myself.  I
think this is definitely a big improvement over how we used to create scanline
decoders.

I'm still not really convinced that this is better than merging
SkScanlineDecoder and SkCodec into one object.
1) In NewFromSDStream, we still need to create a codec to create scanline
decoder.  And then the scanline decoder owns a codec.  I guess since we hide
this from the client, maybe it's not that confusing?
2) Also it seems that SkScanlineDecoder.cpp is quite similar to SkCodec.cpp in
some places.

I know that you started to implement these in the same class and then decided to
keep them as separate objects.  I'm trying to guess what kinds of problems you
ran into.  Maybe the reset() API was awkward because it's not needed for
onGetPixels()?  Maybe the scanline decoders keep a bunch of fields that the
codecs don't need?  I'm sure that you have a better understanding of this than
me since you've worked through it.  I think it would be helpful for me to
understand what convinced you that this was the right path.

https://codereview.chromium.org/1267583002/diff/20001/include/codec/SkScanlin...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1267583002/diff/20001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:80: SkCodec::Result reset(const SkImageInfo&
dstInfo);
So can we get away with not calling reset() at all if the dstColorType is not
kIndex8 and we don't need to pass any options in?

One comments says:
"reset must be called in order to decode any scanlines"

Another says:
"If reset has never been called, this is the info that will be used."

I think I understand it after reading the comments over this function, but maybe
we can be a little more consistent throughout this file?

https://codereview.chromium.org/1267583002/diff/20001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:92: if (fDstInfo.isEmpty()) {
Maybe there is a way to make to this check once rather than every time we call
getScanlines()?  I'm guessing the info reported by NewFromStream will never be
empty, since we are responsible for filling it in?  And then if we also check
that the info is not empty in reset(), can we avoid this check?

https://codereview.chromium.org/1267583002/diff/20001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:113: if (fDstInfo.isEmpty()) {
Same question as getScanlines().

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:603: SkCodec::Result onReset(const SkImageInfo&
dstInfo,
On 2015/07/29 22:40:48, emmaleer wrote:
> Is reset required to be called, for the ScanlineDecoder to work?
> 
> Could we call move what's in this function to NewSDFromStream, so then the
> ScanlineDecoder would work right away, and reset would only be needed if the
> user wanted to update the dstInfo to something else.
> This would require NewSDFromStream to take extra parameters.
> 
> I think if reset is required, it's going to have to be called right after
> NewSDFromStream anyways, so we might as well make them one function. 
> Or is there a different reason we don't want to pass the extra parameters to
> NewSDFromStream?
> 
> If we want to keep the reset function, maybe we should rename is start..
(sorry
> I know I changed your mind earlier from start to reset in the first place, but
I
> didn't know reset would be required to be called)

NewSDFromStream sets an SkImageInfo with the recommended settings (colorType,
alphaType, dimensions) for the decode.  Then the client can look at these
recommended settings and ask for the type of decode that is desired.

I think asking the client to know all of the information before the call to
NewSDFromStream is a little too much to ask.  They would need to know output
dimensions, color type, alpha type, and if they want a color table all without
any information from the codec.

Looking at dm probably makes this a little confusing because we get the
recommended info from Codec at the top and then modify the info to what we want,
so when we create the scanline decoder, we are ready to call reset() right away
because we have already decided what dstInfo we want.

Having said that, I agree that it would be better to not have to call reset()
before getScanlines().  I just can't think of a way to do it that is better than
what we are already doing.

I would be in favor of SkScanlineDecoder having an additional API called start()
which calls reset().  That way we can call start() to set up a scanline decoder
and reset() to reuse one.

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:781: SkAutoTDelete<SkPngCodec> codec
(static_cast<SkPngCodec*>(SkPngCodec::NewFromStream(stream)));
I'm noticing that we still need to create a codec in order to create a scanline
decoder, but now we hide it from the client.  I think this is an improvement
over how we did things before.

[msarett, 2015-07-30 13:05:37]

Oh I forgot to mention something!  I think this is missing some changes to
nanobench.cpp and the subset benchmarks.

[scroggo, 2015-07-30 15:11:35]

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

[scroggo, 2015-07-30 15:11:35]

On 2015/07/30 12:58:46, msarett wrote:
> I'm happy with this landing + a few minor comments from Emmalee and myself.  I
> think this is definitely a big improvement over how we used to create scanline
> decoders.
> 
> I'm still not really convinced that this is better than merging
> SkScanlineDecoder and SkCodec into one object.

I also have a CL which does that, I just have not uploaded it. I had been
mulling it over in my head. The awkward part about that option is that the
single object has two modes which are somewhat incompatible. If you start
reading scanlines, but then read the whole image (i.e. call getPixels) we can do
it, although we'll need to rewind, and then we've lost your place in scanline
decoding (unless we rewound again and then skipped to your place? That might be
unnecessary work though.). OTOH, one could argue that that is a dumb client.

Now, you also pointed out that other APIs do in fact work this way. You made a
comment that we do not need to do what other people's APIs do, which is correct,
but I do want to reiterate that we should definitely think about other APIs -
they've already thought about this stuff. While they may not be 100% right, we
can at least learn from them.

And yes, png_struct for example does support both methods of reading, so maybe
it's not so bad.

> 1) In NewFromSDStream, we still need to create a codec to create scanline
> decoder.  And then the scanline decoder owns a codec.  I guess since we hide
> this from the client, maybe it's not that confusing?

As mentioned in your other comment, this is an implementation detail. The
important thing is that the client is able to directly create a scanline decoder
without doing extra work. They also do not need to duplicate the stream or
reread the header. The former is important for Android's streams which we may
not be able to rewind/duplicate, and the latter is just extra work.

> 2) Also it seems that SkScanlineDecoder.cpp is quite similar to SkCodec.cpp in
> some places.

This is very true. This would be an argument for merging them. Another option I
have thought about is having a base class with the common functionality, and
SkCodec will be one subclass (abstract) and SkScanlineDecoder will be another.
I'm not sure they share that many functions though. They share getInfo, but
their other functions just look very similar.

> 
> I know that you started to implement these in the same class and then decided
to
> keep them as separate objects.  I'm trying to guess what kinds of problems you
> ran into.  Maybe the reset() API was awkward because it's not needed for
> onGetPixels()?

That is a big piece of it yes.

> Maybe the scanline decoders keep a bunch of fields that the
> codecs don't need? 

That is also true. It's nice that we can keep all of the scanline decoding
fields isolated to where they are used.

> I'm sure that you have a better understanding of this than
> me since you've worked through it.  I think it would be helpful for me to
> understand what convinced you that this was the right path.

Let me know if you want me to upload the other CL and we can compare. I'm also
happy to have a meeting if you'd like to discuss further.

On 2015/07/30 13:05:37, msarett wrote:
> Oh I forgot to mention something!  I think this is missing some changes to
> nanobench.cpp and the subset benchmarks.

Yes, I left that out. I've uploaded a new CL which fixes nanobench.

https://codereview.chromium.org/1267583002/diff/20001/include/codec/SkScanlin...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1267583002/diff/20001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:80: SkCodec::Result reset(const SkImageInfo&
dstInfo);
On 2015/07/30 12:58:46, msarett wrote:
> 
> One comments says:
> "reset must be called in order to decode any scanlines"
> 
> Another says:
> "If reset has never been called, this is the info that will be used."
> 
> I think I understand it after reading the comments over this function, but
maybe
> we can be a little more consistent throughout this file?
> 

Agreed. When I originally wrote the header, I made calling reset unnecessary if
you wanted to use the defaults. I later realized that that would require doing
work that is unnecessary if you did *not* want to use the defaults. I changed
some comments, but did not change them all :(

Thank you for diligently reading over the comments!

> So can we get away with not calling reset() at all if the dstColorType is not
> kIndex8 and we don't need to pass any options in?

Haha, good point - if we stuck with the notion of allowing getting scanlines
without calling reset, kIndex_8 would need to be the exception.

https://codereview.chromium.org/1267583002/diff/20001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:92: if (fDstInfo.isEmpty()) {
On 2015/07/30 12:58:46, msarett wrote:
> Maybe there is a way to make to this check once rather than every time we call
> getScanlines()?  I'm guessing the info reported by NewFromStream will never be
> empty, since we are responsible for filling it in?  And then if we also check
> that the info is not empty in reset(), can we avoid this check?

Actually, what I was trying to check was that you *had* called reset.

Alternatively, we could just say: you better not call this without calling
reset, and assert this (instead of having this check). That would be faster in
release, and catch someone in debug trying to do something invalid.

https://codereview.chromium.org/1267583002/diff/20001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:113: if (fDstInfo.isEmpty()) {
On 2015/07/30 12:58:46, msarett wrote:
> Same question as getScanlines().

Acknowledged.

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:603: SkCodec::Result onReset(const SkImageInfo&
dstInfo,
On 2015/07/29 22:40:48, emmaleer wrote:
> Is reset required to be called, for the ScanlineDecoder to work?

Yes. I considered making reset unnecessary, but if you did *not* want the
default settings, the constructor would do unnecessary work.

> 
> Could we call move what's in this function to NewSDFromStream, so then the
> ScanlineDecoder would work right away, and reset would only be needed if the
> user wanted to update the dstInfo to something else.
> This would require NewSDFromStream to take extra parameters.

We could. One weird thing, as Matt points out as well, is that we do not know
all the pertinent information. We could know whether our memory was zero
initialized, but the dimensions and whether we need a color table would have to
wait until we have some info. (In some ways, this justifies the old two step
creation process for a scanline decoder.)

> 
> I think if reset is required, it's going to have to be called right after
> NewSDFromStream anyways, so we might as well make them one function. 
> Or is there a different reason we don't want to pass the extra parameters to
> NewSDFromStream?

Just the above. 

> 
> If we want to keep the reset function, maybe we should rename is start..
(sorry
> I know I changed your mind earlier from start to reset in the first place, but
I
> didn't know reset would be required to be called)

I think either way we have a somewhat weird name. If we had two functions, they
would do the same thing (though start could set a couple less variables, if we
set them in the constructor). If start() can be called to start over, maybe
that's less weird than calling reset() to start?

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:781: SkAutoTDelete<SkPngCodec> codec
(static_cast<SkPngCodec*>(SkPngCodec::NewFromStream(stream)));
On 2015/07/30 12:58:46, msarett wrote:
> I'm noticing that we still need to create a codec in order to create a
scanline
> decoder, but now we hide it from the client.  I think this is an improvement
> over how we did things before.

This is an implementation detail, though. We *could* create some sort of
isolated state machine, which is owned by the codec and the scanline decoder,
but when the scanline decoders were written, we already had codecs with state
machines that were not easily separable, so we just reused them.

The key difference here is that we do not need to do any duplication or
rereading of the header, like we did in the old world.

[msarett, 2015-07-30 15:37:55]

Alright I'm sold!

"The awkward part about that option is that the
single object has two modes which are somewhat incompatible. If you start
reading scanlines, but then read the whole image (i.e. call getPixels) we can do
it, although we'll need to rewind, and then we've lost your place in scanline
decoding (unless we rewound again and then skipped to your place? That might be
unnecessary work though.). OTOH, one could argue that that is a dumb client."

I think it's worth noting that we could completely disable calling onGetPixels()
when using scanline mode and vice versa, and still have the same amount of
flexibility that we have with this design.  But now that I've written that
thought down, it seems like an argument for this design.

My last minor concern is if there might be a situation where we don't know at
the NewFromStream stage whether we want a codec or a scanline decoder.  But I
think Emmalee and I discussed that you can just always create the scanline
decoder and getScanlines(all) in this case.

lgtm

https://codereview.chromium.org/1267583002/diff/20001/include/codec/SkScanlin...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1267583002/diff/20001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:92: if (fDstInfo.isEmpty()) {
On 2015/07/30 15:11:35, scroggo wrote:
> On 2015/07/30 12:58:46, msarett wrote:
> > Maybe there is a way to make to this check once rather than every time we
call
> > getScanlines()?  I'm guessing the info reported by NewFromStream will never
be
> > empty, since we are responsible for filling it in?  And then if we also
check
> > that the info is not empty in reset(), can we avoid this check?
> 
> Actually, what I was trying to check was that you *had* called reset.
> 
> Alternatively, we could just say: you better not call this without calling
> reset, and assert this (instead of having this check). That would be faster in
> release, and catch someone in debug trying to do something invalid.

+1 for using SkASSERT.

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:603: SkCodec::Result onReset(const SkImageInfo&
dstInfo,
On 2015/07/30 15:11:35, scroggo wrote:
> On 2015/07/29 22:40:48, emmaleer wrote:
> > Is reset required to be called, for the ScanlineDecoder to work?
> 
> Yes. I considered making reset unnecessary, but if you did *not* want the
> default settings, the constructor would do unnecessary work.
> 
> > 
> > Could we call move what's in this function to NewSDFromStream, so then the
> > ScanlineDecoder would work right away, and reset would only be needed if the
> > user wanted to update the dstInfo to something else.
> > This would require NewSDFromStream to take extra parameters.
> 
> We could. One weird thing, as Matt points out as well, is that we do not know
> all the pertinent information. We could know whether our memory was zero
> initialized, but the dimensions and whether we need a color table would have
to
> wait until we have some info. (In some ways, this justifies the old two step
> creation process for a scanline decoder.)
> 
> > 
> > I think if reset is required, it's going to have to be called right after
> > NewSDFromStream anyways, so we might as well make them one function. 
> > Or is there a different reason we don't want to pass the extra parameters to
> > NewSDFromStream?
> 
> Just the above. 
> 
> > 
> > If we want to keep the reset function, maybe we should rename is start..
> (sorry
> > I know I changed your mind earlier from start to reset in the first place,
but
> I
> > didn't know reset would be required to be called)
> 
> I think either way we have a somewhat weird name. If we had two functions,
they
> would do the same thing (though start could set a couple less variables, if we
> set them in the constructor). If start() can be called to start over, maybe
> that's less weird than calling reset() to start?

Did you have thoughts on having both a start() and a reset() API where start()
calls reset()?  Maybe this is even more confusing?

[emmaleer, 2015-07-30 15:49:48]

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:603: SkCodec::Result onReset(const SkImageInfo&
dstInfo,
On 2015/07/30 15:37:55, msarett wrote:
> On 2015/07/30 15:11:35, scroggo wrote:
> > On 2015/07/29 22:40:48, emmaleer wrote:
> > > Is reset required to be called, for the ScanlineDecoder to work?
> > 
> > Yes. I considered making reset unnecessary, but if you did *not* want the
> > default settings, the constructor would do unnecessary work.
> > 
> > > 
> > > Could we call move what's in this function to NewSDFromStream, so then the
> > > ScanlineDecoder would work right away, and reset would only be needed if
the
> > > user wanted to update the dstInfo to something else.
> > > This would require NewSDFromStream to take extra parameters.
> > 
> > We could. One weird thing, as Matt points out as well, is that we do not
know
> > all the pertinent information. We could know whether our memory was zero
> > initialized, but the dimensions and whether we need a color table would have
> to
> > wait until we have some info. (In some ways, this justifies the old two step
> > creation process for a scanline decoder.)
> > 
> > > 
> > > I think if reset is required, it's going to have to be called right after
> > > NewSDFromStream anyways, so we might as well make them one function. 
> > > Or is there a different reason we don't want to pass the extra parameters
to
> > > NewSDFromStream?
> > 
> > Just the above. 
> > 
> > > 
> > > If we want to keep the reset function, maybe we should rename is start..
> > (sorry
> > > I know I changed your mind earlier from start to reset in the first place,
> but
> > I
> > > didn't know reset would be required to be called)
> > 
> > I think either way we have a somewhat weird name. If we had two functions,
> they
> > would do the same thing (though start could set a couple less variables, if
we
> > set them in the constructor). If start() can be called to start over, maybe
> > that's less weird than calling reset() to start?
> 
> Did you have thoughts on having both a start() and a reset() API where start()
> calls reset()?  Maybe this is even more confusing?

I think calling reset to start is less intuitive than calling start to start.
Also, since the function will mostly be used to start, not to reset, I think
start is a better name. It may occasionally be used to reset. 
We could also call it setInfo() or something that describes what it does.

[scroggo, 2015-07-30 16:48:48]

Latest patch set renames reset to start. (Luckily, the two have the same number
of letters, making the diff simple - no need to change line breaks or anything
:)

On 2015/07/30 15:37:55, msarett wrote:
> Alright I'm sold!
> 
> "The awkward part about that option is that the
> single object has two modes which are somewhat incompatible. If you start
> reading scanlines, but then read the whole image (i.e. call getPixels) we can
do
> it, although we'll need to rewind, and then we've lost your place in scanline
> decoding (unless we rewound again and then skipped to your place? That might
be
> unnecessary work though.). OTOH, one could argue that that is a dumb client."
> 
> I think it's worth noting that we could completely disable calling
onGetPixels()
> when using scanline mode and vice versa, and still have the same amount of
> flexibility that we have with this design.  But now that I've written that
> thought down, it seems like an argument for this design.

This is true. (In fact, we originally had that model, even though they were two
separate classes...)

> 
> My last minor concern is if there might be a situation where we don't know at
> the NewFromStream stage whether we want a codec or a scanline decoder.  But I
> think Emmalee and I discussed that you can just always create the scanline
> decoder and getScanlines(all) in this case.

That is an argument for combining the two, in my opinion...

getPixels could in theory be implemented as getScanlines(all)

Any idea what situation that would be? Maybe inside the scaled codec?

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1267583002/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:603: SkCodec::Result onReset(const SkImageInfo&
dstInfo,
On 2015/07/30 15:49:48, emmaleer wrote:
> On 2015/07/30 15:37:55, msarett wrote:
> > On 2015/07/30 15:11:35, scroggo wrote:
> > > On 2015/07/29 22:40:48, emmaleer wrote:
> > > > Is reset required to be called, for the ScanlineDecoder to work?
> > > 
> > > Yes. I considered making reset unnecessary, but if you did *not* want the
> > > default settings, the constructor would do unnecessary work.
> > > 
> > > > 
> > > > Could we call move what's in this function to NewSDFromStream, so then
the
> > > > ScanlineDecoder would work right away, and reset would only be needed if
> the
> > > > user wanted to update the dstInfo to something else.
> > > > This would require NewSDFromStream to take extra parameters.
> > > 
> > > We could. One weird thing, as Matt points out as well, is that we do not
> know
> > > all the pertinent information. We could know whether our memory was zero
> > > initialized, but the dimensions and whether we need a color table would
have
> > to
> > > wait until we have some info. (In some ways, this justifies the old two
step
> > > creation process for a scanline decoder.)
> > > 
> > > > 
> > > > I think if reset is required, it's going to have to be called right
after
> > > > NewSDFromStream anyways, so we might as well make them one function. 
> > > > Or is there a different reason we don't want to pass the extra
parameters
> to
> > > > NewSDFromStream?
> > > 
> > > Just the above. 
> > > 
> > > > 
> > > > If we want to keep the reset function, maybe we should rename is start..
> > > (sorry
> > > > I know I changed your mind earlier from start to reset in the first
place,
> > but
> > > I
> > > > didn't know reset would be required to be called)
> > > 
> > > I think either way we have a somewhat weird name. If we had two functions,
> > they
> > > would do the same thing (though start could set a couple less variables,
if
> we
> > > set them in the constructor). If start() can be called to start over,
maybe
> > > that's less weird than calling reset() to start?
> > 
> > Did you have thoughts on having both a start() and a reset() API where
start()
> > calls reset()?  Maybe this is even more confusing?
> 
> I think calling reset to start is less intuitive than calling start to start.
> Also, since the function will mostly be used to start, not to reset, I think
> start is a better name. It may occasionally be used to reset. 
> We could also call it setInfo() or something that describes what it does. 

FWIW, I do not think setInfo is a good name. It sets the destination info (while
leaving the src info unchanged), but it does a lot more than that.

I am not strongly opposed to having two names, but I would prefer to have one,
just for simplicity.

[msarett, 2015-07-30 17:43:08]

"Any idea what situation that would be? Maybe inside the scaled codec?"

Yes the situation we discussed was SkScaledCodec.  But we can get around it
using getScanlines(all).

"getPixels could in theory be implemented as getScanlines(all)"

Maybe we don't need the SkCodec class at all?

[scroggo, 2015-07-30 18:07:18]

On 2015/07/30 17:43:08, msarett wrote:
> "Any idea what situation that would be? Maybe inside the scaled codec?"
> 
> Yes the situation we discussed was SkScaledCodec.  But we can get around it
> using getScanlines(all).
> 
> "getPixels could in theory be implemented as getScanlines(all)"
> 
> Maybe we don't need the SkCodec class at all?

In some sense, SkCodec is a convenience. That said, it might be more efficient
in some cases.

[scroggo, 2015-07-30 18:09:19]

Derek, I'll need an API review for this.

I know we discussed merging SkCodec and SkScanlineDecoder, but I ended up
separating them instead. I think this has some advantages over merging them (it
avoids the intermixing of getPixels and getScanlines, as we discussed), although
I could be convinced that merging is better.

[djsollen, 2015-08-03 13:22:46]

lgtm

API lgtm.  I'm in favor of the separation that makes each individual operation
clear.

[scroggo_chromium, 2015-08-03 18:35:58]

https://codereview.chromium.org/1267583002/diff/100001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1267583002/diff/100001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:395: , fOpts()
The latest patch is a rebase, but I had to make some changes around
SkJpegScanlineDecoder::fOpts.

Previously, it was a const reference. (This is a bad idea anyway, since we do
not know how long the input object will last.)

It is no longer const, and it is no longer a reference. It cannot be const,
since start() (which calls this class's onStart()) may change the options. We
now default construct it in our constructor, and update it in onStart.

[msarett, 2015-08-03 19:21:35]

lgtm

[scroggo, 2015-08-04 13:16:25]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-08-04 13:16:26]

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

[commit-bot: I haz the power, 2015-08-04 13:16:28]

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

[commit-bot: I haz the power, 2015-08-04 13:19:58]

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

[commit-bot: I haz the power, 2015-08-04 13:19:59]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[scroggo, 2015-08-04 15:33:59]

Emmalee, would you mind looking over my changes to the interlaced scanline
decoder?

I had a bug around rewinding, and I think I have fixed it, but maybe there's a
better way?

[scroggo, 2015-08-04 16:10:34]

On 2015/08/04 15:33:59, scroggo wrote:
> Emmalee, would you mind looking over my changes to the interlaced scanline
> decoder?
> 
> I had a bug around rewinding, and I think I have fixed it, but maybe there's a
> better way?

I got a verbal approval from Emmalee.

[scroggo, 2015-08-04 16:10:38]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-08-04 16:10:39]

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/1267583002/#ps140001
(title: "Fix interlaced")

[commit-bot: I haz the power, 2015-08-04 16:10:47]

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

[commit-bot: I haz the power, 2015-08-04 16:24:49]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/1c005e4a38e29d648ecebada25d3a718155043a3