Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
399683007:
JPEG YUV Decoding (Closed)
DescriptionJPEG YUV Decoding
Enabling JPEG YUV Decoding in Skia
BUG=skia:3005, skia:1674, skia:3029
Committed: https://skia.googlesource.com/skia/+/8e6c3b93a39e19111662a760ede97df55e51d39f
Committed: https://skia.googlesource.com/skia/+/b227e37eae36ccf630c4baef00b1354d42b40fd1
Patch Set 1 #
Total comments: 1
Patch Set 2 : Update from blink's version of YUV decoding #
Total comments: 10
Patch Set 3 : Removed SkImagePlanes #Patch Set 4 : API changes #
Total comments: 20
Patch Set 5 : Moved functions/enums into a single block #Patch Set 6 : Trying to get more info on the test issue #Patch Set 7 : Initialized fShouldCancelDecode properly #
Messages
Total messages: 29 (2 generated)
Here's the 2nd part of the YUV decoding in Skia. I fixed most comments, except for some API concerns, which we can discussed here.
https://codereview.chromium.org/399683007/diff/1/include/core/SkImageDecoder.h File include/core/SkImageDecoder.h (right): https://codereview.chromium.org/399683007/diff/1/include/core/SkImageDecoder.... include/core/SkImageDecoder.h:402: virtual void onSetYUVBuffers(void* yuv[3], size_t rowBytes[3]) {} Why not create a new entry point? Right now the client makes calls something like: decoder->getComponentSIzes(); decoder->setYUVBuffers(); SkBitmap ignoredBitmap; decoder->decode(&ignoredBitmap); // Not meaningfully used but cannot be NULL wouldn't it be cleaner if we had some function like SkImageDecoder::decodeIntoYUV(); ?
I did a port of my Blink side changes into Skia. The Blink JPEG decoder is fairly different, so I had to simplify it quite a bit, to the point where it's not that close to Blink's code. I tried to keep a few key functions as similar as possible, like compute_yuv_size(), yuv_subsampling() and output_raw_data(), so that porting fixes from Blink to Skia would be easier.
https://codereview.chromium.org/399683007/diff/20001/include/core/SkImageDeco... File include/core/SkImageDecoder.h (right): https://codereview.chromium.org/399683007/diff/20001/include/core/SkImageDeco... include/core/SkImageDecoder.h:378: SkImagePlanes* imagePlanes) { I thnk passing the planes and sizes arrays is better than creating s small object to hold them. i.e. not sure we need SkImagePlanes object. https://codereview.chromium.org/399683007/diff/20001/src/images/SkDecodingIma... File src/images/SkDecodingImageGenerator.cpp (right): https://codereview.chromium.org/399683007/diff/20001/src/images/SkDecodingIma... src/images/SkDecodingImageGenerator.cpp:210: size_t rowBytes[3], SkYUVColorSpace* colorSpace) { where do we set colorSpace? Seems like we need the codec to tell us that... https://codereview.chromium.org/399683007/diff/20001/src/images/SkImageDecode... File src/images/SkImageDecoder.cpp (right): https://codereview.chromium.org/399683007/diff/20001/src/images/SkImageDecode... src/images/SkImageDecoder.cpp:300: { nit: place { on the same lines as the constructor https://codereview.chromium.org/399683007/diff/20001/src/images/SkImageDecode... src/images/SkImageDecoder.cpp:302: m_planes[i] = 0; m_planes[i] = NULL;
https://codereview.chromium.org/399683007/diff/20001/include/core/SkImageDeco... File include/core/SkImageDecoder.h (right): https://codereview.chromium.org/399683007/diff/20001/include/core/SkImageDeco... include/core/SkImageDecoder.h:378: SkImagePlanes* imagePlanes) { On 2014/10/13 13:15:35, reed1 wrote: > I thnk passing the planes and sizes arrays is better than creating s small > object to hold them. i.e. not sure we need SkImagePlanes object. Done. This was added only to mimic what is used in Blink, but since skia's decoder is much simpler, it's not required. https://codereview.chromium.org/399683007/diff/20001/src/images/SkDecodingIma... File src/images/SkDecodingImageGenerator.cpp (right): https://codereview.chromium.org/399683007/diff/20001/src/images/SkDecodingIma... src/images/SkDecodingImageGenerator.cpp:210: size_t rowBytes[3], SkYUVColorSpace* colorSpace) { On 2014/10/13 13:15:35, reed1 wrote: > where do we set colorSpace? Seems like we need the codec to tell us that... I set it here. For now I set it unconditionally to the JPEG YUV format, as the only other one available currently is for video. I'll add a virtual function in the decoder once we have more than 1 possible choice. https://codereview.chromium.org/399683007/diff/20001/src/images/SkImageDecode... File src/images/SkImageDecoder.cpp (right): https://codereview.chromium.org/399683007/diff/20001/src/images/SkImageDecode... src/images/SkImageDecoder.cpp:300: { On 2014/10/13 13:15:36, reed1 wrote: > nit: place { on the same lines as the constructor Code removed https://codereview.chromium.org/399683007/diff/20001/src/images/SkImageDecode... src/images/SkImageDecoder.cpp:302: m_planes[i] = 0; On 2014/10/13 13:15:35, reed1 wrote: > m_planes[i] = NULL; Code removed
This sort of interface already exists in SkImageGenerator and SkPixelRef.
bool getYUV8Planes(SkISize sizes[3], void* planes[3], size_t rowBytes[3],
SkYUVColorSpace* colorSpace);
Can we not make the decoder have exactly the same api?
if we need to hard-code the particular SkYUVColorSpace, seems like the best place is in libjpeg itself -- since that is where we would determine the "correct" value in some future CL. https://codereview.chromium.org/399683007/diff/20001/src/images/SkDecodingIma... File src/images/SkDecodingImageGenerator.cpp (right): https://codereview.chromium.org/399683007/diff/20001/src/images/SkDecodingIma... src/images/SkDecodingImageGenerator.cpp:210: size_t rowBytes[3], SkYUVColorSpace* colorSpace) { On 2014/10/14 15:07:36, sugoi1 wrote: > On 2014/10/13 13:15:35, reed1 wrote: > > where do we set colorSpace? Seems like we need the codec to tell us that... > > I set it here. For now I set it unconditionally to the JPEG YUV format, as the > only other one available currently is for video. I'll add a virtual function in > the decoder once we have more than 1 possible choice. I don't see where "here" is.
https://codereview.chromium.org/399683007/diff/20001/src/images/SkDecodingIma... File src/images/SkDecodingImageGenerator.cpp (right): https://codereview.chromium.org/399683007/diff/20001/src/images/SkDecodingIma... src/images/SkDecodingImageGenerator.cpp:210: size_t rowBytes[3], SkYUVColorSpace* colorSpace) { On 2014/10/14 15:34:28, reed1 wrote: > On 2014/10/14 15:07:36, sugoi1 wrote: > > On 2014/10/13 13:15:35, reed1 wrote: > > > where do we set colorSpace? Seems like we need the codec to tell us that... > > > > I set it here. For now I set it unconditionally to the JPEG YUV format, as the > > only other one available currently is for video. I'll add a virtual function > in > > the decoder once we have more than 1 possible choice. > > I don't see where "here" is. Sorry, I meant: in this function (next version of the file)
On 2014/10/14 15:30:53, reed1 wrote: > This sort of interface already exists in SkImageGenerator and SkPixelRef. > > bool getYUV8Planes(SkISize sizes[3], void* planes[3], size_t rowBytes[3], > SkYUVColorSpace* colorSpace); > > Can we not make the decoder have exactly the same api? You want me to just keep the function name or the entire signature? If you want the entire signature, do you want me to add a separate call/member variable for the stream (SkImageDecoder receives an SkStream in here, currently)?
I guess I'm shooting for as close a match in API as is possible/reasonable. e.g. we should receive a YUVColorSpace, even if it is (down deep in the impl) hard-coded to some value.
On 2014/10/14 16:01:49, reed1 wrote: > I guess I'm shooting for as close a match in API as is possible/reasonable. > > e.g. we should receive a YUVColorSpace, even if it is (down deep in the impl) > hard-coded to some value. bool decodeYUV8Planes(SkStream* stream, SkISize componentSizes[3], void* planes[3], size_t rowBytes[3], SkYUVColorSpace*]); I'm fine that we've changed "get" to "decode" and added a stream parameter. Not trying to thrash too much, but I think making the parameters and names as nearly the same as possible makes the entire API a little easier to grok.
On 2014/10/14 16:04:21, reed1 wrote: > On 2014/10/14 16:01:49, reed1 wrote: > > I guess I'm shooting for as close a match in API as is possible/reasonable. > > > > e.g. we should receive a YUVColorSpace, even if it is (down deep in the impl) > > hard-coded to some value. > > bool decodeYUV8Planes(SkStream* stream, SkISize componentSizes[3], void* > planes[3], size_t rowBytes[3], SkYUVColorSpace*]); > > I'm fine that we've changed "get" to "decode" and added a stream parameter. > > Not trying to thrash too much, but I think making the parameters and names as > nearly the same as possible makes the entire API a little easier to grok. Cool. API changes done.
lgtm w/ question/suggestion about libjpeg's colorspace support. https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... File src/images/SkImageDecoder_libjpeg.cpp (right): https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:971: if (NULL != colorSpace) { Does libjpeg actually support different spaces? If so, lets add a TODO. // TODO: sniff the data (in cinfo?) to set the correct colorspace)
High level suggestion: SkImageDecoder_libjpeg.cpp is already over 1200 lines, and the new functions you've added seem to be almost independent of the rest of the code. You won't be able to add a separate cpp (which I think would be ideal) without adding a header and doing a lot of cleanup, but could we isolate the new code in a single part of the file? We currently have a separate section for the encoder, and a separate section for the registration functions. https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... File src/images/SkImageDecoder_libjpeg.cpp (right): https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:57: // Enum for YUV decoding Could these enums be moved closer to where they are used? https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:121: static SkISize compute_yuv_size(const jpeg_decompress_struct& info, int component, Again, these functions appear to be defined well before they are used. Is that necessary? https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:123: { style nit: open brace does not get its own line. https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:144: && (info.cur_comp_info[2]->v_samp_factor == 1)) { In this case, I think the open brace should go on its own line (not yet in the style guide, but I think it has unofficial approval), or the if condition's indentation should not match the function's. https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:155: case 1: I think we don't match blink and/or chrome, but we indent 'case' once more from 'switch'. https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:822: SkISize uvSize = compute_yuv_size(cinfo, 1, kSizeForMemoryAllocation_SizeType); nit: indentation (first two lines) https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:903: SkAutoTime atm("JPEG Decode"); Not sure if anyone uses TIME_DECODE, but shouldn't the string specify that it is for decoding YUV8Planes? https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:907: return false; // Resizing not supported Is there a way we could let the client know this? https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:956: /* image_width and image_height are the original dimensions, available This comment appears to have been copied word for word, but does not apply here.
On 2014/10/15 15:16:06, scroggo wrote: > High level suggestion: > > SkImageDecoder_libjpeg.cpp is already over 1200 lines, and the new functions > you've added seem to be almost independent of the rest of the code. You won't be > able to add a separate cpp (which I think would be ideal) without adding a > header and doing a lot of cleanup, but could we isolate the new code in a single > part of the file? We currently have a separate section for the encoder, and a > separate section for the registration functions. > > https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... > File src/images/SkImageDecoder_libjpeg.cpp (right): > > https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... > src/images/SkImageDecoder_libjpeg.cpp:57: // Enum for YUV decoding > Could these enums be moved closer to where they are used? > > https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... > src/images/SkImageDecoder_libjpeg.cpp:121: static SkISize compute_yuv_size(const > jpeg_decompress_struct& info, int component, > Again, these functions appear to be defined well before they are used. Is that > necessary? > > https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... > src/images/SkImageDecoder_libjpeg.cpp:123: { > style nit: open brace does not get its own line. > > https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... > src/images/SkImageDecoder_libjpeg.cpp:144: && > (info.cur_comp_info[2]->v_samp_factor == 1)) { > In this case, I think the open brace should go on its own line (not yet in the > style guide, but I think it has unofficial approval), or the if condition's > indentation should not match the function's. > > https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... > src/images/SkImageDecoder_libjpeg.cpp:155: case 1: > I think we don't match blink and/or chrome, but we indent 'case' once more from > 'switch'. > > https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... > src/images/SkImageDecoder_libjpeg.cpp:822: SkISize uvSize = > compute_yuv_size(cinfo, 1, kSizeForMemoryAllocation_SizeType); > nit: indentation (first two lines) > > https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... > src/images/SkImageDecoder_libjpeg.cpp:903: SkAutoTime atm("JPEG Decode"); > Not sure if anyone uses TIME_DECODE, but shouldn't the string specify that it is > for decoding YUV8Planes? > > https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... > src/images/SkImageDecoder_libjpeg.cpp:907: return false; // Resizing not > supported > Is there a way we could let the client know this? > > https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... > src/images/SkImageDecoder_libjpeg.cpp:956: /* image_width and image_height are > the original dimensions, available > This comment appears to have been copied word for word, but does not apply here. +1 good suggestions. If we can place in a separate file, +1 to that too.
https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... File src/images/SkImageDecoder_libjpeg.cpp (right): https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:57: // Enum for YUV decoding On 2014/10/15 15:16:06, scroggo wrote: > Could these enums be moved closer to where they are used? Done. https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:121: static SkISize compute_yuv_size(const jpeg_decompress_struct& info, int component, On 2014/10/15 15:16:05, scroggo wrote: > Again, these functions appear to be defined well before they are used. Is that > necessary? Moved them lower. https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:123: { On 2014/10/15 15:16:05, scroggo wrote: > style nit: open brace does not get its own line. Done. https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:144: && (info.cur_comp_info[2]->v_samp_factor == 1)) { On 2014/10/15 15:16:06, scroggo wrote: > In this case, I think the open brace should go on its own line (not yet in the > style guide, but I think it has unofficial approval), or the if condition's > indentation should not match the function's. Done. https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:155: case 1: On 2014/10/15 15:16:06, scroggo wrote: > I think we don't match blink and/or chrome, but we indent 'case' once more from > 'switch'. Done. https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:822: SkISize uvSize = compute_yuv_size(cinfo, 1, kSizeForMemoryAllocation_SizeType); On 2014/10/15 15:16:05, scroggo wrote: > nit: indentation (first two lines) Oops, there were tabs in there. Removed. https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:903: SkAutoTime atm("JPEG Decode"); On 2014/10/15 15:16:06, scroggo wrote: > Not sure if anyone uses TIME_DECODE, but shouldn't the string specify that it is > for decoding YUV8Planes? Done. https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:907: return false; // Resizing not supported On 2014/10/15 15:16:05, scroggo wrote: > Is there a way we could let the client know this? Well, it's not that it's impossible, it's just not supported yet. It could be implemented. Worst case scenario here is that it will revert back to standard RGBA decoding, which isn't catastrophic (It's the current case). https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:956: /* image_width and image_height are the original dimensions, available On 2014/10/15 15:16:06, scroggo wrote: > This comment appears to have been copied word for word, but does not apply here. Actually, it does apply, output_width and output_height will be read from cinfo inside output_raw_data (see yWidth, yHeight), so jpeg_start_decompress() has to be executed before output_raw_data(). https://codereview.chromium.org/399683007/diff/160001/src/images/SkImageDecod... src/images/SkImageDecoder_libjpeg.cpp:971: if (NULL != colorSpace) { On 2014/10/15 15:08:08, reed1 wrote: > Does libjpeg actually support different spaces? If so, lets add a TODO. > > // TODO: sniff the data (in cinfo?) to set the correct colorspace) Not that I know of. AFAIK, JPEG always uses the same YUV coefficients.
lgtm
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/399683007/180001
Message was sent while issue was closed.
Committed patchset #5 (id:180001) as 8e6c3b93a39e19111662a760ede97df55e51d39f
Message was sent while issue was closed.
On 2014/10/15 18:04:23, I haz the power (commit-bot) wrote: > Committed patchset #5 (id:180001) as 8e6c3b93a39e19111662a760ede97df55e51d39f Did this cause these failures: 0 tasks left, 1 failed 2476M peak 1111ms test PackBits Failures: test Jpeg_YUV: ../../tests/JpegTest.cpp:474 sizesComputed 1 failures. http://chromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu13.10-GCE-N...
Message was sent while issue was closed.
On 2014/10/15 18:40:15, bsalomon wrote: > On 2014/10/15 18:04:23, I haz the power (commit-bot) wrote: > > Committed patchset #5 (id:180001) as 8e6c3b93a39e19111662a760ede97df55e51d39f > > Did this cause these failures: > > 0 tasks left, 1 failed 2476M peak 1111ms test PackBits > Failures: > test Jpeg_YUV: ../../tests/JpegTest.cpp:474 sizesComputed > 1 failures. > > http://chromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu13.10-GCE-N... Yes, Jpeg_YUV was added here. I'm already investigating, but I can't reproduce the failure locally for some reason.
Message was sent while issue was closed.
On 2014/10/15 18:42:32, sugoi1 wrote: > On 2014/10/15 18:40:15, bsalomon wrote: > > On 2014/10/15 18:04:23, I haz the power (commit-bot) wrote: > > > Committed patchset #5 (id:180001) as > 8e6c3b93a39e19111662a760ede97df55e51d39f > > > > Did this cause these failures: > > > > 0 tasks left, 1 failed 2476M peak 1111ms test PackBits > > Failures: > > test Jpeg_YUV: ../../tests/JpegTest.cpp:474 sizesComputed > > 1 failures. > > > > > http://chromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu13.10-GCE-N... > > Yes, Jpeg_YUV was added here. I'm already investigating, but I can't reproduce > the failure locally for some reason. Should we revert? many builds are red due to this.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:180001) has been created in https://codereview.chromium.org/656163002/ by rmistry@google.com. The reason for reverting is: Caused many test bots to go red: http://build.chromium.org/p/client.skia/builders/Test-ChromeOS-Alex-GMA3150-x... http://build.chromium.org/p/client.skia/builders/Test-ChromeOS-Link-HD4000-x8... http://build.chromium.org/p/client.skia/builders/Test-Ubuntu12-ShuttleA-GTX66....
PTAL! I found the error. fShouldCancelDecode was uninitialized, so if it turned out to be non-zero, the decoding was failing. I simply set it to false to fix the issue.
On 2014/10/16 19:28:02, sugoi1 wrote: > PTAL! > > I found the error. fShouldCancelDecode was uninitialized, so if it turned out to > be non-zero, the decoding was failing. I simply set it to false to fix the > issue. lgtm
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/399683007/220001
Message was sent while issue was closed.
Committed patchset #7 (id:220001) as b227e37eae36ccf630c4baef00b1354d42b40fd1 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
