Issue 1260673002: SkScaledCodec class

emmaleer

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

5 years, 4 months ago (2015-07-27 14:17:13 UTC) #1

scroggo

General comment: I love how much simpler this is than putting the sampling inside each ...

5 years, 4 months ago (2015-07-27 15:09:20 UTC) #3

emmaleer

https://codereview.chromium.org/1260673002/diff/1/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1260673002/diff/1/include/codec/SkCodec.h#newcode152 include/codec/SkCodec.h:152: , fScaled(0) On 2015/07/27 15:09:19, scroggo wrote: > nit: ...

5 years, 4 months ago (2015-07-27 18:31:39 UTC) #4

https://codereview.chromium.org/1260673002/diff/1/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1260673002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:152: , fScaled(0)
On 2015/07/27 15:09:19, scroggo wrote:
> nit: false

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:165: bool            fScaled;
On 2015/07/27 15:09:19, scroggo wrote:
> please add comments explaining how this is used (and its default value)

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/include/codec/SkCodec.h#new...
include/codec/SkCodec.h:249: bool isInterlaced() {
On 2015/07/27 15:09:19, scroggo wrote:
> Please add comments, here and on the virtual method.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/include/codec/SkScaledCodec.h
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1260673002/diff/1/include/codec/SkScaledCodec...
include/codec/SkScaledCodec.h:22: static SkCodec* NewFromStream(SkStream*);
On 2015/07/27 15:09:19, scroggo wrote:
> This (and other functions) should be indented four characters, rather than
> eight.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/include/codec/SkScaledCodec...
include/codec/SkScaledCodec.h:46: SkAutoTDelete<SkCodec>   fCodec;
On 2015/07/27 15:09:19, scroggo wrote:
> These fields should also be indented only four characters.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/include/codec/SkScaledCodec...
include/codec/SkScaledCodec.h:47: int                      fSampleY;   // step
between Y samples. Is constant
On 2015/07/27 15:09:19, scroggo wrote:
> These variables do not appear to be const.
> 
> They are set each time onGetPixels is called.
> 
> Therefore, they do not even need to be member variables. You can just declare
> them inside onGetPixels. (In that case, they *can* be const, since they will
be
> const for their entire lifetimes - the rest of the function they are in.)

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/include/codec/SkScanlineDec...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1260673002/diff/1/include/codec/SkScanlineDec...
include/codec/SkScanlineDecoder.h:78: bool setSampleX(int sampleX) {
On 2015/07/27 15:09:19, scroggo wrote:
> Please add comments explaining what this does.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/include/codec/SkScanlineDec...
include/codec/SkScanlineDecoder.h:83: SkScanlineDecoder(const SkImageInfo&
requested, const SkImageInfo src)
On 2015/07/27 15:09:19, scroggo wrote:
> This should be a const reference. That way we do not do a copy when passing
the
> parameter. (The field needs to not be a reference, and we'll still do a copy
> when it is initialized.)

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkCodec_libpng.cpp
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkCodec_libpng.cp...
src/codec/SkCodec_libpng.cpp:428: if (1 ==
png_set_interlace_handling(fPng_ptr)){
On 2015/07/27 15:09:19, scroggo wrote:
> You actually do not need a conditional here. You can just say:
> 
> return 1 != png_set_interlace_handling(fPng_ptr);
> 
> Maybe add a comment that we can not depend on fNumberPasses, since it may not
> have been set yet.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkCodec_libpng.cp...
src/codec/SkCodec_libpng.cpp:749: if(options.fScaled == false) {
On 2015/07/27 15:09:19, scroggo wrote:
> Please add a comment explaining this. Something like:
> 
> // If the caller is sampling, it is okay for the dimensions to not match.
> 
> (Which makes me wonder if the variable should be called fSampled?)
> 
> That said, IIUC, the width may be smaller (due to sampling), but it needs to
be
> a supported width, and the height should be the same?

Changed to fSampled.

For normal images the dstHeight and dstWidth can both be smaller. These are the
dimensions the user requests. They are checked in SkScaledCodec.cpp in
onGetPixels() by calling scaling_supported()

For interlaced images, the dstHeight must be the same. (as we do the y sampling
after)
In SkScaledCodec we set the dstHeight = srcHeight if the image is interlaced,
before creating the scanline decoder.

I could add a check saying: If interlaced and sampled, make sure dstHeight and
srcHeight are equal.
But since SkScaledCodec previously set them equal, I don't think it's really
necessary.
Let me know if you think it's still beneficial, and I can add the check.

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkScaledCodec.cpp
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkScaledCodec.cpp...
src/codec/SkScaledCodec.cpp:14: SkAutoTDelete<SkCodec>
codec(SkCodec::NewFromStream(stream));
On 2015/07/27 15:09:19, scroggo wrote:
> This should be indented only four characters.
> 
> A lot of text editors let you customize how many character indents it should
> use. I'm not familiar with Sublime (which I believe you are using), but it
> should have a way to set it (I'm guessing it is fighting with you to choose
the
> spacing?).

Yes, it's usually set to 4 spaced per indent.. I think because I created a new
file it defaulted to 8. I've fixed the spacing issues now.

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkScaledCodec.cpp...
src/codec/SkScaledCodec.cpp:18: return SkNEW_ARGS(SkScaledCodec,
(codec.detach()));
On 2015/07/27 15:09:19, scroggo wrote:
> Since SkWebpCodec supports subsets and scaling natively, I think it would make
> sense to check the encoded type, and return the codec itself if it is webp.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkScaledCodec.cpp...
src/codec/SkScaledCodec.cpp:37: * TODO: This should be the default
implementation once the other codecs support this.
On 2015/07/27 15:09:19, scroggo wrote:
> This TODO is no longer needed.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkScaledCodec.cpp...
src/codec/SkScaledCodec.cpp:97: if (kInvalidScale != result) {
On 2015/07/27 15:09:19, scroggo wrote:
> I wonder if there are other cases this should handle in the future.
> 
> Once we add subset decoding to this class, it needs to consider a return value
> that indicated that fCodec could not handle the subset. Currently, we return
> kUnimplemented. (I think it is currently only used for subsets, but if we
wanted
> to use it for something else, we may need to distinguish between types of
> unimplemented.)
> 
> On the other hand, if the factory returns an SkWebpCodec directly, then fCodec
> will never support subsetting, so we could skip calling fCodec->getPixels if
> options.fSubset is not NULL (not sure whether adding that check would be
better
> than just waiting for getPixels to return kUnimplemented, though).

Maybe we should add a return value of kInvalidSubset
We could check if options.fSubset == NULL before calling getPixels
Calling getPixels is not very expensive if it fails, as it only check a bunch of
if statements at the beginning.
That being said, it would still be faster without those if statements.

I'm wondering about the case where we are sampling and request a subset (is this
going be be supported?)
Maybe we should wait until implementing Subset Decoding to worry about this.

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkScaledCodec.cpp...
src/codec/SkScaledCodec.cpp:135: if (!isInterlaced) {
On 2015/07/27 15:09:19, scroggo wrote:
> nit: I tend to prefer:
> 
> if (condition) {
> 
> } else {
>   // !condition
> }
> 
> over:
> 
> if (!condition) {
> 
> } else {
>   // condition
> }
> 
> Not sure whether others have this preference, but I find it more readable that
> way.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:206: *  set the sample size in the x direction
On 2015/07/27 15:09:19, scroggo wrote:
> Must be >= 1

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:244: const int           fDeltaSrc;        // if
bitsPerPixel % 8 == 0
On 2015/07/27 15:09:20, scroggo wrote:
> Maybe there is a better name for this than "fDeltaSrc", which is intended to
> mean "delta between source pixels". Now the true delta ends up being fDeltaSrc
*
> fSampleX. I haven't come up with a better name yet, though.

Well fDeltaSrc is always the bytes or bits between pixels.
Then, we pass fDeltaSrc * fSampleX to next(), which is the real deltaSrc.
What about calling fDeltaSrc fDeltaPixels, or fPixelDelta?
I think having pixel in the name would make sense, what do you think?

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:252: int                 fX0;              // first X
coord to sample. Is constant
On 2015/07/27 15:09:19, scroggo wrote:
> These are actually not const - they are modified inside setSampleX

Acknowledged.

scroggo

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkCodec_libpng.cpp File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1260673002/diff/1/src/codec/SkCodec_libpng.cpp#newcode749 src/codec/SkCodec_libpng.cpp:749: if(options.fScaled == false) { On 2015/07/27 18:31:38, emmaleer wrote: ...

5 years, 4 months ago (2015-07-27 19:29:57 UTC) #5

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkCodec_libpng.cpp
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkCodec_libpng.cp...
src/codec/SkCodec_libpng.cpp:749: if(options.fScaled == false) {
On 2015/07/27 18:31:38, emmaleer wrote:
> On 2015/07/27 15:09:19, scroggo wrote:
> > Please add a comment explaining this. Something like:
> > 
> > // If the caller is sampling, it is okay for the dimensions to not match.
> > 
> > (Which makes me wonder if the variable should be called fSampled?)
> > 
> > That said, IIUC, the width may be smaller (due to sampling), but it needs to
> be
> > a supported width, and the height should be the same?
> 
> Changed to fSampled.
> 
> For normal images the dstHeight and dstWidth can both be smaller. These are
the
> dimensions the user requests. They are checked in SkScaledCodec.cpp in
> onGetPixels() by calling scaling_supported()

Does the SkCodec need to know that dstHeight is smaller though? It seems like
from this SkCodec's perspective, the dstHeight is unchanged - the caller needs
to call skip/get scanline for each scanline in the original height, right?

> 
> For interlaced images, the dstHeight must be the same. (as we do the y
sampling
> after)
> In SkScaledCodec we set the dstHeight = srcHeight if the image is interlaced,
> before creating the scanline decoder.
> 
> I could add a check saying: If interlaced and sampled, make sure dstHeight and
> srcHeight are equal.
> But since SkScaledCodec previously set them equal, I don't think it's really
> necessary.
> Let me know if you think it's still beneficial, and I can add the check. 

If there is a condition that we assume in a function, then that is a place we
can add an assert. It checks that you were right about your assumption, and
allows a reader to know you assume that.

That said, the info passed in comes from a public function, so someone could
call with an invalid value, right?

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkScaledCodec.cpp
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkScaledCodec.cpp...
src/codec/SkScaledCodec.cpp:97: if (kInvalidScale != result) {
On 2015/07/27 18:31:38, emmaleer wrote:
> On 2015/07/27 15:09:19, scroggo wrote:
> > I wonder if there are other cases this should handle in the future.
> > 
> > Once we add subset decoding to this class, it needs to consider a return
value
> > that indicated that fCodec could not handle the subset. Currently, we return
> > kUnimplemented. (I think it is currently only used for subsets, but if we
> wanted
> > to use it for something else, we may need to distinguish between types of
> > unimplemented.)
> > 
> > On the other hand, if the factory returns an SkWebpCodec directly, then
fCodec
> > will never support subsetting, so we could skip calling fCodec->getPixels if
> > options.fSubset is not NULL (not sure whether adding that check would be
> better
> > than just waiting for getPixels to return kUnimplemented, though).
> 
> Maybe we should add a return value of kInvalidSubset
> We could check if options.fSubset == NULL before calling getPixels
> Calling getPixels is not very expensive if it fails, as it only check a bunch
of
> if statements at the beginning.
> That being said, it would still be faster without those if statements.
> 
> I'm wondering about the case where we are sampling and request a subset (is
this
> going be be supported?)

Yes. This is currently supported by BitmapRegionDecoder, so we need to support
it somehow.

> Maybe we should wait until implementing Subset Decoding to worry about this. 

Agreed, but it would be nice to have a TODO/FIXME.

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:244: const int           fDeltaSrc;        // if
bitsPerPixel % 8 == 0
On 2015/07/27 18:31:38, emmaleer wrote:
> On 2015/07/27 15:09:20, scroggo wrote:
> > Maybe there is a better name for this than "fDeltaSrc", which is intended to
> > mean "delta between source pixels". Now the true delta ends up being
fDeltaSrc
> *
> > fSampleX. I haven't come up with a better name yet, though.
> 
> Well fDeltaSrc is always the bytes or bits between pixels.
> Then, we pass fDeltaSrc * fSampleX to next(), which is the real deltaSrc.
> What about calling fDeltaSrc fDeltaPixels, or fPixelDelta?
> I think having pixel in the name would make sense, what do you think?

Maybe. I'm fine to leave it as-is for now. It is still the delta not including
sampling.

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

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:166: * If true, the codec is being sampled
This could be more concise, e.g.

Whether the codec is being sampled.

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:169: * Used to determine if dstDimensions can be
different than srcDimensions
To be fair, dstDimensions can be different than srcDimensions in Jpeg or WebP,
without setting fSampled to true.

I think this is only used for sampling in the X direction? Maybe it should even
be an integer, representing the sample size in the x direction? I think that
would allow us to remove setSampleX, since it would always be set at the outset.

This might also let us check to make sure the dst width matches the desired
sampling.

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:256: * returns true if the image is interlaced
These two lines can be more concise, e.g.

  Returns whether the image is interlaced

I think it would also be good to explain why a client would want to call it.
e.g.

For an interlaced image, calling getScanlines once (regardless of the count
used) needs to read the entire image, therefore it is inefficient to call
getScanlines more than once. Instead, it should only ever be called with all the
rows needed.

I don't know if we want to be so explicit about the implementation, but I tend
to think we should say something about why someone might want to call it.

I also wonder if this makes more sense to be on the SkScanlineDecoder (this
question is moot if we merge it with SkCodec).

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

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:80: * returns true if successful
Returns whether sampleX is supported.

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

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:428: // we cannot depend on fNumberPasses, since it
may not have been set yet
If we moved this onto the scanline decoder, we would already know it.

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

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:15: bool isWebp = SkWebpCodec::IsWebp(stream);
I'd prefer to not include SkWebpCodec here, and require rewinding here.

Instead, you can do the following:

SkAutoTDelete<SkCodec> codec(SkCodec::NewFromStream(stream));
if (codec->getEncodedFormat() == SkEncodedFormat::kWEBP_SkEncodedFormat) {
  // webp codec supports scaling and subsetting natively
  return codec.release();
}

// wrap in new SkScaledCodec.

msarett

I'm really excited about how this looks! I think most of my comments are nits ...

5 years, 4 months ago (2015-07-27 20:42:18 UTC) #6

I'm really excited about how this looks!

I think most of my comments are nits and fyis.

https://codereview.chromium.org/1260673002/diff/20001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1260673002/diff/20001/gyp/codec.gyp#newcode48
gyp/codec.gyp:48: '../src/codec/SkScaledCodec.cpp'
nit:

I think we are keeping these in alphabetical order.  Maybe it makes sense to
organize them a different way?  But I think we should organize them somehow.

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

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:169: * Used to determine if dstDimensions can be
different than srcDimensions
On 2015/07/27 19:29:57, scroggo wrote:
> To be fair, dstDimensions can be different than srcDimensions in Jpeg or WebP,
> without setting fSampled to true.
> 
> I think this is only used for sampling in the X direction? Maybe it should
even
> be an integer, representing the sample size in the x direction? I think that
> would allow us to remove setSampleX, since it would always be set at the
outset.
> 
> This might also let us check to make sure the dst width matches the desired
> sampling.

+1 for some sort of int fSampleSize with 1 as the default.  As a warning, I ran
into bugs at one point because the default sampleSize is 0 when passed from
Android (and SkImageDecoder interprets this as the default, which is 1).

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkScaledC...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkScaledC...
include/codec/SkScaledCodec.h:38: SkPMColor ctable[], int* ctableCount)
override{
nit: space between "override" and "{".

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

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:746: if(options.fSampled == false) {
I think we should still be able to check for invalid dimensions, based on the
sample size.

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

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:63: int scaledHeight = this->getInfo().height() /
sampleY;
You're probably already aware of this, but we need to be really thoughtful about
how to deal with cases where width/height and sampleX/Y don't divide evenly.

The decision to round down (as you do here) makes just as much sense as anything
else (as far as I know).

As an fyi, I think the built in scaling in libjpeg-turbo rounds up.

And then we control the rounding for webp (since we pass the desired dimensions
to libwebp).  It looks like we actually round for webp, rather taking the
floor() or ceil().

Not sure if any of this is relevant, but it feels like it may get tricky.

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:104: Result result =
fCodec->getPixels(requestedInfo, dst, rowBytes, &options, ctable, ctableCount);
I was going to comment that this seems strange, and maybe we should just check
if the dimensions of requestedInfo match the original dimensions.

But then I realized that this will allow codecs that support some scaling (ex:
jpeg) to try to use their built in scaling before using SkScaledCodec as a
fallback.  Was this cleverness intended?  Maybe some comments would help me feel
better about this :).

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:115: bool isInterlaced = fCodec->isInterlaced();
For now, isInterlaced() is a decent name for this.  I have a feeling that as we
go along, we will encounter some non-interlaced images that we can't scale for
one reason or another.  And also, there are some interlaced images (jpegs and
maybe gifs) that we can scale.  Maybe we should rename this isScalable() or
something?

scroggo

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkCodec.h#newcode169 include/codec/SkCodec.h:169: * Used to determine if dstDimensions can be different ...

5 years, 4 months ago (2015-07-27 20:47:24 UTC) #7

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

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:169: * Used to determine if dstDimensions can be
different than srcDimensions
On 2015/07/27 20:42:17, msarett wrote:
> On 2015/07/27 19:29:57, scroggo wrote:
> > To be fair, dstDimensions can be different than srcDimensions in Jpeg or
WebP,
> > without setting fSampled to true.
> > 
> > I think this is only used for sampling in the X direction? Maybe it should
> even
> > be an integer, representing the sample size in the x direction? I think that
> > would allow us to remove setSampleX, since it would always be set at the
> outset.
> > 
> > This might also let us check to make sure the dst width matches the desired
> > sampling.
> 
> +1 for some sort of int fSampleSize with 1 as the default.  As a warning, I
ran
> into bugs at one point because the default sampleSize is 0 when passed from
> Android (and SkImageDecoder interprets this as the default, which is 1).

We're in charge of the Android code, so we should make them use a sensible
default.

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

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:115: bool isInterlaced = fCodec->isInterlaced();
On 2015/07/27 20:42:18, msarett wrote:
> For now, isInterlaced() is a decent name for this.  I have a feeling that as
we
> go along, we will encounter some non-interlaced images that we can't scale for
> one reason or another.  And also, there are some interlaced images (jpegs and
> maybe gifs) that we can scale.  Maybe we should rename this isScalable() or
> something?

My first thought was the name isSampleable(), but even for images that return
false, we still sample (as a means to scale) them, we just do it after getting
all the scanlines.

emmaleer

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkCodec_libpng.cpp File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1260673002/diff/1/src/codec/SkCodec_libpng.cpp#newcode749 src/codec/SkCodec_libpng.cpp:749: if(options.fScaled == false) { On 2015/07/27 19:29:56, scroggo wrote: ...

5 years, 4 months ago (2015-07-28 14:19:17 UTC) #8

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkCodec_libpng.cpp
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkCodec_libpng.cp...
src/codec/SkCodec_libpng.cpp:749: if(options.fScaled == false) {
On 2015/07/27 19:29:56, scroggo wrote:
> On 2015/07/27 18:31:38, emmaleer wrote:
> > On 2015/07/27 15:09:19, scroggo wrote:
> > > Please add a comment explaining this. Something like:
> > > 
> > > // If the caller is sampling, it is okay for the dimensions to not match.
> > > 
> > > (Which makes me wonder if the variable should be called fSampled?)
> > > 
> > > That said, IIUC, the width may be smaller (due to sampling), but it needs
to
> > be
> > > a supported width, and the height should be the same?
> > 
> > Changed to fSampled.
> > 
> > For normal images the dstHeight and dstWidth can both be smaller. These are
> the
> > dimensions the user requests. They are checked in SkScaledCodec.cpp in
> > onGetPixels() by calling scaling_supported()
> 
> Does the SkCodec need to know that dstHeight is smaller though? It seems like
> from this SkCodec's perspective, the dstHeight is unchanged - the caller needs
> to call skip/get scanline for each scanline in the original height, right?
Yes, I have changed the height to be the original height
> 
> > 
> > For interlaced images, the dstHeight must be the same. (as we do the y
> sampling
> > after)
> > In SkScaledCodec we set the dstHeight = srcHeight if the image is
interlaced,
> > before creating the scanline decoder.
> > 
> > I could add a check saying: If interlaced and sampled, make sure dstHeight
and
> > srcHeight are equal.
> > But since SkScaledCodec previously set them equal, I don't think it's really
> > necessary.
> > Let me know if you think it's still beneficial, and I can add the check. 
> 
> If there is a condition that we assume in a function, then that is a place we
> can add an assert. It checks that you were right about your assumption, and
> allows a reader to know you assume that.
> 
> That said, the info passed in comes from a public function, so someone could
> call with an invalid value, right?
I added a check that returns NULL if the heights are no equal when scaling

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkScaledCodec.cpp
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkScaledCodec.cpp...
src/codec/SkScaledCodec.cpp:97: if (kInvalidScale != result) {
On 2015/07/27 19:29:57, scroggo wrote:
> On 2015/07/27 18:31:38, emmaleer wrote:
> > On 2015/07/27 15:09:19, scroggo wrote:
> > > I wonder if there are other cases this should handle in the future.
> > > 
> > > Once we add subset decoding to this class, it needs to consider a return
> value
> > > that indicated that fCodec could not handle the subset. Currently, we
return
> > > kUnimplemented. (I think it is currently only used for subsets, but if we
> > wanted
> > > to use it for something else, we may need to distinguish between types of
> > > unimplemented.)
> > > 
> > > On the other hand, if the factory returns an SkWebpCodec directly, then
> fCodec
> > > will never support subsetting, so we could skip calling fCodec->getPixels
if
> > > options.fSubset is not NULL (not sure whether adding that check would be
> > better
> > > than just waiting for getPixels to return kUnimplemented, though).
> > 
> > Maybe we should add a return value of kInvalidSubset
> > We could check if options.fSubset == NULL before calling getPixels
> > Calling getPixels is not very expensive if it fails, as it only check a
bunch
> of
> > if statements at the beginning.
> > That being said, it would still be faster without those if statements.
> > 
> > I'm wondering about the case where we are sampling and request a subset (is
> this
> > going be be supported?)
> 
> Yes. This is currently supported by BitmapRegionDecoder, so we need to support
> it somehow.
> 
> > Maybe we should wait until implementing Subset Decoding to worry about this.

> 
> Agreed, but it would be nice to have a TODO/FIXME.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1260673002/diff/1/src/codec/SkSwizzler.h#newc...
src/codec/SkSwizzler.h:244: const int           fDeltaSrc;        // if
bitsPerPixel % 8 == 0
On 2015/07/27 19:29:57, scroggo wrote:
> On 2015/07/27 18:31:38, emmaleer wrote:
> > On 2015/07/27 15:09:20, scroggo wrote:
> > > Maybe there is a better name for this than "fDeltaSrc", which is intended
to
> > > mean "delta between source pixels". Now the true delta ends up being
> fDeltaSrc
> > *
> > > fSampleX. I haven't come up with a better name yet, though.
> > 
> > Well fDeltaSrc is always the bytes or bits between pixels.
> > Then, we pass fDeltaSrc * fSampleX to next(), which is the real deltaSrc.
> > What about calling fDeltaSrc fDeltaPixels, or fPixelDelta?
> > I think having pixel in the name would make sense, what do you think?
> 
> Maybe. I'm fine to leave it as-is for now. It is still the delta not including
> sampling.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/20001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1260673002/diff/20001/gyp/codec.gyp#newcode48
gyp/codec.gyp:48: '../src/codec/SkScaledCodec.cpp'
On 2015/07/27 20:42:17, msarett wrote:
> nit:
> 
> I think we are keeping these in alphabetical order.  Maybe it makes sense to
> organize them a different way?  But I think we should organize them somehow.

Thanks, I didn't notice that

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

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:169: * Used to determine if dstDimensions can be
different than srcDimensions
On 2015/07/27 20:42:17, msarett wrote:
> On 2015/07/27 19:29:57, scroggo wrote:
> > To be fair, dstDimensions can be different than srcDimensions in Jpeg or
WebP,
> > without setting fSampled to true.
> > 
> > I think this is only used for sampling in the X direction? Maybe it should
> even
> > be an integer, representing the sample size in the x direction? I think that
> > would allow us to remove setSampleX, since it would always be set at the
> outset.
> > 
> > This might also let us check to make sure the dst width matches the desired
> > sampling.
> 
> +1 for some sort of int fSampleSize with 1 as the default.  As a warning, I
ran
> into bugs at one point because the default sampleSize is 0 when passed from
> Android (and SkImageDecoder interprets this as the default, which is 1).

I've change bool fSampled to int fSampleX. setSampleX functions have been
removed. 

I've also added a check in onGetScanlineDecoder to make sure dst width matches
the desired sampling.

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkCodec.h...
include/codec/SkCodec.h:256: * returns true if the image is interlaced
On 2015/07/27 19:29:57, scroggo wrote:
> These two lines can be more concise, e.g.
> 
>   Returns whether the image is interlaced
> 
> I think it would also be good to explain why a client would want to call it.
> e.g.
> 
> For an interlaced image, calling getScanlines once (regardless of the count
> used) needs to read the entire image, therefore it is inefficient to call
> getScanlines more than once. Instead, it should only ever be called with all
the
> rows needed.
> 
> I don't know if we want to be so explicit about the implementation, but I tend
> to think we should say something about why someone might want to call it.
> 
> I also wonder if this makes more sense to be on the SkScanlineDecoder (this
> question is moot if we merge it with SkCodec).

Yes, I think it's good to explain how the entire interlaced image must be read
regardless of the count.
I've added that comment in.
Also, I agree that it makes more sense to add this function to the
scanlineDecoder, since you only would need it if you are using a
scanlineDecoder.

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkScaledC...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1260673002/diff/20001/include/codec/SkScaledC...
include/codec/SkScaledCodec.h:38: SkPMColor ctable[], int* ctableCount)
override{
On 2015/07/27 20:42:18, msarett wrote:
> nit: space between "override" and "{".

Acknowledged.

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

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:428: // we cannot depend on fNumberPasses, since it
may not have been set yet
On 2015/07/27 19:29:57, scroggo wrote:
> If we moved this onto the scanline decoder, we would already know it.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:746: if(options.fSampled == false) {
On 2015/07/27 20:42:18, msarett wrote:
> I think we should still be able to check for invalid dimensions, based on the
> sample size.

Yes, I now check to make sure dstHeight == srcHeight, and dstWidth = srcWidth /
SampleX

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

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:15: bool isWebp = SkWebpCodec::IsWebp(stream);
On 2015/07/27 19:29:57, scroggo wrote:
> I'd prefer to not include SkWebpCodec here, and require rewinding here.
> 
> Instead, you can do the following:
> 
> SkAutoTDelete<SkCodec> codec(SkCodec::NewFromStream(stream));
> if (codec->getEncodedFormat() == SkEncodedFormat::kWEBP_SkEncodedFormat) {
>   // webp codec supports scaling and subsetting natively
>   return codec.release();
> }
> 
> // wrap in new SkScaledCodec.

Okay, I like that method better too.
Just checking, when you said codec.release() you meant codec.detach() right?

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:63: int scaledHeight = this->getInfo().height() /
sampleY;
On 2015/07/27 20:42:18, msarett wrote:
> You're probably already aware of this, but we need to be really thoughtful
about
> how to deal with cases where width/height and sampleX/Y don't divide evenly.
> 
> The decision to round down (as you do here) makes just as much sense as
anything
> else (as far as I know).
> 
> As an fyi, I think the built in scaling in libjpeg-turbo rounds up.
> 
> And then we control the rounding for webp (since we pass the desired
dimensions
> to libwebp).  It looks like we actually round for webp, rather taking the
> floor() or ceil().
> 
> Not sure if any of this is relevant, but it feels like it may get tricky.

This is inconsistent. I think it would be better if they all did the same thing.
I think scaling down is best, but SkJpeg rounds up, and I don't think we should
choose to round up just because that's what Jpeg does... Unless we think
consistency is most important.  
Matt, did you find any information or comments saying why Jpeg chooses to round
up?
I've added a comment stating that we currently round down, but we can change
that in the future if we want.

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:104: Result result =
fCodec->getPixels(requestedInfo, dst, rowBytes, &options, ctable, ctableCount);
On 2015/07/27 20:42:18, msarett wrote:
> I was going to comment that this seems strange, and maybe we should just check
> if the dimensions of requestedInfo match the original dimensions.
> 
> But then I realized that this will allow codecs that support some scaling (ex:
> jpeg) to try to use their built in scaling before using SkScaledCodec as a
> fallback.  Was this cleverness intended?  Maybe some comments would help me
feel
> better about this :).

Yes, this was intended. Okay, I've added a comment describing what's happening.
Also, I call the decode an "fCodec native decode", does that make sense? For the
call to fCodec->getPixels()
If not I'll try to think of something better.

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:115: bool isInterlaced = fCodec->isInterlaced();
On 2015/07/27 20:47:24, scroggo wrote:
> On 2015/07/27 20:42:18, msarett wrote:
> > For now, isInterlaced() is a decent name for this.  I have a feeling that as
> we
> > go along, we will encounter some non-interlaced images that we can't scale
for
> > one reason or another.  And also, there are some interlaced images (jpegs
and
> > maybe gifs) that we can scale.  Maybe we should rename this isScalable() or
> > something?
> 
> My first thought was the name isSampleable(), but even for images that return
> false, we still sample (as a means to scale) them, we just do it after getting
> all the scanlines.

What about PostYSample()? Since we have to sample after reading all the lines,
instead of during.

scroggo

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec.cpp File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec.cpp#newcode15 src/codec/SkScaledCodec.cpp:15: bool isWebp = SkWebpCodec::IsWebp(stream); On 2015/07/28 14:19:16, emmaleer wrote: ...

5 years, 4 months ago (2015-07-28 15:58:10 UTC) #9

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

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:15: bool isWebp = SkWebpCodec::IsWebp(stream);
On 2015/07/28 14:19:16, emmaleer wrote:
> On 2015/07/27 19:29:57, scroggo wrote:
> > I'd prefer to not include SkWebpCodec here, and require rewinding here.
> > 
> > Instead, you can do the following:
> > 
> > SkAutoTDelete<SkCodec> codec(SkCodec::NewFromStream(stream));
> > if (codec->getEncodedFormat() == SkEncodedFormat::kWEBP_SkEncodedFormat) {
> >   // webp codec supports scaling and subsetting natively
> >   return codec.release();
> > }
> > 
> > // wrap in new SkScaledCodec.
> 
> Okay, I like that method better too.
> Just checking, when you said codec.release() you meant codec.detach() right? 

Yeah.

For a long time, C++ did not have a standard smart pointer, so everybody wrote
their own. Skia's (SkAutoTDelete) calls that method detach(), but others call
their analogous methods release().

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:115: bool isInterlaced = fCodec->isInterlaced();
On 2015/07/28 14:19:16, emmaleer wrote:
> On 2015/07/27 20:47:24, scroggo wrote:
> > On 2015/07/27 20:42:18, msarett wrote:
> > > For now, isInterlaced() is a decent name for this.  I have a feeling that
as
> > we
> > > go along, we will encounter some non-interlaced images that we can't scale
> for
> > > one reason or another.  And also, there are some interlaced images (jpegs
> and
> > > maybe gifs) that we can scale.  Maybe we should rename this isScalable()
or
> > > something?
> > 
> > My first thought was the name isSampleable(), but even for images that
return
> > false, we still sample (as a means to scale) them, we just do it after
getting
> > all the scanlines.
> 
> What about PostYSample()? Since we have to sample after reading all the lines,
> instead of during. 

PostYSample() sounds to me like it actually does something (rather than
returning a property of the object). Along those lines, I might push for
requiresPostYSampling(), although I'm not sure that's clear either.

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkCodec_libbm...
File src/codec/SkCodec_libbmp.cpp (right):

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkCodec_libbm...
src/codec/SkCodec_libbmp.cpp:1137: colorPtr, dstInfo, dst, dstRowBytes,
kNo_ZeroInitialized, opts.fSampleX));
If we're going to pass zeroInitialized (which is a field on options) and sampleX
(also a field on options), it seems like it would make more sense to just pass
the Options object.

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

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:740: if(options.fSampleX == 1) {
It would be nice to be able to share this code with other decoders somehow.

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:60: // Can be changed in the future to round up, or
round to closest integer if we want
FWIW, I think you would need to update your code that is used for sampling as
well. Ideally, we'd like it to behave the same way as the old code
(SkImageDecoder), so we can drop in replace.

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:121: int sampleY = srcHeight / dstHeight;
I think we need to consolidate our calculations (along with their assumptions),
to make sure they do not get out of sync.

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkSwizzler.h#...
src/codec/SkSwizzler.h:125: *  @param sampleX the step between samples in the x
direction.
As stated elsewhere, I think we should consider combining this with
ZeroInitialized, and just pass the Options object.

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkSwizzler.h#...
src/codec/SkSwizzler.h:250: int                 fSampleX;         // step
between X samples
This can now be const.

fX0 can also be const, if you initialize it in the intializer list like so:

fX0(sampleX == 1 ? 0 : sampleX >> 1)

On a related note, I thought we needed to check to make sure the sample size was
not too big. Do we do that elsewhere?

msarett

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec.cpp File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec.cpp#newcode63 src/codec/SkScaledCodec.cpp:63: int scaledHeight = this->getInfo().height() / sampleY; On 2015/07/28 14:19:16, ...

5 years, 4 months ago (2015-07-28 20:53:11 UTC) #10

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

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:63: int scaledHeight = this->getInfo().height() /
sampleY;
On 2015/07/28 14:19:16, emmaleer wrote:
> On 2015/07/27 20:42:18, msarett wrote:
> > You're probably already aware of this, but we need to be really thoughtful
> about
> > how to deal with cases where width/height and sampleX/Y don't divide evenly.
> > 
> > The decision to round down (as you do here) makes just as much sense as
> anything
> > else (as far as I know).
> > 
> > As an fyi, I think the built in scaling in libjpeg-turbo rounds up.
> > 
> > And then we control the rounding for webp (since we pass the desired
> dimensions
> > to libwebp).  It looks like we actually round for webp, rather taking the
> > floor() or ceil().
> > 
> > Not sure if any of this is relevant, but it feels like it may get tricky.
> 
> This is inconsistent. I think it would be better if they all did the same
thing.
> I think scaling down is best, but SkJpeg rounds up, and I don't think we
should
> choose to round up just because that's what Jpeg does... Unless we think
> consistency is most important.  
> Matt, did you find any information or comments saying why Jpeg chooses to
round
> up?
> I've added a comment stating that we currently round down, but we can change
> that in the future if we want.

AFAICT it's an arbitrary decision.

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:104: Result result =
fCodec->getPixels(requestedInfo, dst, rowBytes, &options, ctable, ctableCount);
On 2015/07/28 14:19:16, emmaleer wrote:
> On 2015/07/27 20:42:18, msarett wrote:
> > I was going to comment that this seems strange, and maybe we should just
check
> > if the dimensions of requestedInfo match the original dimensions.
> > 
> > But then I realized that this will allow codecs that support some scaling
(ex:
> > jpeg) to try to use their built in scaling before using SkScaledCodec as a
> > fallback.  Was this cleverness intended?  Maybe some comments would help me
> feel
> > better about this :).
> 
> Yes, this was intended. Okay, I've added a comment describing what's
happening.
> Also, I call the decode an "fCodec native decode", does that make sense? For
the
> call to fCodec->getPixels()
> If not I'll try to think of something better. 

Thanks!

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:115: bool isInterlaced = fCodec->isInterlaced();
On 2015/07/28 15:58:10, scroggo wrote:
> On 2015/07/28 14:19:16, emmaleer wrote:
> > On 2015/07/27 20:47:24, scroggo wrote:
> > > On 2015/07/27 20:42:18, msarett wrote:
> > > > For now, isInterlaced() is a decent name for this.  I have a feeling
that
> as
> > > we
> > > > go along, we will encounter some non-interlaced images that we can't
scale
> > for
> > > > one reason or another.  And also, there are some interlaced images
(jpegs
> > and
> > > > maybe gifs) that we can scale.  Maybe we should rename this isScalable()
> or
> > > > something?
> > > 
> > > My first thought was the name isSampleable(), but even for images that
> return
> > > false, we still sample (as a means to scale) them, we just do it after
> getting
> > > all the scanlines.
> > 
> > What about PostYSample()? Since we have to sample after reading all the
lines,
> > instead of during. 
> 
> PostYSample() sounds to me like it actually does something (rather than
> returning a property of the object). Along those lines, I might push for
> requiresPostYSampling(), although I'm not sure that's clear either.

What about isHardToSample()?

emmaleer

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec.cpp File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec.cpp#newcode15 src/codec/SkScaledCodec.cpp:15: bool isWebp = SkWebpCodec::IsWebp(stream); On 2015/07/28 15:58:10, scroggo wrote: ...

5 years, 4 months ago (2015-07-29 21:55:08 UTC) #11

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

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:15: bool isWebp = SkWebpCodec::IsWebp(stream);
On 2015/07/28 15:58:10, scroggo wrote:
> On 2015/07/28 14:19:16, emmaleer wrote:
> > On 2015/07/27 19:29:57, scroggo wrote:
> > > I'd prefer to not include SkWebpCodec here, and require rewinding here.
> > > 
> > > Instead, you can do the following:
> > > 
> > > SkAutoTDelete<SkCodec> codec(SkCodec::NewFromStream(stream));
> > > if (codec->getEncodedFormat() == SkEncodedFormat::kWEBP_SkEncodedFormat) {
> > >   // webp codec supports scaling and subsetting natively
> > >   return codec.release();
> > > }
> > > 
> > > // wrap in new SkScaledCodec.
> > 
> > Okay, I like that method better too.
> > Just checking, when you said codec.release() you meant codec.detach() right?

> 
> Yeah.
> 
> For a long time, C++ did not have a standard smart pointer, so everybody wrote
> their own. Skia's (SkAutoTDelete) calls that method detach(), but others call
> their analogous methods release().

Acknowledged.

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:115: bool isInterlaced = fCodec->isInterlaced();
On 2015/07/28 20:53:10, msarett wrote:
> On 2015/07/28 15:58:10, scroggo wrote:
> > On 2015/07/28 14:19:16, emmaleer wrote:
> > > On 2015/07/27 20:47:24, scroggo wrote:
> > > > On 2015/07/27 20:42:18, msarett wrote:
> > > > > For now, isInterlaced() is a decent name for this.  I have a feeling
> that
> > as
> > > > we
> > > > > go along, we will encounter some non-interlaced images that we can't
> scale
> > > for
> > > > > one reason or another.  And also, there are some interlaced images
> (jpegs
> > > and
> > > > > maybe gifs) that we can scale.  Maybe we should rename this
isScalable()
> > or
> > > > > something?
> > > > 
> > > > My first thought was the name isSampleable(), but even for images that
> > return
> > > > false, we still sample (as a means to scale) them, we just do it after
> > getting
> > > > all the scanlines.
> > > 
> > > What about PostYSample()? Since we have to sample after reading all the
> lines,
> > > instead of during. 
> > 
> > PostYSample() sounds to me like it actually does something (rather than
> > returning a property of the object). Along those lines, I might push for
> > requiresPostYSampling(), although I'm not sure that's clear either.
> 
> What about isHardToSample()?

I like isHardToSample, since it's more broad, and we will be using this function
for a variety of things.

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:60: // Can be changed in the future to round up, or
round to closest integer if we want
On 2015/07/28 15:58:10, scroggo wrote:
> FWIW, I think you would need to update your code that is used for sampling as
> well. Ideally, we'd like it to behave the same way as the old code
> (SkImageDecoder), so we can drop in replace.

Okay. Do you think I should remove the comment saying it can be changed in the
future if we want?
Since we want it to behave the same as SkImageDecoder, and SkImageDecoder rounds
down.

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:121: int sampleY = srcHeight / dstHeight;
On 2015/07/28 15:58:10, scroggo wrote:
> I think we need to consolidate our calculations (along with their
assumptions),
> to make sure they do not get out of sync.

Do you mean consolidate the calculations of sampleY here and in
scaling_supported()?

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkSwizzler.h#...
src/codec/SkSwizzler.h:125: *  @param sampleX the step between samples in the x
direction.
On 2015/07/28 15:58:10, scroggo wrote:
> As stated elsewhere, I think we should consider combining this with
> ZeroInitialized, and just pass the Options object.

SampleX is no longer needed to be stored in the options object, so I left this
as is.

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkSwizzler.h#...
src/codec/SkSwizzler.h:250: int                 fSampleX;         // step
between X samples
On 2015/07/28 15:58:10, scroggo wrote:
> This can now be const.
> 
> fX0 can also be const, if you initialize it in the intializer list like so:
> 
> fX0(sampleX == 1 ? 0 : sampleX >> 1)
> 
> On a related note, I thought we needed to check to make sure the sample size
was
> not too big. Do we do that elsewhere?

SampleX is set to srcWidth / dstWidth in initializeSwizzler, so will never be
too big

msarett

Looks good! Just to write down what we talked about in person about how this ...

5 years, 4 months ago (2015-07-30 13:55:38 UTC) #12

emmaleer

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec.cpp File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec.cpp#newcode121 src/codec/SkScaledCodec.cpp:121: int sampleY = srcHeight / dstHeight; On 2015/07/29 21:55:08, ...

5 years, 4 months ago (2015-07-30 17:50:39 UTC) #13

scroggo

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec.cpp File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec.cpp#newcode115 src/codec/SkScaledCodec.cpp:115: bool isInterlaced = fCodec->isInterlaced(); On 2015/07/29 21:55:08, emmaleer wrote: ...

5 years, 4 months ago (2015-07-30 17:53:02 UTC) #14

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

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:115: bool isInterlaced = fCodec->isInterlaced();
On 2015/07/29 21:55:08, emmaleer wrote:
> On 2015/07/28 20:53:10, msarett wrote:
> > On 2015/07/28 15:58:10, scroggo wrote:
> > > On 2015/07/28 14:19:16, emmaleer wrote:
> > > > On 2015/07/27 20:47:24, scroggo wrote:
> > > > > On 2015/07/27 20:42:18, msarett wrote:
> > > > > > For now, isInterlaced() is a decent name for this.  I have a feeling
> > that
> > > as
> > > > > we
> > > > > > go along, we will encounter some non-interlaced images that we can't
> > scale
> > > > for
> > > > > > one reason or another.  And also, there are some interlaced images
> > (jpegs
> > > > and
> > > > > > maybe gifs) that we can scale.  Maybe we should rename this
> isScalable()
> > > or
> > > > > > something?
> > > > > 
> > > > > My first thought was the name isSampleable(), but even for images that
> > > return
> > > > > false, we still sample (as a means to scale) them, we just do it after
> > > getting
> > > > > all the scanlines.
> > > > 
> > > > What about PostYSample()? Since we have to sample after reading all the
> > lines,
> > > > instead of during. 
> > > 
> > > PostYSample() sounds to me like it actually does something (rather than
> > > returning a property of the object). Along those lines, I might push for
> > > requiresPostYSampling(), although I'm not sure that's clear either.
> > 
> > What about isHardToSample()?
> 
> I like isHardToSample, since it's more broad, and we will be using this
function
> for a variety of things. 

It is broad, but it's not obvious to me what hard to sample means. What we want
it to mean is that we have to just decode every line and then sample, but that
doesn't seem obvious to me from the name. (To be fair, I haven't thought of a
better name...)

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:60: // Can be changed in the future to round up, or
round to closest integer if we want
On 2015/07/29 21:55:08, emmaleer wrote:
> On 2015/07/28 15:58:10, scroggo wrote:
> > FWIW, I think you would need to update your code that is used for sampling
as
> > well. Ideally, we'd like it to behave the same way as the old code
> > (SkImageDecoder), so we can drop in replace.
> 
> Okay. Do you think I should remove the comment saying it can be changed in the
> future if we want?
> Since we want it to behave the same as SkImageDecoder, and SkImageDecoder
rounds
> down.

I would probably keep it simple. Something like "We chose to round down because
it matches the behavior of SkImageDecoder".

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:121: int sampleY = srcHeight / dstHeight;
On 2015/07/29 21:55:08, emmaleer wrote:
> On 2015/07/28 15:58:10, scroggo wrote:
> > I think we need to consolidate our calculations (along with their
> assumptions),
> > to make sure they do not get out of sync.
> 
> Do you mean consolidate the calculations of sampleY here and in
> scaling_supported()?
> 
>  

I'm not sure where all we have it, but we should just make sure that if we ever
decide to change how we compute the sampling parameters (for example, if we
decided we wanted to round up instead of down), we only need to change one
isolated part of the code.

https://codereview.chromium.org/1260673002/diff/60001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1260673002/diff/60001/include/codec/SkCodec.h...
include/codec/SkCodec.h:55: *  This is a suggestion to the user. The dimensions
getScanlineDecoder
The first sentence sounds good. For the second, I am not really sure what you're
trying to communicate. The user is going to call getScanlineDecoder, and they're
in control of the dimensions they use. Are you trying to state that they do not
have to use these dimensions?

https://codereview.chromium.org/1260673002/diff/60001/include/codec/SkCodec.h...
include/codec/SkCodec.h:77: 
nit: This extra line is not needed.

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

https://codereview.chromium.org/1260673002/diff/60001/include/codec/SkScanlin...
include/codec/SkScanlineDecoder.h:79: * Returns whether the image is interlaced
These comments should match the name of the function.

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

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:474: int sampleX = this->getInfo().width() /
requestedInfo.width();
This should also be in some central place, to be shared with other decoders.

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:740: Options scaledOptions = options;
It looks like we do not modify scaledOptions, so we could just use options.

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:744: // heights must be equal as SkCodec_libpng has
no native y sampling
What about widths? We don't want them to request a width that is greater than
getInfo().width(), for example. Can we support all widths that are smaller?

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

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:217: int sampleX = this->getInfo().width() /
requestedInfo.width();
I think this should be shared with png, etc.

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:326: this->fDecoderMgr->dinfo()->scale_num = 8;
Could you add a comment explaining this? I think the idea is that we reset to a
1:1 decode, and let the swizzler take care of sampling?

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

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:139: scanlineDecoder->getScanlines(storagePtr,
srcHeight, rowBytes);
In one sense, this is correct, to remove result if we do not use it. But I think
it would be better to use it and exit if it's not kSuccess.

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

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:22: static SkSwizzler::ResultAlpha sample(void*
SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src,
Could you add a description?

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:24: uint8_t* SK_RESTRICT dst = (uint8_t*) dstRow;
Does this do what we want it to? It looks like it is used to sample 565, but it
works in 1-byte increments. It seems like this would not line up with the actual
pixels?

I think you would need a different version for different colortypes.

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:28: }
This method should return a value.

For 565, we do not need to check anything (it is always opaque), so you can just
return a max alpha of 0xFF. (Matt will have to remind me what zeroAlpha is.)

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:550: 
On 2015/07/30 13:55:38, msarett wrote:
> nit: extra new line
> 
> Also, you might want to try rebasing.  I have updated this function to fix the
> bug you caught :).  As long as this new line isn't here I don't think there
> should be a merge conflict.

Just as a general piece of advice, I find it useful to rebase and upload without
other changes; otherwise when looking at the difference between two revisions
it's unclear which changes you introduced vs someone else introduced.

scroggo

Sorry, I published my last set of comments late, so you've already addressed many, but ...

5 years, 4 months ago (2015-07-30 18:05:57 UTC) #15

scroggo

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.cpp#newcode426 src/codec/SkJpegCodec.cpp:426: fStorage.reset(fCodec->getInfo().width() * dstInfo.minRowBytes()); We only need to do this ...

5 years, 4 months ago (2015-07-30 18:28:23 UTC) #16

emmaleer

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec.cpp File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec.cpp#newcode115 src/codec/SkScaledCodec.cpp:115: bool isInterlaced = fCodec->isInterlaced(); On 2015/07/30 17:53:01, scroggo wrote: ...

5 years, 4 months ago (2015-07-30 22:27:56 UTC) #17

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

https://codereview.chromium.org/1260673002/diff/20001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:115: bool isInterlaced = fCodec->isInterlaced();
On 2015/07/30 17:53:01, scroggo wrote:
> On 2015/07/29 21:55:08, emmaleer wrote:
> > On 2015/07/28 20:53:10, msarett wrote:
> > > On 2015/07/28 15:58:10, scroggo wrote:
> > > > On 2015/07/28 14:19:16, emmaleer wrote:
> > > > > On 2015/07/27 20:47:24, scroggo wrote:
> > > > > > On 2015/07/27 20:42:18, msarett wrote:
> > > > > > > For now, isInterlaced() is a decent name for this.  I have a
feeling
> > > that
> > > > as
> > > > > > we
> > > > > > > go along, we will encounter some non-interlaced images that we
can't
> > > scale
> > > > > for
> > > > > > > one reason or another.  And also, there are some interlaced images
> > > (jpegs
> > > > > and
> > > > > > > maybe gifs) that we can scale.  Maybe we should rename this
> > isScalable()
> > > > or
> > > > > > > something?
> > > > > > 
> > > > > > My first thought was the name isSampleable(), but even for images
that
> > > > return
> > > > > > false, we still sample (as a means to scale) them, we just do it
after
> > > > getting
> > > > > > all the scanlines.
> > > > > 
> > > > > What about PostYSample()? Since we have to sample after reading all
the
> > > lines,
> > > > > instead of during. 
> > > > 
> > > > PostYSample() sounds to me like it actually does something (rather than
> > > > returning a property of the object). Along those lines, I might push for
> > > > requiresPostYSampling(), although I'm not sure that's clear either.
> > > 
> > > What about isHardToSample()?
> > 
> > I like isHardToSample, since it's more broad, and we will be using this
> function
> > for a variety of things. 
> 
> It is broad, but it's not obvious to me what hard to sample means. What we
want
> it to mean is that we have to just decode every line and then sample, but that
> doesn't seem obvious to me from the name. (To be fair, I haven't thought of a
> better name...)

Should we use requiresPostSampling() ?
or what about canSampleMidWay() ?
I think canSampleMidWay makes more sense than requiresPostSampling

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/40001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:60: // Can be changed in the future to round up, or
round to closest integer if we want
On 2015/07/30 17:53:01, scroggo wrote:
> On 2015/07/29 21:55:08, emmaleer wrote:
> > On 2015/07/28 15:58:10, scroggo wrote:
> > > FWIW, I think you would need to update your code that is used for sampling
> as
> > > well. Ideally, we'd like it to behave the same way as the old code
> > > (SkImageDecoder), so we can drop in replace.
> > 
> > Okay. Do you think I should remove the comment saying it can be changed in
the
> > future if we want?
> > Since we want it to behave the same as SkImageDecoder, and SkImageDecoder
> rounds
> > down.
> 
> I would probably keep it simple. Something like "We chose to round down
because
> it matches the behavior of SkImageDecoder".

Acknowledged.

https://codereview.chromium.org/1260673002/diff/60001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1260673002/diff/60001/include/codec/SkCodec.h...
include/codec/SkCodec.h:55: *  This is a suggestion to the user. The dimensions
getScanlineDecoder
On 2015/07/30 17:53:01, scroggo wrote:
> The first sentence sounds good. For the second, I am not really sure what
you're
> trying to communicate. The user is going to call getScanlineDecoder, and
they're
> in control of the dimensions they use. Are you trying to state that they do
not
> have to use these dimensions?

Yes that's what I was trying to say. I think mentioning that the dimensions are
a suggestion states that already though.

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

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:740: Options scaledOptions = options;
On 2015/07/30 17:53:01, scroggo wrote:
> It looks like we do not modify scaledOptions, so we could just use options.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkCodec_libpn...
src/codec/SkCodec_libpng.cpp:744: // heights must be equal as SkCodec_libpng has
no native y sampling
On 2015/07/30 17:53:01, scroggo wrote:
> What about widths? We don't want them to request a width that is greater than
> getInfo().width(), for example. Can we support all widths that are smaller?

Good point. I've added a check to only allow smaller or equal widths.

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

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:326: this->fDecoderMgr->dinfo()->scale_num = 8;
On 2015/07/30 17:53:01, scroggo wrote:
> Could you add a comment explaining this? I think the idea is that we reset to
a
> 1:1 decode, and let the swizzler take care of sampling?

Yes, the idea is if scaleToDimensions fails, then the native scaling settings
should not be set. (before they were being set, but we failed creating the
scanline decoder so it didn't matter)
I think it makes sense for no scaling settings to change if the function fails.

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

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:139: scanlineDecoder->getScanlines(storagePtr,
srcHeight, rowBytes);
On 2015/07/30 17:53:01, scroggo wrote:
> In one sense, this is correct, to remove result if we do not use it. But I
think
> it would be better to use it and exit if it's not kSuccess.

Acknowledged.

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

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:22: static SkSwizzler::ResultAlpha sample(void*
SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src,
On 2015/07/30 17:53:01, scroggo wrote:
> Could you add a description?

Yes.
Also, we can use the sample function for all jpeg images.
I haven't found a nice way to do that yet, or if that is even possible.

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:24: uint8_t* SK_RESTRICT dst = (uint8_t*) dstRow;
On 2015/07/30 17:53:01, scroggo wrote:
> Does this do what we want it to? It looks like it is used to sample 565, but
it
> works in 1-byte increments. It seems like this would not line up with the
actual
> pixels?
> 
> I think you would need a different version for different colortypes.

I think this is working somehow..
When I test it, deltaSrc is 16 (so I guess deltaSrc is in bits sometimes and not
bytes)
So each time 16 bits is added to src.
I'm not exactly sure how the src pointer knows to add bits and not bytes
though.. 
Usually when you add 1 to a pointer it adds 1 byte right?
The images looks correct when I run dm.

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:28: }
On 2015/07/30 17:53:01, scroggo wrote:
> This method should return a value.
> 
> For 565, we do not need to check anything (it is always opaque), so you can
just
> return a max alpha of 0xFF. (Matt will have to remind me what zeroAlpha is.)

Acknowledged.

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

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:426: fStorage.reset(fCodec->getInfo().width() *
dstInfo.minRowBytes());
On 2015/07/30 18:28:23, scroggo wrote:
> We only need to do this if there is a swizzler, right?

Yes, I've added an if statement to only do this if there is a swizzler

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:449: if (fCodec->fSwizzler) {
On 2015/07/30 18:28:23, scroggo wrote:
> So it looks like these introduce a slight performance hit? Maybe we could try
> something like the following:
> 
> constructor {
>   if (fCodec->fSwizzler) {
>     mScanlineProc = &decodeAndSwizzle;
>   } else {
>     mScanlineProc = &decodeDirect;
>   }
> }
> 
> onGetScanlineInternal(void* dstRow) {
>   // Does everything inside the loop
> }
> 
> onGetScanlines() {
>   for (int i = 0; i < count; i++) {
>     mScanlineProc;
>   }
> }
> 
> decodeAndSwizzle() {
>   // setup
>   onGetScanlineInternal
> }
> 
> decodeDirect() {
>   onGetScanlineInternal
> }
> 
> Not sure if it would be faster, but something to consider. Maybe the
difference
> is neglible?

I think it's negligible. Also, I don't think this one specific if should drive a
function change this big, since all if statements decrease performance. 
We could, for example, do the same thing as above, but with different if
statements in onGetScanlines.
(such as "JCS_CMYK == fCodec->fDecoderMgr->dinfo()->out_color_space") 
Adding that change could potentially speed it up for any of the if statements.
Why should we choose to optimize this specific if statement?
I think if SkJpegCodec was performing too slow for users, then maybe we could go
back and optimize if statements. 
But since there is a minimal performance decrease, and the reason why we are
worried about this if statement and not other if statements isn't clear, I don't
think this change is worth it.
(Leon or Matt, if you think this change is worth it still, let me know)

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:477: dst += rowBytes;
On 2015/07/30 18:28:23, scroggo wrote:
> Why not use SkTAddOffset?

Changed to to SkTAddOffset.

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:507: SkAutoMalloc               fStorage;
On 2015/07/30 18:28:23, scroggo wrote:
> // Only used if sampling is needed.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:276: static int get_sample_size(int srcDimension, int
dstDimension) {
On 2015/07/30 18:05:57, scroggo wrote:
> Can this be shared with the other function that looks like this?

Yes, it now calls the function in SkScaledCodec.h
Currently it is a public function, but I remember you saying that we didn't want
it to be public?
Why not? I don't see any harm of it being public, since it doesn't change any
variables. 
I could change it to private, and make SkScaledCodec and SkSwizzler friend
classes. Would that be better? (if so, why would that be better)

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:456: int sampleX = get_sample_size(srcInfo.width(),
dstInfo.width());
On 2015/07/30 18:05:57, scroggo wrote:
> It seems a little weird that we pass the full image info, when all we need is
> the width.

Right. I've changed this to only pass the width.

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkSwizzler.h#...
src/codec/SkSwizzler.h:125: *  @param sampleX the step between samples in the x
direction.
On 2015/07/30 18:05:57, scroggo wrote:
> Update the comment to match the new parameter?

Acknowledged.

msarett

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.cpp#newcode449 src/codec/SkJpegCodec.cpp:449: if (fCodec->fSwizzler) { On 2015/07/30 22:27:56, emmaleer wrote: > ...

5 years, 4 months ago (2015-07-31 12:17:59 UTC) #18

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

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:449: if (fCodec->fSwizzler) {
On 2015/07/30 22:27:56, emmaleer wrote:
> On 2015/07/30 18:28:23, scroggo wrote:
> > So it looks like these introduce a slight performance hit? Maybe we could
try
> > something like the following:
> > 
> > constructor {
> >   if (fCodec->fSwizzler) {
> >     mScanlineProc = &decodeAndSwizzle;
> >   } else {
> >     mScanlineProc = &decodeDirect;
> >   }
> > }
> > 
> > onGetScanlineInternal(void* dstRow) {
> >   // Does everything inside the loop
> > }
> > 
> > onGetScanlines() {
> >   for (int i = 0; i < count; i++) {
> >     mScanlineProc;
> >   }
> > }
> > 
> > decodeAndSwizzle() {
> >   // setup
> >   onGetScanlineInternal
> > }
> > 
> > decodeDirect() {
> >   onGetScanlineInternal
> > }
> > 
> > Not sure if it would be faster, but something to consider. Maybe the
> difference
> > is neglible?
> 
> I think it's negligible. Also, I don't think this one specific if should drive
a
> function change this big, since all if statements decrease performance. 
> We could, for example, do the same thing as above, but with different if
> statements in onGetScanlines.
> (such as "JCS_CMYK == fCodec->fDecoderMgr->dinfo()->out_color_space") 
> Adding that change could potentially speed it up for any of the if statements.
> Why should we choose to optimize this specific if statement?
> I think if SkJpegCodec was performing too slow for users, then maybe we could
go
> back and optimize if statements. 
> But since there is a minimal performance decrease, and the reason why we are
> worried about this if statement and not other if statements isn't clear, I
don't
> think this change is worth it.
> (Leon or Matt, if you think this change is worth it still, let me know)  

I think Leon's suggestion is a good one.  We did regress performance by 2.5%,
and while it's not awful, it's worth it to see if we can do better.  Taking a
closer look, it seems to me that the two possible causes are:
1) Memory allocation of fSrcRow (it looks like you've already fixed this to only
allocate when we need a swizzler)
2) If statements inside for loop in onGetScanlines()

Unless we gain the whole 2.5% back from the fix to (1), I think we should try
fixing (2) as well.  I think the idea of Leon's design is to factor the:
if (swizzler) {
 // code
} else {
  //other code
}
outside of the for loop, so we only check the branch once instead of height
times.  It seems logical to me.

scroggo

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cpp#newcode24 src/codec/SkSwizzler.cpp:24: uint8_t* SK_RESTRICT dst = (uint8_t*) dstRow; On 2015/07/30 22:27:56, ...

5 years, 4 months ago (2015-07-31 13:35:33 UTC) #19

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

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:24: uint8_t* SK_RESTRICT dst = (uint8_t*) dstRow;
On 2015/07/30 22:27:56, emmaleer wrote:
> On 2015/07/30 17:53:01, scroggo wrote:
> > Does this do what we want it to? It looks like it is used to sample 565, but
> it
> > works in 1-byte increments. It seems like this would not line up with the
> actual
> > pixels?
> > 
> > I think you would need a different version for different colortypes.
> 
> I think this is working somehow..
> When I test it, deltaSrc is 16 (so I guess deltaSrc is in bits sometimes and
not
> bytes)

That is correct. For the "small index" versions (all BMP, I believe), it will be
in bits.

> So each time 16 bits is added to src.
> I'm not exactly sure how the src pointer knows to add bits and not bytes
> though.. 
> Usually when you add 1 to a pointer it adds 1 byte right?

When you add 1 to a pointer it will add the size of the pointer.

If you have a uint32_t*, adding one will move to the next uint32_t, meaning it
will add four bytes. (On a related note, you cannot add to a void*, where it is
not clear what you would add.)

Adding one to a uint8_t*, as you are doing here, will add one byte. So if
deltaSrc is 16, you'll move 16 bytes forward.

> The images looks correct when I run dm.

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:276: static int get_sample_size(int srcDimension, int
dstDimension) {
On 2015/07/30 22:27:56, emmaleer wrote:
> On 2015/07/30 18:05:57, scroggo wrote:
> > Can this be shared with the other function that looks like this?
> 
> Yes, it now calls the function in SkScaledCodec.h
> Currently it is a public function, but I remember you saying that we didn't
want
> it to be public?
> Why not? I don't see any harm of it being public, since it doesn't change any
> variables. 

I just meant that it should not be a part of the public API - i.e. not in
include/ (not on SkCodec). You've made it a public function on SkScaledCodec,
which is in src/codec, which I think is appropriate.

> I could change it to private, and make SkScaledCodec and SkSwizzler friend
> classes. Would that be better? (if so, why would that be better)

No, friend classes should be used sparingly.

https://codereview.chromium.org/1260673002/diff/100001/include/codec/SkScaled...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1260673002/diff/100001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:31: static int get_sample_size(int srcDimension,
int dstDimension) {
style nit: Skia has different conventions for static functions and static member
functions. While we name static functions get_sample_size, we name static member
functions GetSampleSize.

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:742: if (dstInfo.height() !=
this->getInfo().height()) {
Maybe this should be another static method on SkScaledCodec, something like:

bool DimensionsSupportedForSampling(srcInfo, dstInfo);

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

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:510: SkAutoMalloc               fStorage;
This one is only needed for sampling, too, right?

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:550: if (dstInfo.height() != this->getInfo().height())
{
This should call DimensionsSupportedForSampling

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:42: // NOTE: we round down here for scaled
dimension, beucase it matches the behavior of SkImageDecoder
because*

(Although I think you can be a little more concise: "to match the behavior of
SkImageDecoder")

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:137: bool isHardToSample =
scanlineDecoder->isHardToSample();
nit: can be const

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

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:31: return 0xFF;
Since this is opaque, what you want is to return 0xFFFF (see the comment in
GetResult).

I refreshed my memory - zeroAlpha is to track if the entire image is
transparent, so it is non-zero only if the image is not transparent.

The name is probably not great, although I do not currently have an idea for a
better one.

On a related note, I just realized that UPDATE_RESULT_ALPHA (not used in this
function) will compute its expression twice. In many cases we use the following
pattern:

alpha = <expression>;
UPDATE_RESULT_ALPHA(alpha)

but occasionally we use the following pattern:

UPDATE_RESULT_ALPHA(<expression>);

We should probably switch to the former everywhere (and verify that it makes a
difference). (That should be its own CL.)

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:118: *  @param dstInfo dimensions() describe destination.
The comment used to distinguish between the dimensions, which applied to both
source and width, and the other fields, which only applied to the dst. Now the
whole struct describes the dst (not just the dimensions).

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:124: *  @param srcWidth is the width of the source. Used
to calculate the width samplesize.
I think this could use a little bit more explanation. I think it's correct that
we support a width sample size but not a height, but it's not necessarily
intuitive. It would be good to explain the use here.

emmaleer

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cpp#newcode24 src/codec/SkSwizzler.cpp:24: uint8_t* SK_RESTRICT dst = (uint8_t*) dstRow; On 2015/07/30 17:53:01, ...

5 years, 4 months ago (2015-07-31 18:41:56 UTC) #20

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

https://codereview.chromium.org/1260673002/diff/60001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:24: uint8_t* SK_RESTRICT dst = (uint8_t*) dstRow;
On 2015/07/30 17:53:01, scroggo wrote:
> Does this do what we want it to? It looks like it is used to sample 565, but
it
> works in 1-byte increments. It seems like this would not line up with the
actual
> pixels?
> 
> I think you would need a different version for different colortypes.

I've fixed this to work with 565

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

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:449: if (fCodec->fSwizzler) {
On 2015/07/31 12:17:59, msarett wrote:
> On 2015/07/30 22:27:56, emmaleer wrote:
> > On 2015/07/30 18:28:23, scroggo wrote:
> > > So it looks like these introduce a slight performance hit? Maybe we could
> try
> > > something like the following:
> > > 
> > > constructor {
> > >   if (fCodec->fSwizzler) {
> > >     mScanlineProc = &decodeAndSwizzle;
> > >   } else {
> > >     mScanlineProc = &decodeDirect;
> > >   }
> > > }
> > > 
> > > onGetScanlineInternal(void* dstRow) {
> > >   // Does everything inside the loop
> > > }
> > > 
> > > onGetScanlines() {
> > >   for (int i = 0; i < count; i++) {
> > >     mScanlineProc;
> > >   }
> > > }
> > > 
> > > decodeAndSwizzle() {
> > >   // setup
> > >   onGetScanlineInternal
> > > }
> > > 
> > > decodeDirect() {
> > >   onGetScanlineInternal
> > > }
> > > 
> > > Not sure if it would be faster, but something to consider. Maybe the
> > difference
> > > is neglible?
> > 
> > I think it's negligible. Also, I don't think this one specific if should
drive
> a
> > function change this big, since all if statements decrease performance. 
> > We could, for example, do the same thing as above, but with different if
> > statements in onGetScanlines.
> > (such as "JCS_CMYK == fCodec->fDecoderMgr->dinfo()->out_color_space") 
> > Adding that change could potentially speed it up for any of the if
statements.
> > Why should we choose to optimize this specific if statement?
> > I think if SkJpegCodec was performing too slow for users, then maybe we
could
> go
> > back and optimize if statements. 
> > But since there is a minimal performance decrease, and the reason why we are
> > worried about this if statement and not other if statements isn't clear, I
> don't
> > think this change is worth it.
> > (Leon or Matt, if you think this change is worth it still, let me know)  
> 
> I think Leon's suggestion is a good one.  We did regress performance by 2.5%,
> and while it's not awful, it's worth it to see if we can do better.  Taking a
> closer look, it seems to me that the two possible causes are:
> 1) Memory allocation of fSrcRow (it looks like you've already fixed this to
only
> allocate when we need a swizzler)
> 2) If statements inside for loop in onGetScanlines()
> 
> Unless we gain the whole 2.5% back from the fix to (1), I think we should try
> fixing (2) as well.  I think the idea of Leon's design is to factor the:
> if (swizzler) {
>  // code
> } else {
>   //other code
> }
> outside of the for loop, so we only check the branch once instead of height
> times.  It seems logical to me.

So I re-ran the performance tests, after changing fSrcRow to only be allocated
if we are sampling, and the code is now faster than master by 0.61%. So that
change seemed to fix the slow down! 
I've added the new performance results to the same google-doc.

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:276: static int get_sample_size(int srcDimension, int
dstDimension) {
On 2015/07/31 13:35:33, scroggo wrote:
> On 2015/07/30 22:27:56, emmaleer wrote:
> > On 2015/07/30 18:05:57, scroggo wrote:
> > > Can this be shared with the other function that looks like this?
> > 
> > Yes, it now calls the function in SkScaledCodec.h
> > Currently it is a public function, but I remember you saying that we didn't
> want
> > it to be public?
> > Why not? I don't see any harm of it being public, since it doesn't change
any
> > variables. 
> 
> I just meant that it should not be a part of the public API - i.e. not in
> include/ (not on SkCodec). You've made it a public function on SkScaledCodec,
> which is in src/codec, which I think is appropriate.
> 
> > I could change it to private, and make SkScaledCodec and SkSwizzler friend
> > classes. Would that be better? (if so, why would that be better)
> 
> No, friend classes should be used sparingly.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/100001/include/codec/SkScaled...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1260673002/diff/100001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:31: static int get_sample_size(int srcDimension,
int dstDimension) {
On 2015/07/31 13:35:33, scroggo wrote:
> style nit: Skia has different conventions for static functions and static
member
> functions. While we name static functions get_sample_size, we name static
member
> functions GetSampleSize.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:742: if (dstInfo.height() !=
this->getInfo().height()) {
On 2015/07/31 13:35:33, scroggo wrote:
> Maybe this should be another static method on SkScaledCodec, something like:
> 
> bool DimensionsSupportedForSampling(srcInfo, dstInfo);

Acknowledged.

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

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:510: SkAutoMalloc               fStorage;
On 2015/07/31 13:35:33, scroggo wrote:
> This one is only needed for sampling, too, right?

Yep! I've added a comment.

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:550: if (dstInfo.height() != this->getInfo().height())
{
On 2015/07/31 13:35:33, scroggo wrote:
> This should call DimensionsSupportedForSampling

Acknowledged.

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:42: // NOTE: we round down here for scaled
dimension, beucase it matches the behavior of SkImageDecoder
On 2015/07/31 13:35:33, scroggo wrote:
> because*
> 
> (Although I think you can be a little more concise: "to match the behavior of
> SkImageDecoder")

Acknowledged.

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:137: bool isHardToSample =
scanlineDecoder->isHardToSample();
On 2015/07/31 13:35:33, scroggo wrote:
> nit: can be const

Acknowledged.

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

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:31: return 0xFF;
On 2015/07/31 13:35:33, scroggo wrote:
> Since this is opaque, what you want is to return 0xFFFF (see the comment in
> GetResult).
> 
> I refreshed my memory - zeroAlpha is to track if the entire image is
> transparent, so it is non-zero only if the image is not transparent.
> 
> The name is probably not great, although I do not currently have an idea for a
> better one.
> 
> On a related note, I just realized that UPDATE_RESULT_ALPHA (not used in this
> function) will compute its expression twice. In many cases we use the
following
> pattern:
I don't understand what you mean by "will compute its expression twice" Do you
mean will compute zeroAlpha twice? When looking at UPDATE_RESULT_ALPHA it looks
like it updates zeroAlpha and maxAlpha, but I don't understand now it computes
zeroAlpha twice. 
> 
> alpha = <expression>;
> UPDATE_RESULT_ALPHA(alpha)
> 
> but occasionally we use the following pattern:
> 
> UPDATE_RESULT_ALPHA(<expression>);
> 
> We should probably switch to the former everywhere (and verify that it makes a
> difference). (That should be its own CL.)
Do you think it will make a difference by increasing performance?

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:118: *  @param dstInfo dimensions() describe destination.
On 2015/07/31 13:35:33, scroggo wrote:
> The comment used to distinguish between the dimensions, which applied to both
> source and width, and the other fields, which only applied to the dst. Now the
> whole struct describes the dst (not just the dimensions).

Acknowledged.

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:124: *  @param srcWidth is the width of the source. Used
to calculate the width samplesize.
On 2015/07/31 13:35:33, scroggo wrote:
> I think this could use a little bit more explanation. I think it's correct
that
> we support a width sample size but not a height, but it's not necessarily
> intuitive. It would be good to explain the use here.

Acknowledged.

scroggo

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

5 years, 4 months ago (2015-07-31 19:31:57 UTC) #21

scroggo

This is looking pretty good. Adding another set of eyes/API check. https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): ...

5 years, 4 months ago (2015-07-31 19:31:59 UTC) #22

This is looking pretty good. Adding another set of eyes/API check.

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

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:449: if (fCodec->fSwizzler) {
On 2015/07/31 18:41:56, emmaleer wrote:
> On 2015/07/31 12:17:59, msarett wrote:
> > On 2015/07/30 22:27:56, emmaleer wrote:
> > > On 2015/07/30 18:28:23, scroggo wrote:
> > > > So it looks like these introduce a slight performance hit? Maybe we
could
> > try
> > > > something like the following:
> > > > 
> > > > constructor {
> > > >   if (fCodec->fSwizzler) {
> > > >     mScanlineProc = &decodeAndSwizzle;
> > > >   } else {
> > > >     mScanlineProc = &decodeDirect;
> > > >   }
> > > > }
> > > > 
> > > > onGetScanlineInternal(void* dstRow) {
> > > >   // Does everything inside the loop
> > > > }
> > > > 
> > > > onGetScanlines() {
> > > >   for (int i = 0; i < count; i++) {
> > > >     mScanlineProc;
> > > >   }
> > > > }
> > > > 
> > > > decodeAndSwizzle() {
> > > >   // setup
> > > >   onGetScanlineInternal
> > > > }
> > > > 
> > > > decodeDirect() {
> > > >   onGetScanlineInternal
> > > > }
> > > > 
> > > > Not sure if it would be faster, but something to consider. Maybe the
> > > difference
> > > > is neglible?
> > > 
> > > I think it's negligible. Also, I don't think this one specific if should
> drive
> > a
> > > function change this big, since all if statements decrease performance. 
> > > We could, for example, do the same thing as above, but with different if
> > > statements in onGetScanlines.
> > > (such as "JCS_CMYK == fCodec->fDecoderMgr->dinfo()->out_color_space") 
> > > Adding that change could potentially speed it up for any of the if
> statements.
> > > Why should we choose to optimize this specific if statement?
> > > I think if SkJpegCodec was performing too slow for users, then maybe we
> could
> > go
> > > back and optimize if statements. 
> > > But since there is a minimal performance decrease, and the reason why we
are
> > > worried about this if statement and not other if statements isn't clear, I
> > don't
> > > think this change is worth it.
> > > (Leon or Matt, if you think this change is worth it still, let me know)  
> > 
> > I think Leon's suggestion is a good one.  We did regress performance by
2.5%,
> > and while it's not awful, it's worth it to see if we can do better.  Taking
a
> > closer look, it seems to me that the two possible causes are:
> > 1) Memory allocation of fSrcRow (it looks like you've already fixed this to
> only
> > allocate when we need a swizzler)
> > 2) If statements inside for loop in onGetScanlines()
> > 
> > Unless we gain the whole 2.5% back from the fix to (1), I think we should
try
> > fixing (2) as well.  I think the idea of Leon's design is to factor the:
> > if (swizzler) {
> >  // code
> > } else {
> >   //other code
> > }
> > outside of the for loop, so we only check the branch once instead of height
> > times.  It seems logical to me.
> 
> So I re-ran the performance tests, after changing fSrcRow to only be allocated
> if we are sampling, and the code is now faster than master by 0.61%. So that
> change seemed to fix the slow down! 
> I've added the new performance results to the same google-doc.

That's great! I'm a little surprised that it is faster than master, though. That
is comparing running nanobench on JPEG, without used the scaled codec? (If it is
using the scaled codec, the difference may be us sampling versus letting JPEG
sample - not that I expect us to be faster there either, necessarily.)

Glad you tried the simple change and tested it!

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

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:31: return 0xFF;
On 2015/07/31 18:41:56, emmaleer wrote:
> On 2015/07/31 13:35:33, scroggo wrote:
> > Since this is opaque, what you want is to return 0xFFFF (see the comment in
> > GetResult).
> > 
> > I refreshed my memory - zeroAlpha is to track if the entire image is
> > transparent, so it is non-zero only if the image is not transparent.
> > 
> > The name is probably not great, although I do not currently have an idea for
a
> > better one.
> > 
> > On a related note, I just realized that UPDATE_RESULT_ALPHA (not used in
this
> > function) will compute its expression twice. In many cases we use the
> following
> > pattern:
> I don't understand what you mean by "will compute its expression twice" Do you
> mean will compute zeroAlpha twice? When looking at UPDATE_RESULT_ALPHA it
looks
> like it updates zeroAlpha and maxAlpha, but I don't understand now it computes
> zeroAlpha twice. 

No, I mean the expression that is passed to the macro. The macro is defined in
SkCodecPriv.h as:

#define UPDATE_RESULT_ALPHA(alpha)              \
    zeroAlpha |= (alpha);                       \
    maxAlpha  &= (alpha);

So the expression is "alpha". If you pass a variable to it, it will just use
that variable twice.

However, if you pass an expression, like "ctable[index] >> SK_A32_SHIFT" to the
macro, it will be computed twice. (Maybe a smart compiler will recognize that
this has no side effects, so it will store the result to use for the second one.
I'm often advised not to rely on the compiler though, and I'm not sure whether
it will given our settings.)

> > 
> > alpha = <expression>;
> > UPDATE_RESULT_ALPHA(alpha)
> > 
> > but occasionally we use the following pattern:
> > 
> > UPDATE_RESULT_ALPHA(<expression>);
> > 
> > We should probably switch to the former everywhere (and verify that it makes
a
> > difference). (That should be its own CL.)
> Do you think it will make a difference by increasing performance?

It could, in cases where we call UPDATE_RESULT_ALPHA with an expression as
opposed to a local variable. It seems like a small difference, but if we are
computing it for every pixel, it could make a difference.

https://codereview.chromium.org/1260673002/diff/140001/include/codec/SkScaled...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1260673002/diff/140001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:29: static bool
DimensionsSupportedForSampling(const SkImageInfo& srcInfo,
nit: newline between functions.

https://codereview.chromium.org/1260673002/diff/140001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:30: const SkImageInfo& dstInfo){
nit: space between ) and {

https://codereview.chromium.org/1260673002/diff/140001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:56: bool onReallyHasAlpha() const override{
nit: There should be a newline before this method.

https://codereview.chromium.org/1260673002/diff/140001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:65: 
nit: Unnecessary newline.

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

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:193: SkCodec::Result
SkJpegCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
It looks like the swizzler is never used in getPixels? That's probably correct
(it's only used for sampling in X), but maybe add comments explaining that?

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:326: // reset scale settings on failure
Can you explain why? "because this may be supported by the swizzler." or
something like that.

Also, it seems like this is no longer a failure case?

I think the name of the function and its comments should probably be updated to
reflect that this now returns whether it can *natively* scale to these
dimensions.

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:329: return fDecoderMgr->returnFalse("could not scale
to requested dimensions");
I wonder if this error message is necessary now that we will try a backup? (Or
maybe it should just be updated?)

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:31: return 0xFFFF;
// 565 is always opaque.

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:123: *  Width sampling is supported by the swizzler, by
skipping pixels when swizzling the row.
Maybe this should also explain why we chose to support width sampling, but not
height? Something like "sampling in Y can be done by a client with a scanline
decoder, but sampling in X allows the swizzler to skip swizzling pixels and
reading from and writing to memory."

nit: this should be indented. (I do not have a strong preference for how much,
and I am not sure whether our guides tell us.)

emmaleer

Should we commit the DMSrcSink.cpp changes? Or should we add separate tests to test SkScaledCodec ...

5 years, 4 months ago (2015-07-31 20:43:10 UTC) #23

Should we commit the DMSrcSink.cpp changes?
Or should we add separate tests to test SkScaledCodec

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

https://codereview.chromium.org/1260673002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:449: if (fCodec->fSwizzler) {
On 2015/07/31 19:31:58, scroggo wrote:
> On 2015/07/31 18:41:56, emmaleer wrote:
> > On 2015/07/31 12:17:59, msarett wrote:
> > > On 2015/07/30 22:27:56, emmaleer wrote:
> > > > On 2015/07/30 18:28:23, scroggo wrote:
> > > > > So it looks like these introduce a slight performance hit? Maybe we
> could
> > > try
> > > > > something like the following:
> > > > > 
> > > > > constructor {
> > > > >   if (fCodec->fSwizzler) {
> > > > >     mScanlineProc = &decodeAndSwizzle;
> > > > >   } else {
> > > > >     mScanlineProc = &decodeDirect;
> > > > >   }
> > > > > }
> > > > > 
> > > > > onGetScanlineInternal(void* dstRow) {
> > > > >   // Does everything inside the loop
> > > > > }
> > > > > 
> > > > > onGetScanlines() {
> > > > >   for (int i = 0; i < count; i++) {
> > > > >     mScanlineProc;
> > > > >   }
> > > > > }
> > > > > 
> > > > > decodeAndSwizzle() {
> > > > >   // setup
> > > > >   onGetScanlineInternal
> > > > > }
> > > > > 
> > > > > decodeDirect() {
> > > > >   onGetScanlineInternal
> > > > > }
> > > > > 
> > > > > Not sure if it would be faster, but something to consider. Maybe the
> > > > difference
> > > > > is neglible?
> > > > 
> > > > I think it's negligible. Also, I don't think this one specific if should
> > drive
> > > a
> > > > function change this big, since all if statements decrease performance. 
> > > > We could, for example, do the same thing as above, but with different if
> > > > statements in onGetScanlines.
> > > > (such as "JCS_CMYK == fCodec->fDecoderMgr->dinfo()->out_color_space") 
> > > > Adding that change could potentially speed it up for any of the if
> > statements.
> > > > Why should we choose to optimize this specific if statement?
> > > > I think if SkJpegCodec was performing too slow for users, then maybe we
> > could
> > > go
> > > > back and optimize if statements. 
> > > > But since there is a minimal performance decrease, and the reason why we
> are
> > > > worried about this if statement and not other if statements isn't clear,
I
> > > don't
> > > > think this change is worth it.
> > > > (Leon or Matt, if you think this change is worth it still, let me know) 

> > > 
> > > I think Leon's suggestion is a good one.  We did regress performance by
> 2.5%,
> > > and while it's not awful, it's worth it to see if we can do better. 
Taking
> a
> > > closer look, it seems to me that the two possible causes are:
> > > 1) Memory allocation of fSrcRow (it looks like you've already fixed this
to
> > only
> > > allocate when we need a swizzler)
> > > 2) If statements inside for loop in onGetScanlines()
> > > 
> > > Unless we gain the whole 2.5% back from the fix to (1), I think we should
> try
> > > fixing (2) as well.  I think the idea of Leon's design is to factor the:
> > > if (swizzler) {
> > >  // code
> > > } else {
> > >   //other code
> > > }
> > > outside of the for loop, so we only check the branch once instead of
height
> > > times.  It seems logical to me.
> > 
> > So I re-ran the performance tests, after changing fSrcRow to only be
allocated
> > if we are sampling, and the code is now faster than master by 0.61%. So that
> > change seemed to fix the slow down! 
> > I've added the new performance results to the same google-doc.
> 
> That's great! I'm a little surprised that it is faster than master, though.
That
> is comparing running nanobench on JPEG, without used the scaled codec? (If it
is
> using the scaled codec, the difference may be us sampling versus letting JPEG
> sample - not that I expect us to be faster there either, necessarily.)
> 
> Glad you tried the simple change and tested it!

Yes, it is comparing running nanobench on JPEG, without using the scaled codec.

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

https://codereview.chromium.org/1260673002/diff/100001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:31: return 0xFF;
On 2015/07/31 19:31:58, scroggo wrote:
> On 2015/07/31 18:41:56, emmaleer wrote:
> > On 2015/07/31 13:35:33, scroggo wrote:
> > > Since this is opaque, what you want is to return 0xFFFF (see the comment
in
> > > GetResult).
> > > 
> > > I refreshed my memory - zeroAlpha is to track if the entire image is
> > > transparent, so it is non-zero only if the image is not transparent.
> > > 
> > > The name is probably not great, although I do not currently have an idea
for
> a
> > > better one.
> > > 
> > > On a related note, I just realized that UPDATE_RESULT_ALPHA (not used in
> this
> > > function) will compute its expression twice. In many cases we use the
> > following
> > > pattern:
> > I don't understand what you mean by "will compute its expression twice" Do
you
> > mean will compute zeroAlpha twice? When looking at UPDATE_RESULT_ALPHA it
> looks
> > like it updates zeroAlpha and maxAlpha, but I don't understand now it
computes
> > zeroAlpha twice. 
> 
> No, I mean the expression that is passed to the macro. The macro is defined in
> SkCodecPriv.h as:
> 
> #define UPDATE_RESULT_ALPHA(alpha)              \
>     zeroAlpha |= (alpha);                       \
>     maxAlpha  &= (alpha);
> 
> So the expression is "alpha". If you pass a variable to it, it will just use
> that variable twice.
> 
> However, if you pass an expression, like "ctable[index] >> SK_A32_SHIFT" to
the
> macro, it will be computed twice. (Maybe a smart compiler will recognize that
> this has no side effects, so it will store the result to use for the second
one.
> I'm often advised not to rely on the compiler though, and I'm not sure whether
> it will given our settings.)
> 
> > > 
> > > alpha = <expression>;
> > > UPDATE_RESULT_ALPHA(alpha)
> > > 
> > > but occasionally we use the following pattern:
> > > 
> > > UPDATE_RESULT_ALPHA(<expression>);
> > > 
> > > We should probably switch to the former everywhere (and verify that it
makes
> a
> > > difference). (That should be its own CL.)
> > Do you think it will make a difference by increasing performance?
> 
> It could, in cases where we call UPDATE_RESULT_ALPHA with an expression as
> opposed to a local variable. It seems like a small difference, but if we are
> computing it for every pixel, it could make a difference.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/140001/include/codec/SkScaled...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1260673002/diff/140001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:29: static bool
DimensionsSupportedForSampling(const SkImageInfo& srcInfo,
On 2015/07/31 19:31:58, scroggo wrote:
> nit: newline between functions.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/140001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:30: const SkImageInfo& dstInfo){
On 2015/07/31 19:31:58, scroggo wrote:
> nit: space between ) and {

Acknowledged.

https://codereview.chromium.org/1260673002/diff/140001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:56: bool onReallyHasAlpha() const override{
On 2015/07/31 19:31:58, scroggo wrote:
> nit: There should be a newline before this method.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/140001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:65: 
On 2015/07/31 19:31:58, scroggo wrote:
> nit: Unnecessary newline.

Acknowledged.

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

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:193: SkCodec::Result
SkJpegCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
On 2015/07/31 19:31:58, scroggo wrote:
> It looks like the swizzler is never used in getPixels? That's probably correct
> (it's only used for sampling in X), but maybe add comments explaining that?

Acknowledged.

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:326: // reset scale settings on failure
On 2015/07/31 19:31:58, scroggo wrote:
> Can you explain why? "because this may be supported by the swizzler." or
> something like that.
> 
> Also, it seems like this is no longer a failure case?
> 
> I think the name of the function and its comments should probably be updated
to
> reflect that this now returns whether it can *natively* scale to these
> dimensions.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:329: return fDecoderMgr->returnFalse("could not scale
to requested dimensions");
On 2015/07/31 19:31:58, scroggo wrote:
> I wonder if this error message is necessary now that we will try a backup? (Or
> maybe it should just be updated?)

I removed the error. I think it's okay for the function to return false, so it
should not be an error.

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:31: return 0xFFFF;
On 2015/07/31 19:31:58, scroggo wrote:
> // 565 is always opaque.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:123: *  Width sampling is supported by the swizzler, by
skipping pixels when swizzling the row.
On 2015/07/31 19:31:58, scroggo wrote:
> Maybe this should also explain why we chose to support width sampling, but not
> height? Something like "sampling in Y can be done by a client with a scanline
> decoder, but sampling in X allows the swizzler to skip swizzling pixels and
> reading from and writing to memory."
> 
> nit: this should be indented. (I do not have a strong preference for how much,
> and I am not sure whether our guides tell us.)

Acknowledged.

scroggo

On 2015/07/31 20:43:10, emmaleer wrote: > Should we commit the DMSrcSink.cpp changes? > Or should ...

5 years, 4 months ago (2015-07-31 21:11:57 UTC) #24

On 2015/07/31 20:43:10, emmaleer wrote:
> Should we commit the DMSrcSink.cpp changes?
> Or should we add separate tests to test SkScaledCodec

Good question. DM already tests at various scales. It will only make a
difference for the following cases:
- For codecs that do not support scaling (and already support scanlines - i.e.
png), we will now
  start testing them scaled
- For jpeg, if the scale is not supported natively, we will use the scaled
codec. It seems like we may bypass
  jpeg's native scaling, though. SkScaledCodec::onGetScaledDimensions does not
check with fCodec, and as we
  discussed it seems like jpeg rounds differently. So if you pass a scale of
.875 to getScaledDimensions,
  ScaledCodec may suggest something that jpeg does not support, so we'll stop
testing it. (This can be
  fixed somewhat simply - I think it leaves the following question though: what
does SkScaledCodec do
  if fCodec can scale part of the way?)

This reminded me of a difference between SkImageDecoder and SkScaledCodec - the
jpeg version of image decoder
lets jpeg do the downscaling it supports, and then samples further if necessary.
SkScaledCodec skips to
sampling if jpeg cannot get all the way. Does that difference matter?

I think with the JPEG issues fixed it would make sense to commit the DM changes.

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

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:329: return fDecoderMgr->returnFalse("could not scale
to requested dimensions");
On 2015/07/31 20:43:09, emmaleer wrote:
> On 2015/07/31 19:31:58, scroggo wrote:
> > I wonder if this error message is necessary now that we will try a backup?
(Or
> > maybe it should just be updated?)
> 
> I removed the error. I think it's okay for the function to return false, so it
> should not be an error.

Agreed for your case, although we should perhaps make the caller from
onGetPixels (which does not allow sampling) print this error message.

https://codereview.chromium.org/1260673002/diff/160001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/160001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:53: SkISize
SkScaledCodec::onGetScaledDimensions(float desiredScale) const {
Should this check with fCodec first? If fCodec is an SkJpegCodec, it may be able
to approximate the scale, right?

djsollen

https://codereview.chromium.org/1260673002/diff/160001/include/codec/SkCodec.h File include/codec/SkCodec.h (right): https://codereview.chromium.org/1260673002/diff/160001/include/codec/SkCodec.h#newcode55 include/codec/SkCodec.h:55: * This is a suggestion to the user. what ...

5 years, 4 months ago (2015-08-03 13:13:59 UTC) #25

emmaleer

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkJpegCodec.cpp#newcode329 src/codec/SkJpegCodec.cpp:329: return fDecoderMgr->returnFalse("could not scale to requested dimensions"); On 2015/07/31 ...

5 years, 4 months ago (2015-08-03 14:29:41 UTC) #26

emmaleer

Leon, I've added a function, best_scaled_dimensions, to compare the native getScaledDimensions return value with SkScaledCodec's ...

5 years, 4 months ago (2015-08-03 14:41:26 UTC) #27

scroggo

> https://codereview.chromium.org/1260673002/diff/160001/include/codec/SkScanlineDecoder.h#newcode85 > include/codec/SkScanlineDecoder.h:85: bool isHardToSample() { > On 2015/08/03 13:13:59, djsollen wrote: > > ...

5 years, 4 months ago (2015-08-03 17:02:55 UTC) #28

>
https://codereview.chromium.org/1260673002/diff/160001/include/codec/SkScanli...
> include/codec/SkScanlineDecoder.h:85: bool isHardToSample() {
> On 2015/08/03 13:13:59, djsollen wrote:
> > isInterlaced?
> 
> This function return's whether we can sample normally, or if we have to decode
> every line and then sample afterwards.
> Interlaced png's is one example, but there may be others in the future.
> Also, other types of interlaced images, such as gif and jpeg, can be scaled
> normally.
> Options for the name so far are: canSampleMidway, requiresPostSampling, and
> isHardToSample.
> Matt and I were talking, and think requiresPostSampling might work.
> I'm going to ask Leon what he thinks, and then will change the name.
> If you have any better suggestions for a name, please let me know.

My vote is requiresPostYSampling.

On 2015/08/03 14:41:26, emmaleer wrote:
> Leon, I've added a function, best_scaled_dimensions, to 
> compare the native getScaledDimensions return value with 
> SkScaledCodec's getScaledDimensions return value. Let me know 
> what you think.

The function is good, with some inline comments.

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

https://codereview.chromium.org/1260673002/diff/140001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:329: return fDecoderMgr->returnFalse("could not scale
to requested dimensions");
On 2015/08/03 14:29:41, emmaleer wrote:
> On 2015/07/31 21:11:57, scroggo wrote:
> > On 2015/07/31 20:43:09, emmaleer wrote:
> > > On 2015/07/31 19:31:58, scroggo wrote:
> > > > I wonder if this error message is necessary now that we will try a
backup?
> > (Or
> > > > maybe it should just be updated?)
> > > 
> > > I removed the error. I think it's okay for the function to return false,
so
> it
> > > should not be an error.
> > 
> > Agreed for your case, although we should perhaps make the caller from
> > onGetPixels (which does not allow sampling) print this error message.
> 
> If the function fails onGetPixels already returns:
> fDecoderMgr->returnFailure("cannot scale to requested dims", kInvalidScale);

Ok, great, so this is fine to remove.

https://codereview.chromium.org/1260673002/diff/180001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/180001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:50: static SkISize best_scaled_dimensions(SkISize
origDims, SkISize nativeDims,
All the SkISize parameters should be const references, so we do not copy them.

https://codereview.chromium.org/1260673002/diff/180001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:60: float nativeWDiff = fabs(idealWidth -
nativeDims.width());
nit: You should use SkTAbs, from SkTypes.h

https://codereview.chromium.org/1260673002/diff/180001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:70: return nativeDiff > scaledCodecDiff ?
scaledCodecDims : nativeDims;
This might deserve a comment - did you deliberately set this up so that if the
differences are the same we would choose the native dimensions? - e.g. the case
we were talking about today:

JPEG 101 x 101
ask for scale of .5

native dim: 51 x 51
scaled dim: 50 x 50

ideal dim: 50.5 x 50.5

Difference is .5 in both cases.

I think we want to pick the native dim, and I think this code does that, but I
want to make sure that it does, and that it is intentional.

https://codereview.chromium.org/1260673002/diff/180001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:77: // support scaling down by integer numbers. Ex:
1/2, 1/3, 1/4 ...
This comment actually applies to line 79. The immediately following line now
checks native scaling.

emmaleer

https://codereview.chromium.org/1260673002/diff/180001/src/codec/SkScaledCodec.cpp File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/180001/src/codec/SkScaledCodec.cpp#newcode50 src/codec/SkScaledCodec.cpp:50: static SkISize best_scaled_dimensions(SkISize origDims, SkISize nativeDims, On 2015/08/03 17:02:55, ...

5 years, 4 months ago (2015-08-03 19:00:40 UTC) #29

scroggo

I thought you were going to make SkScaledCodec do sampling on top of the native ...

5 years, 4 months ago (2015-08-03 19:16:21 UTC) #30

emmaleer

Added partial native scaling functionality to SkJpegCodec https://codereview.chromium.org/1260673002/diff/200001/include/codec/SkScaledCodec.h File include/codec/SkScaledCodec.h (right): https://codereview.chromium.org/1260673002/diff/200001/include/codec/SkScaledCodec.h#newcode76 include/codec/SkScaledCodec.h:76: SkScaledCodec(SkCodec*); On ...

5 years, 4 months ago (2015-08-04 18:56:53 UTC) #31

scroggo

The rest of your changes in this latest CL look good, but I'm hesitant on ...

5 years, 4 months ago (2015-08-04 20:05:21 UTC) #32

The rest of your changes in this latest CL look good, but I'm hesitant on the
partiallyNativeScaling. It adds a bit of state to SkCodec, and it seems to push
SkScaledCodec into the implementations a little too much. (We already do that
for plain sampling, but it seems rather isolated in that case.) Maybe there's a
cleaner way to do it, and maybe it's not vitally important. I would recommend
pulling out the partial native sampling, filing a bug, and landing the change
without it (once it gets approval - other than partial native sampling, I am
happy to give mine).

https://codereview.chromium.org/1260673002/diff/240001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1260673002/diff/240001/dm/DM.cpp#newcode215
dm/DM.cpp:215: const float scales[] = { 0.1f, 0.125f, 0.16666666f, 0.2f, 0.25f,
0.3f, 0.375f, 0.4f, 0.5f, 0.6f,
Could you add comments explaining why we chose these particular scale values?

https://codereview.chromium.org/1260673002/diff/240001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1260673002/diff/240001/include/codec/SkCodec....
include/codec/SkCodec.h:63: * Checks if we can patially natively scale to the
desiredScale, and partially natively scales
"partially" is missing r (the first time)

https://codereview.chromium.org/1260673002/diff/240001/include/codec/SkCodec....
include/codec/SkCodec.h:70: int partiallyNativelyScale(float desiredScale) {
I'm not a huge fan of this name. I am not sure I have a better name though. This
does the following:

A) Tells the codec to scale
B) Returns the sample size needed afterwards

On a related note, this function behaves a lot differently from the other
functions. Although I've been arguing recently that SkCodec is not really
stateless, the only state it currently has is related to the stream - no other
APIs should change future function calls.

This function also feels like it leaks a lot of SkScaledCodec into SkCodec and
SkJpegCodec.

Right now I'm leaning towards removing the partial scaling, with a TODO and a
bug to do it later.

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

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:345: float nativeScales[7] = {0.125, 0.25, 0.375, 0.5,
0.625, 0.75, 0.875};
nit: I think these can (should?) be static const (which we would label as
gNativeScales).

nit: Please add spaces to the beginning and end (i.e. before 0.125 and after
.875).

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:351: float totalScale = nativeScales[index] /
sampleSize;
Can this just be "nativeScales[0]"?

Or do you think it's valuable to make it clear that this is the same calculation
done in the loop?

Maybe this would be better written as

while (true) {
  totalScale = nativeScales[index] / sampleSize;
  if (totalScale == desiredScale) {
    break; // success!
  }

  if (totalScale < desiredScale) {
    if (index == 7) {
      return -1;
    }
  } else {
    sampleSize++
  }
}

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:353: while(totalScale != desiredScale) {
nit: there should be a space between while and (

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:193: scanlineDecoder->skipScanlines(Y0);
Can't this return a result other than success? I think this should return if we
could not skip.

msarett

Agreed with Leon. Partial scaling is tricky and shouldn't hold up the rest of this ...

5 years, 4 months ago (2015-08-04 20:39:37 UTC) #33

Agreed with Leon.  Partial scaling is tricky and shouldn't hold up the rest of
this CL.

I'm wondering if we can handle partial scaling using onGetScaledDimensions(). 
Instead of returning the "closest scale" like we do right now, we could return
the scale that is most useful in reaching the target scale.

Then SkScaledCodec can look at what onGetScaledDimensions() wants to do and
decide if it wants to use the suggestion and do the rest of the partial scale,
or if it wants to ignore the suggestion and do all of the scaling on its own.

Regardless of the approach, this is complicated and we should demonstrate that
there is something to be gained by using this strategy.

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

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:344: int SkJpegCodec::onPartiallyNativelyScale(float
desiredScale) {
I think this function is clever :)

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:474:
fStorage.reset(codec->fDecoderMgr->dinfo()->output_width *
This will always be enough, but is overkill in the case of 565 (3 color
components stored in 2 bytes).

Maybe it should be:
colorBytes = (cinfo->out_color_space == JCS_RGB565) ? 2 :
cinfo->out_color_components;

I realize that this is not your bug, it's mine on line 541.  Can you fix both?
:)

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:525: convert_CMYK_to_RGBA(dstRow,
fCodec->fDecoderMgr->dinfo()->output_width);
Nice!

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:605: // update num and denom as partiallyNativelyScale
was previously called
Ah this is the part where we are unhappy.  We need to remember to partially
native scale because we did it the last time.  I agree that this is awkward.

emmaleer

PartialNativeScaling is removed. https://codereview.chromium.org/1260673002/diff/240001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1260673002/diff/240001/dm/DM.cpp#newcode215 dm/DM.cpp:215: const float scales[] = { 0.1f, ...

5 years, 4 months ago (2015-08-05 14:47:51 UTC) #34

PartialNativeScaling is removed.

https://codereview.chromium.org/1260673002/diff/240001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1260673002/diff/240001/dm/DM.cpp#newcode215
dm/DM.cpp:215: const float scales[] = { 0.1f, 0.125f, 0.16666666f, 0.2f, 0.25f,
0.3f, 0.375f, 0.4f, 0.5f, 0.6f,
On 2015/08/04 20:05:21, scroggo wrote:
> Could you add comments explaining why we chose these particular scale values?

Before we were only testing scaled values that Jpeg natively supports. 
I wanted to test values that it didn't natively support, to test the partial
scaling. 
I think it's good to leave them, so we test the sampling functionality for jpeg
and don't always use the native scaling method.

https://codereview.chromium.org/1260673002/diff/240001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1260673002/diff/240001/include/codec/SkCodec....
include/codec/SkCodec.h:63: * Checks if we can patially natively scale to the
desiredScale, and partially natively scales
On 2015/08/04 20:05:21, scroggo wrote:
> "partially" is missing r (the first time)

Acknowledged.

https://codereview.chromium.org/1260673002/diff/240001/include/codec/SkCodec....
include/codec/SkCodec.h:70: int partiallyNativelyScale(float desiredScale) {
On 2015/08/04 20:05:21, scroggo wrote:
> I'm not a huge fan of this name. I am not sure I have a better name though.
This
> does the following:
> 
> A) Tells the codec to scale
> B) Returns the sample size needed afterwards
> 
> On a related note, this function behaves a lot differently from the other
> functions. Although I've been arguing recently that SkCodec is not really
> stateless, the only state it currently has is related to the stream - no other
> APIs should change future function calls.
> 
> This function also feels like it leaks a lot of SkScaledCodec into SkCodec and
> SkJpegCodec.
> 
> Right now I'm leaning towards removing the partial scaling, with a TODO and a
> bug to do it later.

Okay, I'll first upload the changes for future reference, then remove partial
scaling.

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

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:345: float nativeScales[7] = {0.125, 0.25, 0.375, 0.5,
0.625, 0.75, 0.875};
On 2015/08/04 20:05:21, scroggo wrote:
> nit: I think these can (should?) be static const (which we would label as
> gNativeScales).
> 
> nit: Please add spaces to the beginning and end (i.e. before 0.125 and after
> .875).

Acknowledged.

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:351: float totalScale = nativeScales[index] /
sampleSize;
On 2015/08/04 20:05:21, scroggo wrote:
> Can this just be "nativeScales[0]"?
> 
> Or do you think it's valuable to make it clear that this is the same
calculation
> done in the loop?
Yes, it can just be gNativeScales[0]
> 
> Maybe this would be better written as
> 
> while (true) {
>   totalScale = nativeScales[index] / sampleSize;
>   if (totalScale == desiredScale) {
>     break; // success!
>   }
> 
>   if (totalScale < desiredScale) {
>     if (index == 7) {
>       return -1;
>     }
>   } else {
>     sampleSize++
>   }
> }

Okay, I like that method

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:353: while(totalScale != desiredScale) {
On 2015/08/04 20:05:21, scroggo wrote:
> nit: there should be a space between while and (

Acknowledged.

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:474:
fStorage.reset(codec->fDecoderMgr->dinfo()->output_width *
On 2015/08/04 20:39:36, msarett wrote:
> This will always be enough, but is overkill in the case of 565 (3 color
> components stored in 2 bytes).
> 
> Maybe it should be:
> colorBytes = (cinfo->out_color_space == JCS_RGB565) ? 2 :
> cinfo->out_color_components;
> 
> I realize that this is not your bug, it's mine on line 541.  Can you fix both?
> :) 

Acknowledged.

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/240001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:193: scanlineDecoder->skipScanlines(Y0);
On 2015/08/04 20:05:21, scroggo wrote:
> Can't this return a result other than success? I think this should return if
we
> could not skip.

Acknowledged.

scroggo

https://codereview.chromium.org/1260673002/diff/280001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1260673002/diff/280001/dm/DM.cpp#newcode216 dm/DM.cpp:216: // We test natively supported scales and non natively ...

5 years, 4 months ago (2015-08-05 15:36:01 UTC) #35

msarett

https://codereview.chromium.org/1260673002/diff/280001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1260673002/diff/280001/dm/DM.cpp#newcode218 dm/DM.cpp:218: 0.625f, 0.7f, 0.750f, 0.8f, 0.875f, 0.9f, 1.0f }; Per ...

5 years, 4 months ago (2015-08-05 15:54:56 UTC) #36

emmaleer

https://codereview.chromium.org/1260673002/diff/280001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1260673002/diff/280001/dm/DM.cpp#newcode216 dm/DM.cpp:216: // We test natively supported scales and non natively ...

5 years, 4 months ago (2015-08-05 18:41:52 UTC) #37

https://codereview.chromium.org/1260673002/diff/280001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1260673002/diff/280001/dm/DM.cpp#newcode216
dm/DM.cpp:216: // We test natively supported scales and non natively supported
scales, to test all functionalty
On 2015/08/05 15:36:00, scroggo wrote:
> Do we need all of these before we start supporting partial native scales?

I removed an couple, but I think we need most of them.

https://codereview.chromium.org/1260673002/diff/280001/dm/DM.cpp#newcode218
dm/DM.cpp:218: 0.625f, 0.7f, 0.750f, 0.8f, 0.875f, 0.9f, 1.0f };
On 2015/08/05 15:54:56, msarett wrote:
> Per our discussion in person, let's try to make it a little clearer what the
> scales are intended to test and maybe remove a few?
> 
> // SkJpegCodec natively supports scaling to: 0.125, 0.25, 0.375, 0.5, 0.625,
> 0.75, 0.875
> // 0.1, 0.16, 0.2 etc allow us to test SkScaledCodec with sampleSize 10, 6, 5,
> etc
> // 0.4, 0.7 etc allow to test what happens when the client requests a scale
that
> does not exactly match a sampleSize or native scaling capability

Acknowledged.

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

https://codereview.chromium.org/1260673002/diff/280001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:198: void* dst, size_t rowBytes,
On 2015/08/05 15:36:00, scroggo wrote:
> It looks like these parameters are not needed? I think you only need
> requestedInfo and options?

Acknowledged.

https://codereview.chromium.org/1260673002/diff/280001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:203: const SkColorType srcColorType =
requestedInfo.colorType();
On 2015/08/05 15:36:00, scroggo wrote:
> It seems weird that the requestedInfo yields the srcColorType. Oh wait - the
> swizzler for JPEG does not actually do any color conversion, right? So the src
> and dst colorType are always going to be the same? In that case, maybe the
names
> should just be "info" and "colorType"? (FWIW, it looks like we do not even
need
> a variable for color type - we can just say
> 
> switch (info.colorType())
> 
> )

Yes, the destination and source colorTypes are the same. Okay, I've changed
requestedInfo to info.

https://codereview.chromium.org/1260673002/diff/280001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:223:
fSwizzler.reset(SkSwizzler::CreateSwizzler(srcConfig, NULL, requestedInfo,
On 2015/08/05 15:36:00, scroggo wrote:
> Sorry, I've gone back and forth on this. If the swizzler is only needed by the
> SD, maybe this (and the swizzler) should go on the SD?

Yes, that makes sense. Now that the creation function, onStart, is in the SD
class, we can move initializeSwizzler, and the swizzler, into that class too. 
I need to first merge your code into my branch, and then I will move the
swizzler to the SD.

https://codereview.chromium.org/1260673002/diff/280001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:442: int colorBytes =
(codec->fDecoderMgr->dinfo()->out_color_space == JCS_RGB565) ? 2 :
On 2015/08/05 15:54:56, msarett wrote:
> On 2015/08/05 15:36:00, scroggo wrote:
> > Maybe make this a static function that takes a dinfo and returns colorBytes?
> > 
> > That said, it looks like both times you actually want the row bytes - maybe
> just
> > a function that returns rowbytes?
> > 
> > (it could also be a function on JpegDecoderMgr, although if it's only needed
> in
> > this file, and only operates on a dinfo, a static function might be better.
I
> > also wonder if jpeg has a function that computes this?)
> 
> Sorry for making a suggestion that led to code duplication...ugh.
> 
> I think a static helper function that computes rowBytes makes sense.
> 
> I'm not aware of jpeg have anything built in to compute this.  When I was
> allocating a row buffer inside libjpeg-turbo, DRC suggested something similar
to
> this code - which I think is a good indication that jpeg doesn't have anything
> better.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/280001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:510: int colorBytes =
(codec->fDecoderMgr->dinfo()->out_color_space == JCS_RGB565) ? 2 :        \
On 2015/08/05 15:36:00, scroggo wrote:
> colorBytes is unused - I assume you want to change storage below to
output_width
> * colorBytes? (That said, as stated above, I think you can just write a
function
> that returns the row bytes.)

Acknowledged.

scroggo

I have a couple of small comments, but this 1gtm. Go ahead and merge with ...

5 years, 4 months ago (2015-08-05 19:10:04 UTC) #38

emmaleer

Jpeg Swizzler is now in the ScanlineDecoder instead of SkJpegCodec. https://codereview.chromium.org/1260673002/diff/280001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/280001/src/codec/SkJpegCodec.cpp#newcode223 ...

5 years, 4 months ago (2015-08-06 13:45:46 UTC) #39

Jpeg Swizzler is now in the ScanlineDecoder instead of SkJpegCodec.

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

https://codereview.chromium.org/1260673002/diff/280001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:223:
fSwizzler.reset(SkSwizzler::CreateSwizzler(srcConfig, NULL, requestedInfo,
On 2015/08/05 19:10:04, scroggo wrote:
> On 2015/08/05 18:41:52, emmaleer wrote:
> > On 2015/08/05 15:36:00, scroggo wrote:
> > > Sorry, I've gone back and forth on this. If the swizzler is only needed by
> the
> > > SD, maybe this (and the swizzler) should go on the SD?
> > 
> > Yes, that makes sense. Now that the creation function, onStart, is in the SD
> > class, we can move initializeSwizzler, and the swizzler, into that class
too. 
> > I need to first merge your code into my branch, and then I will move the
> > swizzler to the SD.
> 
> sounds good

Swizzler is now in the ScanlineDecoder.

https://codereview.chromium.org/1260673002/diff/300001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1260673002/diff/300001/dm/DM.cpp#newcode219
dm/DM.cpp:219: const float scales[] = { 0.1f, 0.125f, 0.166f, 0.2f, 0.25f,
0.333f, 0.375f, 0.4f, 0.5f, 0.6f,
On 2015/08/05 19:10:04, scroggo wrote:
> This isn't new with your change, but I find all these scales clutter my output
> folders. How would you feel about appending the scale to  the name we pass to
> push_src when scale is not 1?
> 
> e.g.
> 
> push_src("image", "codec_kGray8_0.625f", ...
> 
> I'm happy to have that be a separate change, but I think it would make it
easier
> to find the images I want to find.
> 
> (or maybe the name should just be "codec_kGray8_scaled"?)

Okay. So then there would be a lot more folders right?
That might be harder to find mistakes. Since right now, I can just open 1 - 4
folders and glance over all the images to make sure they are okay. But if
there's a folder for each scale, there will be a lot of folders to open and
check.
I think codec_Gray8_scaled would be okay, since there would only be 2X as many
folders to check. 
I can make this change in a separate CL.

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

https://codereview.chromium.org/1260673002/diff/300001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:157: static int get_row_bytes(j_decompress_ptr dinfo)
{
On 2015/08/05 19:10:04, scroggo wrote:
> can this parameter be const?

Acknowledged.

https://codereview.chromium.org/1260673002/diff/300001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:206: const Options& options) {
On 2015/08/05 19:10:04, scroggo wrote:
> nit: spacing. This should line up with const, above it, or be indented 8
spaces.
> (Although it also looks like it might fit on the prior line?

Acknowledged.

https://codereview.chromium.org/1260673002/diff/300001/src/codec/SkJpegCodec.h
File src/codec/SkJpegCodec.h (right):

https://codereview.chromium.org/1260673002/diff/300001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.h:122: int                           fPartialDenom;
On 2015/08/05 19:10:04, scroggo wrote:
> These are no longer needed, right?

Acknowledged.

msarett

https://codereview.chromium.org/1260673002/diff/300001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1260673002/diff/300001/dm/DM.cpp#newcode219 dm/DM.cpp:219: const float scales[] = { 0.1f, 0.125f, 0.166f, 0.2f, ...

5 years, 4 months ago (2015-08-06 14:08:59 UTC) #40

scroggo

https://codereview.chromium.org/1260673002/diff/300001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1260673002/diff/300001/dm/DM.cpp#newcode219 dm/DM.cpp:219: const float scales[] = { 0.1f, 0.125f, 0.166f, 0.2f, ...

5 years, 4 months ago (2015-08-06 15:09:11 UTC) #41

https://codereview.chromium.org/1260673002/diff/300001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1260673002/diff/300001/dm/DM.cpp#newcode219
dm/DM.cpp:219: const float scales[] = { 0.1f, 0.125f, 0.166f, 0.2f, 0.25f,
0.333f, 0.375f, 0.4f, 0.5f, 0.6f,
On 2015/08/06 13:45:46, emmaleer wrote:
> On 2015/08/05 19:10:04, scroggo wrote:
> > This isn't new with your change, but I find all these scales clutter my
output
> > folders. How would you feel about appending the scale to  the name we pass
to
> > push_src when scale is not 1?
> > 
> > e.g.
> > 
> > push_src("image", "codec_kGray8_0.625f", ...
> > 
> > I'm happy to have that be a separate change, but I think it would make it
> easier
> > to find the images I want to find.
> > 
> > (or maybe the name should just be "codec_kGray8_scaled"?)
> 
> Okay. So then there would be a lot more folders right?
> That might be harder to find mistakes. Since right now, I can just open 1 - 4
> folders and glance over all the images to make sure they are okay. But if
> there's a folder for each scale, there will be a lot of folders to open and
> check.
> I think codec_Gray8_scaled would be okay, since there would only be 2X as many
> folders to check. 
> I can make this change in a separate CL. 

It would be more folders, but less to look at within each folder. Currently,
when I run "dm --src image --images resources/ -w <out>", and look in
<out>/8888/image/codec, I see the same image over and over again. Since JPEG is
the only one that currently supports scaling, it's hard to find other images.
With this CL, we'll see scaled images for all types (at least all the ones that
support scanline decoding), so that problem will go away, but we'll see even
more copies of the same image (one more for each new scale). We'll still be able
to compare them all in gold, but it seems to me that it would be simpler to see
one version of each image. Maybe I'm wrong though.

https://codereview.chromium.org/1260673002/diff/340001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1260673002/diff/340001/gyp/codec.gyp#newcode50
gyp/codec.gyp:50: '../src/codec/SkWebpCodec.cpp'
nit: typically we keep this comma here, so we can add another line without
having to add the comma then.

https://codereview.chromium.org/1260673002/diff/340001/include/codec/SkScanli...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1260673002/diff/340001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:53: *  scale that it can natively support
Maybe this should have a FIXME: share this with SkCodec?

I just filed skbug.com/4175 to track the changing relationship between SkCodec
and SkScanlineDecoder. Right now I'm toying with the idea of an abstract
interface that is implemented by both that holds the common methods etc (which I
also mention in that bug).

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

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:478: if (this->initializeSwizzler(dstInfo, options) !=
SkCodec::kSuccess) {
initializeSwizzler may return kUnimplemented, and then we compare to kSuccess.
If it is not kSuccess, we return kInvalidScale.

Maybe initializeSwizzler should just return an SkSwizzler (and maybe it should
instead be called createSwizzler), and then we can just compare to NULL to
decide to return kInvalidScale?

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:484: }
Should we set fSrcRow to NULL otherwise? Same question for the constructor.

That way, if we made a mistake and tried to write to it when we never
initialized it we will see more predictable results (we'll segfault instead of
writing to random memory).

Oh, and we definitely need to reset fSwizzler. If the first call to onStart
requires a swizzler, but the next does not, we will be attempting to swizzle
when we should have just written directly to the output memory. We should also
have a test for this.

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkJpegCodec.h
File src/codec/SkJpegCodec.h (right):

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.h:104: * Create the swizzler based on the encoded format
This should probably state that it is only used in scanline decoding.

Actually, I do not see an implementation. The SD has its own function, so I
think this is no longer needed.

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:30: return SkNEW_ARGS(SkScaledCodec,
(scanlineDecoder.detach()));
Reviewing this code, I realize that if there is a scale that JPEG can natively
support, we create a scanline decoder when we could have just used a codec.
(This is a result of the current separation between codec and SD, not your
code.) Maybe that's fine though.

emmaleer

https://codereview.chromium.org/1260673002/diff/340001/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1260673002/diff/340001/gyp/codec.gyp#newcode50 gyp/codec.gyp:50: '../src/codec/SkWebpCodec.cpp' On 2015/08/06 15:09:10, scroggo wrote: > nit: typically ...

5 years, 4 months ago (2015-08-06 18:59:53 UTC) #42

https://codereview.chromium.org/1260673002/diff/340001/gyp/codec.gyp
File gyp/codec.gyp (right):

https://codereview.chromium.org/1260673002/diff/340001/gyp/codec.gyp#newcode50
gyp/codec.gyp:50: '../src/codec/SkWebpCodec.cpp'
On 2015/08/06 15:09:10, scroggo wrote:
> nit: typically we keep this comma here, so we can add another line without
> having to add the comma then.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/340001/include/codec/SkScanli...
File include/codec/SkScanlineDecoder.h (right):

https://codereview.chromium.org/1260673002/diff/340001/include/codec/SkScanli...
include/codec/SkScanlineDecoder.h:53: *  scale that it can natively support
On 2015/08/06 15:09:10, scroggo wrote:
> Maybe this should have a FIXME: share this with SkCodec?
> 
> I just filed skbug.com/4175 to track the changing relationship between SkCodec
> and SkScanlineDecoder. Right now I'm toying with the idea of an abstract
> interface that is implemented by both that holds the common methods etc (which
I
> also mention in that bug).

Acknowledged.

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

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:478: if (this->initializeSwizzler(dstInfo, options) !=
SkCodec::kSuccess) {
On 2015/08/06 15:09:11, scroggo wrote:
> initializeSwizzler may return kUnimplemented, and then we compare to kSuccess.
> If it is not kSuccess, we return kInvalidScale.
> 
> Maybe initializeSwizzler should just return an SkSwizzler (and maybe it should
> instead be called createSwizzler), and then we can just compare to NULL to
> decide to return kInvalidScale?

I've changed this to check the result from initializeSwizzler, and if the result
!= kSuccess, return result.
I think this is the same outcome as what you're suggestion would do?
If not, I can change it to your suggestion.

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:484: }
On 2015/08/06 15:09:10, scroggo wrote:
> Should we set fSrcRow to NULL otherwise? Same question for the constructor.
> 
> That way, if we made a mistake and tried to write to it when we never
> initialized it we will see more predictable results (we'll segfault instead of
> writing to random memory).
> 
> Oh, and we definitely need to reset fSwizzler. If the first call to onStart
> requires a swizzler, but the next does not, we will be attempting to swizzle
> when we should have just written directly to the output memory. We should also
> have a test for this.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkJpegCodec.h
File src/codec/SkJpegCodec.h (right):

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.h:104: * Create the swizzler based on the encoded format
On 2015/08/06 15:09:11, scroggo wrote:
> This should probably state that it is only used in scanline decoding.
> 
> Actually, I do not see an implementation. The SD has its own function, so I
> think this is no longer needed.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:30: return SkNEW_ARGS(SkScaledCodec,
(scanlineDecoder.detach()));
On 2015/08/06 15:09:11, scroggo wrote:
> Reviewing this code, I realize that if there is a scale that JPEG can natively
> support, we create a scanline decoder when we could have just used a codec.
> (This is a result of the current separation between codec and SD, not your
> code.) Maybe that's fine though.

Yes that's true. But we call getScanlines with height for count in that case. 
I'm not sure how to avoid that, since we do not know the dstInfo before
getPixels is called.

scroggo

lgtm https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkJpegCodec.cpp#newcode478 src/codec/SkJpegCodec.cpp:478: if (this->initializeSwizzler(dstInfo, options) != SkCodec::kSuccess) { On 2015/08/06 ...

5 years, 4 months ago (2015-08-06 20:38:00 UTC) #43

lgtm

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

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:478: if (this->initializeSwizzler(dstInfo, options) !=
SkCodec::kSuccess) {
On 2015/08/06 18:59:52, emmaleer wrote:
> On 2015/08/06 15:09:11, scroggo wrote:
> > initializeSwizzler may return kUnimplemented, and then we compare to
kSuccess.
> > If it is not kSuccess, we return kInvalidScale.
> > 
> > Maybe initializeSwizzler should just return an SkSwizzler (and maybe it
should
> > instead be called createSwizzler), and then we can just compare to NULL to
> > decide to return kInvalidScale?
> 
> I've changed this to check the result from initializeSwizzler, and if the
result
> != kSuccess, return result.
> I think this is the same outcome as what you're suggestion would do?
> If not, I can change it to your suggestion.

Your approach makes sense. My approach would have meant one less if check, but
this is not a performance-critical piece of the code where we need to eliminate
if checks, IMO.

Returning result is probably more correct, in case it had more knowledge of its
failure.

https://codereview.chromium.org/1260673002/diff/360001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1260673002/diff/360001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:617: if
(!SkScaledCodec::DimensionsSupportedForSampling(this->getInfo(), dstInfo)) {
Matt has landed wbmp scanline decoding, so please update SkWbmpCodec as well.

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

https://codereview.chromium.org/1260673002/diff/360001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:445: // FIXME: CreateSwizzler could fail for another
reason.
I think we have this FIXME: sprinkled through SkCodec, but I think it only fails
if unimplemented (which maybe means invalid conversion). That said, it seems
like we should never get an invalid conversion, since we should have exited
before trying to create a swizzler.

Doesn't need to be fixed in this CL; just something for us to think about.

https://codereview.chromium.org/1260673002/diff/360001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/360001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:32: return 0xFFFF;
I recently realized we have a variable for this: SkSwizzler::kOpaque_ResultAlpha

emmaleer

I had to change the wbmp swizzling functions quite a bit to make them work ...

5 years, 4 months ago (2015-08-07 18:38:56 UTC) #44

I had to change the wbmp swizzling functions quite a bit to make them work for
sampling.
Also, adding the offset to the beginning of the row was moved inside the
swizzling functions.
Could you review those changes before I commit this?

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

https://codereview.chromium.org/1260673002/diff/340001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:478: if (this->initializeSwizzler(dstInfo, options) !=
SkCodec::kSuccess) {
On 2015/08/06 20:37:59, scroggo wrote:
> On 2015/08/06 18:59:52, emmaleer wrote:
> > On 2015/08/06 15:09:11, scroggo wrote:
> > > initializeSwizzler may return kUnimplemented, and then we compare to
> kSuccess.
> > > If it is not kSuccess, we return kInvalidScale.
> > > 
> > > Maybe initializeSwizzler should just return an SkSwizzler (and maybe it
> should
> > > instead be called createSwizzler), and then we can just compare to NULL to
> > > decide to return kInvalidScale?
> > 
> > I've changed this to check the result from initializeSwizzler, and if the
> result
> > != kSuccess, return result.
> > I think this is the same outcome as what you're suggestion would do?
> > If not, I can change it to your suggestion.
> 
> Your approach makes sense. My approach would have meant one less if check, but
> this is not a performance-critical piece of the code where we need to
eliminate
> if checks, IMO.
> 
> Returning result is probably more correct, in case it had more knowledge of
its
> failure.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/360001/src/codec/SkCodec_libp...
File src/codec/SkCodec_libpng.cpp (right):

https://codereview.chromium.org/1260673002/diff/360001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:617: if
(!SkScaledCodec::DimensionsSupportedForSampling(this->getInfo(), dstInfo)) {
On 2015/08/06 20:38:00, scroggo wrote:
> Matt has landed wbmp scanline decoding, so please update SkWbmpCodec as well.

Acknowledged.

scroggo

On 2015/08/07 18:38:56, emmaleer wrote: > I had to change the wbmp swizzling functions quite ...

5 years, 4 months ago (2015-08-07 19:39:49 UTC) #45

emmaleer

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp#newcode45 src/codec/SkSwizzler.cpp:45: void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width, ...

5 years, 4 months ago (2015-08-11 20:10:36 UTC) #46

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:45: void* SK_RESTRICT dstRow, const uint8_t*
SK_RESTRICT src, int width,
On 2015/08/07 19:39:48, scroggo wrote:
> Refresh my memory: is width the full width of the src? Or is it scaled, or
does
> it take the offset into account?

Width is the scaled dstWidth. 
The offset is used to choose the first pixel from the src.
Offset does not affect width in any way. 
Do you think we should change the variable width to dstWidth?
I could do that to make it more clear.

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:52: src += offset / 8;
On 2015/08/07 19:39:48, scroggo wrote:
> We can do this divide much faster with:
> 
> offset >> 3

Acknowledged.

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:53: int currBit = offset % 8;
On 2015/08/07 19:39:48, scroggo wrote:
> On the other hand, if you also want to do a mod, I think SkTDivMod (in
SkMath.h)
> is faster.

I looked at SkTDivMod, and it does a divide /, not a shift. I think it needs to
do that to work for all numbers, not just powers of 2.
I think the shift is faster than this, so I'm going to use the shift.
Also do do the mod, SkTDivMod does offset - byteOffset*8, so I changed my code
to do the same so it's faster.

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:55: U8CPU currByte = src[0];
On 2015/08/07 19:39:48, scroggo wrote:
> nit: curByte is a value, while currBit is an index. Maybe currBit would be
> better named bitIndex?

Acknowledged.

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:59: if (currBit == 8) {
On 2015/08/07 19:39:48, scroggo wrote:
> nit: constant should go on the left

Acknowledged.

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:61: currByte = src[0];
On 2015/08/07 19:39:48, scroggo wrote:
> nit: This works, but is confusing to read - you've mixed pointer arithmetic
with
> indices. I think this is better as either
> 
> currByte = ++src;
> 
> or do not increment src and use
> 
> currByte = src[<some index>];

I changed this to:
src++
currByte = *src

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:98: dst[x] = ((currByte >> (7-currBit)) & 1);
On 2015/08/07 19:39:48, scroggo wrote:
> Is this the only line that is different in the three functions? I wonder if
> there is a way to share more code?

Yes this is the only line that's different
I thought the point of the swizzler was to go fast, and copied code didn't
matter?
If copied code does in fact matter, I could try to create a function for the
shared parts.
Although, calling a function each time may make it slightly slower.

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:200: const SkPMColor ctable[], int offset);
On 2015/08/07 19:39:49, scroggo wrote:
> nit: I might change the order, so it's next to deltaSrc.
> 
> (on the other hand, that change is kind of repetitive without a whole lot of
> benefit...)

Acknowledged.

scroggo

Still lgtm. Matt, could you look over the bit swizzling functions? https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): ...

5 years, 4 months ago (2015-08-11 21:33:31 UTC) #47

Still lgtm. Matt, could you look over the bit swizzling functions?

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:45: void* SK_RESTRICT dstRow, const uint8_t*
SK_RESTRICT src, int width,
On 2015/08/11 20:10:35, emmaleer wrote:
> On 2015/08/07 19:39:48, scroggo wrote:
> > Refresh my memory: is width the full width of the src? Or is it scaled, or
> does
> > it take the offset into account?
> 
> Width is the scaled dstWidth. 
> The offset is used to choose the first pixel from the src.
> Offset does not affect width in any way. 
> Do you think we should change the variable width to dstWidth?
> I could do that to make it more clear.

Yes, I think that would make it more clear.

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:53: int currBit = offset % 8;
On 2015/08/11 20:10:35, emmaleer wrote:
> On 2015/08/07 19:39:48, scroggo wrote:
> > On the other hand, if you also want to do a mod, I think SkTDivMod (in
> SkMath.h)
> > is faster.
> 
> I looked at SkTDivMod, and it does a divide /, not a shift. I think it needs
to
> do that to work for all numbers, not just powers of 2.
> I think the shift is faster than this, so I'm going to use the shift.
> Also do do the mod, SkTDivMod does offset - byteOffset*8, so I changed my code
> to do the same so it's faster.

Right, of course! The fact that you are using a power of two makes the mod call
easier, too, right? I think we can just do:

src += offset >> 3;
int currBit = offset & 7;

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:61: currByte = src[0];
On 2015/08/11 20:10:35, emmaleer wrote:
> On 2015/08/07 19:39:48, scroggo wrote:
> > nit: This works, but is confusing to read - you've mixed pointer arithmetic
> with
> > indices. I think this is better as either
> > 
> > currByte = ++src;
> > 
> > or do not increment src and use
> > 
> > currByte = src[<some index>];
> 
> I changed this to:
> src++
> currByte = *src

FWIW, this is the same as:

currByte = ++src;

You may find the two step version more readable though, which I think is fine.

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:98: dst[x] = ((currByte >> (7-currBit)) & 1);
On 2015/08/11 20:10:35, emmaleer wrote:
> On 2015/08/07 19:39:48, scroggo wrote:
> > Is this the only line that is different in the three functions? I wonder if
> > there is a way to share more code?
> 
> Yes this is the only line that's different
> I thought the point of the swizzler was to go fast, and copied code didn't
> matter?

Yes, the point is to go fast, and I think it's okay that there's some
duplication. The bit and small index functions seem to have a bit more duplicate
work than the others though. (This isn't new to your change, I'll admit.)

> If copied code does in fact matter, I could try to create a function for the
> shared parts.
> Although, calling a function each time may make it slightly slower.

I agree that calling a function may make it slower. We could also use a macro,
but that has its own downsides. Maybe just add some comments that these are the
same?

https://codereview.chromium.org/1260673002/diff/420001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/420001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:84: int x = 0;
nit: x is defined several lines before it's used. I think at least some of your
variables might end up being defined lines earlier, since they're used together
in the while loop, but maybe it should instead be a for loop, where x is
declared only in scope of the loop?

https://codereview.chromium.org/1260673002/diff/420001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:99: if (deltaSrc == sample) {
Ok, I think I understand this code: we basically loop over each bit, but we do
not actually need each bit, correct?

Could we do something more like the following?

while (x < width) {
  oldByteIndex = byteIndex
  byteIndex = x >> 3;
  if (oldByteIndex != byteIndex) {
    currByte = src[byteIndex];
  }
  dst[x] = // same calculation as today
  x += deltaSrc;
}

I've probably made some mistakes, but basically the idea is to iterate over the
samples we want, rather than iterating over each bit - where some iterations are
just updating values. Do you think my version would faster, slower, depends?

Also, Matt, could you look over these functions? You're more familiar with this
code than I.

emmaleer

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp#newcode45 src/codec/SkSwizzler.cpp:45: void* SK_RESTRICT dstRow, const uint8_t* SK_RESTRICT src, int width, ...

5 years, 4 months ago (2015-08-12 14:04:34 UTC) #48

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:45: void* SK_RESTRICT dstRow, const uint8_t*
SK_RESTRICT src, int width,
On 2015/08/11 21:33:30, scroggo wrote:
> On 2015/08/11 20:10:35, emmaleer wrote:
> > On 2015/08/07 19:39:48, scroggo wrote:
> > > Refresh my memory: is width the full width of the src? Or is it scaled, or
> > does
> > > it take the offset into account?
> > 
> > Width is the scaled dstWidth. 
> > The offset is used to choose the first pixel from the src.
> > Offset does not affect width in any way. 
> > Do you think we should change the variable width to dstWidth?
> > I could do that to make it more clear.
> 
> Yes, I think that would make it more clear.

Acknowledged.

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:53: int currBit = offset % 8;
On 2015/08/11 21:33:30, scroggo wrote:
> On 2015/08/11 20:10:35, emmaleer wrote:
> > On 2015/08/07 19:39:48, scroggo wrote:
> > > On the other hand, if you also want to do a mod, I think SkTDivMod (in
> > SkMath.h)
> > > is faster.
> > 
> > I looked at SkTDivMod, and it does a divide /, not a shift. I think it needs
> to
> > do that to work for all numbers, not just powers of 2.
> > I think the shift is faster than this, so I'm going to use the shift.
> > Also do do the mod, SkTDivMod does offset - byteOffset*8, so I changed my
code
> > to do the same so it's faster.
> 
> Right, of course! The fact that you are using a power of two makes the mod
call
> easier, too, right? I think we can just do:
> 
> src += offset >> 3;
> int currBit = offset & 7;

Acknowledged.

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:61: currByte = src[0];
On 2015/08/11 21:33:30, scroggo wrote:
> On 2015/08/11 20:10:35, emmaleer wrote:
> > On 2015/08/07 19:39:48, scroggo wrote:
> > > nit: This works, but is confusing to read - you've mixed pointer
arithmetic
> > with
> > > indices. I think this is better as either
> > > 
> > > currByte = ++src;
> > > 
> > > or do not increment src and use
> > > 
> > > currByte = src[<some index>];
> > 
> > I changed this to:
> > src++
> > currByte = *src
> 
> FWIW, this is the same as:
> 
> currByte = ++src;
> 
> You may find the two step version more readable though, which I think is fine.

I don't think it's the same thing.
Since src is a pointer and currByte is an integer.
It would be the same as:
currByte = *(++src);
which I thought was more clear in 2 steps.

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:98: dst[x] = ((currByte >> (7-currBit)) & 1);
On 2015/08/11 21:33:30, scroggo wrote:
> On 2015/08/11 20:10:35, emmaleer wrote:
> > On 2015/08/07 19:39:48, scroggo wrote:
> > > Is this the only line that is different in the three functions? I wonder
if
> > > there is a way to share more code?
> > 
> > Yes this is the only line that's different
> > I thought the point of the swizzler was to go fast, and copied code didn't
> > matter?
> 
> Yes, the point is to go fast, and I think it's okay that there's some
> duplication. The bit and small index functions seem to have a bit more
duplicate
> work than the others though. (This isn't new to your change, I'll admit.)
> 
> > If copied code does in fact matter, I could try to create a function for the
> > shared parts.
> > Although, calling a function each time may make it slightly slower.
> 
> I agree that calling a function may make it slower. We could also use a macro,
> but that has its own downsides. Maybe just add some comments that these are
the
> same?

Acknowledged.

https://codereview.chromium.org/1260673002/diff/420001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/420001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:99: if (deltaSrc == sample) {
On 2015/08/11 21:33:30, scroggo wrote:
> Ok, I think I understand this code: we basically loop over each bit, but we do
> not actually need each bit, correct?
> 
> Could we do something more like the following?
> 
> while (x < width) {
>   oldByteIndex = byteIndex
>   byteIndex = x >> 3;
>   if (oldByteIndex != byteIndex) {
>     currByte = src[byteIndex];
>   }
>   dst[x] = // same calculation as today
>   x += deltaSrc;
> }
> 
> I've probably made some mistakes, but basically the idea is to iterate over
the
> samples we want, rather than iterating over each bit - where some iterations
are
> just updating values. Do you think my version would faster, slower, depends?
> 
> Also, Matt, could you look over these functions? You're more familiar with
this
> code than I.

I don't think that way would work, or maybe I'm not undstanding it.
the value assigned to dst[x] depends on bitIndex. So if we don't iterate over
every bit, bit Index won't be correct, then we won't get correct values for
dst[x] (i think).
How does your method deal with bitIndex?

Matt let me know what you think. If we still want to change this function I can
look into making a change similar to what you suggested Leon.

scroggo

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp#newcode61 src/codec/SkSwizzler.cpp:61: currByte = src[0]; On 2015/08/12 14:04:33, emmaleer wrote: > ...

5 years, 4 months ago (2015-08-12 14:35:23 UTC) #49

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:61: currByte = src[0];
On 2015/08/12 14:04:33, emmaleer wrote:
> On 2015/08/11 21:33:30, scroggo wrote:
> > On 2015/08/11 20:10:35, emmaleer wrote:
> > > On 2015/08/07 19:39:48, scroggo wrote:
> > > > nit: This works, but is confusing to read - you've mixed pointer
> arithmetic
> > > with
> > > > indices. I think this is better as either
> > > > 
> > > > currByte = ++src;
> > > > 
> > > > or do not increment src and use
> > > > 
> > > > currByte = src[<some index>];
> > > 
> > > I changed this to:
> > > src++
> > > currByte = *src
> > 
> > FWIW, this is the same as:
> > 
> > currByte = ++src;
> > 
> > You may find the two step version more readable though, which I think is
fine.
> 
> I don't think it's the same thing.
> Since src is a pointer and currByte is an integer.
> It would be the same as:
> currByte = *(++src);
> which I thought was more clear in 2 steps.

Oops; yes, I need to dereference the pointer. I'm fine with doing it in two
steps though.

https://codereview.chromium.org/1260673002/diff/420001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/420001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:99: if (deltaSrc == sample) {
On 2015/08/12 14:04:33, emmaleer wrote:
> On 2015/08/11 21:33:30, scroggo wrote:
> > Ok, I think I understand this code: we basically loop over each bit, but we
do
> > not actually need each bit, correct?
> > 
> > Could we do something more like the following?
> > 
> > while (x < width) {
> >   oldByteIndex = byteIndex
> >   byteIndex = x >> 3;
> >   if (oldByteIndex != byteIndex) {
> >     currByte = src[byteIndex];
> >   }
> >   dst[x] = // same calculation as today
> >   x += deltaSrc;
> > }
> > 
> > I've probably made some mistakes, but basically the idea is to iterate over
> the
> > samples we want, rather than iterating over each bit - where some iterations
> are
> > just updating values. Do you think my version would faster, slower, depends?
> > 
> > Also, Matt, could you look over these functions? You're more familiar with
> this
> > code than I.
> 
> I don't think that way would work, or maybe I'm not undstanding it.
> the value assigned to dst[x] depends on bitIndex. So if we don't iterate over
> every bit, bit Index won't be correct, then we won't get correct values for
> dst[x] (i think).
> How does your method deal with bitIndex?

Similar to the first calculation of bitIndex - using a mod operation (using &7).
> 
> Matt let me know what you think. If we still want to change this function I
can
> look into making a change similar to what you suggested Leon.

I'm fine with the function as is, but I was just wondering if the other way
might be more efficient.

msarett

https://codereview.chromium.org/1260673002/diff/420001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1260673002/diff/420001/src/codec/SkSwizzler.cpp#newcode99 src/codec/SkSwizzler.cpp:99: if (deltaSrc == sample) { On 2015/08/12 14:04:33, emmaleer ...

5 years, 4 months ago (2015-08-12 14:37:41 UTC) #50

https://codereview.chromium.org/1260673002/diff/420001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/420001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:99: if (deltaSrc == sample) {
On 2015/08/12 14:04:33, emmaleer wrote:
> On 2015/08/11 21:33:30, scroggo wrote:
> > Ok, I think I understand this code: we basically loop over each bit, but we
do
> > not actually need each bit, correct?
> > 
> > Could we do something more like the following?
> > 
> > while (x < width) {
> >   oldByteIndex = byteIndex
> >   byteIndex = x >> 3;
> >   if (oldByteIndex != byteIndex) {
> >     currByte = src[byteIndex];
> >   }
> >   dst[x] = // same calculation as today
> >   x += deltaSrc;
> > }
> > 
> > I've probably made some mistakes, but basically the idea is to iterate over
> the
> > samples we want, rather than iterating over each bit - where some iterations
> are
> > just updating values. Do you think my version would faster, slower, depends?
> > 
> > Also, Matt, could you look over these functions? You're more familiar with
> this
> > code than I.
> 
> I don't think that way would work, or maybe I'm not undstanding it.
> the value assigned to dst[x] depends on bitIndex. So if we don't iterate over
> every bit, bit Index won't be correct, then we won't get correct values for
> dst[x] (i think).
> How does your method deal with bitIndex?
> 
> Matt let me know what you think. If we still want to change this function I
can
> look into making a change similar to what you suggested Leon.

Emmalee and I revised this function in person to take the approach you are
suggesting :).

I think the result is a swizzling function that is better than the original
without sampling.

https://codereview.chromium.org/1260673002/diff/440001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/440001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:62: currByte = *src;
FWIW, I prefer:
currByte = *(++src);

But I'm late to this conversation and don't feel strongly.

emmaleer

The wbmp swizzling functions now use your idea Leon. I think this way will improve ...

5 years, 4 months ago (2015-08-12 15:08:44 UTC) #51

scroggo

https://codereview.chromium.org/1260673002/diff/460001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1260673002/diff/460001/src/codec/SkSwizzler.cpp#newcode56 src/codec/SkSwizzler.cpp:56: while (x < dstWidth) { Now these look even ...

5 years, 4 months ago (2015-08-12 15:24:49 UTC) #52

emmaleer

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, djsollen@google.com Link to ...

5 years, 4 months ago (2015-08-12 20:55:36 UTC) #55

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-12 20:55:54 UTC) #56

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-12 20:58:29 UTC) #57

commit-bot: I haz the power

Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-Debug-Trybot/builds/2611)

5 years, 4 months ago (2015-08-12 20:58:31 UTC) #58

emmaleer

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com, djsollen@google.com Link to ...

5 years, 4 months ago (2015-08-12 21:41:15 UTC) #60

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-12 21:41:29 UTC) #61

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-12 21:43:10 UTC) #62

commit-bot: I haz the power

Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x86_64-Debug-Trybot/builds/2638)

5 years, 4 months ago (2015-08-12 21:43:11 UTC) #63

scroggo

https://codereview.chromium.org/1260673002/diff/520001/src/codec/SkScaledCodec.cpp File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/520001/src/codec/SkScaledCodec.cpp#newcode195 src/codec/SkScaledCodec.cpp:195: dstRow += rowBytes; You could also do: dstRow = ...

5 years, 4 months ago (2015-08-12 21:49:26 UTC) #64

mtklein

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

5 years, 4 months ago (2015-08-12 21:51:10 UTC) #65

mtklein

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp#newcode52 src/codec/SkSwizzler.cpp:52: src += offset / 8; On 2015/08/07 19:39:48, scroggo ...

5 years, 4 months ago (2015-08-12 21:51:11 UTC) #66

emmaleer

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp#newcode52 src/codec/SkSwizzler.cpp:52: src += offset / 8; On 2015/08/12 21:51:11, mtklein ...

5 years, 4 months ago (2015-08-12 22:19:20 UTC) #67

emmaleer

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com, scroggo@google.com Link to ...

5 years, 4 months ago (2015-08-12 22:19:33 UTC) #69

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-12 22:19:42 UTC) #70

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-12 22:23:10 UTC) #71

commit-bot: I haz the power

Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-Debug-Trybot/builds/2613) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, ...

5 years, 4 months ago (2015-08-12 22:23:11 UTC) #72

emmaleer

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com, scroggo@google.com Link to ...

5 years, 4 months ago (2015-08-12 22:40:47 UTC) #74

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-12 22:41:05 UTC) #75

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-12 22:48:15 UTC) #76

commit-bot: I haz the power

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-x86_64-Release-Shared-Trybot/builds/2560)

5 years, 4 months ago (2015-08-12 22:48:16 UTC) #77

emmaleer

Leon and Matt, could you review this change before I commit again? There was an ...

5 years, 4 months ago (2015-08-13 15:30:47 UTC) #78

msarett

Looks good to me. https://codereview.chromium.org/1260673002/diff/580001/include/codec/SkScaledCodec.h File include/codec/SkScaledCodec.h (right): https://codereview.chromium.org/1260673002/diff/580001/include/codec/SkScaledCodec.h#newcode45 include/codec/SkScaledCodec.h:45: static int getScaledDimension(int srcDimension, int ...

5 years, 4 months ago (2015-08-13 15:45:10 UTC) #79

scroggo

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp#newcode52 src/codec/SkSwizzler.cpp:52: src += offset / 8; On 2015/08/12 22:19:20, emmaleer ...

5 years, 4 months ago (2015-08-13 16:10:45 UTC) #80

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1260673002/diff/400001/src/codec/SkSwizzler.c...
src/codec/SkSwizzler.cpp:52: src += offset / 8;
On 2015/08/12 22:19:20, emmaleer wrote:
> On 2015/08/12 21:51:11, mtklein wrote:
> > On 2015/08/07 19:39:48, scroggo wrote:
> > > We can do this divide much faster with:
> > > 
> > > offset >> 3
> > 
> > Woah woah, there's absolutely no way the compiler's not doing that for you. 
> You
> > should never bother strength reducing a divide by a constant like this
> yourself.
> >  You're depriving the compiler of all its joy in life!
> >   src += offset / 8;
> >   int currBit = offset % 8;
> > will run identically as fast as
> >   src += offset >> 3;
> >   int currBit = offset & 7;
> > but using / and % is way more readable.
> > 
> > I would strongly encourage favoring the most readable version of any code
you
> > can write.  This sort of speculation about speed is really best not
speculated
> > on: profiling is the only good way to approach performance, and if we were
> > profiling this we'd see /8+%8 or >>3+&7 here is totally moot.
> 
> Okay, I've changed this back to use / and %.
> Thanks for mentioning that! 

Maybe we should use SkTDivMod then?

https://codereview.chromium.org/1260673002/diff/580001/include/codec/SkScaled...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1260673002/diff/580001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:45: static int getScaledDimension(int
srcDimension, int sampleSize) {
On 2015/08/13 15:45:10, msarett wrote:
> nit: GetScaledDimension because it is static.
> 
> Leon, what is your opinion about a member function that does not use any
fields?

In general, that is a sign it may not need to be a member function. If this
function were used by other classes, I think it would be fine to put it here for
context (i.e. so callers will have to call SkScaledCodec::GetScaledDimensions).
That said, it looks like it is only used by SkScaledCodec, and by the SampleSize
constructor. If the SampleSize constructor were defined in the implementation
file, this would not be needed here. So I would say move that calculation to
.cpp, and move this back to cpp as a static function.

> 
> It is here in the header file because it is needed here and also in the cpp
> file.  Maybe it can moved out of the class but still in this file?  It could
go
> to SkCodecPriv but it seems to me that it belongs with SkScaledCodec.

https://codereview.chromium.org/1260673002/diff/580001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:52: struct SampleSize {
I do not think this object is necessary. It has a constructor which can
potentially create bad values, and it is really only used to do some
calculations. I think it would be better to just have a function that does this:

static bool ComputeSampleSize(const SkImageInfo& srcInfo,
    const SkImageInfo& dstInfo, int* sampleX, int* sampleY)
{
  // Do calculation...
  if (/* not valid */) {
    return false;
  }

  if (sampleX) {
    *sampleX = <value computed above>;
  }
  if (sampleY) {
    *sampleY = <value computed above>;
  }

  return true;
}

https://codereview.chromium.org/1260673002/diff/580001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/580001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:97: static bool scaling_supported(const
SkScaledCodec::SampleSize& sampleSize,
It seems a little odd to me that you pass a SampleSize to this method, which
could have instead computed the SampleSize inside.

It also seems weird that SampleSize would ever be created with invalid values.
Should this function be combined with the SampleSize constructor (/the function
that I suggested you turn it into)?

emmaleer

https://codereview.chromium.org/1260673002/diff/580001/include/codec/SkScaledCodec.h File include/codec/SkScaledCodec.h (right): https://codereview.chromium.org/1260673002/diff/580001/include/codec/SkScaledCodec.h#newcode45 include/codec/SkScaledCodec.h:45: static int getScaledDimension(int srcDimension, int sampleSize) { On 2015/08/13 ...

5 years, 4 months ago (2015-08-13 17:49:11 UTC) #81

https://codereview.chromium.org/1260673002/diff/580001/include/codec/SkScaled...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1260673002/diff/580001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:45: static int getScaledDimension(int
srcDimension, int sampleSize) {
On 2015/08/13 16:10:45, scroggo wrote:
> On 2015/08/13 15:45:10, msarett wrote:
> > nit: GetScaledDimension because it is static.
> > 
> > Leon, what is your opinion about a member function that does not use any
> fields?
> 
> In general, that is a sign it may not need to be a member function. If this
> function were used by other classes, I think it would be fine to put it here
for
> context (i.e. so callers will have to call
SkScaledCodec::GetScaledDimensions).
> That said, it looks like it is only used by SkScaledCodec, and by the
SampleSize
> constructor. If the SampleSize constructor were defined in the implementation
> file, this would not be needed here. So I would say move that calculation to
> .cpp, and move this back to cpp as a static function.
> 
> > 
> > It is here in the header file because it is needed here and also in the cpp
> > file.  Maybe it can moved out of the class but still in this file?  It could
> go
> > to SkCodecPriv but it seems to me that it belongs with SkScaledCodec.
> 

This is no longer a member function.

https://codereview.chromium.org/1260673002/diff/580001/include/codec/SkScaled...
include/codec/SkScaledCodec.h:52: struct SampleSize {
On 2015/08/13 16:10:45, scroggo wrote:
> I do not think this object is necessary. It has a constructor which can
> potentially create bad values, and it is really only used to do some
> calculations. I think it would be better to just have a function that does
this:
> 
> static bool ComputeSampleSize(const SkImageInfo& srcInfo,
>     const SkImageInfo& dstInfo, int* sampleX, int* sampleY)
> {
>   // Do calculation...
>   if (/* not valid */) {
>     return false;
>   }
> 
>   if (sampleX) {
>     *sampleX = <value computed above>;
>   }
>   if (sampleY) {
>     *sampleY = <value computed above>;
>   }
> 
>   return true;
> }

Okay, I like that way better too.

https://codereview.chromium.org/1260673002/diff/580001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/580001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:97: static bool scaling_supported(const
SkScaledCodec::SampleSize& sampleSize,
On 2015/08/13 16:10:45, scroggo wrote:
> It seems a little odd to me that you pass a SampleSize to this method, which
> could have instead computed the SampleSize inside.
> 
> It also seems weird that SampleSize would ever be created with invalid values.
> Should this function be combined with the SampleSize constructor (/the
function
> that I suggested you turn it into)?

This function is still necessary, as when we call ComputeSampleSize in the
swizzler, we do not want to do call the scaling_supported function, as we pass
the srcHeight to the swizzler (as swizzler does not do any height sampling)
So, we need to have scaling_supported as it's own function.
I also thought we could combine them, and after I combined them I remembered why
it wouldn't work.

Do you think it's better to calculate sampleSize again inside scaling_supported?
I pass sampleSize to scaling_supported because I thought it would be a waste of
time to calculate it again, when we just previously calculated it.

scroggo

lgtm https://codereview.chromium.org/1260673002/diff/580001/src/codec/SkScaledCodec.cpp File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1260673002/diff/580001/src/codec/SkScaledCodec.cpp#newcode97 src/codec/SkScaledCodec.cpp:97: static bool scaling_supported(const SkScaledCodec::SampleSize& sampleSize, On 2015/08/13 17:49:11, ...

5 years, 4 months ago (2015-08-13 18:01:21 UTC) #82

lgtm

https://codereview.chromium.org/1260673002/diff/580001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/580001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:97: static bool scaling_supported(const
SkScaledCodec::SampleSize& sampleSize,
On 2015/08/13 17:49:11, emmaleer wrote:
> On 2015/08/13 16:10:45, scroggo wrote:
> > It seems a little odd to me that you pass a SampleSize to this method, which
> > could have instead computed the SampleSize inside.
> > 
> > It also seems weird that SampleSize would ever be created with invalid
values.
> > Should this function be combined with the SampleSize constructor (/the
> function
> > that I suggested you turn it into)?
> 
> This function is still necessary, as when we call ComputeSampleSize in the
> swizzler, we do not want to do call the scaling_supported function, as we pass
> the srcHeight to the swizzler (as swizzler does not do any height sampling)
> So, we need to have scaling_supported as it's own function.
> I also thought we could combine them, and after I combined them I remembered
why
> it wouldn't work.
> 
> Do you think it's better to calculate sampleSize again inside
scaling_supported?
> I pass sampleSize to scaling_supported because I thought it would be a waste
of
> time to calculate it again, when we just previously calculated it.

What if you did the following:

static bool scaling_supported(dstInfo, srcInfo, int* sampleX, int* sampleY) {
  SkScaledCodec::ComputeSampleSize(dstInfo, srcInfo, sampleX, sampleY);
  // rest of the function
}

Then you still only compute sampleSize once.

I prefer that a little bit, because it seems weird to pass sampleX and sampleY
to scaling_supported, when it could have been computed from the other
parameters.

https://codereview.chromium.org/1260673002/diff/600001/src/codec/SkScaledCode...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1260673002/diff/600001/src/codec/SkScaledCode...
src/codec/SkScaledCodec.cpp:137: int* sampleSizeX, int* sampleSizeY) {
nit: I would probably name these "sampleXPtr" and "sampleYPtr". Not a big deal,
but it seems arbitrary that the difference between it and the local variable on
the stack is "Size".

emmaleer

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com Link to the ...

5 years, 4 months ago (2015-08-13 18:02:44 UTC) #84

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-13 18:02:55 UTC) #85

emmaleer

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com, scroggo@google.com Link to ...

5 years, 4 months ago (2015-08-13 18:18:23 UTC) #88

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-13 18:18:37 UTC) #89

commit-bot: I haz the power

Committed patchset #32 (id:620001) as https://skia.googlesource.com/skia/+/0944100ac89f797714eeae0cf2875e2335ff52ee

5 years, 4 months ago (2015-08-13 18:27:02 UTC) #90

emmaleer

A revert of this CL (patchset #32 id:620001) has been created in https://codereview.chromium.org/1294593002/ by emmaleer@google.com. ...

5 years, 4 months ago (2015-08-13 18:35:07 UTC) #91

emmaleer

There was a memory overwrite bug in the swizzler, in the function swizzler_index_to_index. Src was ...

5 years, 4 months ago (2015-08-13 19:37:12 UTC) #92

scroggo

A couple small nits on the latest patch set, but they're not too big of ...

5 years, 4 months ago (2015-08-13 19:44:44 UTC) #93

emmaleer

https://codereview.chromium.org/1260673002/diff/640001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1260673002/diff/640001/src/codec/SkSwizzler.cpp#newcode169 src/codec/SkSwizzler.cpp:169: SkASSERT(0 == offset); On 2015/08/13 19:44:44, scroggo wrote: > ...

5 years, 4 months ago (2015-08-13 19:58:40 UTC) #94

emmaleer

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com, scroggo@google.com Link to ...

5 years, 4 months ago (2015-08-13 19:58:51 UTC) #96

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-13 19:59:10 UTC) #97

commit-bot: I haz the power

Committed patchset #34 (id:660001) as https://skia.googlesource.com/skia/+/d518ea7927f9f4e0ed5b4134d1b4f48243855a47

5 years, 4 months ago (2015-08-13 20:07:08 UTC) #98

emmaleer

A revert of this CL (patchset #34 id:660001) has been created in https://codereview.chromium.org/1294613002/ by emmaleer@google.com. ...

5 years, 4 months ago (2015-08-13 20:25:34 UTC) #99

rmistry

On 2015/08/13 20:25:34, emmaleer wrote: > A revert of this CL (patchset #34 id:660001) has ...

5 years, 4 months ago (2015-08-13 22:37:10 UTC) #100

emmaleer

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com, scroggo@google.com Link to ...

5 years, 4 months ago (2015-08-14 12:38:38 UTC) #102

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-14 12:38:45 UTC) #103

commit-bot: I haz the power

Committed patchset #35 (id:680001) as https://skia.googlesource.com/skia/+/b157917507d4f7d2651f0aeb566d31603cc02240

5 years, 4 months ago (2015-08-14 12:46:16 UTC) #104

egdaniel

A revert of this CL (patchset #35 id:680001) has been created in https://codereview.chromium.org/1285173003/ by egdaniel@google.com. ...

5 years, 4 months ago (2015-08-14 13:37:23 UTC) #105

mtklein

On 2015/08/14 13:37:23, egdaniel wrote: > A revert of this CL (patchset #35 id:680001) has ...

5 years, 4 months ago (2015-08-14 13:44:45 UTC) #106

msarett

Yeah we're happy to do that! We've been debating whether the out of memory errors ...

5 years, 4 months ago (2015-08-14 13:51:26 UTC) #107

mtklein

On 2015/08/14 13:51:26, msarett wrote: > Yeah we're happy to do that! > > We've ...

5 years, 4 months ago (2015-08-14 13:54:38 UTC) #108

mtklein

On 2015/08/14 13:54:38, mtklein wrote: > On 2015/08/14 13:51:26, msarett wrote: > > Yeah we're ...

5 years, 4 months ago (2015-08-14 13:56:11 UTC) #109

emmaleer

The 32-bit Ubuntu GCE bots were running out of memory in DM, when running the ...

5 years, 4 months ago (2015-08-14 14:24:57 UTC) #110

emmaleer

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com, scroggo@google.com Link to ...

5 years, 4 months ago (2015-08-14 14:37:47 UTC) #113

commit-bot: I haz the power

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

5 years, 4 months ago (2015-08-14 14:37:57 UTC) #114

commit-bot: I haz the power

5 years, 4 months ago (2015-08-14 14:44:50 UTC) #115

Message was sent while issue was closed.

Committed patchset #36 (id:700001) as
https://skia.googlesource.com/skia/+/8f4ba76742c329bc4d5e1b8ca376d27922bd00b1

Issue 1260673002: SkScaledCodec class (Closed)

Description

Patch Set 1 #

Patch Set 2 : use SkWebpCodec native scaling, update comments and spacing #

Patch Set 3 : remove setSampleX(), move IsInterlaced() to scanlineDecoder #

Patch Set 4 : Works for jpeg images #

Patch Set 5 : add helper functions to calculate sampleSize and scaledDimensions #

Patch Set 6 : Swizzler and SkScaledCodec use same get_sample_size() function #

Patch Set 7 : Merge with master #

Patch Set 8 : fix sample function for 565 images #

Patch Set 9 : changing scaleToDimensions() name to nativelyScaleToDimensions() #

Patch Set 10 : onGetScaledDimensions checks fCodec for natice scale, update comments #

Patch Set 11 : Use SkTAbs, update comments #

Patch Set 12 : Partial Native Scale for Jpeg #

Patch Set 13 : Make SkScaledCodec constructor explicit #

Patch Set 14 : Update PartialNativeScaling for future reference before removing #

Patch Set 15 : Remove PartialNativeScaling #

Patch Set 16 : Add get_row_bytes function to SkJpegCodec #

Patch Set 17 : Merge with Master #

Patch Set 18 : Move Jpeg Swizzler to ScanlineDecoder #

Patch Set 19 : Reset fSwizzler in onStart #

Patch Set 20 : Merge with wmp scanlineDecoder #

Patch Set 21 : Make wbmp swizzle functions work for sampling #

Patch Set 22 : Update wbmp swizzling functions #

Patch Set 23 : Update wbmp swizzling functions again #

Patch Set 24 : Improve wbmp swizzling functions #

Patch Set 25 : Fix wbmp 565 failure, update wbmp swizzling functions #

Patch Set 26 : Update DM to handle if SkScaled Codec not supported, merge with master #

Patch Set 27 : Fix error caused by adding to void pointer #

Patch Set 28 : Fix void pointer error, make divide and mod more readable #

Patch Set 29 : Fix conversion from float to int error #

Patch Set 30 : Fix sampleSize rounding error by creating SampleSize struct #

Patch Set 31 : Replace SampleSize struct with ComputeSampleSize function #

Patch Set 32 : Move ComputeSampleSize call inside scaling_supported #

Patch Set 33 : Fix memory overwrite bug in swizzler #

Patch Set 34 : Update comment, remove mixing pointer arithmetic with indexing #

Patch Set 35 : Change wbmp swizzling functions to update currByte before assigning dst value #

Patch Set 36 : Stop DM from running large interlaced images on 32-bit Ubuntu GCE bots b/c they are running out of … #

Messages