Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1076923002:
SkJpegCodec (Closed)
DescriptionSkJpegCodec
Enables basic decoding for jpegs
Includes rewinding
565, YUV, and Jpeg encoding are not yet implemented
BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/e16b04aa6041efb6507546547737e9603fa1606e
Patch Set 1 : SkJpegCodec #
Total comments: 69
Patch Set 2 : JpegAutoClean is easier to use, Scaling is tested #
Total comments: 37
Patch Set 3 : Introduction of DecoderMgr #
Total comments: 16
Patch Set 4 : Fixes from last set #Patch Set 5 : SwizzlerTest fix #
Total comments: 2
Patch Set 6 : Added comment #
Messages
Total messages: 22 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
msarett@google.com changed reviewers: + scroggo@google.com
It's finally here!
https://codereview.chromium.org/1076923002/diff/80001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/dm/DM.cpp#newcode203 dm/DM.cpp:203: // With fall through we test jpg decodes to kIndex8, which are disallowed. Won't we just return Error::Nonfatal? https://codereview.chromium.org/1076923002/diff/80001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/dm/DMSrcSink.cpp#newcode83 dm/DMSrcSink.cpp:83: if (kIndex_8_SkColorType != decodeInfo.colorType()) { I'd prefer just setting decodeInfo to use kIndex8, and then returning Error::Nonfatal when we get a kInvalidConversion. Alternatively, it seems like kIndex8_Always_DstColorType and kGrayscale_Always_DstColorType could be merged into kGetFromInfo_DstColorType. (These Nonfatal cases should never happen, since we'll only ever use an Always if the color type is gray or index 8, right?) https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... File src/codec/SkJpegAutoClean.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... src/codec/SkJpegAutoClean.cpp:17: free(); nit: this->free() https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... src/codec/SkJpegAutoClean.cpp:24: jpeg_decompress_struct* JpegAutoClean::get() { Down below I think I suggest that you do not call get() so much, but now I realized that you're using a custom class. You could implement operator* and operator-> so you do not need to call get(), but as written, you do need to call get(). https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... File src/codec/SkJpegAutoClean.h (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... src/codec/SkJpegAutoClean.h:12: #include <stdio.h> We should not be using stdio. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... src/codec/SkJpegAutoClean.h:13: extern "C" { For our other codec headers, we've discovered that the headers themselves use use extern C, so this ended up not being necessary. Not sure whether that is the case here. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... src/codec/SkJpegAutoClean.h:20: class JpegAutoClean { This should inherit from SkNoncopyable. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... src/codec/SkJpegAutoClean.h:26: JpegAutoClean(jpeg_decompress_struct* jpegInfo, bool init); nit: Traditionally, jpeg_decompress_struct is named cinfo. At least, I thought so - looking in the Chromium code base, it looks like sometimes it is called dinfo, ci, and info... https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:32: * Dummy error reporting functions for when the client wants to suppress messages Instead of using these, use SkCodecPrintf. Then they can be turned on/off globally with the rest of the codec output. Ok, now I see that you check SK_PRINT_CODEC_MESSAGES to decide whether to use this. That seems fine. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:48: // TODO: (msarett) This matches SkImageDecoder. Why were these values chosen? Good question. I am not sure why these values were chosen, or whether they matter any more (I think even the lowest end devices today have more memory than our original low memory devices). What's worse, using the smaller value apparently causes skbug.com/1282 (though I am not sure why). https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:82: * Note that this method moves the row pointer in its processing. Could you specify what width means? It appears that width is the number of four byte pixels in row? Also, it looks like this code came from SkImageDecoder_libjpeg.cpp. Any reason not to include the comments from the analogous function there? https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:101: // reduce the visual quality. In practice it's hard to see a difference. It does. See https://codereview.chromium.org/973563002/#msg14 Go ahead and leave these optimizations out. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:113: static void set_dct_method(jpeg_decompress_struct* jpegInfo) { Not sure this needs to be its own method, given that it is only used once and it is so small. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:140: case JCS_RGB: Why did you make this explicit only to fall through to the default? https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:149: // Will we want to dither when we enable decodes to 565? No. We do not want to do dithering. If the image would require dithering to get 565, we will not support 565. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:162: } else if (3 == jpegInfo->out_color_components && JCS_RGB == jpegInfo->out_color_space) { nit: these do not need to be else statements, since each one returns. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:194: srcMgr->fBuffer = buffer.get(); This seems dangerous. Why did you not make fBuffer fully owned by srcMgr, like in the old code? https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:215: jpegInfo.setInit(true); setInit seems like an odd function to me. It does not change any state of the jpeg_decompress_struct; it just changes whether we do something later. It seems cleaner to have a function which calls jpeg_create_decompress, and then sets fInit to true. We could take this further, and make the JpegAutoClean hold everything (source_manager, error_manager,...) instead of passing ownership around. We can then put the whole JpegAutoClean into an SkAutoTDelete. When we end up creating the codec, we'll detach it and pass to the codec. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:275: if (desiredScale > 0.75) { nit: .75f https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:286: // This code is risky because it may become incorrect if libjpeg changes how it calculates I tend to think libjpeg will not change how it makes this calculation, but I *do* think it would be better to use jpeg_calc_output_dimensions. What if we change scale_denom, and then change it back? Also, please add a test that checks that if we attempt to decode with different scales that the desired dimensions are supported. The test should not enforce the heuristic about which scale to use depending on desiredScale (since it is somewhat arbitrary - it may be right to split in the middle, but maybe another heuristic would be better?); only that the dimensions we suggested are supported. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:355: SkASSERT(1 == fJpegInfo.get()->scale_num); If you implement operator->, you can just use fJpegInfo->scale_num https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:364: if (8 == fJpegInfo.get()->scale_denom) { It seems like we could also early exit (as a failure) if dstWidth > output_width || dstHeight > output_height. That means potentially extra checks, and potentially less checks, so not sure the best way to do it. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:380: SkSwizzler::SrcConfig srcConfig = get_src_config(fJpegInfo.get()); I think this and the following several local variables can be const? https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:391: uint32_t rowsPerDecode = fJpegInfo.get()->rec_outbuf_height; We should also implement scanline decoding, although we can leave that to another CL. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:411: if (rowsDecoded < rowsPerDecode && y + rowsDecoded < dstHeight) { extra space here between && and y? https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:423: SkSwizzler::Fill(dst, dstInfo, dstRowBytes, y + rowsDecoded, SK_ColorWHITE, NULL); Don't we use a different color for other images? I tend to think we should choose the same everywhere. Fill does not accept kGray, correct? https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:426: fJpegInfo.get()->output_scanline = dstHeight; What does this do? https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:432: if (JCS_CMYK == fJpegInfo.get()->out_color_space) { Isn't this code the same whether we reached the end prematurely or not? Can we instead always do this first, then check if we're incomplete? https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegUtility... File src/codec/SkJpegUtility.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegUtility... src/codec/SkJpegUtility.cpp:21: if (!src->fStream->rewind()) { We should not need to rewind here? https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegUtility... src/codec/SkJpegUtility.cpp:31: static boolean sk_seek_input_data(j_decompress_ptr jpegInfo, long byteOffset) { This function (and the rest of the SK_BUILD_FOR_ANDROID code in this file is for tile based decoding, which we are not intending to support. You can leave it out. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:137: memcpy(dstRow, src, width * bytesPerPixel); bytesPerPixel will always be 1 here, right?
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
https://codereview.chromium.org/1076923002/diff/80001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/dm/DM.cpp#newcode203 dm/DM.cpp:203: // With fall through we test jpg decodes to kIndex8, which are disallowed. On 2015/04/10 17:19:05, scroggo wrote: > Won't we just return Error::Nonfatal? This code from my last CL was actually buggy. It would decode to kGray in both the kGrayscale_Always case and the kIndex_Always case. Except in the kIndex_Always case it would *say* that it decoded to kIndex. I didn't catch this until I ran jpegs through it because I knew jpegs couldn't possibly be decoded to kIndex. My fix was to disallow fall through and add checks to DMSrcSink. That was probably overkill. All I need to do, as you mentioned, is to keep the fall through and set the color type to kIndex in the kIndexAlways case etc. Then we will just get the Error:Nonfatal in conversion possible. https://codereview.chromium.org/1076923002/diff/80001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/dm/DMSrcSink.cpp#newcode83 dm/DMSrcSink.cpp:83: if (kIndex_8_SkColorType != decodeInfo.colorType()) { On 2015/04/10 17:19:05, scroggo wrote: > I'd prefer just setting decodeInfo to use kIndex8, and then returning > Error::Nonfatal when we get a kInvalidConversion. > > Alternatively, it seems like kIndex8_Always_DstColorType and > kGrayscale_Always_DstColorType could be merged into kGetFromInfo_DstColorType. > (These Nonfatal cases should never happen, since we'll only ever use an Always > if the color type is gray or index 8, right?) Completely agree. See my comment in DM.cpp. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... File src/codec/SkJpegAutoClean.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... src/codec/SkJpegAutoClean.cpp:17: free(); On 2015/04/10 17:19:05, scroggo wrote: > nit: this->free() Done. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... src/codec/SkJpegAutoClean.cpp:24: jpeg_decompress_struct* JpegAutoClean::get() { On 2015/04/10 17:19:05, scroggo wrote: > Down below I think I suggest that you do not call get() so much, but now I > realized that you're using a custom class. > > You could implement operator* and operator-> so you do not need to call get(), > but as written, you do need to call get(). I added three operators and removed about one million calls to get(). I think this makes things look nicer! https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... File src/codec/SkJpegAutoClean.h (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... src/codec/SkJpegAutoClean.h:12: #include <stdio.h> On 2015/04/10 17:19:05, scroggo wrote: > We should not be using stdio. jpeglib has some sort of odd dependence on stdio. SkImageDecoder_libjpeg uses this include as well. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... src/codec/SkJpegAutoClean.h:13: extern "C" { On 2015/04/10 17:19:05, scroggo wrote: > For our other codec headers, we've discovered that the headers themselves use > use extern C, so this ended up not being necessary. Not sure whether that is the > case here. Looks like that is the case here. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... src/codec/SkJpegAutoClean.h:20: class JpegAutoClean { On 2015/04/10 17:19:05, scroggo wrote: > This should inherit from SkNoncopyable. Done. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegAutoCle... src/codec/SkJpegAutoClean.h:26: JpegAutoClean(jpeg_decompress_struct* jpegInfo, bool init); On 2015/04/10 17:19:05, scroggo wrote: > nit: Traditionally, jpeg_decompress_struct is named cinfo. At least, I thought > so - looking in the Chromium code base, it looks like sometimes it is called > dinfo, ci, and info... Maybe for "compress info" and "decompress info"? I changed the name because cinfo didn't really mean anything to me. I think I'll go with dinfo. Happy to tweak this again if my guess about the names is off the mark. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:32: * Dummy error reporting functions for when the client wants to suppress messages On 2015/04/10 17:19:06, scroggo wrote: > Instead of using these, use SkCodecPrintf. Then they can be turned on/off > globally with the rest of the codec output. > > Ok, now I see that you check SK_PRINT_CODEC_MESSAGES to decide whether to use > this. That seems fine. This actually isn't fine. When SK_PRINT_CODEC_MESSAGES is defined, compiling fails because these methods are unused. I removed this code. I have yet to see jpeglib print any messages, so for now, I think are good without this. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:48: // TODO: (msarett) This matches SkImageDecoder. Why were these values chosen? On 2015/04/10 17:19:06, scroggo wrote: > Good question. I am not sure why these values were chosen, or whether they > matter any more (I think even the lowest end devices today have more memory than > our original low memory devices). > > What's worse, using the smaller value apparently causes skbug.com/1282 (though I > am not sure why). I'm going to investigate this bug further after submitting my fixes. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:82: * Note that this method moves the row pointer in its processing. On 2015/04/10 17:19:05, scroggo wrote: > Could you specify what width means? > > It appears that width is the number of four byte pixels in row? > > Also, it looks like this code came from SkImageDecoder_libjpeg.cpp. Any reason > not to include the comments from the analogous function there? I meant to have a TODO here. I removed the comments because AFAICT the comments did not match what the code was doing. On second look, it turns out that they make some sense. I think I understand this well enough now to document what is going on. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:101: // reduce the visual quality. In practice it's hard to see a difference. On 2015/04/10 17:19:06, scroggo wrote: > It does. See https://codereview.chromium.org/973563002/#msg14 > > Go ahead and leave these optimizations out. Done. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:113: static void set_dct_method(jpeg_decompress_struct* jpegInfo) { On 2015/04/10 17:19:06, scroggo wrote: > Not sure this needs to be its own method, given that it is only used once and it > is so small. Done. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:140: case JCS_RGB: On 2015/04/10 17:19:06, scroggo wrote: > Why did you make this explicit only to fall through to the default? You're right this is unnecessary. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:149: // Will we want to dither when we enable decodes to 565? On 2015/04/10 17:19:06, scroggo wrote: > No. We do not want to do dithering. If the image would require dithering to get > 565, we will not support 565. Hmm ok. Looking at libjpeg, it appears that the default dither mode is Floyd-Steinberg dithering. So to avoid doing any dithering at all, we would have to set dither_mode to JDITHER_NONE for all cases. To match the behavior of SkImageDecoder, we would set dither_mode to JDITHER_NONE for the 8888 case, JDITHER_ORDERED for 565 (simpler dithering algorithm than FS), and leave it as default for all others. Let me know what you think it best. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:162: } else if (3 == jpegInfo->out_color_components && JCS_RGB == jpegInfo->out_color_space) { On 2015/04/10 17:19:06, scroggo wrote: > nit: these do not need to be else statements, since each one returns. Done. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:194: srcMgr->fBuffer = buffer.get(); On 2015/04/10 17:19:05, scroggo wrote: > This seems dangerous. Why did you not make fBuffer fully owned by srcMgr, like > in the old code? The old code allocated fBuffer on the stack, which we cannot do because it is used in both NewFromStream and onGetPixels. I think using an AutoTDelete inside the struct is far superior to what I did. I guess I just didn't know this was possible in C++. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:215: jpegInfo.setInit(true); On 2015/04/10 17:19:06, scroggo wrote: > setInit seems like an odd function to me. It does not change any state of the > jpeg_decompress_struct; it just changes whether we do something later. It seems > cleaner to have a function which calls jpeg_create_decompress, and then sets > fInit to true. > > We could take this further, and make the JpegAutoClean hold everything > (source_manager, error_manager,...) instead of passing ownership around. We can > then put the whole JpegAutoClean into an SkAutoTDelete. When we end up creating > the codec, we'll detach it and pass to the codec. Let's take it further. I think the new version is what you suggested and is better. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:275: if (desiredScale > 0.75) { On 2015/04/10 17:19:06, scroggo wrote: > nit: .75f Done. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:286: // This code is risky because it may become incorrect if libjpeg changes how it calculates On 2015/04/10 17:19:06, scroggo wrote: > I tend to think libjpeg will not change how it makes this calculation, but I > *do* think it would be better to use jpeg_calc_output_dimensions. > > What if we change scale_denom, and then change it back? > > Also, please add a test that checks that if we attempt to decode with different > scales that the desired dimensions are supported. > > The test should not enforce the heuristic about which scale to use depending on > desiredScale (since it is somewhat arbitrary - it may be right to split in the > middle, but maybe another heuristic would be better?); only that the dimensions > we suggested are supported. The compiler complains if I change scale_denom at all since it if part of fJpegInfo, which is a field. One option would possibly be to make a copy of fJpegInfo and call jpeg_calc_output_dimensions on the copy. I considered this option, but I tend to think the current approach is better. I have added the test you suggested. On the scaling heuristic. It's worth noting that I chose a different heuristic than libjpeg does internally. You can give libjpeg any fraction (scale_num, scale_denom), and if the fraction is less than or equal to 1/8 you get 1/8, <= 1/4 you get 1/4 etc. I debated whether to match this or choose another one. I guess as you say it is somewhat arbitrary and easy to change. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:355: SkASSERT(1 == fJpegInfo.get()->scale_num); On 2015/04/10 17:19:06, scroggo wrote: > If you implement operator->, you can just use > > fJpegInfo->scale_num Done. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:364: if (8 == fJpegInfo.get()->scale_denom) { On 2015/04/10 17:19:06, scroggo wrote: > It seems like we could also early exit (as a failure) if dstWidth > output_width > || dstHeight > output_height. > > That means potentially extra checks, and potentially less checks, so not sure > the best way to do it. Yeah this is something I thought about and ultimately decided not to implement. It helps us fail quicker on bad inputs but adds checks to potentially valid requests. Given that you thought about it as well, I will add. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:380: SkSwizzler::SrcConfig srcConfig = get_src_config(fJpegInfo.get()); On 2015/04/10 17:19:06, scroggo wrote: > I think this and the following several local variables can be const? Done. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:391: uint32_t rowsPerDecode = fJpegInfo.get()->rec_outbuf_height; On 2015/04/10 17:19:06, scroggo wrote: > We should also implement scanline decoding, although we can leave that to > another CL. Will do. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:411: if (rowsDecoded < rowsPerDecode && y + rowsDecoded < dstHeight) { On 2015/04/10 17:19:06, scroggo wrote: > extra space here between && and y? Done. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:423: SkSwizzler::Fill(dst, dstInfo, dstRowBytes, y + rowsDecoded, SK_ColorWHITE, NULL); On 2015/04/10 17:19:06, scroggo wrote: > Don't we use a different color for other images? I tend to think we should > choose the same everywhere. > > Fill does not accept kGray, correct? We will consistently fill all opaque images with black. I added support for kGray to Fill(). I think it makes sense to always fill with black here as well. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:426: fJpegInfo.get()->output_scanline = dstHeight; On 2015/04/10 17:19:06, scroggo wrote: > What does this do? This suppresses a warning message from libjpeg. "Application transferred too few scanlines" We will report the warning anyway if SK_PRINT_CODEC_MESSAGES is defined. I am adding a comment. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:432: if (JCS_CMYK == fJpegInfo.get()->out_color_space) { On 2015/04/10 17:19:06, scroggo wrote: > Isn't this code the same whether we reached the end prematurely or not? > > Can we instead always do this first, then check if we're incomplete? Done. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegUtility... File src/codec/SkJpegUtility.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegUtility... src/codec/SkJpegUtility.cpp:21: if (!src->fStream->rewind()) { On 2015/04/10 17:19:07, scroggo wrote: > We should not need to rewind here? Yes agreed, this is not necessary here. This was adapted from SkImageDecoder. I'm not really sure why it was necessary there either. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegUtility... src/codec/SkJpegUtility.cpp:31: static boolean sk_seek_input_data(j_decompress_ptr jpegInfo, long byteOffset) { On 2015/04/10 17:19:07, scroggo wrote: > This function (and the rest of the SK_BUILD_FOR_ANDROID code in this file is for > tile based decoding, which we are not intending to support. You can leave it > out. Done. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkSwizzler.cp... src/codec/SkSwizzler.cpp:137: memcpy(dstRow, src, width * bytesPerPixel); On 2015/04/10 17:19:07, scroggo wrote: > bytesPerPixel will always be 1 here, right? Yes. There is no need for this multiplication. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:229: #ifdef DCT_IFAST_SUPPORTED Do we want to use this? DCT_IFAST is faster but less accurate. AFAICT this is similar to the scaling define, where it seems like it should be defined but it is always not defined. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:278: SkISize SkJpegCodec::onGetScaledDimensions(float desiredScale) const { This function is not in a finished state, but I didn't want to put any more time into it until we discuss it further. (1) I no longer for check for IDCT_SCALE_SUPPORTED. I included an additional file which made IDCT_SCALE_SUPPORTED always defined instead of always undefined. And then I realized that if it will always be defined, there is no point in checking. (2) I don't think I can use jcalc_output_dimensions(). The problem is that jpeglib will error exit if jcalc_output_dimensions() is called before fDecodeMgr is reset. So if we try to call getScaledDimensions() after calling getPixels() it will fail (the new test exposes this). We also cannot call rewindIfNeeded() here because of the const-ness of this function. Regardless of if we were to figure out a way to rewind in this function, I feel that calling this function should not require rewinding. I think the best solution is to use jdiv_round_up(). (3) I have no idea why the linker thinks that jdiv_round_up is declared but not defined. The identical helper function that I have created is a temporary workaround. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:502: memset(dstStartRow, 0xFF, bytesToFill); I'm not exactly sure what the best decision is here. I chose to always fill with black for kGray images. Another possible choice would be to take the low 8 bits of colorOrIndex and fill with that. It would be a little more confusing but also more flexible. It works equally well for jpeg where we want to fill with black and will pass in SK_ColorBLACK as the colorOrIndex parameter.
https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:32: * Dummy error reporting functions for when the client wants to suppress messages On 2015/04/13 20:54:05, msarett wrote: > On 2015/04/10 17:19:06, scroggo wrote: > > Instead of using these, use SkCodecPrintf. Then they can be turned on/off > > globally with the rest of the codec output. > > > > Ok, now I see that you check SK_PRINT_CODEC_MESSAGES to decide whether to use > > this. That seems fine. > > This actually isn't fine. When SK_PRINT_CODEC_MESSAGES is defined, compiling > fails because these methods are unused. I removed this code. I have yet to see > jpeglib print any messages, so for now, I think are good without this. No, jpeglib will print messages. Just always use the same function, which will call SkCodecPrintf, which will do nothing if SK_PRINT_CODEC_MESSAGES is not defined. Alternatively, you could surround these functions with: #ifndef SK_PRINT_CODEC_MESSAGES ... #endif https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:149: // Will we want to dither when we enable decodes to 565? On 2015/04/13 20:54:05, msarett wrote: > On 2015/04/10 17:19:06, scroggo wrote: > > No. We do not want to do dithering. If the image would require dithering to > get > > 565, we will not support 565. > > Hmm ok. Looking at libjpeg, it appears that the default dither mode is > Floyd-Steinberg dithering. So to avoid doing any dithering at all, we would > have to set dither_mode to JDITHER_NONE for all cases. To match the behavior of > SkImageDecoder, we would set dither_mode to JDITHER_NONE for the 8888 case, > JDITHER_ORDERED for 565 (simpler dithering algorithm than FS), and leave it as > default for all others. > > Let me know what you think it best. Oh, I meant we would not do dithering after the fact. I do not think we want to support 565, but I'm not sure about what to do for 8888. What does Chromium do, and what is the default for 8888? https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:194: srcMgr->fBuffer = buffer.get(); On 2015/04/13 20:54:04, msarett wrote: > On 2015/04/10 17:19:05, scroggo wrote: > > This seems dangerous. Why did you not make fBuffer fully owned by srcMgr, like > > in the old code? > > The old code allocated fBuffer on the stack, which we cannot do because it is > used in both NewFromStream and onGetPixels. > > I think using an AutoTDelete inside the struct is far superior to what I did. I > guess I just didn't know this was possible in C++. Let's discuss this further in person, so I can better understand what you did not think possible, and hopefully I can help you understand. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:286: // This code is risky because it may become incorrect if libjpeg changes how it calculates On 2015/04/13 20:54:05, msarett wrote: > On 2015/04/10 17:19:06, scroggo wrote: > > I tend to think libjpeg will not change how it makes this calculation, but I > > *do* think it would be better to use jpeg_calc_output_dimensions. > > > > What if we change scale_denom, and then change it back? > > > > Also, please add a test that checks that if we attempt to decode with > different > > scales that the desired dimensions are supported. > > > > The test should not enforce the heuristic about which scale to use depending > on > > desiredScale (since it is somewhat arbitrary - it may be right to split in the > > middle, but maybe another heuristic would be better?); only that the > dimensions > > we suggested are supported. > > The compiler complains if I change scale_denom at all since it if part of > fJpegInfo, which is a field. One option would possibly be to make a copy of > fJpegInfo and call jpeg_calc_output_dimensions on the copy. I considered this > option, but I tend to think the current approach is better. > > I have added the test you suggested. > > On the scaling heuristic. It's worth noting that I chose a different heuristic > than libjpeg does internally. You can give libjpeg any fraction (scale_num, > scale_denom), and if the fraction is less than or equal to 1/8 you get 1/8, <= > 1/4 you get 1/4 etc. I debated whether to match this or choose another one. I > guess as you say it is somewhat arbitrary and easy to change. Yeah, it might just depend on what the client wants. I was initially thinking we should do something along the lines of libjpeg, so we do not scale down extra and then scale back up. On the other hand, you've implemented something that picks the closest, even if it means scaling down more than necessary, meaning we do the least amount of (post) scaling possible. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:423: SkSwizzler::Fill(dst, dstInfo, dstRowBytes, y + rowsDecoded, SK_ColorWHITE, NULL); On 2015/04/13 20:54:05, msarett wrote: > On 2015/04/10 17:19:06, scroggo wrote: > > Don't we use a different color for other images? I tend to think we should > > choose the same everywhere. > > > > Fill does not accept kGray, correct? > > We will consistently fill all opaque images with black. > > I added support for kGray to Fill(). I think it makes sense to always fill with > black here as well. If we want to enforce using the same values everywhere, we could add a static function which chooses the fill color based on the SkImageInfo. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:616: return decodeMask(dstInfo, dst, dstRowBytes, opts); nit: Rebasing between uploads can make it harder to tell what has changed between revisions. (This line changed in a different CL.) If I need to rebase, I'll often upload the rebase without any other changes, to isolate the changes that were made by me for this code review. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:1150: if (kNo_ZeroInitialized == opts.fZeroInitialized) { Since fillColorOrIndex may not be 0, being zero initialized does not necessarily mean we can skip this. We either need to do it all the time, or we need to make extra checks to know whether we can skip (maybe included in the switch statement above). https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegAutoCl... File src/codec/SkJpegAutoClean.h (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegAutoCl... src/codec/SkJpegAutoClean.h:25: * Takes ownership of all thre input pointers. three* https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegAutoCl... src/codec/SkJpegAutoClean.h:33: * Owned pointers will be delted automatically. deleted* https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegAutoCl... src/codec/SkJpegAutoClean.h:44: SkAutoTDelete<jpeg_decompress_struct> fDInfo; Do these all need to be passed in? What if you make them full members, instead of using SkAutoTDelete? (Not sure what the right term is, but as if you were stack allocating them, except this is not a stack frame so they will be allocated along with SkJpegAutoClean). https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:19: #define IDCT_SCALING_SUPPORTED Is this necessary? Does it need to be defined before we include these other files? Otherwise, please place this after. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:21: #include <stdio.h> Again, please keep these separate from the Skia includes. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:184: return SkSwizzler::kUnknown; I'm assuming CreateSwizzler will fail for kUnknown? https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:197: jpeg_decompress_struct* dinfo = SkNEW(jpeg_decompress_struct); Here is what I would do. Make dinfo a member of JpegAutoClean. Here, you'll have: SkAutoTDelete<JpegAutoClean> jpegAutoClean(SkNEW(JpegAutoClean); It might be better named JpegDecoderMgr or something, since we're no longer using it as an autodeleter. You'll always use the difno from this object, rather than passing it to this object. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:229: #ifdef DCT_IFAST_SUPPORTED On 2015/04/13 20:54:06, msarett wrote: > Do we want to use this? > > DCT_IFAST is faster but less accurate. AFAICT this is similar to the scaling > define, where it seems like it should be defined but it is always not defined. I've been following a bug where people are complaining that IFAST is uglier. I'll send you the bug once I find it, but I think we do *not* want to use it. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:278: SkISize SkJpegCodec::onGetScaledDimensions(float desiredScale) const { On 2015/04/13 20:54:06, msarett wrote: > This function is not in a finished state, but I didn't want to put any more time > into it until we discuss it further. > > (1) I no longer for check for IDCT_SCALE_SUPPORTED. I included an additional > file which made IDCT_SCALE_SUPPORTED always defined instead of always undefined. > And then I realized that if it will always be defined, there is no point in > checking. It looks like you defined it yourself at the top of the file? > > (2) I don't think I can use jcalc_output_dimensions(). The problem is that > jpeglib will error exit if jcalc_output_dimensions() is called before fDecodeMgr > is reset. So if we try to call getScaledDimensions() after calling getPixels() > it will fail (the new test exposes this). We also cannot call rewindIfNeeded() > here because of the const-ness of this function. Regardless of if we were to > figure out a way to rewind in this function, I feel that calling this function > should not require rewinding. I think the best solution is to use > jdiv_round_up(). Agreed that we should not have to rewind here. I'm not clear on how we ever get into a situation where we error exit. const_cast allows you to modify const variables, if you needed to modify the scale_denom. > > (3) I have no idea why the linker thinks that jdiv_round_up is declared but not > defined. The identical helper function that I have created is a temporary > workaround. Me neither. I'd like to better understand why we cannot use cal_output_dim before investigating this further. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.h File src/codec/SkJpegCodec.h (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.h:16: #include "jpeglib.h" nit: We might keep this separate from the other includes (i.e. add a newline before it), since it is from an external library. Not sure if we're consistent about that, though. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.h:87: SkJpegCodec(const SkImageInfo& srcInfo, SkStream* stream, JpegAutoClean* decodeMgr); @param decodeMgr Holds jpeg_decompress_struct etc. Takes ownership. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegUtility.h File src/codec/SkJpegUtility.h (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegUtilit... src/codec/SkJpegUtility.h:45: SkAutoTDeleteArray<uint8_t> fBuffer; This can just be: uint8_t fBuffer[kBufferSize]; https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:502: memset(dstStartRow, 0xFF, bytesToFill); On 2015/04/13 20:54:06, msarett wrote: > I'm not exactly sure what the best decision is here. I chose to always fill > with black for kGray images. > > Another possible choice would be to take the low 8 bits of colorOrIndex and fill > with that. It would be a little more confusing but also more flexible. It > works equally well for jpeg where we want to fill with black and will pass in > SK_ColorBLACK as the colorOrIndex parameter. I am not familiar with kGray, but is 0xFF black? I would guess that is white?
I'll be interested in what you think about the division of responsibility between JpegDecoderMgr and SkJpegCodec. I had originally thought to completely encapsulate dinfo inside the Mgr, but that led to a bunch of one line wrapper functions for libjpeg functions. I'm also not sure if you were against something like this at the top of onGetPixels: jpeg_decompress_struct* dinfo = fDecoderMgr->dinfo(); It seems like it might clear up the code considering how often dinfo is used. Finally, I need to take a closer look at all of the remaining jpeg #defines. I haven't yet figured out if any of them are ever defined. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:149: // Will we want to dither when we enable decodes to 565? On 2015/04/14 13:10:32, scroggo wrote: > On 2015/04/13 20:54:05, msarett wrote: > > On 2015/04/10 17:19:06, scroggo wrote: > > > No. We do not want to do dithering. If the image would require dithering to > > get > > > 565, we will not support 565. > > > > Hmm ok. Looking at libjpeg, it appears that the default dither mode is > > Floyd-Steinberg dithering. So to avoid doing any dithering at all, we would > > have to set dither_mode to JDITHER_NONE for all cases. To match the behavior > of > > SkImageDecoder, we would set dither_mode to JDITHER_NONE for the 8888 case, > > JDITHER_ORDERED for 565 (simpler dithering algorithm than FS), and leave it as > > default for all others. > > > > Let me know what you think it best. > > Oh, I meant we would not do dithering after the fact. > > I do not think we want to support 565, but I'm not sure about what to do for > 8888. What does Chromium do, and what is the default for 8888? The default dithering for all destinations is Floyd-Steinberg. SkImageDecoder turns off dithering for 8888. SkImageDecoder uses ordered dithering (faster than FS) for 565. SkImageDecoder uses the default for other cases (non ANDORID_RGB). We always use FS dithering and chromium also always uses FS. I think this is a good choice? https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkCodec_libb... File src/codec/SkCodec_libbmp.cpp (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:616: return decodeMask(dstInfo, dst, dstRowBytes, opts); On 2015/04/14 13:10:32, scroggo wrote: > nit: Rebasing between uploads can make it harder to tell what has changed > between revisions. (This line changed in a different CL.) > > If I need to rebase, I'll often upload the rebase without any other changes, to > isolate the changes that were made by me for this code review. Sorry about that. Good to know. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkCodec_libb... src/codec/SkCodec_libbmp.cpp:1150: if (kNo_ZeroInitialized == opts.fZeroInitialized) { On 2015/04/14 13:10:32, scroggo wrote: > Since fillColorOrIndex may not be 0, being zero initialized does not necessarily > mean we can skip this. > > We either need to do it all the time, or we need to make extra checks to know > whether we can skip (maybe included in the switch statement above). Yes you are right! https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegAutoCl... File src/codec/SkJpegAutoClean.h (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegAutoCl... src/codec/SkJpegAutoClean.h:25: * Takes ownership of all thre input pointers. On 2015/04/14 13:10:32, scroggo wrote: > three* Acknowledged. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegAutoCl... src/codec/SkJpegAutoClean.h:33: * Owned pointers will be delted automatically. On 2015/04/14 13:10:32, scroggo wrote: > deleted* Acknowledged. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegAutoCl... src/codec/SkJpegAutoClean.h:44: SkAutoTDelete<jpeg_decompress_struct> fDInfo; On 2015/04/14 13:10:32, scroggo wrote: > Do these all need to be passed in? What if you make them full members, instead > of using SkAutoTDelete? > > (Not sure what the right term is, but as if you were stack allocating them, > except this is not a stack frame so they will be allocated along with > SkJpegAutoClean). Done. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:19: #define IDCT_SCALING_SUPPORTED On 2015/04/14 13:10:33, scroggo wrote: > Is this necessary? > > Does it need to be defined before we include these other files? Otherwise, > please place this after. This is a mistake. I meant to delete this. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:21: #include <stdio.h> On 2015/04/14 13:10:33, scroggo wrote: > Again, please keep these separate from the Skia includes. Done. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:184: return SkSwizzler::kUnknown; On 2015/04/14 13:10:32, scroggo wrote: > I'm assuming CreateSwizzler will fail for kUnknown? Yes it will return NULL. And we do a NULL check on the return value from CreateSwizzler. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:197: jpeg_decompress_struct* dinfo = SkNEW(jpeg_decompress_struct); On 2015/04/14 13:10:32, scroggo wrote: > Here is what I would do. Make dinfo a member of JpegAutoClean. Here, you'll > have: > > SkAutoTDelete<JpegAutoClean> jpegAutoClean(SkNEW(JpegAutoClean); > > It might be better named JpegDecoderMgr or something, since we're no longer > using it as an autodeleter. > > You'll always use the difno from this object, rather than passing it to this > object. Done. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:229: #ifdef DCT_IFAST_SUPPORTED On 2015/04/14 13:10:33, scroggo wrote: > On 2015/04/13 20:54:06, msarett wrote: > > Do we want to use this? > > > > DCT_IFAST is faster but less accurate. AFAICT this is similar to the scaling > > define, where it seems like it should be defined but it is always not defined. > > I've been following a bug where people are complaining that IFAST is uglier. > I'll send you the bug once I find it, but I think we do *not* want to use it. Cool that will simplify things. AFAICT we have the same issue with DCT_IFAST_SUPPORTED where it is not defined when it seems that it should be. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:278: SkISize SkJpegCodec::onGetScaledDimensions(float desiredScale) const { On 2015/04/14 13:10:32, scroggo wrote: > On 2015/04/13 20:54:06, msarett wrote: > > This function is not in a finished state, but I didn't want to put any more > time > > into it until we discuss it further. > > > > (1) I no longer for check for IDCT_SCALE_SUPPORTED. I included an additional > > file which made IDCT_SCALE_SUPPORTED always defined instead of always > undefined. > > And then I realized that if it will always be defined, there is no point in > > checking. > > It looks like you defined it yourself at the top of the file? > > > > > (2) I don't think I can use jcalc_output_dimensions(). The problem is that > > jpeglib will error exit if jcalc_output_dimensions() is called before > fDecodeMgr > > is reset. So if we try to call getScaledDimensions() after calling > getPixels() > > it will fail (the new test exposes this). We also cannot call > rewindIfNeeded() > > here because of the const-ness of this function. Regardless of if we were to > > figure out a way to rewind in this function, I feel that calling this function > > should not require rewinding. I think the best solution is to use > > jdiv_round_up(). > > Agreed that we should not have to rewind here. I'm not clear on how we ever get > into a situation where we error exit. > > const_cast allows you to modify const variables, if you needed to modify the > scale_denom. > > > > > (3) I have no idea why the linker thinks that jdiv_round_up is declared but > not > > defined. The identical helper function that I have created is a temporary > > workaround. > > Me neither. I'd like to better understand why we cannot use cal_output_dim > before investigating this further. > Yeah I forgot to remove the define at the top of the file. I honestly prefer the jdiv approach to the calc_output_dimensions approach. You will notice that I was able to get calc_output_dimensions to work by setting a few extra fields, but it's a bit hacky. If num_components is not 0, it will seg fault when trying to iterate over "components". That said, I'm equally happy to not change it again :). https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.h File src/codec/SkJpegCodec.h (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.h:16: #include "jpeglib.h" On 2015/04/14 13:10:33, scroggo wrote: > nit: We might keep this separate from the other includes (i.e. add a newline > before it), since it is from an external library. > > Not sure if we're consistent about that, though. I will make sure to do that. Also, it turns out we need the "extern C" for it to link properly on android. I found this out with Derek's help trying to reproduce the pink bug. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.h:87: SkJpegCodec(const SkImageInfo& srcInfo, SkStream* stream, JpegAutoClean* decodeMgr); On 2015/04/14 13:10:33, scroggo wrote: > @param decodeMgr Holds jpeg_decompress_struct etc. Takes ownership. Done. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegUtility.h File src/codec/SkJpegUtility.h (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegUtilit... src/codec/SkJpegUtility.h:45: SkAutoTDeleteArray<uint8_t> fBuffer; On 2015/04/14 13:10:33, scroggo wrote: > This can just be: > > uint8_t fBuffer[kBufferSize]; You are right. I have had several misunderstandings on this but this is finally correct. The srcMgr was originally stack allocated which I had to change. Then I erroneously thought that this buffer would also be on the stack if I didn't change how it was allocated. Now that you suggest it, it is quite obvious that the buffer will simply be owned by the source_mgr if it is allocated in this way. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:502: memset(dstStartRow, 0xFF, bytesToFill); On 2015/04/14 13:10:33, scroggo wrote: > On 2015/04/13 20:54:06, msarett wrote: > > I'm not exactly sure what the best decision is here. I chose to always fill > > with black for kGray images. > > > > Another possible choice would be to take the low 8 bits of colorOrIndex and > fill > > with that. It would be a little more confusing but also more flexible. It > > works equally well for jpeg where we want to fill with black and will pass in > > SK_ColorBLACK as the colorOrIndex parameter. > > I am not familiar with kGray, but is 0xFF black? I would guess that is white? Wow yes you are right. This is a nice reminder to create an incomplete gray image to test on.
On 2015/04/14 19:30:37, msarett wrote: > I'll be interested in what you think about the division of responsibility > between JpegDecoderMgr and SkJpegCodec. What you have in patch set 3 (latest, right now) looks like the right thing to do. > I had originally thought to completely > encapsulate dinfo inside the Mgr, but that led to a bunch of one line wrapper > functions for libjpeg functions. I agree that that is not very useful. > > I'm also not sure if you were against something like this at the top of > onGetPixels: > jpeg_decompress_struct* dinfo = fDecoderMgr->dinfo(); > > It seems like it might clear up the code considering how often dinfo is used. Yes, that sounds good. https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1076923002/diff/80001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:149: // Will we want to dither when we enable decodes to 565? On 2015/04/14 19:30:36, msarett wrote: > On 2015/04/14 13:10:32, scroggo wrote: > > On 2015/04/13 20:54:05, msarett wrote: > > > On 2015/04/10 17:19:06, scroggo wrote: > > > > No. We do not want to do dithering. If the image would require dithering > to > > > get > > > > 565, we will not support 565. > > > > > > Hmm ok. Looking at libjpeg, it appears that the default dither mode is > > > Floyd-Steinberg dithering. So to avoid doing any dithering at all, we would > > > have to set dither_mode to JDITHER_NONE for all cases. To match the > behavior > > of > > > SkImageDecoder, we would set dither_mode to JDITHER_NONE for the 8888 case, > > > JDITHER_ORDERED for 565 (simpler dithering algorithm than FS), and leave it > as > > > default for all others. > > > > > > Let me know what you think it best. > > > > Oh, I meant we would not do dithering after the fact. > > > > I do not think we want to support 565, but I'm not sure about what to do for > > 8888. What does Chromium do, and what is the default for 8888? > > The default dithering for all destinations is Floyd-Steinberg. > > SkImageDecoder turns off dithering for 8888. > SkImageDecoder uses ordered dithering (faster than FS) for 565. > SkImageDecoder uses the default for other cases (non ANDORID_RGB). > > We always use FS dithering and chromium also always uses FS. I think this is a > good choice? If Chromium always uses FS, that seems like a good choice. We'll be able to see the difference in Gold, since SkImageDecoder does not use FS. https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:229: #ifdef DCT_IFAST_SUPPORTED On 2015/04/14 19:30:36, msarett wrote: > On 2015/04/14 13:10:33, scroggo wrote: > > On 2015/04/13 20:54:06, msarett wrote: > > > Do we want to use this? > > > > > > DCT_IFAST is faster but less accurate. AFAICT this is similar to the > scaling > > > define, where it seems like it should be defined but it is always not > defined. > > > > I've been following a bug where people are complaining that IFAST is uglier. > > I'll send you the bug once I find it, but I think we do *not* want to use it. > > Cool that will simplify things. AFAICT we have the same issue with > DCT_IFAST_SUPPORTED where it is not defined when it seems that it should be. FYI: The bug I referenced is here: https://code.google.com/p/chromium/issues/detail?id=385515 https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:278: SkISize SkJpegCodec::onGetScaledDimensions(float desiredScale) const { On 2015/04/14 19:30:36, msarett wrote: > On 2015/04/14 13:10:32, scroggo wrote: > > On 2015/04/13 20:54:06, msarett wrote: > > > This function is not in a finished state, but I didn't want to put any more > > time > > > into it until we discuss it further. > > > > > > (1) I no longer for check for IDCT_SCALE_SUPPORTED. I included an > additional > > > file which made IDCT_SCALE_SUPPORTED always defined instead of always > > undefined. > > > And then I realized that if it will always be defined, there is no point in > > > checking. > > > > It looks like you defined it yourself at the top of the file? > > > > > > > > (2) I don't think I can use jcalc_output_dimensions(). The problem is that > > > jpeglib will error exit if jcalc_output_dimensions() is called before > > fDecodeMgr > > > is reset. So if we try to call getScaledDimensions() after calling > > getPixels() > > > it will fail (the new test exposes this). We also cannot call > > rewindIfNeeded() > > > here because of the const-ness of this function. Regardless of if we were > to > > > figure out a way to rewind in this function, I feel that calling this > function > > > should not require rewinding. I think the best solution is to use > > > jdiv_round_up(). > > > > Agreed that we should not have to rewind here. I'm not clear on how we ever > get > > into a situation where we error exit. > > > > const_cast allows you to modify const variables, if you needed to modify the > > scale_denom. > > > > > > > > (3) I have no idea why the linker thinks that jdiv_round_up is declared but > > not > > > defined. The identical helper function that I have created is a temporary > > > workaround. > > > > Me neither. I'd like to better understand why we cannot use cal_output_dim > > before investigating this further. > > > > Yeah I forgot to remove the define at the top of the file. > > I honestly prefer the jdiv approach to the calc_output_dimensions approach. You > will notice that I was able to get calc_output_dimensions to work by setting a > few extra fields, but it's a bit hacky. If num_components is not 0, it will seg > fault when trying to iterate over "components". > > That said, I'm equally happy to not change it again :). It seems like both are a bit hacky: Option 1: Copy the code that calc_output_dimensions uses, and hope that it never changes. Maybe in practice it doesn't. Also, we're in control of the version of libjpeg used by Chrome and Android, so we ought to find out if something changed by running our tests - unless, of course, it works the same for the numbers we test, but not for other sizes. My main concern about things changing is if we're suddenly reporting off by one, when the caller asks for a scaled version, we'll just fail completely (returning kInvalidScale). Option 2: Build a fake dinfo. This seems just as dangerous, in terms of relying on libjpeg internals, but maybe if something about this changes we'll have a more obvious failure (possibly a crash or failure to compile)? https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.h File src/codec/SkJpegCodec.h (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.h:16: #include "jpeglib.h" On 2015/04/14 19:30:36, msarett wrote: > On 2015/04/14 13:10:33, scroggo wrote: > > nit: We might keep this separate from the other includes (i.e. add a newline > > before it), since it is from an external library. > > > > Not sure if we're consistent about that, though. > > I will make sure to do that. Also, it turns out we need the "extern C" for it > to link properly on android. > > I found this out with Derek's help trying to reproduce the pink bug. I wonder if that is because we're stuck on an old version of libjpeg on Android. No big deal though. https://codereview.chromium.org/1076923002/diff/180001/gyp/android_deps.gyp File gyp/android_deps.gyp (right): https://codereview.chromium.org/1076923002/diff/180001/gyp/android_deps.gyp#n... gyp/android_deps.gyp:62: #'external/jpeg', Why this change? https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:35: SkSwizzler::SrcConfig get_src_config(jpeg_decompress_struct* dinfo) { Do we modify dinfo? Could it be const& instead? https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:329: dstHeight - y - rowsDecoded, SK_ColorBLACK, NULL); Maybe add a comment that SK_ColorBLACK is valid to use for kGray as well. https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegDecode... File src/codec/SkJpegDecoderMgr.cpp (right): https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegDecode... src/codec/SkJpegDecoderMgr.cpp:96: jpeg_destroy_decompress(&fDInfo); Don't we only want to do this if init was called? https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegDecode... src/codec/SkJpegDecoderMgr.cpp:103: skjpeg_error_mgr* JpegDecoderMgr::errorMgr() { You appear to only use this to get the jump buffer. I think you could instead just have a method which returns that? https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegUtilit... File src/codec/SkJpegUtility.cpp (right): https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegUtilit... src/codec/SkJpegUtility.cpp:81: * Exit on an error nit: This does not actually exit; instead, it calls longjmp so we can continue execution. https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegUtilit... src/codec/SkJpegUtility.cpp:85: // JpegDecoderMgr should take care of freeing memory JpegDecoderMgr will* take care of freeing memory (I hope?) https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:501: // If the destination is kGray, the caller passes in an 8-bit color Please update the header as well.
https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1076923002/diff/160001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:278: SkISize SkJpegCodec::onGetScaledDimensions(float desiredScale) const { On 2015/04/15 00:31:05, scroggo wrote: > On 2015/04/14 19:30:36, msarett wrote: > > On 2015/04/14 13:10:32, scroggo wrote: > > > On 2015/04/13 20:54:06, msarett wrote: > > > > This function is not in a finished state, but I didn't want to put any > more > > > time > > > > into it until we discuss it further. > > > > > > > > (1) I no longer for check for IDCT_SCALE_SUPPORTED. I included an > > additional > > > > file which made IDCT_SCALE_SUPPORTED always defined instead of always > > > undefined. > > > > And then I realized that if it will always be defined, there is no point > in > > > > checking. > > > > > > It looks like you defined it yourself at the top of the file? > > > > > > > > > > > (2) I don't think I can use jcalc_output_dimensions(). The problem is > that > > > > jpeglib will error exit if jcalc_output_dimensions() is called before > > > fDecodeMgr > > > > is reset. So if we try to call getScaledDimensions() after calling > > > getPixels() > > > > it will fail (the new test exposes this). We also cannot call > > > rewindIfNeeded() > > > > here because of the const-ness of this function. Regardless of if we were > > to > > > > figure out a way to rewind in this function, I feel that calling this > > function > > > > should not require rewinding. I think the best solution is to use > > > > jdiv_round_up(). > > > > > > Agreed that we should not have to rewind here. I'm not clear on how we ever > > get > > > into a situation where we error exit. > > > > > > const_cast allows you to modify const variables, if you needed to modify the > > > scale_denom. > > > > > > > > > > > (3) I have no idea why the linker thinks that jdiv_round_up is declared > but > > > not > > > > defined. The identical helper function that I have created is a temporary > > > > workaround. > > > > > > Me neither. I'd like to better understand why we cannot use cal_output_dim > > > before investigating this further. > > > > > > > Yeah I forgot to remove the define at the top of the file. > > > > I honestly prefer the jdiv approach to the calc_output_dimensions approach. > You > > will notice that I was able to get calc_output_dimensions to work by setting a > > few extra fields, but it's a bit hacky. If num_components is not 0, it will > seg > > fault when trying to iterate over "components". > > > > That said, I'm equally happy to not change it again :). > > It seems like both are a bit hacky: > > Option 1: > Copy the code that calc_output_dimensions uses, and hope that it never changes. > Maybe in practice it doesn't. Also, we're in control of the version of libjpeg > used by Chrome and Android, so we ought to find out if something changed by > running our tests - unless, of course, it works the same for the numbers we > test, but not for other sizes. > > My main concern about things changing is if we're suddenly reporting off by one, > when the caller asks for a scaled version, we'll just fail completely (returning > kInvalidScale). > > Option 2: > Build a fake dinfo. This seems just as dangerous, in terms of relying on libjpeg > internals, but maybe if something about this changes we'll have a more obvious > failure (possibly a crash or failure to compile)? I agree that it is unfortunate that both approaches rely on libjpeg internals. Option 2 sounds good! I would rather debug the more obvious failure. https://codereview.chromium.org/1076923002/diff/180001/gyp/android_deps.gyp File gyp/android_deps.gyp (right): https://codereview.chromium.org/1076923002/diff/180001/gyp/android_deps.gyp#n... gyp/android_deps.gyp:62: #'external/jpeg', On 2015/04/15 00:31:06, scroggo wrote: > Why this change? Sorry this was not intended. It's fixed. https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:35: SkSwizzler::SrcConfig get_src_config(jpeg_decompress_struct* dinfo) { On 2015/04/15 00:31:06, scroggo wrote: > Do we modify dinfo? Could it be const& instead? Done. https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:329: dstHeight - y - rowsDecoded, SK_ColorBLACK, NULL); On 2015/04/15 00:31:06, scroggo wrote: > Maybe add a comment that SK_ColorBLACK is valid to use for kGray as well. Done. https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegDecode... File src/codec/SkJpegDecoderMgr.cpp (right): https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegDecode... src/codec/SkJpegDecoderMgr.cpp:96: jpeg_destroy_decompress(&fDInfo); On 2015/04/15 00:31:06, scroggo wrote: > Don't we only want to do this if init was called? Yes it is unnecessary otherwise! Fixed. https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegDecode... src/codec/SkJpegDecoderMgr.cpp:103: skjpeg_error_mgr* JpegDecoderMgr::errorMgr() { On 2015/04/15 00:31:06, scroggo wrote: > You appear to only use this to get the jump buffer. I think you could instead > just have a method which returns that? I had exactly the same though. I wrote: jmp_buf getJmpBuf() { return fErrorMgr.fJmpBuf; } The compiler complains about this because the function returns an array, so I wrote: jmp_buf* getJmpBuf() { return &fErrorMgr.fJmpBuf; } Then to use it I wrote: *fDecoderMgr->getJmpBuf() Then I didn't like how this looked and I got rid of it. However, given that we had the same thought, I think this is worth doing. The new version returns a reference, which I think is a good solution. https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegUtilit... File src/codec/SkJpegUtility.cpp (right): https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegUtilit... src/codec/SkJpegUtility.cpp:81: * Exit on an error On 2015/04/15 00:31:06, scroggo wrote: > nit: This does not actually exit; instead, it calls longjmp so we can continue > execution. Done. https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkJpegUtilit... src/codec/SkJpegUtility.cpp:85: // JpegDecoderMgr should take care of freeing memory On 2015/04/15 00:31:06, scroggo wrote: > JpegDecoderMgr will* take care of freeing memory > > (I hope?) Me too haha. https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1076923002/diff/180001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:501: // If the destination is kGray, the caller passes in an 8-bit color On 2015/04/15 00:31:06, scroggo wrote: > Please update the header as well. Done.
Forgot to update the SwizzlerTest for the new kGray fill. This update fixes the test so it actually passes.
lgtm https://codereview.chromium.org/1076923002/diff/220001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1076923002/diff/220001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:502: memset(dstStartRow, (uint8_t) colorOrIndex, bytesToFill); Interestingly, we assert for index8 that the input was 8 bit, but here we take advantage of the fact that it wasn't... Maybe worth a comment.
https://codereview.chromium.org/1076923002/diff/220001/src/codec/SkSwizzler.cpp File src/codec/SkSwizzler.cpp (right): https://codereview.chromium.org/1076923002/diff/220001/src/codec/SkSwizzler.c... src/codec/SkSwizzler.cpp:502: memset(dstStartRow, (uint8_t) colorOrIndex, bytesToFill); On 2015/04/15 14:00:18, scroggo wrote: > Interestingly, we assert for index8 that the input was 8 bit, but here we take > advantage of the fact that it wasn't... > > Maybe worth a comment. Agreed.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1076923002/#ps240001 (title: "Added comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1076923002/240001
Message was sent while issue was closed.
Committed patchset #6 (id:240001) as https://skia.googlesource.com/skia/+/e16b04aa6041efb6507546547737e9603fa1606e |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
