Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1092303003:
Scanline decoding for jpeg (Closed)
DescriptionImplementing a scanline decoder for jpeg
BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/97fdea6c4393cf0102d7eee5790782509fb4f57b
Patch Set 1 : #
Total comments: 21
Patch Set 2 : Initial round of improvements #Patch Set 3 : Switch statement to handle rewind cases #
Total comments: 2
Patch Set 4 : Return a reference to ImageInfo #
Messages
Total messages: 20 (7 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
msarett@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:407: uint32_t rowsDecoded = jpeg_read_scanlines(fCodec->fDecoderMgr->dinfo(), &fSrcRow, 1); Here we intentionally decode rows one at a time. In the regular decoder, we decode rows in chunks of 1, 2, or 4 depending on a recommendation from the decoder. I chose one at a time because it's simpler to implement and **I think** it fits better with the goal of the scanline decoder. But we have the option to change this to be more similar to the original decoder. https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:435: jpeg_read_scanlines(fCodec->fDecoderMgr->dinfo(), &fSrcRow, 1); AFAICT there is not a skip function in libjpeg. We could read all the rows at once to skip them rather than skip them one at a time. The drawback is that we would have to allocate a 2D array of memory large enough for count rows in order to read them all at once. I chose this implementation because it seems simpler. Let me know if you feel that we should go the other way.
msarett@google.com changed reviewers: + reed@google.com
Don't rush to review this, I'm about to start working on a different version. We would like to have to the option to read or skip in chunks of rows (rather than simply 1, 2, or 4 at time). We need to add performance benchmarks to find the optimal chunk size, and we want to experiment with different ways of skipping rows (I think I just came up with a cool idea).
https://codereview.chromium.org/1092303003/diff/40001/include/codec/SkScanlin... File include/codec/SkScanlineDecoder.h (right): https://codereview.chromium.org/1092303003/diff/40001/include/codec/SkScanlin... include/codec/SkScanlineDecoder.h:80: const SkImageInfo dstInfo() { return fDstInfo; } This method can be const https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:64: static void convert_CMYK_to_RGB(uint8_t* row, size_t rowBytes) { It seems very weird to me to pass rowBytes here. Why not stick with width? https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:247: return true; nit: I think this should go inside the else statement, since no other paths lead to it. https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:253: bool SkJpegCodec::handleScaling(const SkImageInfo& dstInfo) { You only need width and height here. Why not pass those two as parameters instead? Also, can we just name this "scale"? (or maybe something like "scaleToDimensions"?) https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:327: return fDecoderMgr->returnFailure("getSwizzler", kInvalidInput); This is not new to this change, but I think kInvalidInput might not be correct here. It means the stream was not valid. But initializing the swizzler does not involve reading the stream, right? In SkPngCodec, I returned kUnimplemented, thinking it would mean we had not implemented something in the swizzler, but maybe it should be kInvalidParameters, meaning the row bytes or dst info was not supported? I guess it's possible that the SrcConfig was invalid, which might mean the stream is no good? https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:337: SkAutoTDeleteArray<uint8_t> srcBuffer(SkNEW_ARRAY(uint8_t, fSrcRowBytes * rowsPerDecode)); Maybe SkASSERT(fSrcRowBytes != 0) https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:435: jpeg_read_scanlines(fCodec->fDecoderMgr->dinfo(), &fSrcRow, 1); On 2015/04/22 19:52:21, msarett wrote: > AFAICT there is not a skip function in libjpeg. > > We could read all the rows at once to skip them rather than skip them one at a > time. The drawback is that we would have to allocate a 2D array of memory large > enough for count rows in order to read them all at once. > > I chose this implementation because it seems simpler. Let me know if you feel > that we should go the other way. This seems fine to me, until/unless we discover it is a problem. I am also not aware of a way to skip in libjpeg (and SkImageDecoder_libjpeg also behaves like this). https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:453: uint8_t* fSrcRow; // pointer into fStorage https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.h File src/codec/SkJpegCodec.h (right): https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.h... src/codec/SkJpegCodec.h:103: * Checks if we can scale to the requested dimensions and performs the scaling This doesn't actually scale, right? It just sets up the jpeg_decompress_struct to scale in the future.
I didn't address address any performance tuning or benchmarking with this next iteration (as I intended at one point). It feels like that belongs with a different CL to me (once we straighten out our priorities), but let me know if you disagree. https://codereview.chromium.org/1092303003/diff/40001/include/codec/SkScanlin... File include/codec/SkScanlineDecoder.h (right): https://codereview.chromium.org/1092303003/diff/40001/include/codec/SkScanlin... include/codec/SkScanlineDecoder.h:80: const SkImageInfo dstInfo() { return fDstInfo; } On 2015/04/24 13:14:28, scroggo wrote: > This method can be const Done. https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:64: static void convert_CMYK_to_RGB(uint8_t* row, size_t rowBytes) { On 2015/04/24 13:14:28, scroggo wrote: > It seems very weird to me to pass rowBytes here. Why not stick with width? Yeah you are absolutely right. I made the change because I felt it was more convenient. Looking back, it is just as convenient to pass width and certainly more clear. https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:247: return true; On 2015/04/24 13:14:28, scroggo wrote: > nit: I think this should go inside the else statement, since no other paths lead > to it. Hmmm I'm not quite sure I understand, but let me know if you like this better. https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:253: bool SkJpegCodec::handleScaling(const SkImageInfo& dstInfo) { On 2015/04/24 13:14:28, scroggo wrote: > You only need width and height here. Why not pass those two as parameters > instead? > > Also, can we just name this "scale"? (or maybe something like > "scaleToDimensions"?) Agreed. https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:327: return fDecoderMgr->returnFailure("getSwizzler", kInvalidInput); On 2015/04/24 13:14:28, scroggo wrote: > This is not new to this change, but I think kInvalidInput might not be correct > here. It means the stream was not valid. But initializing the swizzler does not > involve reading the stream, right? In SkPngCodec, I returned kUnimplemented, > thinking it would mean we had not implemented something in the swizzler, but > maybe it should be kInvalidParameters, meaning the row bytes or dst info was not > supported? I guess it's possible that the SrcConfig was invalid, which might > mean the stream is no good? I think kUnimplemented sounds like the right choice until we can prove its something else. At least it is good to be consistent. https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:337: SkAutoTDeleteArray<uint8_t> srcBuffer(SkNEW_ARRAY(uint8_t, fSrcRowBytes * rowsPerDecode)); On 2015/04/24 13:14:28, scroggo wrote: > Maybe SkASSERT(fSrcRowBytes != 0) Done. https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:453: uint8_t* fSrcRow; On 2015/04/24 13:14:28, scroggo wrote: > // pointer into fStorage Done. https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.h File src/codec/SkJpegCodec.h (right): https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.h... src/codec/SkJpegCodec.h:103: * Checks if we can scale to the requested dimensions and performs the scaling On 2015/04/24 13:14:28, scroggo wrote: > This doesn't actually scale, right? It just sets up the jpeg_decompress_struct > to scale in the future. Yes this is unclear. I changed the function name and comments.
lgtm https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:247: return true; On 2015/04/24 15:08:44, msarett wrote: > On 2015/04/24 13:14:28, scroggo wrote: > > nit: I think this should go inside the else statement, since no other paths > lead > > to it. > > Hmmm I'm not quite sure I understand, but let me know if you like this better. Oh no, I was confused. I like it better the first way. I forgot there was a third option for rewindState. Which reminds me that I think a switch statement is more clear here.
Think we just need Mike to take a look at the API. https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1092303003/diff/40001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:247: return true; On 2015/04/24 15:23:13, scroggo wrote: > On 2015/04/24 15:08:44, msarett wrote: > > On 2015/04/24 13:14:28, scroggo wrote: > > > nit: I think this should go inside the else statement, since no other paths > > lead > > > to it. > > > > Hmmm I'm not quite sure I understand, but let me know if you like this better. > > Oh no, I was confused. I like it better the first way. I forgot there was a > third option for rewindState. Which reminds me that I think a switch statement > is more clear here. Done.
Interesting note: No matter how many lines you ask for in the call to jpeg_read_scanlines(), libjpeg generally just decides to give a single line back.
msarett@google.com changed reviewers: + djsollen@google.com
https://codereview.chromium.org/1092303003/diff/80001/include/codec/SkScanlin... File include/codec/SkScanlineDecoder.h (right): https://codereview.chromium.org/1092303003/diff/80001/include/codec/SkScanlin... include/codec/SkScanlineDecoder.h:80: const SkImageInfo dstInfo() const { return fDstInfo; } shouldn't this be const SkImageInfo& dstInfo()?
https://codereview.chromium.org/1092303003/diff/80001/include/codec/SkScanlin... File include/codec/SkScanlineDecoder.h (right): https://codereview.chromium.org/1092303003/diff/80001/include/codec/SkScanlin... include/codec/SkScanlineDecoder.h:80: const SkImageInfo dstInfo() const { return fDstInfo; } On 2015/04/29 13:59:45, djsollen wrote: > shouldn't this be const SkImageInfo& dstInfo()? Agreed!
lgtm
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/1092303003/#ps100001 (title: "Return a reference to ImageInfo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1092303003/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://skia.googlesource.com/skia/+/97fdea6c4393cf0102d7eee5790782509fb4f57b |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
