Non DCTSIZE multiple width support for JPEG YUV decoding

Non DCTSIZE multiple width support for JPEG YUV decoding

Since jpeg_read_raw_data reads blocks of memory, we have to make sure
that the memory allocated is a multiple of the DCT block size. For
example, if DCTSIZE is 8, which is generally the case, we have to make
sure that the allocated memory to read the raw JPEG data has a width
rounded up to the next multiple of 8 (if it's not already a multiple
of 8) to avoid writing data out of the bounds of the memory. If this
isn't done properly, the end of some decode image lines can overwrite
the beginning of the following lines.

BUG=411189
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181749

[sugoi1, 2014-09-05 20:14:56]

sugoi@chromium.org changed reviewers:
+ junov@chromium.org, noel@chromium.org

[sugoi1, 2014-09-05 20:14:58]

This fixes the issue, but if you have better suggestions for the how the API
should be presented, feel free to suggest :)

https://codereview.chromium.org/544323002/diff/1/Source/platform/graphics/Dec...
File Source/platform/graphics/DecodingImageGenerator.cpp (right):

https://codereview.chromium.org/544323002/diff/1/Source/platform/graphics/Dec...
Source/platform/graphics/DecodingImageGenerator.cpp:89: return
m_frameGenerator->getYUVComponentSizes(sizes);
This line now returns sizes to allocate.

https://codereview.chromium.org/544323002/diff/1/Source/platform/graphics/Dec...
Source/platform/graphics/DecodingImageGenerator.cpp:94: bool decoded =
m_frameGenerator->decodeToYUV(sizes, planes, rowBytes);
... and this will update the size to the actual image size. The code I wrote in
Skia already supports this properly.

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:479: m_uvSize =
computeYUVSize(&m_info, 1, true); // U size and V size have to be the same if we
got here
This is unfortunate, but I have to cache this value here, because this data can
be erased or modified if jpeg_consume_input() is called (see progressive case
for an example).

[sugoi1, 2014-09-08 14:36:26]

sugoi@chromium.org changed reviewers:
+ senorblanco@chromium.org
- junov@chromium.org

[sugoi1, 2014-09-08 14:36:27]

Adding senorblanco@ as reviewer since he's unexpectedly here for a few days.

[Noel Gordon, 2014-09-08 16:13:04]

On 2014/09/05 20:14:58, sugoi1 wrote:
> This fixes the issue, but if you have better suggestions for the how the API
> should be presented, feel free to suggest :)

Don't have a better suggestion at the moment.

Perhaps the change description could better explain the "why" aspects.

[Noel Gordon, 2014-09-08 16:13:33]

https://codereview.chromium.org/544323002/diff/1/Source/platform/graphics/Ima...
File Source/platform/graphics/ImageFrameGenerator.cpp (right):

https://codereview.chromium.org/544323002/diff/1/Source/platform/graphics/Ima...
Source/platform/graphics/ImageFrameGenerator.cpp:168: if
(decoder->isSizeAvailable())
ASSERT this?

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:479: m_uvSize =
computeYUVSize(&m_info, 1, true); // U size and V size have to be the same if we
got here
On 2014/09/05 20:14:57, sugoi1 wrote:
> This is unfortunate, but I have to cache this value here, because this data
can
> be erased or modified if jpeg_consume_input() is called (see progressive case
> for an example).

"// U size and V size have to be the same if we got here"

not quite sure what that comment means.

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:722: if (((component >=
0) && (component <= 2)) && m_reader.get()) {
nit: m_reader.get() -> m_reader

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:724: if (info &&
(info->out_color_space == JCS_YCbCr)) {
nit: info() is never null

[Stephen White, 2014-09-08 16:58:20]

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/ImageDecoder.h:123: virtual IntSize
decodedYUVSize(int component, bool memoryAllocation) const { return
decodedSize(); }
Naming nit: memoryAllocation seems a little confusing to me (sounds like it's
requesting an actual memory allocation). Maybe forMemoryAllocation?

Of course, if you change this to an enum (see below), you can change the names
how you see fit.

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right):

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:96: IntSize ySize =
decoder->decodedYUVSize(0, false);
Blink style is to use an enum instead of a bool as argument type 
whenever the caller needs to use literals at the call site, such as
here.

[Noel Gordon, 2014-09-09 01:30:23]

On 2014/09/08 16:13:04, Noel Gordon wrote:
> On 2014/09/05 20:14:58, sugoi1 wrote:
> > This fixes the issue, but if you have better suggestions for the how the API
> > should be presented, feel free to suggest :)
> 
> Don't have a better suggestion at the moment.
> 
> Perhaps the change description could better explain the "why" aspects.

Thanks.  Just a note: the review tool is a bit lame in that it doesn't
automatically wrap your description near column 72.  git cl upload provides a
wrapping "-*------" guide for this, therefore.  Wrapping lines manually makes
reverts and so on easier to read so let's do that.

[sugoi1, 2014-09-09 15:07:04]

On 2014/09/09 01:30:23, Noel Gordon wrote:
> On 2014/09/08 16:13:04, Noel Gordon wrote:
> > On 2014/09/05 20:14:58, sugoi1 wrote:
> > > This fixes the issue, but if you have better suggestions for the how the
API
> > > should be presented, feel free to suggest :)
> > 
> > Don't have a better suggestion at the moment.
> > 
> > Perhaps the change description could better explain the "why" aspects.
> 
> Thanks.  Just a note: the review tool is a bit lame in that it doesn't
> automatically wrap your description near column 72.  git cl upload provides a
> wrapping "-*------" guide for this, therefore.  Wrapping lines manually makes
> reverts and so on easier to read so let's do that.

Done.

[sugoi1, 2014-09-09 15:08:15]

Darn, I forgot to publish my drafts yesterday. Here they are:

https://codereview.chromium.org/544323002/diff/1/Source/platform/graphics/Ima...
File Source/platform/graphics/ImageFrameGenerator.cpp (right):

https://codereview.chromium.org/544323002/diff/1/Source/platform/graphics/Ima...
Source/platform/graphics/ImageFrameGenerator.cpp:168: if
(decoder->isSizeAvailable())
On 2014/09/08 16:13:32, Noel Gordon wrote:
> ASSERT this?

isSizeAvailable() has to actually get called, because this makes sure that the
jpeg's header is read and that the size will be properly updated. I could write
something like:

bool sizeIsAvailable = decoder->isSizeAvailable();
ASSERT(sizeIsAvailable);
if (sizeIsAvailable)
    getYUVComponentSizes(...);

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/ImageDecoder.h:123: virtual IntSize
decodedYUVSize(int component, bool memoryAllocation) const { return
decodedSize(); }
On 2014/09/08 16:58:20, Stephen White wrote:
> Naming nit: memoryAllocation seems a little confusing to me (sounds like it's
> requesting an actual memory allocation). Maybe forMemoryAllocation?
> 
> Of course, if you change this to an enum (see below), you can change the names
> how you see fit.

Acknowledged.

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:479: m_uvSize =
computeYUVSize(&m_info, 1, true); // U size and V size have to be the same if we
got here
On 2014/09/08 16:13:32, Noel Gordon wrote:
> On 2014/09/05 20:14:57, sugoi1 wrote:
> > This is unfortunate, but I have to cache this value here, because this data
> can
> > be erased or modified if jpeg_consume_input() is called (see progressive
case
> > for an example).
> 
> "// U size and V size have to be the same if we got here"
> 
> not quite sure what that comment means.

If we have reached this point in the code, it means overrideColorSpace is
JCS_YCbCr, which means yuvSubsampling(m_info) != YUV_UNKNOWN, which means that
the sizes of U and V are the same, because, for now, having different sizes for
the U and V components is not supported.

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:722: if (((component >=
0) && (component <= 2)) && m_reader.get()) {
On 2014/09/08 16:13:33, Noel Gordon wrote:
> nit: m_reader.get() -> m_reader

Done.

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:724: if (info &&
(info->out_color_space == JCS_YCbCr)) {
On 2014/09/08 16:13:32, Noel Gordon wrote:
> nit: info() is never null

Done.

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right):

https://codereview.chromium.org/544323002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:96: IntSize ySize =
decoder->decodedYUVSize(0, false);
On 2014/09/08 16:58:20, Stephen White wrote:
> Blink style is to use an enum instead of a bool as argument type 
> whenever the caller needs to use literals at the call site, such as
> here.

Acknowledged.

[Stephen White, 2014-09-09 15:33:51]

https://codereview.chromium.org/544323002/diff/40001/Source/platform/image-de...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):

https://codereview.chromium.org/544323002/diff/40001/Source/platform/image-de...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:722: if (((component >=
0) && (component <= 2)) && m_reader) {
Nit: spurious parens (now that these are all &&'s).

Although, maybe these should all be ASSERTs now instead? Is there a 
valid reason to fall back to m_decodedSize?

[sugoi1, 2014-09-10 14:25:06]

PTAL, thanks

[Stephen White, 2014-09-10 14:29:47]

LGTM

[Noel Gordon, 2014-09-10 17:18:15]

On 2014/09/09 15:08:15, sugoi1 wrote:
> > Noel wote:
> > "// U size and V size have to be the same if we got here"
> > 
> > not quite sure what that comment means.
> 
> If we have reached this point in the code, it means overrideColorSpace is
> JCS_YCbCr, which means yuvSubsampling(m_info) != YUV_UNKNOWN, which means that
> the sizes of U and V are the same, because, for now, having different sizes
for
> the U and V components is not supported.

Yes, hard to write a good comment for that.  Perhaps an ASSERT would work here
(make the comment code), and maybe that in a subsequent patch.

[sugoi1, 2014-09-10 17:24:52]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-09-10 17:25:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sugoi@chromium.org/544323002/60001

[Noel Gordon, 2014-09-10 17:29:07]

LGTM

[commit-bot: I haz the power, 2014-09-10 18:27:22]

Committed patchset #4 (id:60001) as 181749