Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22290002:
Beginning work to refactor jpeg tile decoding. (Closed)
|
Created:
7 years, 4 months ago by scroggo Modified:
7 years, 4 months ago Reviewers:
mtklein CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionBeginning work to refactor jpeg tile decoding.
Keep common code together, so we can fix bugs in tile and normal decode
simultaneously.
Convert if-then chains to switch statements for readability.
Add getBitmapConfig, common to both normal and tile decode, which
ensures that they behave the same. getBitmapConfig uses the code
originally in onDecode, so subsetting grayscale into A8 now works.
In getBitmapConfig, handle JCS_YCCK properly.
Fix a bug where requesting A8 from a JCS_CMYK image would
result in a total failure to decode, since we would change
out_color_space to an invalid choice.
Factor common code for applying dither and changing the
out_color_space into apply_dither_mode (final name TBD). Skips
CMYK like normal decoding did before.
BUG=skia:1472
BUG=https://b.corp.google.com/issue?id=9466275
BUG=https://b.corp.google.com/issue?id=9189955
R=mtklein@google.com
Committed: https://code.google.com/p/skia/source/detail?r=10629
Patch Set 1 #
Total comments: 39
Patch Set 2 : Respond to comments. #Messages
Total messages: 8 (0 generated)
In the refactor, a bunch of code has been both moved and rewritten, so I added some comments to (hopefully) explain what's going on. There are some more bits that I may refactor in a future change, but this is already getting big, and it fixes several bugs. TODO: add more tests. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... File src/images/SkImageDecoder_libjpeg.cpp (left): https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:347: cinfo.scale_num = 1; scale_num defaults to 1, so this line is unnecessary. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:380: cinfo.out_color_space = JCS_RGB; This line would incorrectly change out_color_space to JCS_RGB even if jpeg_color_space was JCS_CMYK (or JCS_CYYK), resulting in libjpeg failing. The new code fixes this. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:625: if (this->getPreferQualityOverSpeed()) { The new method checks #ifdef's where appropraiate. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:631: SkBitmap::Config config = this->getPrefConfig(k32Bit_SrcDepth, false); The new code has the behavior used in onDecode, so it respects a request for A8, when appropriate. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:639: cinfo->out_color_space = JCS_RGB; Likewise, the new code uses a valid color space. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:643: if (SkBitmap::kARGB_8888_Config == config) { The factored out function checks for cmyk. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... File src/images/SkImageDecoder_libjpeg.cpp (right): https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:387: case JCS_YCCK: In the old code, JCS_YCCK results in a request for JCS_RGB, which libjpeg refuses to do. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:409: * FIXME: Improve this name. Here is what this function does: Suggestions? https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:417: if (JCS_CMYK == cinfo->out_color_space) { This check was not happening in tile based decode.
I'm sorry that a lot of these comments are tangential, but I figured if I had questions on my first read through I might as well ask them. Is it feasible to add a regression test for each of the bugs you've fixed? https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... File src/images/SkImageDecoder_libjpeg.cpp (left): https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:347: cinfo.scale_num = 1; On 2013/08/05 21:47:51, scroggo wrote: > scale_num defaults to 1, so this line is unnecessary. maybe add an assert like in the tiled code? https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... File src/images/SkImageDecoder_libjpeg.cpp (right): https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:33: // disable for the moment, as we have some glitches when width != multiple of 4 Off topic, but maybe we should delete this line, given that it's _not_ disabled. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:42: static void overwrite_mem_buffer_size(j_decompress_ptr cinfo) { Is j_decompress_ptr == jpeg_decompress_struct* ? If so, let's use just one of them? (I'd use jpeg_decompress_struct*). https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:58: static void initialize_info(jpeg_decompress_struct* cinfo, skjpeg_source_mgr* src_mgr) { cinfo is a pretty cryptic name. even just decompressor / fDecompressor would be a step up I think. I have a real hard time with names involving "info". Everything is info... Is this just canonically called cinfo? https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:78: ~SkJPEGImageIndex() { Can you add a note in here about why we set the flags to false before calling jpeg_*? https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:124: jpeg_decompress_struct* cinfo() { return &fCInfo; } these two guys can be const methods? https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:354: SkBitmap::Config SkJPEGImageDecoder::getBitmapConfig(jpeg_decompress_struct* cinfo) { can we make this guy take a const&? https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:362: SkBitmap::Config config = this->getPrefConfig(srcDepth, false); Would help my reading to add a reminder what the false argument means here. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:402: break; just for consistency i'd add a break above or remove this one. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:413: static void apply_dither_mode(jpeg_decompress_struct* cinfo, SkBitmap::Config config, funky arg indent? https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:413: static void apply_dither_mode(jpeg_decompress_struct* cinfo, SkBitmap::Config config, maybe adjustOutColorSpaceForDither? https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:475: SkBitmap::Config config = this->getBitmapConfig(&cinfo); const? https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:716: SkBitmap::Config config = this->getBitmapConfig(cinfo); const?
> I'm sorry that a lot of these comments are tangential, > but I figured if I had questions on my first read through > I might as well ask them. No problem. I support cleaning up code as I go :) > Is it feasible to add a regression test for each of the > bugs you've fixed? Yes, I will follow up with more tests. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... File src/images/SkImageDecoder_libjpeg.cpp (left): https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:347: cinfo.scale_num = 1; On 2013/08/05 22:31:09, mtklein wrote: > On 2013/08/05 21:47:51, scroggo wrote: > > scale_num defaults to 1, so this line is unnecessary. > > maybe add an assert like in the tiled code? Done. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... File src/images/SkImageDecoder_libjpeg.cpp (right): https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:33: // disable for the moment, as we have some glitches when width != multiple of 4 On 2013/08/05 22:31:09, mtklein wrote: > Off topic, but maybe we should delete this line, given that it's _not_ disabled. Done. This line has been here since we imported the code from Android, so I'm not sure when we reenabled it... https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:42: static void overwrite_mem_buffer_size(j_decompress_ptr cinfo) { On 2013/08/05 22:31:09, mtklein wrote: > Is j_decompress_ptr == jpeg_decompress_struct* ? > > If so, let's use just one of them? (I'd use jpeg_decompress_struct*). Yes. This is the only instance of j_decompress_ptr. Changed. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:58: static void initialize_info(jpeg_decompress_struct* cinfo, skjpeg_source_mgr* src_mgr) { On 2013/08/05 22:31:09, mtklein wrote: > cinfo is a pretty cryptic name. even just decompressor / fDecompressor would be > a step up I think. I have a real hard time with names involving "info". > Everything is info... Is this just canonically called cinfo? Agreed. Yes, the name is canonical. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:78: ~SkJPEGImageIndex() { On 2013/08/05 22:31:09, mtklein wrote: > Can you add a note in here about why we set the flags to false before calling > jpeg_*? Done. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:124: jpeg_decompress_struct* cinfo() { return &fCInfo; } On 2013/08/05 22:31:09, mtklein wrote: > these two guys can be const methods? No, these are modified and passed to functions that modify them. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:354: SkBitmap::Config SkJPEGImageDecoder::getBitmapConfig(jpeg_decompress_struct* cinfo) { On 2013/08/05 22:31:09, mtklein wrote: > can we make this guy take a const&? The Skia style guide states to use a pointer when you're going to modify an object/struct. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:362: SkBitmap::Config config = this->getPrefConfig(srcDepth, false); On 2013/08/05 22:31:09, mtklein wrote: > Would help my reading to add a reminder what the false argument means here. Done. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:402: break; On 2013/08/05 22:31:09, mtklein wrote: > just for consistency i'd add a break above or remove this one. Done. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:413: static void apply_dither_mode(jpeg_decompress_struct* cinfo, SkBitmap::Config config, On 2013/08/05 22:31:09, mtklein wrote: > maybe adjustOutColorSpaceForDither? How about adjust_out_color_space_and_dither? https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:413: static void apply_dither_mode(jpeg_decompress_struct* cinfo, SkBitmap::Config config, On 2013/08/05 22:31:09, mtklein wrote: > funky arg indent? Done. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:475: SkBitmap::Config config = this->getBitmapConfig(&cinfo); On 2013/08/05 22:31:09, mtklein wrote: > const? Done. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:716: SkBitmap::Config config = this->getBitmapConfig(cinfo); On 2013/08/05 22:31:09, mtklein wrote: > const? Done.
https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... File src/images/SkImageDecoder_libjpeg.cpp (right): https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:124: jpeg_decompress_struct* cinfo() { return &fCInfo; } On 2013/08/06 15:06:27, scroggo wrote: > On 2013/08/05 22:31:09, mtklein wrote: > > these two guys can be const methods? > > No, these are modified and passed to functions that modify them. just meant jpeg_decompress_struct* cinfo() const { ... }, etc. Not important. https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:354: SkBitmap::Config SkJPEGImageDecoder::getBitmapConfig(jpeg_decompress_struct* cinfo) { On 2013/08/06 15:06:27, scroggo wrote: > On 2013/08/05 22:31:09, mtklein wrote: > > can we make this guy take a const&? > > The Skia style guide states to use a pointer when you're going to modify an > object/struct. Right. It looks to me like it's only being read here?
https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... File src/images/SkImageDecoder_libjpeg.cpp (right): https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:124: jpeg_decompress_struct* cinfo() { return &fCInfo; } On 2013/08/06 15:22:48, mtklein wrote: > On 2013/08/06 15:06:27, scroggo wrote: > > On 2013/08/05 22:31:09, mtklein wrote: > > > these two guys can be const methods? > > > > No, these are modified and passed to functions that modify them. > > just meant > > jpeg_decompress_struct* cinfo() const { ... }, etc. > > Not important. That gives me the following error: ../../../src/images/SkImageDecoder_libjpeg.cpp:132:53: error: invalid conversion from 'const jpeg_decompress_struct*' to 'jpeg_decompress_struct*' [-fpermissive] https://codereview.chromium.org/22290002/diff/1/src/images/SkImageDecoder_lib... src/images/SkImageDecoder_libjpeg.cpp:354: SkBitmap::Config SkJPEGImageDecoder::getBitmapConfig(jpeg_decompress_struct* cinfo) { On 2013/08/06 15:22:48, mtklein wrote: > On 2013/08/06 15:06:27, scroggo wrote: > > On 2013/08/05 22:31:09, mtklein wrote: > > > can we make this guy take a const&? > > > > The Skia style guide states to use a pointer when you're going to modify an > > object/struct. > > Right. It looks to me like it's only being read here? No, the second switch statement will modify it.
gotcha. lgtm
Message was sent while issue was closed.
Committed patchset #2 manually as r10629 (presubmit successful). |
