Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
418653002:
Allowing YUV data to be retrieved from the JPEG Decoder. (Closed)
DescriptionAllowing YUV data to be retrieved from the JPEG Decoder.
Adding the code to extract the raw YUV channels from the JPEGImageDedcoder and
the plumbing to go from the skia derived classes all the way down to the decoder.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178963
Patch Set 1 #Patch Set 2 : Removed useless added parameter #
Total comments: 57
Patch Set 3 : Fixed style nits #Patch Set 4 : Changed unsigned char to appropriate JSAMP... types #
Total comments: 4
Patch Set 5 : Removed changes to ImageFrameGenerator for now #
Total comments: 52
Patch Set 6 : Simplification and clean up #Patch Set 7 : Fixed comments #
Total comments: 3
Patch Set 8 : Removed unnecessary default arguments #Patch Set 9 : Adding revert to RGB when we have color profiles #
Total comments: 1
Patch Set 10 : Added a unit test #
Total comments: 1
Patch Set 11 : Added a constant #Patch Set 12 : Added missing PLATFORM_EXPORT #
Total comments: 13
Messages
Total messages: 46 (0 generated)
Here's a first draft of the YUV extraction code in blink in order to get some comments.
Looks ok (modulo nits), but I'd like one of {hclam, noel, pkasting} to take a
look, too.
https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/ImageFrameGenerator.cpp (right):
https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics...
Source/platform/graphics/ImageFrameGenerator.cpp:203: return m_hardwareDecoding;
Naming nit: generally we use "accelerated" (or sometimes "GPU") in Blink, rather
than "hardware" (CPU is hardware too). Suggest m_acceleratedDecoding.
In fact, since we aren't accelerating the whole JPG stack, maybe this should be
m_acceleratedYUVDecoding?
https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de...
File Source/platform/image-decoders/ImageDecoder.h (right):
https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de...
Source/platform/image-decoders/ImageDecoder.h:73: return ((i >= 0) && i < 3) ?
m_planes[i] : 0;
Should this be an assert instead?
https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de...
Source/platform/image-decoders/ImageDecoder.h:78: return ((i >= 0) && i < 3) ?
m_rowBytes[i] : 0;
Same here.
https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right):
https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:245: static void
computeUVSize(const jpeg_decompress_struct* info, int& width, int& height)
I prefer pointers to non-const refs, but that's just me -- Blink doesn't care,
so I can't stop you.
https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:362: && (DCTSIZE == 8)
// This code handles a DCTSIZE of 8
Comment seems superfluous to me.
https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:382: // printf("YUV
4:%d:%d\n", 4 / h, (v == 1) ? 4 / h : 0);
Please remove commented-out code.
https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:673: int w(0), h(0);
Initializers look superfluous here: computeUVSize() always assigns.
https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de...
File Source/platform/image-decoders/jpeg/JPEGImageDecoder.h (right):
https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de...
Source/platform/image-decoders/jpeg/JPEGImageDecoder.h:57: virtual void
setHardwareDecoding(bool hardwareDecoding) OVERRIDE { m_hardwareDecoding =
hardwareDecoding; }
As above: maybe setAcceleratedYUVDecoding?
One of the others should review for functionality; I don't really know enough about how this works to review for anything other than style. https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:382: // Try to create an ImageDecoder Nit: This comment adds little to the code. https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:383: if (!decoder && m_imageDecoderFactory) { Nit: No {} (several places) https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:391: if (!decoder) { Nit: This can be moved inside the conditional above https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:395: // If the decoder isn't cached, take ownership of it here Nit: Another poor-quality comment. https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:396: OwnPtr<ImageDecoder> decoderContainer; Nit: Instead of declaring this down here, why not declare it much higher up, and take ownership directly in the places you currently call leakPtr()? That seems clearer and safer. https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... File Source/platform/graphics/ImageFrameGenerator.h (right): https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.h:77: // Decodes components directly into the provided memory planes Nit: Trailing period https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.h:93: bool getComponentSizes(SkISize* componentSizes); Above you pass arrays with sizes, but here you pass a pointer. Be consistent. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:57: for (int i = 0; i < 3; ++i) { Nit: For these methods that require more than a line or so to implement, I'd prefer to place the implementation in the .cpp file unless there's a performance reason not to. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:80: private: Nit: Blank line above this https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:245: static void computeUVSize(const jpeg_decompress_struct* info, int& width, int& height) On 2014/07/23 21:45:53, Stephen White wrote: > I prefer pointers to non-const refs, but that's just me -- Blink doesn't care, > so I can't stop you. I agree -- pointers seem both more common and clearer. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:247: int h = info->cur_comp_info[0]->h_samp_factor; Nit: Personally, I wouldn't add spaces to line up these equals signs. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:362: && (DCTSIZE == 8) // This code handles a DCTSIZE of 8 On 2014/07/23 21:45:53, Stephen White wrote: > Comment seems superfluous to me. Indeed, please remove. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:465: m_samples = (*m_info.mem->alloc_sarray)(reinterpret_cast<j_common_ptr>(&m_info), JPOOL_IMAGE, m_info.output_width * 4, m_info.out_color_space == JCS_YCbCr ? 2: 1); Nit: Space before ':' https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:798: bufferraw[0] = (unsigned char*)&bufferraw2[0]; Nit: Use C++-style casts. (Many places) https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:801: int yWidth = info->output_width; Nit: Again, I personally wouldn't line up the equals signs. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:825: int scanline = (info->output_scanline + i); Nit: Extra space https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:839: bufferraw2[16+i] = &((unsigned char*)outputU)[scanline * rowBytesU]; Nit: Spaces around '+' https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:851: if (scanlinesRead == 0) { Nit: No {} https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:855: memcpy(&outputY[ yMaxH * rowBytesY], yLastRow, yWidth); Nit: Extra space https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:864: info->output_scanline = info->output_height; Nit: Shorter: info->output_scanline = std::min(info->output_scanline, info->output_height);
https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:203: return m_hardwareDecoding; On 2014/07/23 21:45:53, Stephen White wrote: > Naming nit: generally we use "accelerated" (or sometimes "GPU") in Blink, rather > than "hardware" (CPU is hardware too). Suggest m_acceleratedDecoding. > > In fact, since we aren't accelerating the whole JPG stack, maybe this should be > m_acceleratedYUVDecoding? Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:382: // Try to create an ImageDecoder On 2014/07/23 22:17:36, Peter Kasting wrote: > Nit: This comment adds little to the code. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:383: if (!decoder && m_imageDecoderFactory) { On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: No {} (several places) Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:391: if (!decoder) { On 2014/07/23 22:17:36, Peter Kasting wrote: > Nit: This can be moved inside the conditional above Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:395: // If the decoder isn't cached, take ownership of it here On 2014/07/23 22:17:36, Peter Kasting wrote: > Nit: Another poor-quality comment. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:396: OwnPtr<ImageDecoder> decoderContainer; On 2014/07/23 22:17:36, Peter Kasting wrote: > Nit: Instead of declaring this down here, why not declare it much higher up, and > take ownership directly in the places you currently call leakPtr()? That seems > clearer and safer. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... File Source/platform/graphics/ImageFrameGenerator.h (right): https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.h:77: // Decodes components directly into the provided memory planes On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: Trailing period Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.h:93: bool getComponentSizes(SkISize* componentSizes); On 2014/07/23 22:17:37, Peter Kasting wrote: > Above you pass arrays with sizes, but here you pass a pointer. Be consistent. Done. Used arrays in both cases. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:57: for (int i = 0; i < 3; ++i) { On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: For these methods that require more than a line or so to implement, I'd > prefer to place the implementation in the .cpp file unless there's a performance > reason not to. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:73: return ((i >= 0) && i < 3) ? m_planes[i] : 0; On 2014/07/23 21:45:53, Stephen White wrote: > Should this be an assert instead? Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:78: return ((i >= 0) && i < 3) ? m_rowBytes[i] : 0; On 2014/07/23 21:45:53, Stephen White wrote: > Same here. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:80: private: On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: Blank line above this Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:245: static void computeUVSize(const jpeg_decompress_struct* info, int& width, int& height) On 2014/07/23 21:45:53, Stephen White wrote: > I prefer pointers to non-const refs, but that's just me -- Blink doesn't care, > so I can't stop you. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:245: static void computeUVSize(const jpeg_decompress_struct* info, int& width, int& height) On 2014/07/23 22:17:37, Peter Kasting wrote: > On 2014/07/23 21:45:53, Stephen White wrote: > > I prefer pointers to non-const refs, but that's just me -- Blink doesn't care, > > so I can't stop you. > > I agree -- pointers seem both more common and clearer. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:247: int h = info->cur_comp_info[0]->h_samp_factor; On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: Personally, I wouldn't add spaces to line up these equals signs. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:362: && (DCTSIZE == 8) // This code handles a DCTSIZE of 8 On 2014/07/23 21:45:53, Stephen White wrote: > Comment seems superfluous to me. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:362: && (DCTSIZE == 8) // This code handles a DCTSIZE of 8 On 2014/07/23 22:17:37, Peter Kasting wrote: > On 2014/07/23 21:45:53, Stephen White wrote: > > Comment seems superfluous to me. > > Indeed, please remove. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:382: // printf("YUV 4:%d:%d\n", 4 / h, (v == 1) ? 4 / h : 0); On 2014/07/23 21:45:53, Stephen White wrote: > Please remove commented-out code. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:465: m_samples = (*m_info.mem->alloc_sarray)(reinterpret_cast<j_common_ptr>(&m_info), JPOOL_IMAGE, m_info.output_width * 4, m_info.out_color_space == JCS_YCbCr ? 2: 1); On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: Space before ':' Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:673: int w(0), h(0); On 2014/07/23 21:45:53, Stephen White wrote: > Initializers look superfluous here: computeUVSize() always assigns. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:798: bufferraw[0] = (unsigned char*)&bufferraw2[0]; On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: Use C++-style casts. (Many places) Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:801: int yWidth = info->output_width; On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: Again, I personally wouldn't line up the equals signs. Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:825: int scanline = (info->output_scanline + i); On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: Extra space Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:839: bufferraw2[16+i] = &((unsigned char*)outputU)[scanline * rowBytesU]; On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: Spaces around '+' Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:851: if (scanlinesRead == 0) { On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:855: memcpy(&outputY[ yMaxH * rowBytesY], yLastRow, yWidth); On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: Extra space Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:864: info->output_scanline = info->output_height; On 2014/07/23 22:17:37, Peter Kasting wrote: > Nit: Shorter: > > info->output_scanline = std::min(info->output_scanline, > info->output_height); Done. https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/20001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.h:57: virtual void setHardwareDecoding(bool hardwareDecoding) OVERRIDE { m_hardwareDecoding = hardwareDecoding; } On 2014/07/23 21:45:53, Stephen White wrote: > As above: maybe setAcceleratedYUVDecoding? Done.
Looks good, but as discussed, needs tests.
YUV decoding is long needed. But this change crosses some hairy bits in ImageFrameGenerator and deferred image decoding, and I would like to clean that up first. Please separate the changes related to ImageFrameGenerator: you can have JPEGImageDecoder in this change. I'll make sure IFG is cleaned up before you land this change.
LGTM with comments for style -- again, I'm not doing a functional review. https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/418653002/diff/20001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:395: // If the decoder isn't cached, take ownership of it here On 2014/07/24 15:48:29, sugoi1 wrote: > On 2014/07/23 22:17:36, Peter Kasting wrote: > > Nit: Another poor-quality comment. > > Done. Actually you don't seem to have removed or improved the comment. https://codereview.chromium.org/418653002/diff/60001/Source/platform/graphics... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/418653002/diff/60001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:375: ImageDecoder* decoder = 0; Nit: Seems like this block should move down below the SharedBuffer block to be with the rest of the code that mucks with |decoder|. https://codereview.chromium.org/418653002/diff/60001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:390: decoderContainer = adoptPtr(m_imageDecoderFactory->create().leakPtr()); Do we actually have to call leakPtr() explicitly here and below? (I don't use the Blink smart pointer types enough to know.) https://codereview.chromium.org/418653002/diff/60001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/60001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:57: void set(void* planes[3], size_t rowBytes[3]); Nit: Do we really have to use void* for these planes? Wouldn't char* be a more appropriate type, since this represents "block of bytes"? https://codereview.chromium.org/418653002/diff/60001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:58: void* getPlane(int) const; Const functions should not return non-const pointers. Either make the pointer const, or make the function non-const.
On 2014/07/24 18:08:14, Peter Kasting wrote: > https://codereview.chromium.org/418653002/diff/60001/Source/platform/image-de... > Source/platform/image-decoders/ImageDecoder.h:57: void set(void* planes[3], > size_t rowBytes[3]); > Nit: Do we really have to use void* for these planes? Wouldn't char* be a more > appropriate type, since this represents "block of bytes"? Well, that's what was decided in Skia and since these come from an overloaded virtual function, I would need to go back in Skia to change the API to put "char*" or "unsigned char*" in Skia. If it's really an issue, I can try and go back to do that. > https://codereview.chromium.org/418653002/diff/60001/Source/platform/image-de... > Source/platform/image-decoders/ImageDecoder.h:58: void* getPlane(int) const; > Const functions should not return non-const pointers. Either make the pointer > const, or make the function non-const. Done.
On 2014/07/24 20:28:56, sugoi1 wrote: > On 2014/07/24 18:08:14, Peter Kasting wrote: > > > https://codereview.chromium.org/418653002/diff/60001/Source/platform/image-de... > > Source/platform/image-decoders/ImageDecoder.h:57: void set(void* planes[3], > > size_t rowBytes[3]); > > Nit: Do we really have to use void* for these planes? Wouldn't char* be a > more > > appropriate type, since this represents "block of bytes"? > > Well, that's what was decided in Skia and since these come from an overloaded > virtual function, I would need to go back in Skia to change the API to put > "char*" or "unsigned char*" in Skia. If it's really an issue, I can try and go > back to do that. I think it's worth at least raising the issue in Skia. "void*" means "we have no idea what the type of this is", but in reality it seems like it's always a chunk of bytes, so I'd think char* would make more sense. I'm willing to be overruled, though.
On 2014/07/24 20:30:21, Peter Kasting wrote: > On 2014/07/24 20:28:56, sugoi1 wrote: > > On 2014/07/24 18:08:14, Peter Kasting wrote: > > > > > > https://codereview.chromium.org/418653002/diff/60001/Source/platform/image-de... > > > Source/platform/image-decoders/ImageDecoder.h:57: void set(void* planes[3], > > > size_t rowBytes[3]); > > > Nit: Do we really have to use void* for these planes? Wouldn't char* be a > > more > > > appropriate type, since this represents "block of bytes"? > > > > Well, that's what was decided in Skia and since these come from an overloaded > > virtual function, I would need to go back in Skia to change the API to put > > "char*" or "unsigned char*" in Skia. If it's really an issue, I can try and go > > back to do that. > > I think it's worth at least raising the issue in Skia. "void*" means "we have > no idea what the type of this is", but in reality it seems like it's always a > chunk of bytes, so I'd think char* would make more sense. I'm willing to be > overruled, though. I'll see if I can make that change without causing any controversy. I have no personal preference either way.
https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:53: class DecodingBuffers { This should be called ImagePlanes to be clear. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:57: void set(void* planes[3], size_t rowBytes[3]); It will not always be 3 planes but it's good for now. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:268: How do I know if the code that uses this decoder can draw the YUV subsampled data? I think it's good to have method to query YUV subsampling. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:269: virtual void setAcceleratedYUVDecoding(bool) { } nit: drop "accelerated" from methods names. Decoding is still in software so nothing is accelerated from the point of view of decoders. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:371: // Only set YUV mode if the format is supported nit: period at the end of sentence. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:796: bufferraw[0] = &bufferraw2[0]; Please document why. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:813: int scanlinesToRead = DCTSIZE * v; nit: This should be yScanlinesToRead. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:822: for (int i = 0; i < scanlinesToRead; ++i) { nit: Add a comment that this is reading the Y scanlines. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:834: for (int i = 0; i < 8; ++i) { Add a comment saying this is reading the UV scanlines. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:874: if ((JCS_YCbCr == info->out_color_space) && (m_decodingBuffers.get())) { If you have set YUV decoding but didn't assign the buffers then the following decoding will fail. I suggest you drop the flag for YUV decoding and just use decoding buffers instead. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.h:57: virtual void setAcceleratedYUVDecoding(bool acceleratedYUVDecoding) OVERRIDE { m_acceleratedYUVDecoding = acceleratedYUVDecoding; } This should be enableAcceleratedYUVDecoding() and be merged with setDecodingBuffers(). https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.h:80: bool m_acceleratedYUVDecoding; If there's DecodingBuffers there's no need to have this flag.
https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:53: class DecodingBuffers { On 2014/07/24 22:07:27, Alpha wrote: > This should be called ImagePlanes to be clear. Acknowledged. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:57: void set(void* planes[3], size_t rowBytes[3]); On 2014/07/24 22:07:27, Alpha wrote: > It will not always be 3 planes but it's good for now. Acknowledged. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:268: On 2014/07/24 22:07:26, Alpha wrote: > How do I know if the code that uses this decoder can draw the YUV subsampled data? I think it's good to have method to query YUV subsampling. I'm not sure I understand your question. The code would only call these methods if it wants the YUV data, no? There shouldn't be any surprises for the code calling these. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:269: virtual void setAcceleratedYUVDecoding(bool) { } On 2014/07/24 22:07:26, Alpha wrote: > nit: drop "accelerated" from methods names. Decoding is still in software so > nothing is accelerated from the point of view of decoders. Acknowledged. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:371: // Only set YUV mode if the format is supported On 2014/07/24 22:07:27, Alpha wrote: > nit: period at the end of sentence. Acknowledged. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:813: int scanlinesToRead = DCTSIZE * v; On 2014/07/24 22:07:27, Alpha wrote: > nit: This should be yScanlinesToRead. Acknowledged. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:822: for (int i = 0; i < scanlinesToRead; ++i) { On 2014/07/24 22:07:27, Alpha wrote: > nit: Add a comment that this is reading the Y scanlines. Acknowledged. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:834: for (int i = 0; i < 8; ++i) { On 2014/07/24 22:07:27, Alpha wrote: > Add a comment saying this is reading the UV scanlines. Acknowledged. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:874: if ((JCS_YCbCr == info->out_color_space) && (m_decodingBuffers.get())) { On 2014/07/24 22:07:27, Alpha wrote: > If you have set YUV decoding but didn't assign the buffers then the following > decoding will fail. I suggest you drop the flag for YUV decoding and just use > decoding buffers instead. If it fails for any reason, the caller should fallback to regular RGBA decoding instead using the YUV path. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.h:80: bool m_acceleratedYUVDecoding; On 2014/07/24 22:07:27, Alpha wrote: > If there's DecodingBuffers there's no need to have this flag. The current workflow does require a separate flag, let me explain why: Step 1: Query the YUV size (by setting this flag) Step 2: The caller allocates the memory Step 3: The caller sends the allocated memory through setDecodingBuffers I agree with you that, in step 3, the flag wouldn't be necessary, but the flag is necessary for step 1, at a point where the buffers are not allocated yet. Would you prefer that I send an empty DecodingBuffer object in step 1 instead of the flag?
On 2014/07/24 23:16:38, sugoi1 wrote: > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > File Source/platform/image-decoders/ImageDecoder.h (right): > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/ImageDecoder.h:53: class DecodingBuffers { > On 2014/07/24 22:07:27, Alpha wrote: > > This should be called ImagePlanes to be clear. > > Acknowledged. > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/ImageDecoder.h:57: void set(void* planes[3], > size_t rowBytes[3]); > On 2014/07/24 22:07:27, Alpha wrote: > > It will not always be 3 planes but it's good for now. > > Acknowledged. > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/ImageDecoder.h:268: > On 2014/07/24 22:07:26, Alpha wrote: > > How do I know if the code that uses this decoder can draw the YUV subsampled > data? I think it's good to have method to query YUV subsampling. > > I'm not sure I understand your question. The code would only call these methods > if it wants the YUV data, no? There shouldn't be any surprises for the code > calling these. > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/ImageDecoder.h:269: virtual void > setAcceleratedYUVDecoding(bool) { } > On 2014/07/24 22:07:26, Alpha wrote: > > nit: drop "accelerated" from methods names. Decoding is still in software so > > nothing is accelerated from the point of view of decoders. > > Acknowledged. > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:371: // Only set YUV > mode if the format is supported > On 2014/07/24 22:07:27, Alpha wrote: > > nit: period at the end of sentence. > > Acknowledged. > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:813: int > scanlinesToRead = DCTSIZE * v; > On 2014/07/24 22:07:27, Alpha wrote: > > nit: This should be yScanlinesToRead. > > Acknowledged. > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:822: for (int i = 0; i > < scanlinesToRead; ++i) { > On 2014/07/24 22:07:27, Alpha wrote: > > nit: Add a comment that this is reading the Y scanlines. > > Acknowledged. > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:834: for (int i = 0; i > < 8; ++i) { > On 2014/07/24 22:07:27, Alpha wrote: > > Add a comment saying this is reading the UV scanlines. > > Acknowledged. > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:874: if ((JCS_YCbCr == > info->out_color_space) && (m_decodingBuffers.get())) { > On 2014/07/24 22:07:27, Alpha wrote: > > If you have set YUV decoding but didn't assign the buffers then the following > > decoding will fail. I suggest you drop the flag for YUV decoding and just use > > decoding buffers instead. > > If it fails for any reason, the caller should fallback to regular RGBA decoding > instead using the YUV path. > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > File Source/platform/image-decoders/jpeg/JPEGImageDecoder.h (right): > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/jpeg/JPEGImageDecoder.h:80: bool > m_acceleratedYUVDecoding; > On 2014/07/24 22:07:27, Alpha wrote: > > If there's DecodingBuffers there's no need to have this flag. > > The current workflow does require a separate flag, let me explain why: > > Step 1: Query the YUV size (by setting this flag) > Step 2: The caller allocates the memory > Step 3: The caller sends the allocated memory through setDecodingBuffers > > I agree with you that, in step 3, the flag wouldn't be necessary, but the flag > is necessary for step 1, at a point where the buffers are not allocated yet. > Would you prefer that I send an empty DecodingBuffer object in step 1 instead of > the flag? The sequence of actions you mention is a bit strange. The reason is that caller has to enable YUV decoding before doing any operation to JPEGImageDecoder. What about this? 1. Leave decodedSize() untouched. This will always return the RGB decoded size. 2. Add a new bool decodedYUVSize(...). This will return true if YUV decoding is supported and also the decoded YUV size. 3. Have enableYUVDecoding(...) to replace both setDecodingBuffers() and setYUVDecoding(). Internally it will change the out_color_space to YCbCr.
https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:268: On 2014/07/24 23:16:35, sugoi1 wrote: > On 2014/07/24 22:07:26, Alpha wrote: > > How do I know if the code that uses this decoder can draw the YUV subsampled > data? I think it's good to have method to query YUV subsampling. > > I'm not sure I understand your question. The code would only call these methods > if it wants the YUV data, no? There shouldn't be any surprises for the code > calling these. I mean there's different types of subsampling. My question is if the caller enables YUV decoding, it must handle all subsampling types. Is it true? https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:361: if (m_decoder->acceleratedYUVDecoding() What about by default output RGB and only change out_color_space when YUV decoding is enabled? It will be more clear that YUV path is separate.
https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:268: On 2014/07/24 23:30:56, Alpha wrote: > On 2014/07/24 23:16:35, sugoi1 wrote: > > On 2014/07/24 22:07:26, Alpha wrote: > > > How do I know if the code that uses this decoder can draw the YUV subsampled > > data? I think it's good to have method to query YUV subsampling. > > > > I'm not sure I understand your question. The code would only call these > methods > > if it wants the YUV data, no? There shouldn't be any surprises for the code > > calling these. > > I mean there's different types of subsampling. My question is if the caller > enables YUV decoding, it must handle all subsampling types. Is it true? Well, the caller first receives the size of the Y, U and V planes and could choose to support it or not based on the sizes received. It's fairly easy to deduce the subsampling type from the sizes. But, yes, currently the caller can handle all subsampling types. Note that the JPEG decoder will only accept to set the output type to YUV if the subsampling is one of : 4:4:4, 4:4:0, 4:2:2, 4:2:0, 4:1:1, 4:1:0, so only 6 subsampling types need to be supported currently. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:361: if (m_decoder->acceleratedYUVDecoding() On 2014/07/24 23:30:56, Alpha wrote: > What about by default output RGB and only change out_color_space when YUV decoding is enabled? It will be more clear that YUV path is separate. Isn't that what's already happening here? RGB is set by default at line 360 and when YUV decoding is enabled, we may set it to YUV, if the format is supported.
> https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:361: if > (m_decoder->acceleratedYUVDecoding() > On 2014/07/24 23:30:56, Alpha wrote: > > What about by default output RGB and only change out_color_space when YUV > decoding is enabled? It will be more clear that YUV path is separate. > > Isn't that what's already happening here? RGB is set by default at line 360 and > when YUV decoding is enabled, we may set it to YUV, if the format is supported. My point is to make "case JPEG_HEADER" and "case JCS_YCbCr" default to RGB all the time. ImageDecoder is usually called with decode(xxx, true) first, this is called from JPEGImageDecoder::isSizeAvailable() which is usually the first call to make. My suggestion is to enable YUV decoding in a separate method outside of header parsing. What do you think?
On 2014/07/25 06:50:21, Alpha wrote: > My point is to make "case JPEG_HEADER" and "case JCS_YCbCr" default to RGB all > the time. ImageDecoder is usually called with decode(xxx, true) first, this is > called from JPEGImageDecoder::isSizeAvailable() which is usually the first call > to make. My suggestion is to enable YUV decoding in a separate method outside of > header parsing. > > What do you think? Hmmm... I'm not sure I can do this without duplicating quite a bit of code. I'll upload a new version of the code and you can tell me what you think.
https://codereview.chromium.org/418653002/diff/80001/Source/platform/graphics... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:136: return false; This change seems unrelated to yuv decoding. Perhaps submit it in another patch. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.cpp:85: DecodingBuffers::DecodingBuffers() Not the best place to add this: please move all this new code to the end of file. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:58: void* getPlane(int); blink webcore convention: no "get" prefix in class getters. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:117: virtual IntSize decodedSize(int component = 0) const { return size(); } I believe the Androids might downsample when decoding images. Did you check if yuv decoding interworks with that? https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:76: I think we need to define yuvOutputColorSpace() somewhere around here, and use it everywhere. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:246: { How about this: make this routine return an IntSize? Also, should the routine by called yuvSize() (you only mention uv in its current name, so what's the difference). https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:247: int h = info->cur_comp_info[0]->h_samp_factor; h -> u? https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:361: if (m_decoder->acceleratedYUVDecoding() On 2014/07/25 02:55:13, sugoi1 wrote: > On 2014/07/24 23:30:56, Alpha wrote: > > What about by default output RGB and only change out_color_space when YUV > decoding is enabled? It will be more clear that YUV path is separate. > > Isn't that what's already happening here? RGB is set by default at line 360 and > when YUV decoding is enabled, we may set it to YUV, if the format is supported. The code following here could be moved to it's own function, and somewhere around line 251 would be a good place to define it. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:423: if (!m_decoder->ignoresGammaAndColorProfile()) { Most web jpeg images are YCbCr, and many are photographic images 4:2:2, which come form digital cameras, with a color profile whether the user knows it or not. This YCbCr decoding change breaks all color profiled JPEG web images. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:667: { Did you consider using a separate function that explicitly returned the yuv decoded size and if that'd work better at the call sites? I'm not sure what |component| means -- it needs a better name -- but why is an int? The computed size returned by the function for the yuv case does not seem to depend on it's value. Are there other types of component values > 2 that need decoding in future? If so, what are they? https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:696: { Would yuvDecode() or decodeToYUV() better describe the purpose of this function? Not sure what Accelerated means here. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:741: template <> void setPixel<JCS_RGB>(ImageFrame& buffer, ImageFrame::PixelData* pixel, JSAMPARRAY samples, int column) The setPixel() code from lines 736-762 changed, I'm not sure why. You don't seem to call or use it for the yuv decoding case, so why change it? https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:854: memcpy(&outputY[yMaxH * rowBytesY], yLastRow, yWidth); memcpy: it's perhaps the best way to kill decoding performance. For the RGB[A] and BRG[A] cases, we decoded directly to the frame buffer to avoid memcpy. That alone doubles decoding performance by halving the decoding time compared to copying decoded rows to the frame buffer. That should give you an idea of the cost of a turbo JPEG decode - it's about a memcpy if you've ever wondered about it. Since the decoder can decode direct to whatever buffers you provide, why not organize your planes such that it can? It also occurs to me that you have not measured speed at all, given no evidence of any purpose / gain in your change description. Additional memcpy's of any size tell me there will be a decoding speed regression ... https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:870: return false; If you pass this point and go on to yuv decode, a w x h frame buffer of RGBA must have been allocated. So the memory cost of a yuv decode is an RGBA frame buffer _plus_ the decoding planes caller provides for yuv decoding. So I see a speed regression and an increase in memory costs. If you have you analysed the speed / memory costs (where is that exactly?) and expect them to be better, then the code I see here is not showing any such improvements.
https://codereview.chromium.org/418653002/diff/80001/Source/platform/graphics... File Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/graphics... Source/platform/graphics/ImageFrameGenerator.cpp:136: return false; On 2014/07/25 15:55:01, Noel Gordon wrote: > This change seems unrelated to yuv decoding. Perhaps submit it in another > patch. Done. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.cpp:85: DecodingBuffers::DecodingBuffers() On 2014/07/25 15:55:01, Noel Gordon wrote: > Not the best place to add this: please move all this new code to the end of > file. Done. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/ImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:58: void* getPlane(int); On 2014/07/25 15:55:01, Noel Gordon wrote: > blink webcore convention: no "get" prefix in class getters. Done. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/ImageDecoder.h:117: virtual IntSize decodedSize(int component = 0) const { return size(); } On 2014/07/25 15:55:01, Noel Gordon wrote: > I believe the Androids might downsample when decoding images. Did you check if > yuv decoding interworks with that? Looking at DecodingImageGenerator::onGetPixels() : "Implementation doesn't support scaling yet so make sure we're not given a different size." For now, I could bail out if any scaling factor is given since it's not yet supported. I can add that functionality as a follow up. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:246: { On 2014/07/25 15:55:01, Noel Gordon wrote: > How about this: make this routine return an IntSize? Done. > Also, should the routine by called yuvSize() (you only mention uv in its current name, so what's the difference). This computes the size of the U and V planes (which are always the same size in the supported subsampling modes), so it does compute specifically the UV planes size. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:247: int h = info->cur_comp_info[0]->h_samp_factor; On 2014/07/25 15:55:01, Noel Gordon wrote: > h -> u? In this case: h -> horizontal v -> vertical https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:361: if (m_decoder->acceleratedYUVDecoding() On 2014/07/25 15:55:01, Noel Gordon wrote: > On 2014/07/25 02:55:13, sugoi1 wrote: > > On 2014/07/24 23:30:56, Alpha wrote: > > > What about by default output RGB and only change out_color_space when YUV > > decoding is enabled? It will be more clear that YUV path is separate. > > > > Isn't that what's already happening here? RGB is set by default at line 360 > and > > when YUV decoding is enabled, we may set it to YUV, if the format is > supported. > > The code following here could be moved to it's own function, and somewhere > around line 251 would be a good place to define it. Already done in current patch https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:667: { On 2014/07/25 15:55:01, Noel Gordon wrote: > Did you consider using a separate function that explicitly returned the yuv > decoded size and if that'd work better at the call sites? Already done in current patch. > I'm not sure what |component| means -- it needs a better name -- but why is an > int? The computed size returned by the function for the yuv case does not seem > to depend on it's value. Are there other types of component values > 2 that > need decoding in future? If so, what are they? "Component" as in "color component" 0: Y 1: U 2: V See newest patch https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:696: { On 2014/07/25 15:55:01, Noel Gordon wrote: > Would yuvDecode() or decodeToYUV() better describe the purpose of this function? > Not sure what Accelerated means here. Accelerated has already been removed from the names. I can change it to decodeToYUV(), no problem https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:741: template <> void setPixel<JCS_RGB>(ImageFrame& buffer, ImageFrame::PixelData* pixel, JSAMPARRAY samples, int column) On 2014/07/25 15:55:01, Noel Gordon wrote: > The setPixel() code from lines 736-762 changed, I'm not sure why. You don't > seem to call or use it for the yuv decoding case, so why change it? This is cleanup. Using template arguments in conditions and switches generated unnecessary code. I simply simplified each case by using the template argument. https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:854: memcpy(&outputY[yMaxH * rowBytesY], yLastRow, yWidth); On 2014/07/25 15:55:01, Noel Gordon wrote: > memcpy: it's perhaps the best way to kill decoding performance. For the RGB[A] > and BRG[A] cases, we decoded directly to the frame buffer to avoid memcpy. That > alone doubles decoding performance by halving the decoding time compared to > copying decoded rows to the frame buffer. > > That should give you an idea of the cost of a turbo JPEG decode - it's about a > memcpy if you've ever wondered about it. Since the decoder can decode direct to > whatever buffers you provide, why not organize your planes such that it can? > > It also occurs to me that you have not measured speed at all, given no evidence > of any purpose / gain in your change description. Additional memcpy's of any > size tell me there will be a decoding speed regression ... This is only the last row of a jpeg image, if the jpeg's height is not a multiple of 8, not all the rows. It is done this way because reading the last row can overflow buffer memory. A single row of an image, in a special case, probably won't affect performance significantly. For the gains, take 4:2:0, for example. This format has a U and V channel of 1/2 width and 1/2 height, so 1/4 size, which means that the whole YUV image uses the equivalent of 1 1/2 channels of memory compared to 4 channels for an RGBA texture upload, saving 62.5% of the memory uploaded to the GPU. On bandwidth limited devices (like mobile devices), this should be significant.
https://codereview.chromium.org/418653002/diff/120001/Source/platform/image-d... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/120001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.h:51: virtual IntSize decodedYUVSize(int component = 0) const OVERRIDE; Nit: seems strange to have a default argument here; it's a new function so let's just make all callers be explicit.
https://codereview.chromium.org/418653002/diff/120001/Source/platform/image-d... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/120001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.h:51: virtual IntSize decodedYUVSize(int component = 0) const OVERRIDE; On 2014/07/25 17:03:43, Stephen White wrote: > Nit: seems strange to have a default argument here; it's a new function so let's just make all callers be explicit. You're right, I had originally put this here because I was modifying decodedSize, but since I added decodedYUVSize, the default argument is no longer necessary.
https://codereview.chromium.org/418653002/diff/120001/Source/platform/image-d... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/120001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:259: return IntSize((info->output_width + h - 1) / h, (info->output_height + v - 1) / v); Note: interesting that we have to round up here. This suggests that some images out there have sizes which aren't a multiple of their h or v sampling factor. We should make sure that we're handling this case correctly when uploading to the GPU.
https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:423: if (!m_decoder->ignoresGammaAndColorProfile()) { On 2014/07/25 15:55:01, Noel Gordon wrote: > Most web jpeg images are YCbCr, and many are photographic images 4:2:2, which > come form digital cameras, with a color profile whether the user knows it or > not. This YCbCr decoding change breaks all color profiled JPEG web images. Oh yeah, I know about the color profiles, I don't have an implementation for it yet, but it doesn't seem too complicated in most cases. I'll add a condition for now to exit early when we have a color profile. https://codereview.chromium.org/418653002/diff/160001/Source/platform/image-d... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/160001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:463: if (m_transform && m_info.out_color_space == JCS_YCbCr) { This will be implemented in the next version, but, for now, revert to RGB when we have a color space on a YUV image.
On 2014/07/25 17:20:26, sugoi1 wrote: > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): > > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:423: if > (!m_decoder->ignoresGammaAndColorProfile()) { > On 2014/07/25 15:55:01, Noel Gordon wrote: > > Most web jpeg images are YCbCr, and many are photographic images 4:2:2, which > > come form digital cameras, with a color profile whether the user knows it or > > not. This YCbCr decoding change breaks all color profiled JPEG web images. > > Oh yeah, I know about the color profiles, I don't have an implementation for it > yet, but it doesn't seem too complicated in most cases. I'll add a condition for > now to exit early when we have a color profile. > > https://codereview.chromium.org/418653002/diff/160001/Source/platform/image-d... > File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): > > https://codereview.chromium.org/418653002/diff/160001/Source/platform/image-d... > Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:463: if (m_transform && > m_info.out_color_space == JCS_YCbCr) { > This will be implemented in the next version, but, for now, revert to RGB when > we have a color space on a YUV image. This looks better. We can improve this over time. LGTM.
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sugoi@chromium.org/418653002/160001
The CQ bit was unchecked by sugoi@chromium.org
https://codereview.chromium.org/418653002/diff/180001/Source/platform/image-d... File Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp (right): https://codereview.chromium.org/418653002/diff/180001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoderTest.cpp:220: readYUV(1000 * 1000, &outputYWidth, &outputYHeight, &outputUVWidth, &outputUVHeight, jpegFile); Let's pull that out into a constant at the top-of-file, now that it's used by two tests.
LGTM w/test nit fixed
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sugoi@chromium.org/418653002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
The CQ bit was checked by sugoi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sugoi@chromium.org/418653002/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_gpu_retina_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
Message was sent while issue was closed.
Change committed as 178963
Message was sent while issue was closed.
On 2014/07/25 16:56:48, sugoi1 wrote: > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:741: template <> void > setPixel<JCS_RGB>(ImageFrame& buffer, ImageFrame::PixelData* pixel, JSAMPARRAY > samples, int column) > On 2014/07/25 15:55:01, Noel Gordon wrote: > > The setPixel() code from lines 736-762 changed, I'm not sure why. You don't > > seem to call or use it for the yuv decoding case, so why change it? > > This is cleanup. Using template arguments in conditions and switches generated > unnecessary code. I simply simplified each case by using the template argument. Unrelated cleanup. Where are templates used in conditions, and how did you determine the unnecessary code was generated? Your finding run completely counter to the results of https://bugs.webkit.org/show_bug.cgi?id=88424 and http://trac.webkit.org/changeset/136189 > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:854: > memcpy(&outputY[yMaxH * rowBytesY], yLastRow, yWidth); > On 2014/07/25 15:55:01, Noel Gordon wrote: > > memcpy: it's perhaps the best way to kill decoding performance. For the RGB[A] > > and BRG[A] cases, we decoded directly to the frame buffer to avoid memcpy. > > ... Additional memcpy's of any > > size tell me there will be a decoding speed regression ... > > This is only the last row of a jpeg image, if the jpeg's height is not a > multiple of 8, not all the rows. 815: JDIMENSION scanlinesRead = jpeg_read_raw_data(info, bufferraw, yScanlinesToRead) applies to all rows, right? and appears to copy data to your buffers. > For the gains, take 4:2:0, for example. This format has a U and V channel of 1/2 > width and 1/2 height, so 1/4 size, which means that the whole YUV image uses the > equivalent of 1 1/2 channels of memory compared to 4 channels for an RGBA > texture upload, saving 62.5% of the memory uploaded to the GPU. On bandwidth > limited devices (like mobile devices), this should be significant. In theory, but in fact the frame buffer cache is not empty in the decoder, reading from line 904 onwards 904: bool JPEGImageDecoder::outputScanlines() { if (m_frameBufferCache.isEmpty()) return false; jpeg_decompress_struct* info = m_reader->info(); if (m_imagePlanes.get()) { return outputRawData(m_reader.get(), m_imagePlanes.get()); } ... That's an image w x h RGBA buffer you add your "savings" for a net increase in memory use.
Message was sent while issue was closed.
On 2014/07/25 17:20:26, sugoi1 wrote: > On 2014/07/25 15:55:01, Noel Gordon wrote: > > Most web jpeg images are YCbCr, and many are photographic images 4:2:2, which > > come form digital cameras, with a color profile whether the user knows it or > > not. This YCbCr decoding change breaks all color profiled JPEG web images. > > Oh yeah, I know about the color profiles, I don't have an implementation for it > yet, but it doesn't seem too complicated in most cases. I'll add a condition for > now to exit early when we have a color profile. Ok needs some work though ...
Message was sent while issue was closed.
https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... File Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/ImageDecoder.cpp:215: void ImagePlanes::set(void* planes[3], size_t rowBytes[3]) setPlanes https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:263: { yuvSubsampling(...) https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:416: m_info.out_color_space = JCS_YCbCr; You should somehow record that YCbCr decoding is allowed, but defer the actual setting of m_info.out_color_space = JCS_YCbCr to later ... https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:463: if (m_transform && m_info.out_color_space == JCS_YCbCr) { ... because this code in this block is broken by any presetting of m_info.out_color_space == JCS_YCbCr. colorSpaceHasAlpha() won't work since you subsequently go change the output color space back to some RGB[A] space. Let's remove the code you added here ... https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:475: // Don't allocate a giant and superfluous memory buffer when the ... and make a deferred yuv test around here if (!m_decoder->hasColorProfile() && yuvDecodingAllowed) { info.m_info.out_color_space == JCS_YCbCr; ... } If we end up not yuv decoding, what should happen to m_imagePlanes in that case? https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:911: if (m_imagePlanes.get()) { Can you have imagePlanes but be decoding into RGBA due to the output_color_space switching added to the reader's JPEG_HEADER case ? https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.h (right): https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.h:58: virtual bool YUVDecoding() const OVERRIDE { return m_imagePlanes.get(); } Could you have imagePlanes, but be decoding into output_color_space RGBA?
Message was sent while issue was closed.
It looks like there are still outstanding issues from Noel's review. @sugoi - can you address these and do a followup patch?
Message was sent while issue was closed.
On 2014/07/29 05:35:08, Mike Lawther (Google) wrote: > It looks like there are still outstanding issues from Noel's review. > > @sugoi - can you address these and do a followup patch? Just a quick message to tell you guys that I'm back and will be looking into this now. Thanks.
Message was sent while issue was closed.
Hi Noel, I'll proceed with the follow up soon, but I had some questions about these comments. I don't understand some of them and I need a bit more detail. Thanks On 2014/07/28 06:38:38, Noel Gordon wrote: > Your finding run completely > counter to the results of https://bugs.webkit.org/show_bug.cgi?id=88424 and > http://trac.webkit.org/changeset/136189 I'm sorry, but I don't understand how it counters the results of your cls. I simply changed a generic template function and used template specialization to split it into specialized functions by removing branching from the generic template function. You must be seeing something in my change that I don't see, because I don't understand how template specialization breaks your webkit change. If template specialization results in any kind of loss in performance or hinders compiler optimizations in any way, this is news to me. It should be same or better and if it isn't, please explain, because I'm really interested in finding out why. > https://codereview.chromium.org/418653002/diff/80001/Source/platform/image-de... > > Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:854: > > memcpy(&outputY[yMaxH * rowBytesY], yLastRow, yWidth); > > On 2014/07/25 15:55:01, Noel Gordon wrote: > > > memcpy: it's perhaps the best way to kill decoding performance. For the > RGB[A] > > > and BRG[A] cases, we decoded directly to the frame buffer to avoid memcpy. > > > ... Additional memcpy's of any > > > size tell me there will be a decoding speed regression ... > > > > This is only the last row of a jpeg image, if the jpeg's height is not a > > multiple of 8, not all the rows. > > 815: JDIMENSION scanlinesRead = jpeg_read_raw_data(info, bufferraw, > yScanlinesToRead) > > applies to all rows, right? and appears to copy data to your buffers. Of course, it is the same as calling jpeg_read_scanlines() in the original codepath, except that it reads larger chunks of memory per call and and is called 8 or 16 times less often than jpeg_read_scanlines(), without the YUV decoding applied (and color correction), which results in having to write to less memory. The original codepath also reads pixels into the allocated memory, the workflow is pretty similar. I don't see how jpeg_read_raw_data() is worst than jpeg_read_scanlines(), but I'm not an expert on the jpeg library, so maybe you know something I don't know about the performance of jpeg_read_raw_data() VS jpeg_read_scanlines(). As I understand it, it should result in less memory write operations than the original code. > > For the gains, take 4:2:0, for example. This format has a U and V channel of 1/2 > > width and 1/2 height, so 1/4 size, which means that the whole YUV image uses the > > equivalent of 1 1/2 channels of memory compared to 4 channels for an RGBA > > texture upload, saving 62.5% of the memory uploaded to the GPU. On bandwidth > > limited devices (like mobile devices), this should be significant. > > In theory, but in fact the frame buffer cache is not empty in the decoder, > reading from line 904 onwards > > 904: bool JPEGImageDecoder::outputScanlines() > { > if (m_frameBufferCache.isEmpty()) > return false; > > jpeg_decompress_struct* info = m_reader->info(); > > if (m_imagePlanes.get()) { > return outputRawData(m_reader.get(), m_imagePlanes.get()); > } > ... > > That's an image w x h RGBA buffer you add your "savings" for a net increase in memory use. m_frameBufferCache is a vector of ImageFrame objects. The ImageFrame's internal memory gets allocated within the call to ImageFrame::setSize(), which is called at line 921, so because the code returns early, that frame's memory will never get allocated. The fact the the m_frameBufferCache vector is not empty does not imply that the frames are allocated. I don't know of any reason why these frames would be allocated before we know their size. Maybe this wasn't clear originally : this code is only meant to be used when we have a yet-to-be-decoded JPEG and we want a texture. This is only going to be used by skia when we already know that we want a texture. The RGBA buffer will never exist in system memory. The conversion from YUV to RGBA will happen on the GPU.
Message was sent while issue was closed.
The fixes are available here: https://codereview.chromium.org/480093003/ https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... File Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/ImageDecoder.cpp:215: void ImagePlanes::set(void* planes[3], size_t rowBytes[3]) On 2014/07/28 07:29:07, Noel Gordon wrote: > setPlanes Done. https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... File Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp (right): https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:263: { On 2014/07/28 07:29:07, Noel Gordon wrote: > yuvSubsampling(...) Done. https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:416: m_info.out_color_space = JCS_YCbCr; On 2014/07/28 07:29:07, Noel Gordon wrote: > You should somehow record that YCbCr decoding is allowed, but defer the actual > setting of m_info.out_color_space = JCS_YCbCr to later ... Done. https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:463: if (m_transform && m_info.out_color_space == JCS_YCbCr) { On 2014/07/28 07:29:07, Noel Gordon wrote: > ... because this code in this block is broken by any presetting of > m_info.out_color_space == JCS_YCbCr. colorSpaceHasAlpha() won't work since you > subsequently go change the output color space back to some RGB[A] space. > > Let's remove the code you added here ... Done. https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:475: // Don't allocate a giant and superfluous memory buffer when the On 2014/07/28 07:29:07, Noel Gordon wrote: > ... and make a deferred yuv test around here > > if (!m_decoder->hasColorProfile() && yuvDecodingAllowed) { > info.m_info.out_color_space == JCS_YCbCr; > ... > } > > If we end up not yuv decoding, what should happen to m_imagePlanes in that case? Done. In that case, the planes wouldn't be used. The workflow is to call this with onlySize == true first (see line 479). Then, we can allocate the memory planes and pass them to the decoder, to be used for decoding. The planes shouldn't be allocated at all if the decoding isn't going to do YUV decoding. https://codereview.chromium.org/418653002/diff/220001/Source/platform/image-d... Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:911: if (m_imagePlanes.get()) { On 2014/07/28 07:29:07, Noel Gordon wrote: > Can you have imagePlanes but be decoding into RGBA due to the output_color_space > switching added to the reader's JPEG_HEADER case ? This shouldn't happen. Once the header is read (using onlySize), then the planes are allocated and the decoding can resume. From this point, the out_color_space should no longer change.
Message was sent while issue was closed.
message: On 2014/08/18 18:47:39, sugoi1 wrote: > Hi Noel, > On 2014/07/28 06:38:38, Noel Gordon wrote: > > Your finding run completely > > counter to the results of https://bugs.webkit.org/show_bug.cgi?id=88424 and > > http://trac.webkit.org/changeset/136189 > > I'm sorry, but I don't understand how it counters the results of your cls. The webkit CL's were Viatcheslav's, not mine, we just had to review them :) > I simply changed a generic template function and used template specialization to > split it into specialized functions by removing branching from the generic > template function. The generic template function used a switch / case statement. The question of using template specializations, as you have done, came up In review https://bugs.webkit.org/attachment.cgi?id=176379&action=review Brent: Could this switch be handled as template specializations to avoid branching? Would it even make a difference in performance? Good questions. On the other hand, maybe the compiler does the "right thing" with the generic switch / case template, generating specific code for each case label. > You must be seeing something in my change that I don't see, > because I don't understand how template specialization breaks your webkit > change. If template specialization results in any kind of loss in performance or > hinders compiler optimizations in any way, this is news to me. It should be same > or better and if it isn't, please explain, because I'm really interested in > finding out why. The review response https://bugs.webkit.org/show_bug.cgi?id=88424#c60 said the compiler did the "right thing", so the code was submitted with the generic switch / case template, instead of template specializations. So your findings made me curious: template specialization was better, the generic was generating "unnecessary code". Perhaps Viatcheslav's advice https://bugs.webkit.org/show_bug.cgi?id=88424#c60 was wrong? I currently believe you are both right :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
