Issue 1134113006: dm changes

emmaleeroach

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

5 years, 7 months ago (2015-05-12 20:09:47 UTC) #1

scroggo

You left out a few pieces that were necessary to get it working. See https://codereview.chromium.org/1129903005 ...

5 years, 7 months ago (2015-05-12 20:41:31 UTC) #3

emmaleeroach

New dm mode which decodes the image in 4 subsets, using a scanline decoder. Also ...

5 years, 7 months ago (2015-05-15 17:07:05 UTC) #4

scroggo

Overall the code looks good. I have made some comments where we can make some ...

5 years, 7 months ago (2015-05-18 14:03:59 UTC) #5

Overall the code looks good. I have made some comments where we can make some
improvements.

Also, I'm not sure whether you meant to include the sample changes. Generally we
try to make a single code review issue for a single thing. So your sample should
be its own upload, example.cpp (along with the gyp file that builds it) should
be a separate one, and the DM changes should be another one.

For each self-contained piece of work, do the following:

git checkout -t -b <branchname> master
<do your work>
git commit -a
<write a description for the changes you made>
git cl upload
<go to the website to add reviewers>

That way, we can separately decide whether to check in each piece (and revert if
there is a problem).

https://codereview.chromium.org/1134113006/diff/30001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1134113006/diff/30001/dm/DM.cpp#newcode282
dm/DM.cpp:282: push_codec_srcs(path); //hits bp
Was this accidentally left in?

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:169: int fDivisor = 2;
style nit: Skia adds an 'f' to the beginning of variables to show that they are
member variables (or 'fields'). For local variables, we would just name them
something like "divisor".

Also, this variable could be const. (Not absolutely necessary, but it lets a
reader and the compiler know that you won't be changing it.)

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:182: char* line = new
char[decodeInfo.width()*decodeInfo.bytesPerPixel()];
This will leak. You can solve this a few different ways. The most
straightforward way is to call

delete[] line;

when you are done with it. The problem with this approach is that if you had
some error case that resulted in a return statement before you get to the line
that calls delete[], it would still leak the memory.

We have some ways to make it automatically get deleted when it goes out of
scope, similar to smart pointers, which we have discussed. The Skia objects that
do this are in SkTemplates. For example, you can use SkAutoTDeleteArray. After
the line that declares the variable "line", you would write the following:

SkAutoTDeleteArray lineDeleter(line);

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:183: for (int subset = 0; subset < fDivisor*2; subset++) {
subset < fDivisor*2

Shouldn't this be subset < fDivisor * fDivisor? If you changed fDivisor to 3,
won't you have 9 subsets?

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:185: if (subset == 2) {
Again, this supposes that fDivisor is 2. The nice thing about using the variable
fDivisor is that we can change one line of code and test a different thing.

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:189: return Error::Nonfatal("Cannot use scanline decoder for
all images");
I think in this case we have an actual error. If the scanline decoder cannot be
created the first time, it may mean the image was a progressive image, so we
cannot create one. If we can create it once, we should be able to create it
again for the same image. (Assuming we could rewind the stream, but all of our
streams we use here should be rewindable.)

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:200: //if subset if on right copy right half of line
is*

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:201: memcpy(subsetBm.getAddr(0,y),
Won't this copy to the beginning of this row?

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:203:
subsetDecodeInfo.width()*subsetDecodeInfo.bytesPerPixel());
Won't this copy too much?

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:215: int h;
nit: These variables look like width and height, but they are x and y, right?

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:217: switch (subset) {
nit: This works with a divisor of 2, but I think it will be harder if we try to
make it more general purpose. It might be easier to follow the pattern of
ImageSrc when it has an fDivisor != 0.

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage.cpp
File samplecode/SampleImage.cpp (right):

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:17: ImageView(const char imageFilename[]) {
nit: Skia uses a four space indent.

Not sure if you've seen it, but here are our code style guidelines:
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...

They are neither exhaustive nor accurate, but they should give you a good start.

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:18: SkString resourcePath =
GetResourcePath(imageFilename);
It threw me off for a second that this is lined up with the function signature.
This should be indented four spaces (although once you move the signature back,
this is in the right place).

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:28: } else {
This should line up with the if statement.

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:30: *(fBM.getAddr32(0,0)) = 0xFF0000FF; // red == bad
This is just an example, so it doesn't matter too much, but it's worth
discussing colors.

We have a couple of different types that represent colors: SkColor and SkPMColor
(both are found in SkColor.h, and both are typedefs for uint32_t).

SkColor is platform-independent, and can be represented as a hexadecimal number
that looks like 0xAARRGGBB. This is used for APIs like SkPaint::setColor(),
allowing someone to write the same code to run on multiple platforms.

SkPMColor is the color used for drawing (SkColors will always be converted to
SkPMColor). So the address that getAddr32 points to is an SkPMColor. (Aside: PM
stands for Pre-Multiplied. This means we have multiplied the alpha value by each
component, which is more efficient for blending.) Since your alpha is FF, this
color is premultiplied. Unlike SkColor, SkPMColor has an ordering that can be
determined at compile time. SK_PMCOLOR_BYTE_ORDER specifies the ordering. This
might be red on your build, but we could redefine some values in SkPostConfig.h
(see SK_A32_SHIFT and the macros below) and make it a different color.

There are a couple safe ways to do this so it is the same color regardless of
the order of SkPMColor, which can be found in SkColor.h:

SkPreMultiplyColor(SK_ColorRED)
SkPreMultiplyARGB(0xFF, 0xFF, 0, 0)

Let me know if you want to talk about any of this more.

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:35: SkBitmap fBM;
Generally, this should be private unless you want to allow a subclass to access
it. (It is generally better, though, if a subclass needs to access this, to make
it private with a protected method for access.)

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:37: virtual bool onQuery(SkEvent* evt) {
The spacing is wrong for much of this file. Generally, you want to indent four
spaces for all class members, and again for each level of scope.

emmaleeroach

I didn't want to include the sample changes, but I forgot to create a new ...

5 years, 7 months ago (2015-05-18 20:04:28 UTC) #6

I didn't want to include the sample changes, but I forgot to create a new branch
for the dm work, so they were on the same branch and I didn't know how to upload
one without the other.  I'll make sure to create separate branches next time. 

I changed the subset decoding to work for any number of subsets now.

https://codereview.chromium.org/1134113006/diff/30001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1134113006/diff/30001/dm/DM.cpp#newcode282
dm/DM.cpp:282: push_codec_srcs(path); //hits bp
On 2015/05/18 14:03:58, scroggo wrote:
> Was this accidentally left in?

Yea, I removed it

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:169: int fDivisor = 2;
On 2015/05/18 14:03:58, scroggo wrote:
> style nit: Skia adds an 'f' to the beginning of variables to show that they
are
> member variables (or 'fields'). For local variables, we would just name them
> something like "divisor".
> 
> Also, this variable could be const. (Not absolutely necessary, but it lets a
> reader and the compiler know that you won't be changing it.)

>okay, I changed the name to divisor and made it a const

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:182: char* line = new
char[decodeInfo.width()*decodeInfo.bytesPerPixel()];
On 2015/05/18 14:03:58, scroggo wrote:
> This will leak. You can solve this a few different ways. The most
> straightforward way is to call
> 
> delete[] line;
> 
> when you are done with it. The problem with this approach is that if you had
> some error case that resulted in a return statement before you get to the line
> that calls delete[], it would still leak the memory.
> 
> We have some ways to make it automatically get deleted when it goes out of
> scope, similar to smart pointers, which we have discussed. The Skia objects
that
> do this are in SkTemplates. For example, you can use SkAutoTDeleteArray. After
> the line that declares the variable "line", you would write the following:
> 
> SkAutoTDeleteArray lineDeleter(line);
>
>the above gave me the error: missing template arguements
>
>I changed to:
>SkAutoDeleteArray<char> lineDeleter(line);
>
>Is this correct?

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:183: for (int subset = 0; subset < fDivisor*2; subset++) {
On 2015/05/18 14:03:58, scroggo wrote:
> subset < fDivisor*2
> 
> Shouldn't this be subset < fDivisor * fDivisor? If you changed fDivisor to 3,
> won't you have 9 subsets?
>
>Yes, you are right. I only implemented the case with 4 subsets, but I will
change for any number of subsets.

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:185: if (subset == 2) {
On 2015/05/18 14:03:59, scroggo wrote:
> Again, this supposes that fDivisor is 2. The nice thing about using the
variable
> fDivisor is that we can change one line of code and test a different thing. 
>
>I changed this to if(0 == subset%divisor) to work with any size divisor

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:189: return Error::Nonfatal("Cannot use scanline decoder for
all images");
On 2015/05/18 14:03:59, scroggo wrote:
> I think in this case we have an actual error. If the scanline decoder cannot
be
> created the first time, it may mean the image was a progressive image, so we
> cannot create one. If we can create it once, we should be able to create it
> again for the same image. (Assuming we could rewind the stream, but all of our
> streams we use here should be rewindable.)

>okay, so the check was unnecessary? I removed the check

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:200: //if subset if on right copy right half of line
On 2015/05/18 14:03:58, scroggo wrote:
> is*

Acknowledged.

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:201: memcpy(subsetBm.getAddr(0,y),
On 2015/05/18 14:03:58, scroggo wrote:
> Won't this copy to the beginning of this row?

>Yes, it copies to the beginning of the subsetBm's row
>Then the subsetBm is drawn to a section of the larger canvas based on the x and
y values

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:203:
subsetDecodeInfo.width()*subsetDecodeInfo.bytesPerPixel());
On 2015/05/18 14:03:58, scroggo wrote:
> Won't this copy too much?

>this worked when divisor = 2.  I changed the code to work with any divisor
value, so this changed
>I don't think it copied too much.  For the right half it started copying at
line + subsetDecodeInfo.width()*subsetDecodeInfo.bytesPerPixel(), which is where
the right side of the image starts (width is center of image).
>then it copies subsetDecodeInfo.width()*subsetDecodeInfo.bytesPerPixel() bytes,
which is the amount of bytes in half the line

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:215: int h;
On 2015/05/18 14:03:58, scroggo wrote:
> nit: These variables look like width and height, but they are x and y, right?

>Yes, I changed their names to x and y

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:217: switch (subset) {
On 2015/05/18 14:03:58, scroggo wrote:
> nit: This works with a divisor of 2, but I think it will be harder if we try
to
> make it more general purpose. It might be easier to follow the pattern of
> ImageSrc when it has an fDivisor != 0.

>I changed the code to work with any value of divisor
>I'm not sure what pattern you are referring to above, so I implemented my own
pattern (it wasn't very difficult)
>If you think ImageSrc's pattern is better, I can change it 
>where is the ImageSrc pattern located?

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage.cpp
File samplecode/SampleImage.cpp (right):

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:17: ImageView(const char imageFilename[]) {
On 2015/05/18 14:03:59, scroggo wrote:
> nit: Skia uses a four space indent.
> 
> Not sure if you've seen it, but here are our code style guidelines:
>
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
> 
> They are neither exhaustive nor accurate, but they should give you a good
start.

Acknowledged.

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:18: SkString resourcePath =
GetResourcePath(imageFilename);
On 2015/05/18 14:03:59, scroggo wrote:
> It threw me off for a second that this is lined up with the function
signature.
> This should be indented four spaces (although once you move the signature
back,
> this is in the right place).

Acknowledged.

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:28: } else {
On 2015/05/18 14:03:59, scroggo wrote:
> This should line up with the if statement.

Acknowledged.

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:30: *(fBM.getAddr32(0,0)) = 0xFF0000FF; // red == bad
On 2015/05/18 14:03:59, scroggo wrote:
> This is just an example, so it doesn't matter too much, but it's worth
> discussing colors.
> 
> We have a couple of different types that represent colors: SkColor and
SkPMColor
> (both are found in SkColor.h, and both are typedefs for uint32_t).
> 
> SkColor is platform-independent, and can be represented as a hexadecimal
number
> that looks like 0xAARRGGBB. This is used for APIs like SkPaint::setColor(),
> allowing someone to write the same code to run on multiple platforms.
> 
> SkPMColor is the color used for drawing (SkColors will always be converted to
> SkPMColor). So the address that getAddr32 points to is an SkPMColor. (Aside:
PM
> stands for Pre-Multiplied. This means we have multiplied the alpha value by
each
> component, which is more efficient for blending.) Since your alpha is FF, this
> color is premultiplied. Unlike SkColor, SkPMColor has an ordering that can be
> determined at compile time. SK_PMCOLOR_BYTE_ORDER specifies the ordering. This
> might be red on your build, but we could redefine some values in
SkPostConfig.h
> (see SK_A32_SHIFT and the macros below) and make it a different color.
> 
> There are a couple safe ways to do this so it is the same color regardless of
> the order of SkPMColor, which can be found in SkColor.h:
> 
> SkPreMultiplyColor(SK_ColorRED)
> SkPreMultiplyARGB(0xFF, 0xFF, 0, 0)
> 
> Let me know if you want to talk about any of this more.

>Thanks, the color types make much more sense now.
>What exactly does the pre-multiplying do?  You said it makes it more efficient
for blending, is this because when you blend you need a SkPMColor, so it's
faster to start with an SkPMColor, instead of having to convert while blending?

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:35: SkBitmap fBM;
On 2015/05/18 14:03:59, scroggo wrote:
> Generally, this should be private unless you want to allow a subclass to
access
> it. (It is generally better, though, if a subclass needs to access this, to
make
> it private with a protected method for access.)

>Okay, I made it private

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:37: virtual bool onQuery(SkEvent* evt) {
On 2015/05/18 14:03:59, scroggo wrote:
> The spacing is wrong for much of this file. Generally, you want to indent four
> spaces for all class members, and again for each level of scope.

>I used tabs for this file initially, which caused the spacing to be wrong
>I changed all the tabs to 4 spaces, so hopefully it is correct now

scroggo

On 2015/05/18 20:04:28, emmaleeroach wrote: > I didn't want to include the sample changes, but ...

5 years, 7 months ago (2015-05-18 20:43:18 UTC) #7

On 2015/05/18 20:04:28, emmaleeroach wrote:
> I didn't want to include the sample changes, but I forgot to create a new
branch
> for the dm work, so they were on the same branch and I didn't know how to
upload
> one without the other.  I'll make sure to create separate branches next time. 

There are probably a lot of ways to do it, but in this case, I'll suggest
something simple:

Your samples were really just for to help you learn; we do not really need to
check them in. If you want to just delete them, you can do the following:

git rm samplecode/SampleImage.cpp
git rm experimental/example.cpp
git checkout master -- gyp/SampleApp.gyp
git checkout master -- gyp/example.gyp
git commit
git cl upload

Then we can treat this issue as just being for the changes to DM.

I also recommend reading https://git-scm.com/book/en/v2. You can skim around for
the sections that are useful to you, but it's very helpful to understand all the
things you can do with git (and it may take a while, so it's okay if you don't
know it all right away). If your changes are different commits (i.e. if you did
not use git commit --amend), you could cherry-pick the commits you want onto a
new branch and/or git revert the commits you do not want in your current branch.
I wouldn't worry about this too much for now, since the simple approach will
work.

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:182: char* line = new
char[decodeInfo.width()*decodeInfo.bytesPerPixel()];
On 2015/05/18 20:04:27, emmaleeroach wrote:
> On 2015/05/18 14:03:58, scroggo wrote:
> > This will leak. You can solve this a few different ways. The most
> > straightforward way is to call
> > 
> > delete[] line;
> > 
> > when you are done with it. The problem with this approach is that if you had
> > some error case that resulted in a return statement before you get to the
line
> > that calls delete[], it would still leak the memory.
> > 
> > We have some ways to make it automatically get deleted when it goes out of
> > scope, similar to smart pointers, which we have discussed. The Skia objects
> that
> > do this are in SkTemplates. For example, you can use SkAutoTDeleteArray.
After
> > the line that declares the variable "line", you would write the following:
> > 
> > SkAutoTDeleteArray lineDeleter(line);
> >
> >the above gave me the error: missing template arguements
> >
> >I changed to:
> >SkAutoDeleteArray<char> lineDeleter(line);
> >
> >Is this correct?

Yes.

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:189: return Error::Nonfatal("Cannot use scanline decoder for
all images");
On 2015/05/18 20:04:27, emmaleeroach wrote:
> On 2015/05/18 14:03:59, scroggo wrote:
> > I think in this case we have an actual error. If the scanline decoder cannot
> be
> > created the first time, it may mean the image was a progressive image, so we
> > cannot create one. If we can create it once, we should be able to create it
> > again for the same image. (Assuming we could rewind the stream, but all of
our
> > streams we use here should be rewindable.)
> 
> >okay, so the check was unnecessary? I removed the check

Oh, no, the check is necessary; the behavior if subsetScanlineDecoder is null is
incorrect. Place the check back, and if it is NULL return an error.

DM is a little sneaky: draw() returns an Error, which is a glorified SkString.
You can return an error just by saying:

return "Error message!";

or

return SkStringPrintf("Error %s", "message!");

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:201: memcpy(subsetBm.getAddr(0,y),
On 2015/05/18 20:04:27, emmaleeroach wrote:
> On 2015/05/18 14:03:58, scroggo wrote:
> > Won't this copy to the beginning of this row?
> 
> >Yes, it copies to the beginning of the subsetBm's row
> >Then the subsetBm is drawn to a section of the larger canvas based on the x
and
> y values

Oh, of course!

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:203:
subsetDecodeInfo.width()*subsetDecodeInfo.bytesPerPixel());
On 2015/05/18 20:04:27, emmaleeroach wrote:
> On 2015/05/18 14:03:58, scroggo wrote:
> > Won't this copy too much?
> 
> >this worked when divisor = 2.  I changed the code to work with any divisor
> value, so this changed
> >I don't think it copied too much.  For the right half it started copying at
> line + subsetDecodeInfo.width()*subsetDecodeInfo.bytesPerPixel(), which is
where
> the right side of the image starts (width is center of image).
> >then it copies subsetDecodeInfo.width()*subsetDecodeInfo.bytesPerPixel()
bytes,
> which is the amount of bytes in half the line

Got it. My eyes must have glazed over when I read subsetDecodeInfo.width(), so I
thought it was the width of the whole image instead of the subset, as it clearly
states.

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:217: switch (subset) {
On 2015/05/18 20:04:27, emmaleeroach wrote:
> On 2015/05/18 14:03:58, scroggo wrote:
> > nit: This works with a divisor of 2, but I think it will be harder if we try
> to
> > make it more general purpose. It might be easier to follow the pattern of
> > ImageSrc when it has an fDivisor != 0.
> 
> >I changed the code to work with any value of divisor
> >I'm not sure what pattern you are referring to above, so I implemented my own
> pattern (it wasn't very difficult)
> >If you think ImageSrc's pattern is better, I can change it 
> >where is the ImageSrc pattern located?

https://skia.googlesource.com/skia/+/7be0ce0ab123bbad8ec22b67b8593f1bdd5179e0...,
lines 232-253. I don't know that it's significantly better; it is just a little
more succinct.

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage.cpp
File samplecode/SampleImage.cpp (right):

https://codereview.chromium.org/1134113006/diff/30001/samplecode/SampleImage....
samplecode/SampleImage.cpp:30: *(fBM.getAddr32(0,0)) = 0xFF0000FF; // red == bad
On 2015/05/18 20:04:28, emmaleeroach wrote:
> On 2015/05/18 14:03:59, scroggo wrote:
> > This is just an example, so it doesn't matter too much, but it's worth
> > discussing colors.
> > 
> > We have a couple of different types that represent colors: SkColor and
> SkPMColor
> > (both are found in SkColor.h, and both are typedefs for uint32_t).
> > 
> > SkColor is platform-independent, and can be represented as a hexadecimal
> number
> > that looks like 0xAARRGGBB. This is used for APIs like SkPaint::setColor(),
> > allowing someone to write the same code to run on multiple platforms.
> > 
> > SkPMColor is the color used for drawing (SkColors will always be converted
to
> > SkPMColor). So the address that getAddr32 points to is an SkPMColor. (Aside:
> PM
> > stands for Pre-Multiplied. This means we have multiplied the alpha value by
> each
> > component, which is more efficient for blending.) Since your alpha is FF,
this
> > color is premultiplied. Unlike SkColor, SkPMColor has an ordering that can
be
> > determined at compile time. SK_PMCOLOR_BYTE_ORDER specifies the ordering.
This
> > might be red on your build, but we could redefine some values in
> SkPostConfig.h
> > (see SK_A32_SHIFT and the macros below) and make it a different color.
> > 
> > There are a couple safe ways to do this so it is the same color regardless
of
> > the order of SkPMColor, which can be found in SkColor.h:
> > 
> > SkPreMultiplyColor(SK_ColorRED)
> > SkPreMultiplyARGB(0xFF, 0xFF, 0, 0)
> > 
> > Let me know if you want to talk about any of this more.
> 
> >Thanks, the color types make much more sense now.
> >What exactly does the pre-multiplying do?  You said it makes it more
efficient
> for blending, is this because when you blend you need a SkPMColor, so it's
> faster to start with an SkPMColor, instead of having to convert while
blending?
>    

Yes :)

https://codereview.chromium.org/1134113006/diff/50001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/50001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:186: if (0 == subset%divisor && 0 != subset) {
Alternatively, you could remove the second half of this if statement and remove
the original call to getScanlineDecoder. You'd still need to handle subset 0
differently if null is returned, and it would mean we did extra work in the case
where the image does not support scanline decoding, so maybe this way is better.

emmaleeroach

I deleted the sample files, and changed my algorithm like ImageSrc's, which is cleaner. Also, ...

5 years, 7 months ago (2015-05-18 22:35:15 UTC) #8

I deleted the sample files, and changed my algorithm like ImageSrc's, which is
cleaner.

Also, I will read that git book you linked, it seems really useful!

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:189: return Error::Nonfatal("Cannot use scanline decoder for
all images");
On 2015/05/18 20:43:18, scroggo wrote:
> On 2015/05/18 20:04:27, emmaleeroach wrote:
> > On 2015/05/18 14:03:59, scroggo wrote:
> > > I think in this case we have an actual error. If the scanline decoder
cannot
> > be
> > > created the first time, it may mean the image was a progressive image, so
we
> > > cannot create one. If we can create it once, we should be able to create
it
> > > again for the same image. (Assuming we could rewind the stream, but all of
> our
> > > streams we use here should be rewindable.)
> > 
> > >okay, so the check was unnecessary? I removed the check
> 
> Oh, no, the check is necessary; the behavior if subsetScanlineDecoder is null
is
> incorrect. Place the check back, and if it is NULL return an error.
> 
> DM is a little sneaky: draw() returns an Error, which is a glorified SkString.
> You can return an error just by saying:
> 
> return "Error message!";
> 
> or
> 
> return SkStringPrintf("Error %s", "message!");

>So you just mean the error message being returned was wrong before?
>I changed the message to "Error scanline decoder is NULL"

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:217: switch (subset) {
On 2015/05/18 20:43:18, scroggo wrote:
> On 2015/05/18 20:04:27, emmaleeroach wrote:
> > On 2015/05/18 14:03:58, scroggo wrote:
> > > nit: This works with a divisor of 2, but I think it will be harder if we
try
> > to
> > > make it more general purpose. It might be easier to follow the pattern of
> > > ImageSrc when it has an fDivisor != 0.
> > 
> > >I changed the code to work with any value of divisor
> > >I'm not sure what pattern you are referring to above, so I implemented my
own
> > pattern (it wasn't very difficult)
> > >If you think ImageSrc's pattern is better, I can change it 
> > >where is the ImageSrc pattern located?
> 
>
https://skia.googlesource.com/skia/+/7be0ce0ab123bbad8ec22b67b8593f1bdd5179e0...,
> lines 232-253. I don't know that it's significantly better; it is just a
little
> more succinct.

>This way is cleaner, I changed mine to follow this pattern

scroggo

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

5 years, 7 months ago (2015-05-18 23:02:16 UTC) #9

scroggo

Looks much better, with an open question about what the right subset width to use ...

5 years, 7 months ago (2015-05-18 23:02:17 UTC) #10

Looks much better, with an open question about what the right subset width to
use is.

Please update the change description (click the link that says "Edit Issue" and
change the description to something that describes what this change does).

Adding Matt. Matt, please take a look.

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/30001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:189: return Error::Nonfatal("Cannot use scanline decoder for
all images");
On 2015/05/18 22:35:14, emmaleeroach wrote:
> On 2015/05/18 20:43:18, scroggo wrote:
> > On 2015/05/18 20:04:27, emmaleeroach wrote:
> > > On 2015/05/18 14:03:59, scroggo wrote:
> > > > I think in this case we have an actual error. If the scanline decoder
> cannot
> > > be
> > > > created the first time, it may mean the image was a progressive image,
so
> we
> > > > cannot create one. If we can create it once, we should be able to create
> it
> > > > again for the same image. (Assuming we could rewind the stream, but all
of
> > our
> > > > streams we use here should be rewindable.)
> > > 
> > > >okay, so the check was unnecessary? I removed the check
> > 
> > Oh, no, the check is necessary; the behavior if subsetScanlineDecoder is
null
> is
> > incorrect. Place the check back, and if it is NULL return an error.
> > 
> > DM is a little sneaky: draw() returns an Error, which is a glorified
SkString.
> > You can return an error just by saying:
> > 
> > return "Error message!";
> > 
> > or
> > 
> > return SkStringPrintf("Error %s", "message!");
> 
> >So you just mean the error message being returned was wrong before?
> >I changed the message to "Error scanline decoder is NULL"

Yes. Both the message itself and "Error::Nonfatal", which tells us that it is
not a real error.

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:186: const int w = subsetWidth*divisor;
I think it would be better to use decodeInfo.width().

One problem with the current code shows itself if the width is odd: e.g. 501

subsetWidth will be 250, and w will be 500. So we'll miss out on the last pixel
on the right.

Which maybe means we should round up when finding the subsetWidth? (i.e. we'd
rather have two tiles horizontally, with one at 251 and one at 250 than three,
with two at 250 and one at 1)

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:215: return SkStringPrintf("Error scanline decoder is NULL");
No need for SkStringPrintf. You're returning an Error (defined in DMSrcSink.h),
which has an implicit constructor that takes a char*. So you can just say:

return "Error scanline decoder is NULL";

msarett

The big question for me is if our subset decode test should call skipScanlines(). I ...

5 years, 7 months ago (2015-05-19 14:01:13 UTC) #11

The big question for me is if our subset decode test should call
skipScanlines().  I provided more detail in the comments.

https://codereview.chromium.org/1134113006/diff/90001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1134113006/diff/90001/dm/DM.cpp#newcode210
dm/DM.cpp:210: push_src("image", "scanline_kGray8", new CodecSrc(path,
CodecSrc::kSubset_Mode,
"subset_kGray8"

https://codereview.chromium.org/1134113006/diff/90001/dm/DM.cpp#newcode220
dm/DM.cpp:220: push_src("image", "scanline_kIndex8", new CodecSrc(path,
CodecSrc::kSubset_Mode,
"subset_kIndex8"

https://codereview.chromium.org/1134113006/diff/90001/dm/DM.cpp#newcode233
dm/DM.cpp:233: push_src("image", "subset", new CodecSrc(path,
CodecSrc::kSubset_Mode,
I think we use the same two tags (image, subset) for SkImageDecoder subset
decodes.  We would want subset decodes from SkCodec to be labeled uniquely.

I'm pretty sure the first tag has to be "image" (Leon can you confirm?).  We
should choose a different tag for the second one.  Also, whatever name we choose
here should be used in the above names.

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:182: char* line = new
char[decodeInfo.width()*decodeInfo.bytesPerPixel()];
nit: Let's check with Leon, but I think the preference here is to use
SkNEW_ARRAY.

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:186: const int w = subsetWidth*divisor;
On 2015/05/18 23:02:17, scroggo wrote:
> I think it would be better to use decodeInfo.width().
> 
> One problem with the current code shows itself if the width is odd: e.g. 501
> 
> subsetWidth will be 250, and w will be 500. So we'll miss out on the last
pixel
> on the right.
> 
> Which maybe means we should round up when finding the subsetWidth? (i.e. we'd
> rather have two tiles horizontally, with one at 251 and one at 250 than three,
> with two at 250 and one at 1)

I think this is the tricky part of this test.  If the divisor does not divide
evenly into width and height, we may need to adjust the width and height of the
end subsets to make sure we cover the entire image.

The automated test system will indicate that decodes are failing if we miss a
row or column of pixels.

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:194: for (int subsetY = 0; subsetY < subsetHeight; ++subsetY) {
My preference is to use a different scanline decoder for each of the subsets. 
That is because my personal stake in this test is to test the behavior of
subsetScanlineDecoder->skipScanlines(int numLinesToSkip).

This call is useful when we need to decode a single subset of an image and we
want to skip to that subset without doing extra decoding work on other subsets.

However, there are also cases where we want to decode all of the subsets of an
image, so your choice is completely valid.  Maybe we should be testing both? 
Any thoughts Leon?

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:196: subsetScanlineDecoder->getScanlines(line, 1, 0);
I'm a little confused about how this works.  It looks like you'll call
getScanlines() (subsetHeight * divisor * divisor) times.  Doesn't it stop giving
you back scanlines after height calls?

If the code works then I'm obviously confused (sorry!), but I can't seem to
figure it out.

scroggo

Matt, thanks for the good comments. Sorry I had you two duplicating work :( I ...

5 years, 7 months ago (2015-05-19 15:21:41 UTC) #12

Matt, thanks for the good comments. Sorry I had you two duplicating work :( I
think it ended up giving us some interesting data regarding how you implemented
it differently, though.

> New CodeSrc draw mode, kSubset_Mode, which decodes the image in subsets using
a scanline decoder

nit: When we submit this, this will end up being the commit description in our
git repository. I try to keep this first line short (e.g. 50 chars - if you
write your commit message with VIM, it changes the color once you get past
this), with a blank line followed by a longer description if necessary. My
memory tells me this is they way you're "supposed" to do it. I'm not sure who
originally proposed this, but a quick web search brought me this:
http://stackoverflow.com/questions/2290016/git-commit-messages-50-72-formatting

Like any standard, it seems not everyone agrees. Skia has no official stance
that I'm aware of, and plenty of our commits do not follow this pattern. I
personally find it easier to read, though.

https://codereview.chromium.org/1134113006/diff/90001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1134113006/diff/90001/dm/DM.cpp#newcode233
dm/DM.cpp:233: push_src("image", "subset", new CodecSrc(path,
CodecSrc::kSubset_Mode,
On 2015/05/19 14:01:13, msarett wrote:
> I think we use the same two tags (image, subset) for SkImageDecoder subset
> decodes.  We would want subset decodes from SkCodec to be labeled uniquely.
> 
> I'm pretty sure the first tag has to be "image" (Leon can you confirm?).  We
> should choose a different tag for the second one.  Also, whatever name we
choose
> here should be used in the above names.

Agreed on all counts. So the first one should be "image", and for the second
one, maybe we should use "scanline_subset"? It gets a little unwieldy once we
append kIndex8 or kGray8 to the end, but maybe that's okay.

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:182: char* line = new
char[decodeInfo.width()*decodeInfo.bytesPerPixel()];
On 2015/05/19 14:01:13, msarett wrote:
> nit: Let's check with Leon, but I think the preference here is to use
> SkNEW_ARRAY.

The short answer is yes.

A little background: we have some macros defined in SkPostConfig.h (various
versions of SkNEW and SkDELETE) which are used to call new/delete. The
motivation (as I understand it) is to allow someone using Skia to track memory
usage of Skia objects. By default, they do what you expect - they just call
new/delete, and this is how it is used by Chrome and Android.

Someone *could* redefine them to keep track of Skia memory usage (I believe
Mozilla used this at one point), or they could even use some special allocator.
I do not know whether anyone defines them to anything else. I believe there was
some talk of removing them recently, and providing another way to do it.

Internally, we try to use them everywhere. In our tests, we seem to have a mix
and match. I believe I've heard it argued that our tests are not Skia proper,
and whoever might care to use a different implementation is really concerned
with Skia itself, so there is no reason to use it in tests.

That said, SkAutoTDeleteArray calls SkDELETE_ARRAY, so yes, we should probably
use SkNEW_ARRAY. Otherwise if we were building with alternate implementations (a
big if...), we would not call the right version of delete[].

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:186: const int w = subsetWidth*divisor;
On 2015/05/19 14:01:13, msarett wrote:
> On 2015/05/18 23:02:17, scroggo wrote:
> > I think it would be better to use decodeInfo.width().
> > 
> > One problem with the current code shows itself if the width is odd: e.g. 501
> > 
> > subsetWidth will be 250, and w will be 500. So we'll miss out on the last
> pixel
> > on the right.
> > 
> > Which maybe means we should round up when finding the subsetWidth? (i.e.
we'd
> > rather have two tiles horizontally, with one at 251 and one at 250 than
three,
> > with two at 250 and one at 1)
> 
> I think this is the tricky part of this test.  If the divisor does not divide
> evenly into width and height, we may need to adjust the width and height of
the
> end subsets to make sure we cover the entire image.

Indeed. We need to investigate how exactly BitmapRegionDecoder is used to
determine the right way to do that adjustment.

> 
> The automated test system will indicate that decodes are failing if we miss a
> row or column of pixels.

FYI Emmalee: Matt is referring to gold.skia.org, which Matt or I can show you
how to use.

Although it is automated, we need to make sure we stay on top of baselining
images in order for us to know when we've broken something. Matt, it looks like
you've done most of that (or Derek?) - thanks! There are a few at head that are
untriaged though:

A couple are JPEG, which are slightly different for SkCodec vs SkImageDecoder.
I'm guessing that's the difference between using the visual optimizations? The
others look to be bmp images that we just have never triaged. These can be a
good chance to show Emmalee how to use it.

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:194: for (int subsetY = 0; subsetY < subsetHeight; ++subsetY) {
On 2015/05/19 14:01:13, msarett wrote:
> My preference is to use a different scanline decoder for each of the subsets. 
> That is because my personal stake in this test is to test the behavior of
> subsetScanlineDecoder->skipScanlines(int numLinesToSkip).
> 
> This call is useful when we need to decode a single subset of an image and we
> want to skip to that subset without doing extra decoding work on other
subsets.
> 
> However, there are also cases where we want to decode all of the subsets of an
> image, so your choice is completely valid.  Maybe we should be testing both? 
> Any thoughts Leon?
> 

Good questions. I think it's reasonable to use a new scanline decoder for each
subset and use skipScanlines. In DM, where we're testing correctness, I don't
think it's necessary to test both, since I don't think using the same scanline
decoder for the entire vertical stripe tests anything different from the pure
scanline mode (i.e. kSubset_Mode) that isn't tested by using skipScanlines.

When we write the performance tests, however, maybe we will want to test more
variations. The real question there will be which use cases are most important?
If someone is only decoding a single subset, the question is somewhat moot (we
always will skip to the line we need, and we never need to reuse the scanline
decoder). But if someone potentially wants the whole image (as is the case in
Gallery), how to do the next subset is an interesting question. Unfortunately,
apps are already using BitmapRegionDecoder, which allows requesting arbitrary
subsets at any time. If they happen to decode a subset below the last one, we
could potentially avoid skipping scanlines, although they likely choose which
subset to decode based on which direction the user scrolls.

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:196: subsetScanlineDecoder->getScanlines(line, 1, 0);
On 2015/05/19 14:01:13, msarett wrote:
> I'm a little confused about how this works.  It looks like you'll call
> getScanlines() (subsetHeight * divisor * divisor) times.  Doesn't it stop
giving
> you back scanlines after height calls?
> 
> If the code works then I'm obviously confused (sorry!), but I can't seem to
> figure it out.

I missed this at first, too - at the end of the y loop we call
getScanlineDecoder to get a new one. (API question - it may be cleaner to add a
new API: SkScanlineDecoder::reset().)

(And to answer your question, yes, one scanline decoder would stop giving you
scanlines after height calls.)

emmaleer

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

5 years, 7 months ago (2015-05-21 17:51:12 UTC) #13

emmaleer

I updated the code to work with uneven widths and heights by adjusting the width ...

5 years, 7 months ago (2015-05-21 17:51:13 UTC) #14

I updated the code to work with uneven widths and heights by adjusting the width
and height of the end subsets.  
Each subset now uses a new scanline decoder and skipScanlines(int LineCount)
skips to the first line of the subset.

https://codereview.chromium.org/1134113006/diff/90001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1134113006/diff/90001/dm/DM.cpp#newcode233
dm/DM.cpp:233: push_src("image", "subset", new CodecSrc(path,
CodecSrc::kSubset_Mode,
On 2015/05/19 15:21:41, scroggo wrote:
> On 2015/05/19 14:01:13, msarett wrote:
> > I think we use the same two tags (image, subset) for SkImageDecoder subset
> > decodes.  We would want subset decodes from SkCodec to be labeled uniquely.
> > 
> > I'm pretty sure the first tag has to be "image" (Leon can you confirm?).  We
> > should choose a different tag for the second one.  Also, whatever name we
> choose
> > here should be used in the above names.
> 
> Agreed on all counts. So the first one should be "image", and for the second
> one, maybe we should use "scanline_subset"? It gets a little unwieldy once we
> append kIndex8 or kGray8 to the end, but maybe that's okay.

Acknowledged.

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:182: char* line = new
char[decodeInfo.width()*decodeInfo.bytesPerPixel()];
On 2015/05/19 15:21:41, scroggo wrote:
> On 2015/05/19 14:01:13, msarett wrote:
> > nit: Let's check with Leon, but I think the preference here is to use
> > SkNEW_ARRAY.
> 
> The short answer is yes.
> 
> A little background: we have some macros defined in SkPostConfig.h (various
> versions of SkNEW and SkDELETE) which are used to call new/delete. The
> motivation (as I understand it) is to allow someone using Skia to track memory
> usage of Skia objects. By default, they do what you expect - they just call
> new/delete, and this is how it is used by Chrome and Android.
> 
> Someone *could* redefine them to keep track of Skia memory usage (I believe
> Mozilla used this at one point), or they could even use some special
allocator.
> I do not know whether anyone defines them to anything else. I believe there
was
> some talk of removing them recently, and providing another way to do it.
> 
> Internally, we try to use them everywhere. In our tests, we seem to have a mix
> and match. I believe I've heard it argued that our tests are not Skia proper,
> and whoever might care to use a different implementation is really concerned
> with Skia itself, so there is no reason to use it in tests.
> 
> That said, SkAutoTDeleteArray calls SkDELETE_ARRAY, so yes, we should probably
> use SkNEW_ARRAY. Otherwise if we were building with alternate implementations
(a
> big if...), we would not call the right version of delete[].

Acknowledged.

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:186: const int w = subsetWidth*divisor;
On 2015/05/19 15:21:41, scroggo wrote:
> On 2015/05/19 14:01:13, msarett wrote:
> > On 2015/05/18 23:02:17, scroggo wrote:
> > > I think it would be better to use decodeInfo.width().
> > > 
> > > One problem with the current code shows itself if the width is odd: e.g.
501
> > > 
> > > subsetWidth will be 250, and w will be 500. So we'll miss out on the last
> > pixel
> > > on the right.
> > > 
> > > Which maybe means we should round up when finding the subsetWidth? (i.e.
> we'd
> > > rather have two tiles horizontally, with one at 251 and one at 250 than
> three,
> > > with two at 250 and one at 1)
> > 
> > I think this is the tricky part of this test.  If the divisor does not
divide
> > evenly into width and height, we may need to adjust the width and height of
> the
> > end subsets to make sure we cover the entire image.
> 
> Indeed. We need to investigate how exactly BitmapRegionDecoder is used to
> determine the right way to do that adjustment.
> 
> > 
> > The automated test system will indicate that decodes are failing if we miss
a
> > row or column of pixels.
> 
> FYI Emmalee: Matt is referring to http://gold.skia.org, which Matt or I can
show you
> how to use.
> 
> Although it is automated, we need to make sure we stay on top of baselining
> images in order for us to know when we've broken something. Matt, it looks
like
> you've done most of that (or Derek?) - thanks! There are a few at head that
are
> untriaged though:
> 
> A couple are JPEG, which are slightly different for SkCodec vs SkImageDecoder.
> I'm guessing that's the difference between using the visual optimizations? The
> others look to be bmp images that we just have never triaged. These can be a
> good chance to show Emmalee how to use it.

>I fixed this, now the width and height of the end subsets are adjusted to cover
the entire image

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:194: for (int subsetY = 0; subsetY < subsetHeight; ++subsetY) {
On 2015/05/19 15:21:41, scroggo wrote:
> On 2015/05/19 14:01:13, msarett wrote:
> > My preference is to use a different scanline decoder for each of the
subsets. 
> > That is because my personal stake in this test is to test the behavior of
> > subsetScanlineDecoder->skipScanlines(int numLinesToSkip).
> > 
> > This call is useful when we need to decode a single subset of an image and
we
> > want to skip to that subset without doing extra decoding work on other
> subsets.
> > 
> > However, there are also cases where we want to decode all of the subsets of
an
> > image, so your choice is completely valid.  Maybe we should be testing both?

> > Any thoughts Leon?
> > 
> 
> Good questions. I think it's reasonable to use a new scanline decoder for each
> subset and use skipScanlines. In DM, where we're testing correctness, I don't
> think it's necessary to test both, since I don't think using the same scanline
> decoder for the entire vertical stripe tests anything different from the pure
> scanline mode (i.e. kSubset_Mode) that isn't tested by using skipScanlines.
> 
> When we write the performance tests, however, maybe we will want to test more
> variations. The real question there will be which use cases are most
important?
> If someone is only decoding a single subset, the question is somewhat moot (we
> always will skip to the line we need, and we never need to reuse the scanline
> decoder). But if someone potentially wants the whole image (as is the case in
> Gallery), how to do the next subset is an interesting question. Unfortunately,
> apps are already using BitmapRegionDecoder, which allows requesting arbitrary
> subsets at any time. If they happen to decode a subset below the last one, we
> could potentially avoid skipping scanlines, although they likely choose which
> subset to decode based on which direction the user scrolls.

>Now a different scanline decoder is used for each subset, and I call
subsetScanlineDecoder->skipScanlines(int numLinesToSkip)

>Note: skipScanlines is currently reading all the lines and not actually
skipping them.  skipScanlines needs to be implemented before we benchmark this
mode.

https://codereview.chromium.org/1134113006/diff/90001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:196: subsetScanlineDecoder->getScanlines(line, 1, 0);
On 2015/05/19 15:21:41, scroggo wrote:
> On 2015/05/19 14:01:13, msarett wrote:
> > I'm a little confused about how this works.  It looks like you'll call
> > getScanlines() (subsetHeight * divisor * divisor) times.  Doesn't it stop
> giving
> > you back scanlines after height calls?
> > 
> > If the code works then I'm obviously confused (sorry!), but I can't seem to
> > figure it out.
> 
> I missed this at first, too - at the end of the y loop we call
> getScanlineDecoder to get a new one. (API question - it may be cleaner to add
a
> new API: SkScanlineDecoder::reset().)
> 
> (And to answer your question, yes, one scanline decoder would stop giving you
> scanlines after height calls.)

>I create a new scanline decoder for each subet now, which hopefully makes this
more clear

scroggo

https://codereview.chromium.org/1134113006/diff/130001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1134113006/diff/130001/dm/DMSrcSink.cpp#newcode172 dm/DMSrcSink.cpp:172: const int subsetWidth = decodeInfo.width()/divisor; nit: From here on ...

5 years, 7 months ago (2015-05-21 19:32:36 UTC) #15

emmaleeroach

I defined new variables currentSubsetWidth = subsetWidth + extraX, and currentSubsetHeight - subsetHeight + extraY, ...

5 years, 7 months ago (2015-05-21 22:17:24 UTC) #16

I defined new variables currentSubsetWidth = subsetWidth + extraX, and
currentSubsetHeight - subsetHeight + extraY, added comments, and moved the
declaration of SubsetBm to right before it is used.

https://codereview.chromium.org/1134113006/diff/130001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/130001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:174: SkImageInfo subsetDecodeInfo =
On 2015/05/21 19:32:36, scroggo wrote:
> I think these names might be a little misleading. subsetDecodeInfo has a
> different width from subsetWidth. Maybe rename this largestSubsetDecodeInfo?
> (That does get a little unwieldy though...)

Acknowledged.

https://codereview.chromium.org/1134113006/diff/130001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:178: if (!subsetBm.tryAllocPixels(subsetDecodeInfo, NULL,
colorTable.get())) {
On 2015/05/21 19:32:35, scroggo wrote:
> I don't think you need this. This will set subsetBm's info and allocate an
> SkPixelRef. Down below, you call:
> 
> largestSubsetBm.extractSubset(&subsetBM, bounds);
> 
> which will set the info again and replace the pixel ref. It doesn't look like
> subsetBm is used until then.

Acknowledged.

https://codereview.chromium.org/1134113006/diff/130001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:211: return Error::Nonfatal("Cannot use scanline decoder for
all images");
On 2015/05/21 19:32:36, scroggo wrote:
> This is good for the first time through the loop, but after that, this should
be
> a fatal error.

Acknowledged.

https://codereview.chromium.org/1134113006/diff/130001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:221: return SkStringPrintf("%s failed after skipping %d
scanlines with"
On 2015/05/21 19:32:36, scroggo wrote:
> Maybe rephrase this: y-1 is not the number of scanlines we successfully
skipped;
> y is the number we attempted to skip. We do not know how many lines were
skipped
> before failing. (This is different from pure kScanline_Mode, where we read one
> scanline at a time, so we know how many succeeded.)

Acknowledged.

https://codereview.chromium.org/1134113006/diff/130001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:225: bounds.setXYWH(0, 0, subsetWidth + extrax, subsetHeight +
extray);
On 2015/05/21 19:32:35, scroggo wrote:
> This is always the size of subsetBm, right? Can you move the declaration here?

>Yes, this is always the size of subsetBm.  I moved the declaration here.

https://codereview.chromium.org/1134113006/diff/130001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:226: largestSubsetBm.extractSubset(&subsetBm, bounds);
On 2015/05/21 19:32:36, scroggo wrote:
> Try to declare variables just before they're used. You can declare subsetBm
> here.
> 
> Also, you should check the return value in case it fails. (If it should never
> fail, you can SkASSERT(success);)

Acknowledged.

https://codereview.chromium.org/1134113006/diff/130001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:234: (subsetWidth + extrax)*subsetDecodeInfo.bytesPerPixel());
On 2015/05/21 19:32:36, scroggo wrote:
> I wonder if it would be cleaner to define new variables for 
> 
> subsetWidth + extrax
> 
> and
> 
> subsetHeight + extray
> 
> I was trying to think of the best way to do that, but it might just mean more
> names...

>I defined new variables:
>currentSubsetWidth = subsetWidth + extraX
>currentSubsetHeight = subsetHeight + extraY
>
>I think it's more clear now.  What do you think?
>I can revert to the old way if this just makes it more confusing

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

https://codereview.chromium.org/1134113006/diff/130001/src/codec/SkCodec_libp...
src/codec/SkCodec_libpng.cpp:628: png_read_rows(fCodec->fPng_ptr, &fSrcRow,
png_bytepp_NULL, 1);
On 2015/05/21 19:32:36, scroggo wrote:
> Maybe add a comment here that explains the potential tradeoff we've made by
> putting this in a loop.
> 
> It's possible that it would be faster to pass count to png_read_rows, but it
> would mean allocating more memory. (On the other hand, maybe there isn't much
of
> a tradeoff. I saw you were looking at the code in png_read_rows - is it more
> efficient to do multiple rows at once? Or do they just have their own loop?)

>png_read_rows calls png_read_row in a loop, so it just has its own loop
>it would be slightly faster to do multiple rows at once, since you would only
have to call png_read_rows once, and there are some if checks before it calls
png_read_row which would then only happen once
>However, I think this time is quite minimal, and the benefit of allocating less
memory is better than the slight speed up.
>Is this fine?

scroggo

This is looking good. I think after a few more small changes (below) it will ...

5 years, 7 months ago (2015-05-22 14:38:26 UTC) #17

This is looking good. I think after a few more small changes (below) it will be
ready to check in.

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:206: extraX = w - subsetWidth*divisor;
Can we remove extraX and extraY entirely? I think you can just say:

currentSubsetWidth = subsetWidth + w - subsetWidth*divisor;

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:239: bounds.setXYWH(0, 0, currentSubsetWidth,
currentSubsetHeight);
I think you can move the declaration of bounds here, right before it is used.

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:240: SkASSERT(largestSubsetBm.extractSubset(&subsetBm,
bounds));
This is a little tricky: In Release mode, SkASSERT(something) gets removed
completely. There are a couple ways you can do this so the extract is still done
in Release:

SkDEBUGCODE(bool result =) largestSubsetBm.extractSubset(&subsetBm, bounds);
SkASSERT(result);

or

SkAssertResult(largestSubsetBm.extractSubset(&subsetBm, bounds));

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:247: line + x*largestSubsetDecodeInfo.bytesPerPixel(),
nit: since largestSubsetDecodeInfo.bytesPerPixel() is the same as
decodeInfo.bytesPerPixel(), this might be more readable if you use the latter.
It might even be better to declare a variable for bytesPerPixel, so this can fit
on one line:

const size_t bpp = decodeInfo.bytesPerPixel();
memcpy(substBm.getAddr(0, subsetY), line + x * bpp, currentSubsetWidth * bpp);

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:262: currentSubsetHeight = subsetHeight + extraY;
Alternatively, where you set currentSubsetHeight, you can say:

if (/* bottom subset */) {
  currentSubsetHeight = subsetHeight + /* extraY */;
} else {
  currentSubsetHeight = subsetHeight;
}

This should also mean that you can declare it inside the loop.

emmaleeroach

I removed extraX and extraY variables, and moved the declarations of some variables to right ...

5 years, 7 months ago (2015-05-22 15:58:55 UTC) #18

I removed extraX and extraY variables, and moved the declarations of some
variables to right before they are used.
In general, is it best to declare a variable right before use, even if that
occurs inside a loop?

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:206: extraX = w - subsetWidth*divisor;
On 2015/05/22 14:38:26, scroggo wrote:
> Can we remove extraX and extraY entirely? I think you can just say:
> 
> currentSubsetWidth = subsetWidth + w - subsetWidth*divisor;

>Yes we can.  I removed extraX and extraY

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:239: bounds.setXYWH(0, 0, currentSubsetWidth,
currentSubsetHeight);
On 2015/05/22 14:38:26, scroggo wrote:
> I think you can move the declaration of bounds here, right before it is used.

Acknowledged.

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:240: SkASSERT(largestSubsetBm.extractSubset(&subsetBm,
bounds));
On 2015/05/22 14:38:26, scroggo wrote:
> This is a little tricky: In Release mode, SkASSERT(something) gets removed
> completely. There are a couple ways you can do this so the extract is still
done
> in Release:
> 
> SkDEBUGCODE(bool result =) largestSubsetBm.extractSubset(&subsetBm, bounds);
> SkASSERT(result);
> 
> or
> 
> SkAssertResult(largestSubsetBm.extractSubset(&subsetBm, bounds));

Acknowledged.

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:247: line + x*largestSubsetDecodeInfo.bytesPerPixel(),
On 2015/05/22 14:38:26, scroggo wrote:
> nit: since largestSubsetDecodeInfo.bytesPerPixel() is the same as
> decodeInfo.bytesPerPixel(), this might be more readable if you use the latter.
> It might even be better to declare a variable for bytesPerPixel, so this can
fit
> on one line:
> 
> const size_t bpp = decodeInfo.bytesPerPixel();
> memcpy(substBm.getAddr(0, subsetY), line + x * bpp, currentSubsetWidth * bpp);

Acknowledged.

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:262: currentSubsetHeight = subsetHeight + extraY;
On 2015/05/22 14:38:26, scroggo wrote:
> Alternatively, where you set currentSubsetHeight, you can say:
> 
> if (/* bottom subset */) {
>   currentSubsetHeight = subsetHeight + /* extraY */;
> } else {
>   currentSubsetHeight = subsetHeight;
> }
> 
> This should also mean that you can declare it inside the loop.

>When declaring variables, is it always better to declare them right before use,
even when that is inside a loop?
>Since, when declared inside a loop, the variable will be created each loop,
instead of once if declared outside a loop
>I was wondering what the typical choice for this was, and if there are any
trade offs for doing this

msarett

I added a few thoughts. I realize that I'm jumping in kind of late, so ...

5 years, 7 months ago (2015-05-22 17:42:19 UTC) #19

scroggo

On 2015/05/22 17:42:19, msarett wrote: > I added a few thoughts. I realize that I'm ...

5 years, 7 months ago (2015-05-22 18:39:54 UTC) #20

On 2015/05/22 17:42:19, msarett wrote:
> I added a few thoughts.  I realize that I'm jumping in kind of late, so feel
> free to ignore these comments.  None of them are critically important.

NP - thanks for the input!

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:262: currentSubsetHeight = subsetHeight + extraY;
On 2015/05/22 15:58:55, emmaleeroach wrote:
> On 2015/05/22 14:38:26, scroggo wrote:
> > Alternatively, where you set currentSubsetHeight, you can say:
> > 
> > if (/* bottom subset */) {
> >   currentSubsetHeight = subsetHeight + /* extraY */;
> > } else {
> >   currentSubsetHeight = subsetHeight;
> > }
> > 
> > This should also mean that you can declare it inside the loop.
> 
> >When declaring variables, is it always better to declare them right before
use,
> even when that is inside a loop?
> >Since, when declared inside a loop, the variable will be created each loop,
> instead of once if declared outside a loop
> >I was wondering what the typical choice for this was, and if there are any
> trade offs for doing this

I am hesitant to say "always", but I will say that most of the time it is better
to declare a variable right before it is used.

In this case, IIUC, you are concerned that each time through the loop the
program will need to do extra work to create the variable if it's inside the
loop, whereas it would have only had to create the variable once if it is
outside the loop.

If it is faster to declare it outside the loop, you are giving up some
maintainability. The code may be harder to follow, and a larger scope means that
you could accidentally change it in more places.

I *think* the compiler will generate the same assembly either way. (Someone else
in the office could probably give you a better answer than this. You can also
use a disassembler to compare, which might be interesting.) If not, I think this
could be at most a small performance difference. When it comes to this kind of
micro-optimization, my general rule is to go with clarity unless a profile shows
it to be a problem.

Follow-up: A Google search led me to this excellent discussion on StackOverflow
(StackOverflow is your friend):
http://stackoverflow.com/questions/7959573/declaring-variables-inside-loops-g...

According to that page, it actually may be *faster* to declare the variable
inside the loop, since the compiler has more information about when the variable
is used.

One exception that was wisely pointed out is that it's a different question if
we're talking about a class with a constructor, which would mean calling the
constructor (and destructor) each time through the loop. In this case, we're
just talking about a primitive type, so that is not a problem.

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:199: if (x + 2*subsetWidth > w) {
On 2015/05/22 17:42:19, msarett wrote:
> Is there another way to check for the end subset?
> 
> This does not work for strange cases that will probably never come up.  Ex:
> width = 10, divisor = 7.
> 
> Maybe there is an option to loop over the value of the divisor or something?

I agree that this seems a little clunky. I think it would always work if it
instead said something like

if (x + 2*subsetWidth > subsetWidth*divisor)

which is even clunkier...

+1 to trying using a loop over divisor.

emmaleer

I added currentRow and currentColumn variables to try and make the code cleaner. I check ...

5 years, 7 months ago (2015-05-22 21:55:22 UTC) #21

I added currentRow and currentColumn variables to try and make the code cleaner.
I check if (currentRow == divisor) to see if it is a bottom subset.

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/150001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:262: currentSubsetHeight = subsetHeight + extraY;
On 2015/05/22 18:39:53, scroggo wrote:
> On 2015/05/22 15:58:55, emmaleeroach wrote:
> > On 2015/05/22 14:38:26, scroggo wrote:
> > > Alternatively, where you set currentSubsetHeight, you can say:
> > > 
> > > if (/* bottom subset */) {
> > >   currentSubsetHeight = subsetHeight + /* extraY */;
> > > } else {
> > >   currentSubsetHeight = subsetHeight;
> > > }
> > > 
> > > This should also mean that you can declare it inside the loop.
> > 
> > >When declaring variables, is it always better to declare them right before
> use,
> > even when that is inside a loop?
> > >Since, when declared inside a loop, the variable will be created each loop,
> > instead of once if declared outside a loop
> > >I was wondering what the typical choice for this was, and if there are any
> > trade offs for doing this
> 
> I am hesitant to say "always", but I will say that most of the time it is
better
> to declare a variable right before it is used.
> 
> In this case, IIUC, you are concerned that each time through the loop the
> program will need to do extra work to create the variable if it's inside the
> loop, whereas it would have only had to create the variable once if it is
> outside the loop.
> 
> If it is faster to declare it outside the loop, you are giving up some
> maintainability. The code may be harder to follow, and a larger scope means
that
> you could accidentally change it in more places.
> 
> I *think* the compiler will generate the same assembly either way. (Someone
else
> in the office could probably give you a better answer than this. You can also
> use a disassembler to compare, which might be interesting.) If not, I think
this
> could be at most a small performance difference. When it comes to this kind of
> micro-optimization, my general rule is to go with clarity unless a profile
shows
> it to be a problem.
> 
> Follow-up: A Google search led me to this excellent discussion on
StackOverflow
> (StackOverflow is your friend):
>
http://stackoverflow.com/questions/7959573/declaring-variables-inside-loops-g...
> 
> According to that page, it actually may be *faster* to declare the variable
> inside the loop, since the compiler has more information about when the
variable
> is used.
> 
> One exception that was wisely pointed out is that it's a different question if
> we're talking about a class with a constructor, which would mean calling the
> constructor (and destructor) each time through the loop. In this case, we're
> just talking about a primitive type, so that is not a problem.

>Thanks, good to know!

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:199: if (x + 2*subsetWidth > w) {
On 2015/05/22 18:39:54, scroggo wrote:
> On 2015/05/22 17:42:19, msarett wrote:
> > Is there another way to check for the end subset?
> > 
> > This does not work for strange cases that will probably never come up.  Ex:
> > width = 10, divisor = 7.
> > 
> > Maybe there is an option to loop over the value of the divisor or something?
> 
> I agree that this seems a little clunky. I think it would always work if it
> instead said something like
> 
> if (x + 2*subsetWidth > subsetWidth*divisor)
> 
> which is even clunkier...
> 
> +1 to trying using a loop over divisor.

>I added currentRow and currentColumn variables
>I originally was using a loop over divisor, which made calculating w and h a
lot messier 
>I think adding the currentRow and currentColumn variables is cleaner than
trying to loop over divisor, even though it is 2 more variables

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:200: currentSubsetWidth = subsetWidth + w -
subsetWidth*divisor;
On 2015/05/22 17:42:19, msarett wrote:
> nit: Can this be w%divisor?  Can we reuse the value from above?

>I changed this to w%divisor
>I didn't reuse the value above, because that would essentially reintroduce the
extraX variable which I removed earlier to try and make the code clearer

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:208: currentSubsetHeight = subsetHeight + h -
subsetHeight*divisor;
On 2015/05/22 17:42:18, msarett wrote:
> nit: Can this be h%divisor?  Can we reuse the value from above?

>changed to h%divisor

scroggo

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp#newcode199 dm/DMSrcSink.cpp:199: if (x + 2*subsetWidth > w) { On 2015/05/22 ...

5 years, 7 months ago (2015-05-26 14:09:34 UTC) #22

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:199: if (x + 2*subsetWidth > w) {
On 2015/05/22 21:55:21, emmaleer wrote:
> On 2015/05/22 18:39:54, scroggo wrote:
> > On 2015/05/22 17:42:19, msarett wrote:
> > > Is there another way to check for the end subset?
> > > 
> > > This does not work for strange cases that will probably never come up. 
Ex:
> > > width = 10, divisor = 7.
> > > 
> > > Maybe there is an option to loop over the value of the divisor or
something?
> > 
> > I agree that this seems a little clunky. I think it would always work if it
> > instead said something like
> > 
> > if (x + 2*subsetWidth > subsetWidth*divisor)
> > 
> > which is even clunkier...
> > 
> > +1 to trying using a loop over divisor.
> 
> >I added currentRow and currentColumn variables
> >I originally was using a loop over divisor, which made calculating w and h a
> lot messier 
> >I think adding the currentRow and currentColumn variables is cleaner than
> trying to loop over divisor, even though it is 2 more variables

I still find this loop a little clunky. What if we did something like the
following:

for (int col = 0; col < divisor; col++) {
  const int currentSubsetWidth = (col + 1 == divisor) ?
      subsetWidth + w % divisor : subsetWidth;
  const int x = col * subsetWidth;
  ...
  // Similar for y
}

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:200: currentSubsetWidth = subsetWidth + w -
subsetWidth*divisor;
On 2015/05/22 21:55:21, emmaleer wrote:
> On 2015/05/22 17:42:19, msarett wrote:
> > nit: Can this be w%divisor?  Can we reuse the value from above?
> 
> >I changed this to w%divisor
> >I didn't reuse the value above, because that would essentially reintroduce
the
> extraX variable which I removed earlier to try and make the code clearer 

The thing I did not like about it was that it was a variable which was always
added, but it was sometimes 0 and sometimes w % divisor. I think it would be
fine it if was a constant that was added when needed:

// One of our subsets will be larger to contain any pixels that
// do not divide evenly.
const int extraX = w % divisor;
const int extraY = h % divisor;

SkImageInfo largestSubsetDecodeInfo = decodeInfo.makeWH(
  subsetWidth + extraX, subsetHeight + extraY);
...
for (int col = 0; col < divisor; col++) {
  const int currentSubsetWidth = (col + 1 == divisor) ?
      subsetWidth + extraX : subsetWidth;
  const int x = col * subsetWidth;
  ...
  // Similar for y
}

emmaleer

Now loops over row and col, and extraX and extraY constants are used. https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp File ...

5 years, 7 months ago (2015-05-26 19:15:04 UTC) #23

Now loops over row and col, and extraX and extraY constants are used.

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:199: if (x + 2*subsetWidth > w) {
On 2015/05/26 14:09:34, scroggo wrote:
> On 2015/05/22 21:55:21, emmaleer wrote:
> > On 2015/05/22 18:39:54, scroggo wrote:
> > > On 2015/05/22 17:42:19, msarett wrote:
> > > > Is there another way to check for the end subset?
> > > > 
> > > > This does not work for strange cases that will probably never come up. 
> Ex:
> > > > width = 10, divisor = 7.
> > > > 
> > > > Maybe there is an option to loop over the value of the divisor or
> something?
> > > 
> > > I agree that this seems a little clunky. I think it would always work if
it
> > > instead said something like
> > > 
> > > if (x + 2*subsetWidth > subsetWidth*divisor)
> > > 
> > > which is even clunkier...
> > > 
> > > +1 to trying using a loop over divisor.
> > 
> > >I added currentRow and currentColumn variables
> > >I originally was using a loop over divisor, which made calculating w and h
a
> > lot messier 
> > >I think adding the currentRow and currentColumn variables is cleaner than
> > trying to loop over divisor, even though it is 2 more variables
> 
> I still find this loop a little clunky. What if we did something like the
> following:
> 
> for (int col = 0; col < divisor; col++) {
>   const int currentSubsetWidth = (col + 1 == divisor) ?
>       subsetWidth + w % divisor : subsetWidth;
>   const int x = col * subsetWidth;
>   ...
>   // Similar for y
> }

>Thanks, this way is much more concise!

https://codereview.chromium.org/1134113006/diff/170001/dm/DMSrcSink.cpp#newco...
dm/DMSrcSink.cpp:200: currentSubsetWidth = subsetWidth + w -
subsetWidth*divisor;
On 2015/05/26 14:09:34, scroggo wrote:
> On 2015/05/22 21:55:21, emmaleer wrote:
> > On 2015/05/22 17:42:19, msarett wrote:
> > > nit: Can this be w%divisor?  Can we reuse the value from above?
> > 
> > >I changed this to w%divisor
> > >I didn't reuse the value above, because that would essentially reintroduce
> the
> > extraX variable which I removed earlier to try and make the code clearer 
> 
> The thing I did not like about it was that it was a variable which was always
> added, but it was sometimes 0 and sometimes w % divisor. I think it would be
> fine it if was a constant that was added when needed:
> 
> // One of our subsets will be larger to contain any pixels that
> // do not divide evenly.
> const int extraX = w % divisor;
> const int extraY = h % divisor;
> 
> SkImageInfo largestSubsetDecodeInfo = decodeInfo.makeWH(
>   subsetWidth + extraX, subsetHeight + extraY);
> ...
> for (int col = 0; col < divisor; col++) {
>   const int currentSubsetWidth = (col + 1 == divisor) ?
>       subsetWidth + extraX : subsetWidth;
>   const int x = col * subsetWidth;
>   ...
>   // Similar for y
> }

Acknowledged.

scroggo

> New CodeSrc draw mode kScanline_Subset_Mode nit: CodecSrc* > kScanline_Subset_Mode decodes the image in subsets ...

5 years, 7 months ago (2015-05-26 19:44:30 UTC) #24

scroggo

5 years, 6 months ago (2015-06-11 15:59:25 UTC) #25

On 2015/05/26 19:44:30, scroggo wrote:
> > New CodeSrc draw mode kScanline_Subset_Mode 
> 
> nit: CodecSrc*
> 
> > kScanline_Subset_Mode decodes the image in subsets using a
> > scanline decoder.
> > The number of subsets can be specified by changing the constant divisor.
> > The number of subsets is equal to divisor*divisor.
> 
> nit: Please add a comment about the fix for the png scanline decoder.
> 
> Otherwise, lgtm
> 
> https://codereview.chromium.org/1134113006/diff/210001/dm/DMSrcSink.cpp
> File dm/DMSrcSink.cpp (right):
> 
>
https://codereview.chromium.org/1134113006/diff/210001/dm/DMSrcSink.cpp#newco...
> dm/DMSrcSink.cpp:210: SkScanlineDecoder* subsetScanlineDecoder =
> codec->getScanlineDecoder(decodeInfo,
> nit: Does this go over 100 columns? reitveld looks strange to me; it looks
like
> the line wraps but it does not show anything on the second line. (Is there
> whitespace at the end that takes it over 100 columns?)
> 
> If it's not over 100 characters, just ignore this...

Landed at https://codereview.chromium.org/1157153003

Issue 1134113006: dm changes (Closed)

Description

Patch Set 1 #

Patch Set 2 : example file #

Patch Set 3 : dm subset mode #

Patch Set 4 : changes after code review #

Patch Set 5 : removing samples #

Patch Set 6 : dm subset scanline decoder changes #

Patch Set 7 : odd width and height now working and new scanline decoder for each subset #

Patch Set 8 : fixing spacing #

Patch Set 9 : updating comments and variable names #

Patch Set 10 : removing extraX and extraY variables and moving declarations #

Patch Set 11 : Adding currentRow and currentColumn variables #

Patch Set 12 : Looping over row and col #

Patch Set 13 : deleting extra whitespace #

Messages