YUV scale fix

YUV scale fix

There was a scaling mistake visible in some JPEG images because the ratio between Y, U and V planes were assumed to be the same ratios as the ratio between texture sizes, which was wrong because texture have a minimum size of 16 and are rounded up to the next POT. Since the ratios between Y and UV planes are generally 1, 2 or 4, rounding up to the next POT would generally preserve this ratio, so that this bug was not very visible, apart from very small jpeg images of 8 or less pixels in either width or height.

BUG=457954
Committed: https://skia.googlesource.com/skia/+/e6eddf7dd85add7da41f22f2643bdd573ad1f1cf

Committed: https://skia.googlesource.com/skia/+/4ccce7ea618fd9557a6d5cfa29c1fd6b015c7580

[sugoi1, 2015-02-13 16:15:50]

sugoi@chromium.org changed reviewers:
+ bsalomon@google.com

[bsalomon, 2015-02-13 18:26:09]

shouldn't we have a test that would have failed with the old code?

https://codereview.chromium.org/922273002/diff/1/src/gpu/effects/GrYUVtoRGBEf...
File src/gpu/effects/GrYUVtoRGBEffect.cpp (right):

https://codereview.chromium.org/922273002/diff/1/src/gpu/effects/GrYUVtoRGBEf...
src/gpu/effects/GrYUVtoRGBEffect.cpp:24: w[0] = SkIntToScalar(sizes[0].fWidth) 
/ SkIntToScalar(yTexture ? yTexture->width()  : 1);
can these textures really every be NULL?

[sugoi1, 2015-02-13 18:51:51]

On 2015/02/13 18:26:09, bsalomon wrote:
> shouldn't we have a test that would have failed with the old code?

Not that I know of. Is there a good place to put a test that will try to decode
a very small YUV420 image? 

>
https://codereview.chromium.org/922273002/diff/1/src/gpu/effects/GrYUVtoRGBEf...
> File src/gpu/effects/GrYUVtoRGBEffect.cpp (right):
> 
>
https://codereview.chromium.org/922273002/diff/1/src/gpu/effects/GrYUVtoRGBEf...
> src/gpu/effects/GrYUVtoRGBEffect.cpp:24: w[0] = SkIntToScalar(sizes[0].fWidth)

> / SkIntToScalar(yTexture ? yTexture->width()  : 1);
> can these textures really every be NULL?

Not from the current codepath where this is called from, I believe, but I can
change it to assert that we have non NULL textures here, if you prefer.

[bsalomon, 2015-02-13 19:03:47]

On 2015/02/13 18:51:51, sugoi1 wrote:
> On 2015/02/13 18:26:09, bsalomon wrote:
> > shouldn't we have a test that would have failed with the old code?
> 
> Not that I know of. Is there a good place to put a test that will try to
decode
> a very small YUV420 image? 

Can the GM be updated to draw a second small jpeg?

> 
> >
>
https://codereview.chromium.org/922273002/diff/1/src/gpu/effects/GrYUVtoRGBEf...
> > File src/gpu/effects/GrYUVtoRGBEffect.cpp (right):
> > 
> >
>
https://codereview.chromium.org/922273002/diff/1/src/gpu/effects/GrYUVtoRGBEf...
> > src/gpu/effects/GrYUVtoRGBEffect.cpp:24: w[0] =
SkIntToScalar(sizes[0].fWidth)
> 
> > / SkIntToScalar(yTexture ? yTexture->width()  : 1);
> > can these textures really every be NULL?
> 
> Not from the current codepath where this is called from, I believe, but I can
> change it to assert that we have non NULL textures here, if you prefer.

sgtm

[sugoi1, 2015-02-13 19:34:01]

On 2015/02/13 19:03:47, bsalomon wrote:
> Can the GM be updated to draw a second small jpeg?

Done.

>
https://codereview.chromium.org/922273002/diff/1/src/gpu/effects/GrYUVtoRGBEf...
>
https://codereview.chromium.org/922273002/diff/1/src/gpu/effects/GrYUVtoRGBEf...
> > Not from the current codepath where this is called from, I believe, but I
can
> > change it to assert that we have non NULL textures here, if you prefer.
> 
> sgtm

Done

[bsalomon, 2015-02-13 19:35:32]

lgtm

[sugoi1, 2015-02-13 19:36:09]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2015-02-13 19:36:56]

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

[commit-bot: I haz the power, 2015-02-13 19:47:16]

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

[jvanverth1, 2015-02-13 20:02:01]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/926123002/ by jvanverth@google.com.

The reason for reverting is: Turning Windows compile bots red..

[sugoi1, 2015-02-13 21:30:51]

New patchsets have been uploaded after l-g-t-m from bsalomon@google.com

[sugoi1, 2015-02-13 21:50:35]

On 2015/02/13 20:02:01, jvanverth1 wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/926123002/ by mailto:jvanverth@google.com.
> 
> The reason for reverting is: Turning Windows compile bots red..

Build fix seems to work, will re-commit.

[sugoi1, 2015-02-13 21:50:39]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2015-02-13 21:51:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/922273002/40001

[commit-bot: I haz the power, 2015-02-13 21:57:13]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/4ccce7ea618fd9557a6d5cfa29c1fd6b015c7580