Remove jpegs with uninitialized memory from Gold

Remove jpegs with uninitialized memory from Gold

BitmapRegionSampler (uses SkImageDecoder) will often scale
to a power of 2 regardless of the sampleSize requested.
This is skbug.com/4319.

Consider a 60x60 image.
To decode a subset with sample size 3, we might ask for the
following.
[x, y, w, h] = [-15, -15, 30, 30]
sampleSize = 3

Since w = 30 and h = 30, this should give us a 10x10
result.  Only the bottom right 5x5 quadrant of this 10x10
subset will actually be in the image.  We should get a 5
pixel border on the top and left because we ask for 15
extra pixels on the top and left.

Unfortunately, SkImageDecoder will take our requested
sample size of 3, and then decide to use a sample size of
2.  Not only will it scale the image by 2, but it will also
scale the border by 2.  So while we are expecting pixel
data to begin at offset (5, 5) of the result bitmap, it
actually begins at offset (7, 7).  Making things worse,
the pixels between (5, 5) and (7, 7) are uninitialized,
causing problems on Gold.

Options for fixing this include:
(1) Not testing decodes with a border.
(2) Changing the test to check the size of the output
bitmap.
(3) Disable the tests.

I think it's best to just disable these tests.  We know
they don't work, so why do we need to see the results on
Gold?

BUG=skia:4319
Committed: https://skia.googlesource.com/skia/+/6efbe0537732a729d5bd500707dd0d8b0fc83553

[msarett, 2015-09-09 21:09:04]

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

[scroggo, 2015-09-11 14:59:47]

The code changes LGTM. I do have some questions about the description though:

>Remove jpegs with uninitialized memory from Gold
>
> BitmapRegionSampler (uses SkImageDecoder) will often scale
> to a power of 2 regardless of the sampleSize requested.
> This is a known bug.

Is there a filed bug for this? (I know we've talked about the problem.)

> Consider a 60x60 image and the divisor test for BRDs in DM.
> To decode the top-left subset with sample size 3, we will
> ask for the following.
> [x, y, w, h] = [-15, -15, 60, 60]
> sampleSize = 3

> This should give us a 20x20 result.

I don't follow. That rectangle looks to cover the entire image? And with a
sampleSize of 3, it seems like we would end up with a width of 25 (60 - -15 =
75; 75 / 3 = 25) where the left 5 pixels (-15 / 3) are outside the original
image. I suppose you mean the result that is within the border of the original
image (which is 20 x 20 eliminating the bits outside the image).

> The 10x10 subset that
> we actually want (scaled version of the 30x30 subset)

I'm lost again. You said it "should" give us a 20 x 20 result. What is this 30 x
30 subset?

> is
> surrounded by a 5 pixel border.  We should get a 5 pixel

This matches my understanding...

> border because we ask for 15 extra pixels in every
> direction.

Wait, every direction? It looks like we ask for 15 pixels to the left and above,
but not to the right or below.

> Unfortunately, SkImageDecoder will take our requested
> sample size of 3, and then decide to use a sample size of
> 2.  Not only will it scale the image by 2, but it will also
> scale the border by 2.  So while we are expecting pixel
> data to begin at offset (5, 5) of the result bitmap, it
> actually begins at offset (7, 7).  Making things worse,
> the pixels between (5, 5) and (7, 7) are uninitialized,
> causing problems on Gold.

It seems like there's a larger problem here - not only is the border wrong, but
the whole image is wrong. We should be testing a scaled version of
SkImageDecoder, but I do not think we do currently (I just filed skbug.com/4325
to add that). If we were, we should also see this problem with SkImageDecoder
when testing a scaled JPEG, right?

The uninitialized part obviously makes using Gold difficult. Just to make sure I
understand - the code which handles drawing the output to the bitmap that will
be uploaded to Gold assumes that SkImageDecoder did the right thing?

> Options for fixing this include:
> (1) Not testing decodes with a border.

"But testing with a border is important, because BitmapRegionDecoder is used
with a border."

> (2) Changing the test to check the size of the output
> bitmap.
>
> I think it's best to just disable these tests.  We know
> they don't work, so why do we need to see the results on
> Gold?

This seems like a good option.

I think this is option (3)? Maybe there should be a bug with the flag
TestDisabled?

https://codereview.chromium.org/1313233007/diff/1/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1313233007/diff/1/dm/DM.cpp#newcode408
dm/DM.cpp:408: const uint32_t sampleSizes[] = { 1, 2, 3, 4, 5, 6, 7, 8 };
Why did this move?

(I'm fine with the new location; I just do not understand the motivation for
moving.)

Ok, I think I follow: these are in the order that we check them. sampleSize used
to be in the innermost loop, and now it's in the outermost loop. That seems fine
to me to keep them in the order they'll be looked at.

[msarett, 2015-09-11 15:21:40]

On 2015/09/11 14:59:47, scroggo wrote:
> The code changes LGTM. I do have some questions about the description though:
> 
> >Remove jpegs with uninitialized memory from Gold
> >
> > BitmapRegionSampler (uses SkImageDecoder) will often scale
> > to a power of 2 regardless of the sampleSize requested.
> > This is a known bug.
> 
> Is there a filed bug for this? (I know we've talked about the problem.)

Yeah I filed one after I uploaded this CL skbug.com/4319

> 
> > Consider a 60x60 image and the divisor test for BRDs in DM.
> > To decode the top-left subset with sample size 3, we will
> > ask for the following.
> > [x, y, w, h] = [-15, -15, 60, 60]
> > sampleSize = 3
> 
> > This should give us a 20x20 result.
> 
> I don't follow. That rectangle looks to cover the entire image? And with a
> sampleSize of 3, it seems like we would end up with a width of 25 (60 - -15 =
> 75; 75 / 3 = 25) where the left 5 pixels (-15 / 3) are outside the original
> image. I suppose you mean the result that is within the border of the original
> image (which is 20 x 20 eliminating the bits outside the image).
> 

Since w = 60 and h = 60, the subset actually covers (-15, -15) to (45, 45), so
not
quite the entire image.

Similarly, since w = 60 and h = 60, the subset will be 20x20.

You make a good point that the "border" around the 10x10 subset contains pixels
that
are actually inside the image.  This happens on the bottom and the right because
we
are taking the top-left subset.  I am still considering the pixels that are not
in
the top-left 10x10 subset as "border" pixels.  Maybe this is strange - I thought
it
was the most general way to test subsets with borders.

> > The 10x10 subset that
> > we actually want (scaled version of the 30x30 subset)
> 
> I'm lost again. You said it "should" give us a 20 x 20 result. What is this 30
x
> 30 subset?
> 

Yeah I can see why this description is extremely confusing.

Since we want the top-left subset of a 60x60 image, we want a 30x30 subset. 
Then
with 15 border pixels on each side, we actually request a 60x60 subset.

The API will give us back a 20x20 subset with a 5 pixel border on each side that
we need to strip.  This will give us the 10x10 subset that we want.

> > is
> > surrounded by a 5 pixel border.  We should get a 5 pixel
> 
> This matches my understanding...
> 
> > border because we ask for 15 extra pixels in every
> > direction.
> 
> Wait, every direction? It looks like we ask for 15 pixels to the left and
above,
> but not to the right or below.
> 

Again this is me being unclear.  I am giving an example using the top-left
subset.
So there are "extra" pixels on all sides.

> > Unfortunately, SkImageDecoder will take our requested
> > sample size of 3, and then decide to use a sample size of
> > 2.  Not only will it scale the image by 2, but it will also
> > scale the border by 2.  So while we are expecting pixel
> > data to begin at offset (5, 5) of the result bitmap, it
> > actually begins at offset (7, 7).  Making things worse,
> > the pixels between (5, 5) and (7, 7) are uninitialized,
> > causing problems on Gold.
> 
> It seems like there's a larger problem here - not only is the border wrong,
but
> the whole image is wrong. We should be testing a scaled version of
> SkImageDecoder, but I do not think we do currently (I just filed
skbug.com/4325
> to add that). If we were, we should also see this problem with SkImageDecoder
> when testing a scaled JPEG, right?
> 

Yes.

> The uninitialized part obviously makes using Gold difficult. Just to make sure
I
> understand - the code which handles drawing the output to the bitmap that will
> be uploaded to Gold assumes that SkImageDecoder did the right thing?
> 

Yes the code in BRDSrc that uses kDivisor_Mode, assumes that the implementation
of
BitmapRegionDecoder will do the right thing.

> > Options for fixing this include:
> > (1) Not testing decodes with a border.
> 
> "But testing with a border is important, because BitmapRegionDecoder is used
> with a border."
> 
> > (2) Changing the test to check the size of the output
> > bitmap.
> >
> > I think it's best to just disable these tests.  We know
> > they don't work, so why do we need to see the results on
> > Gold?
> 
> This seems like a good option.
> 
> I think this is option (3)? Maybe there should be a bug with the flag
> TestDisabled?

Yeah I went with option 3.

CL description seems to be a mess.  I will rewrite it and send this for another
review.

> 
> https://codereview.chromium.org/1313233007/diff/1/dm/DM.cpp
> File dm/DM.cpp (right):
> 
> https://codereview.chromium.org/1313233007/diff/1/dm/DM.cpp#newcode408
> dm/DM.cpp:408: const uint32_t sampleSizes[] = { 1, 2, 3, 4, 5, 6, 7, 8 };
> Why did this move?
> 
> (I'm fine with the new location; I just do not understand the motivation for
> moving.)
> 
> Ok, I think I follow: these are in the order that we check them. sampleSize
used
> to be in the innermost loop, and now it's in the outermost loop. That seems
fine
> to me to keep them in the order they'll be looked at.

[msarett, 2015-09-11 15:28:20]

Let me know if this makes more sense.

https://codereview.chromium.org/1313233007/diff/1/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1313233007/diff/1/dm/DM.cpp#newcode408
dm/DM.cpp:408: const uint32_t sampleSizes[] = { 1, 2, 3, 4, 5, 6, 7, 8 };
On 2015/09/11 14:59:47, scroggo wrote:
> Why did this move?
> 
> (I'm fine with the new location; I just do not understand the motivation for
> moving.)
> 
> Ok, I think I follow: these are in the order that we check them. sampleSize
used
> to be in the innermost loop, and now it's in the outermost loop. That seems
fine
> to me to keep them in the order they'll be looked at.

Yes that's why.

[msarett, 2015-09-11 15:55:26]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-09-11 15:55:39]

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

[commit-bot: I haz the power, 2015-09-11 16:01:20]

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/6efbe0537732a729d5bd500707dd0d8b0fc83553