Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1719073002:
Use new jpeg_crop_scanlines() API to optimize jpeg subset decodes (Closed)
DescriptionUse new jpeg_crop_scanlines() API to optimize jpeg subset decodes
This was adapted from:
https://codereview.chromium.org/1530933003
Subset Decode Runtime (Original / Optimized) on Nexus 6P
TopLeft 0.51x
TopRight 0.56x
Middle 0.71x
BottomLeft 0.79x
BottomRight 0.79x
BUG=skia:4256
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1719073002
Committed: https://skia.googlesource.com/skia/+/91c22b2ea6bd13a31321ead01645467f21858cd0
Patch Set 1 #Patch Set 2 : #
Total comments: 33
Patch Set 3 : Response to comments #Patch Set 4 : Only test cropping in kCroppedScanline mode #
Total comments: 5
Patch Set 5 : Response to comments from previous patch set #
Total comments: 3
Patch Set 6 : Add to assert #
Messages
Total messages: 17 (7 generated)
Description was changed from ========== Use new jpeg_crop_scanlines() API to optimize jpeg subset decodes This was adapted from: https://codereview.chromium.org/1530933003 Performance Results TBD BUG=skia:4256 ========== to ========== Use new jpeg_crop_scanlines() API to optimize jpeg subset decodes This was adapted from: https://codereview.chromium.org/1530933003 Performance Results TBD BUG=skia:4256 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
msarett@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/1719073002/diff/20001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1719073002/diff/20001/dm/DM.cpp#newcode349 dm/DM.cpp:349: CodecSrc::Mode nativeModes[5]; nit: As long as you're rewriting this code, why not use an SkTArray like alphaModes does, down below. Then you don't need to worry about using the correct index/size. https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:477: // does not align with image blocks. Given that our implementation changes a bit depending on whether we align with an image block, it seems like it would be interesting to also test when aligning. https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:478: const int tileSize = 51; Why the change from 37 to 51? https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:480: const int tileHeight = (height + tileSize - 1) / tileSize; Why are we concerned with tiles in Y here? The old test messed with Y in order to test scanlines out of order. I guess this sort of does this, since we'll start top down, and then move back up to the top. https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:491: SkCodec::kTopDown_SkScanlineOrder != codec->getScanlineOrder()) { This is now only used for JPEG, so it seems like we should never get a scanline order that is not top down. https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:493: return Error::Nonfatal("Could not start top-down scanline decoder"); Is this not a bug? Why would startScanlineDecode fail that is okay? I could imagine incomplete images (if we still test those in DM), although those could fail past the first tile. https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.h#newcode112 dm/DMSrcSink.h:112: kTile_Mode, // Tests jpeg subset decoding optimizations "Tile" doesn't seem like it describes this. kPartialScanline? kCroppedScanline? Also, have we potentially lost some useful coverage by not testing other decodes in stripe mode? https://codereview.chromium.org/1719073002/diff/20001/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1719073002/diff/20001/gyp/codec.gyp#newcode89 gyp/codec.gyp:89: # TODO (msarett): Add this optimization to Android. Do you have a (NYC triaged) bug open for Android? Reference it here? https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:378: swizzlerOptions.fSubset = &fSwizzlerSubset; Should we assert here that fSwizzlerSubset is initialized logically? It looks like we should never hit a case where we didn't initialize it, but it requires knowing the callers of this method, and the fact that one of them early exits if options.fSubset is non-NULL, and the other one initializes fSwizzlerSubset before calling this method. https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:442: fSwizzlerSubset.setXYWH(options.fSubset->x() - startX, 0, So if I understand correctly, this is set here (rather than inside the if statement below) so that even when we do not need to do our own subsetting, but we still need a swizzler for CMYK, initializeSwizzler can blindly use fSwizzlerSubset? Should we test CMYK images with subsets that do and do not require further subsetting? https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:443: options.fSubset->width(), options.fSubset->height()); It seems odd that we ever care about the startY and height here. These don't actually affect anything, do they? https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:446: // subset that we request. Or if we are converting from CMYK. > Or if we are converting from CMYK. This part of the comment seems unnecessary here. It doesn't really affect this code, and we already have a comment down below where it's relevant. https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.h File src/codec/SkJpegCodec.h (right): https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.h... src/codec/SkJpegCodec.h:121: SkIRect fSwizzlerSubset; Should we initialize this at construction/when we're not using it (or comment that we deliberately didn't do so for performance; though it seems like initializing four ints at construction/when unused is not so bad?)?
Description was changed from ========== Use new jpeg_crop_scanlines() API to optimize jpeg subset decodes This was adapted from: https://codereview.chromium.org/1530933003 Performance Results TBD BUG=skia:4256 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Use new jpeg_crop_scanlines() API to optimize jpeg subset decodes This was adapted from: https://codereview.chromium.org/1530933003 Subset Decode Runtime (Original / Optimized) TopLeft 0.51x TopRight 0.56x Middle 0.71x BottomLeft 0.79x BottomRight 0.79x BUG=skia:4256 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Use new jpeg_crop_scanlines() API to optimize jpeg subset decodes This was adapted from: https://codereview.chromium.org/1530933003 Subset Decode Runtime (Original / Optimized) TopLeft 0.51x TopRight 0.56x Middle 0.71x BottomLeft 0.79x BottomRight 0.79x BUG=skia:4256 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Use new jpeg_crop_scanlines() API to optimize jpeg subset decodes This was adapted from: https://codereview.chromium.org/1530933003 Subset Decode Runtime (Original / Optimized) on Nexus 6P TopLeft 0.51x TopRight 0.56x Middle 0.71x BottomLeft 0.79x BottomRight 0.79x BUG=skia:4256 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/1719073002/diff/20001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1719073002/diff/20001/dm/DM.cpp#newcode349 dm/DM.cpp:349: CodecSrc::Mode nativeModes[5]; On 2016/02/22 15:55:03, scroggo wrote: > nit: As long as you're rewriting this code, why not use an SkTArray like > alphaModes does, down below. Then you don't need to worry about using the > correct index/size. Agreed that's better. I'll fix the color types as well. https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:477: // does not align with image blocks. On 2016/02/22 15:55:03, scroggo wrote: > Given that our implementation changes a bit depending on whether we align with > an image block, it seems like it would be interesting to also test when > aligning. Agreed. Changing this value to one that will test both. https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:478: const int tileSize = 51; On 2016/02/22 15:55:03, scroggo wrote: > Why the change from 37 to 51? The larger the value, the test runs. however, I've decided to bring back kStripe_Mode and use a different value for this test. https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:480: const int tileHeight = (height + tileSize - 1) / tileSize; On 2016/02/22 15:55:03, scroggo wrote: > Why are we concerned with tiles in Y here? The old test messed with Y in order > to test scanlines out of order. I guess this sort of does this, since we'll > start top down, and then move back up to the top. I guess we just use this to know how many times to loop? I've changed this to something that I think is more clear. EDIT: Oh I get it! Let's *only* test subsetting in the x-dimension in this test. I think this is fine. I don't foresee any weird interactions between jpeg_crop_scanlines() and jpeg_skip_scanlines(). Will make this test way faster as well. https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:491: SkCodec::kTopDown_SkScanlineOrder != codec->getScanlineOrder()) { On 2016/02/22 15:55:03, scroggo wrote: > This is now only used for JPEG, so it seems like we should never get a scanline > order that is not top down. Yes, I've fixed this. https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:493: return Error::Nonfatal("Could not start top-down scanline decoder"); On 2016/02/22 15:55:03, scroggo wrote: > Is this not a bug? Why would startScanlineDecode fail that is okay? I could > imagine incomplete images (if we still test those in DM), although those could > fail past the first tile. I believe that we used to need this when kInvalidConversion was an ok return value. Now that we've fixed that, I think this should be a fatal error. I'll fix this elsewhere in a follow-up CL. https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.h#newcode112 dm/DMSrcSink.h:112: kTile_Mode, // Tests jpeg subset decoding optimizations On 2016/02/22 15:55:03, scroggo wrote: > "Tile" doesn't seem like it describes this. kPartialScanline? kCroppedScanline? > > Also, have we potentially lost some useful coverage by not testing other decodes > in stripe mode? You're right, I think I've gone too far. My concern is that the kCroppedScanline test is quite slow, and there are sporadic complaints about the codec tests running for too long. But I think it's ok to keep the stripe test and only run the kCroppedScanline test for jpegs. https://codereview.chromium.org/1719073002/diff/20001/gyp/codec.gyp File gyp/codec.gyp (right): https://codereview.chromium.org/1719073002/diff/20001/gyp/codec.gyp#newcode89 gyp/codec.gyp:89: # TODO (msarett): Add this optimization to Android. On 2016/02/22 15:55:03, scroggo wrote: > Do you have a (NYC triaged) bug open for Android? Reference it here? Just opened one. Done. https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:378: swizzlerOptions.fSubset = &fSwizzlerSubset; On 2016/02/22 15:55:03, scroggo wrote: > Should we assert here that fSwizzlerSubset is initialized logically? It looks > like we should never hit a case where we didn't initialize it, but it requires > knowing the callers of this method, and the fact that one of them early exits if > options.fSubset is non-NULL, and the other one initializes fSwizzlerSubset > before calling this method. Yes that is a bit confusing. I'll add an assert. https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:442: fSwizzlerSubset.setXYWH(options.fSubset->x() - startX, 0, On 2016/02/22 15:55:03, scroggo wrote: > So if I understand correctly, this is set here (rather than inside the if > statement below) so that even when we do not need to do our own subsetting, but > we still need a swizzler for CMYK, initializeSwizzler can blindly use > fSwizzlerSubset? > Yes that's correct. > Should we test CMYK images with subsets that do and do not require further > subsetting? This will be covered by my new implementation of kCroppedScanline_Mode in DMSrcSink. https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:443: options.fSubset->width(), options.fSubset->height()); On 2016/02/22 15:55:03, scroggo wrote: > It seems odd that we ever care about the startY and height here. These don't > actually affect anything, do they? No they don't. They are ignored by SkSwizzler. The base class implementation of startScanlineDecode() requires that they are (0, height), since we don't need/want to specify vertical subsets here. https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:446: // subset that we request. Or if we are converting from CMYK. On 2016/02/22 15:55:03, scroggo wrote: > > Or if we are converting from CMYK. > > This part of the comment seems unnecessary here. It doesn't really affect this > code, and we already have a comment down below where it's relevant. Done. https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.h File src/codec/SkJpegCodec.h (right): https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.h... src/codec/SkJpegCodec.h:121: SkIRect fSwizzlerSubset; On 2016/02/22 15:55:03, scroggo wrote: > Should we initialize this at construction/when we're not using it (or comment > that we deliberately didn't do so for performance; though it seems like > initializing four ints at construction/when unused is not so bad?)? Sure I'm fine with initializing at construction.
lgtm https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:478: const int tileSize = 51; On 2016/02/22 17:38:15, msarett wrote: > On 2016/02/22 15:55:03, scroggo wrote: > > Why the change from 37 to 51? > > The larger the value, the test runs. I'm guessing you left out the word "faster"? > however, I've decided to bring back > kStripe_Mode and use a different value for this test. https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:493: return Error::Nonfatal("Could not start top-down scanline decoder"); On 2016/02/22 17:38:15, msarett wrote: > On 2016/02/22 15:55:03, scroggo wrote: > > Is this not a bug? Why would startScanlineDecode fail that is okay? I could > > imagine incomplete images (if we still test those in DM), although those could > > fail past the first tile. > > I believe that we used to need this when kInvalidConversion was an ok return > value. Now that we've fixed that, I think this should be a fatal error. > > I'll fix this elsewhere in a follow-up CL. sgtm https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:442: fSwizzlerSubset.setXYWH(options.fSubset->x() - startX, 0, On 2016/02/22 17:38:16, msarett wrote: > On 2016/02/22 15:55:03, scroggo wrote: > > So if I understand correctly, this is set here (rather than inside the if > > statement below) so that even when we do not need to do our own subsetting, > but > > we still need a swizzler for CMYK, initializeSwizzler can blindly use > > fSwizzlerSubset? > > > > Yes that's correct. Maybe add a comment? > > > Should we test CMYK images with subsets that do and do not require further > > subsetting? > > This will be covered by my new implementation of kCroppedScanline_Mode in > DMSrcSink. https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:443: options.fSubset->width(), options.fSubset->height()); On 2016/02/22 17:38:16, msarett wrote: > On 2016/02/22 15:55:03, scroggo wrote: > > It seems odd that we ever care about the startY and height here. These don't > > actually affect anything, do they? > > No they don't. They are ignored by SkSwizzler. The base class implementation > of startScanlineDecode() requires that they are (0, height), since we don't > need/want to specify vertical subsets here. Maybe add a comment? https://codereview.chromium.org/1719073002/diff/80001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1719073002/diff/80001/dm/DM.cpp#newcode374 dm/DM.cpp:374: colorTypes.push_back(CodecSrc::kGetFromCanvas_DstColorType); nit: You could include this once outside of the switch statement, and then skip it in the switch statement. https://codereview.chromium.org/1719073002/diff/80001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1719073002/diff/80001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:529: // align with the jpeg block sizes and it will sometimes not. This allows us Is this because it is not a multiple of 8? (But for after the second tile we'll have a multiple of 8?) Doesn't this mean that we'll always end up expanding the width (and every other tile expanding the start) and only cover the case where we needed the swizzler?
https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1719073002/diff/20001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:478: const int tileSize = 51; On 2016/02/22 18:46:47, scroggo wrote: > On 2016/02/22 17:38:15, msarett wrote: > > On 2016/02/22 15:55:03, scroggo wrote: > > > Why the change from 37 to 51? > > > > The larger the value, the test runs. > > I'm guessing you left out the word "faster"? > > > however, I've decided to bring back > > kStripe_Mode and use a different value for this test. > Haha yes. https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:442: fSwizzlerSubset.setXYWH(options.fSubset->x() - startX, 0, On 2016/02/22 18:46:47, scroggo wrote: > On 2016/02/22 17:38:16, msarett wrote: > > On 2016/02/22 15:55:03, scroggo wrote: > > > So if I understand correctly, this is set here (rather than inside the if > > > statement below) so that even when we do not need to do our own subsetting, > > but > > > we still need a swizzler for CMYK, initializeSwizzler can blindly use > > > fSwizzlerSubset? > > > > > > > Yes that's correct. > > Maybe add a comment? > > > > > > Should we test CMYK images with subsets that do and do not require further > > > subsetting? > > > > This will be covered by my new implementation of kCroppedScanline_Mode in > > DMSrcSink. > Done. https://codereview.chromium.org/1719073002/diff/20001/src/codec/SkJpegCodec.c... src/codec/SkJpegCodec.cpp:443: options.fSubset->width(), options.fSubset->height()); On 2016/02/22 18:46:47, scroggo wrote: > On 2016/02/22 17:38:16, msarett wrote: > > On 2016/02/22 15:55:03, scroggo wrote: > > > It seems odd that we ever care about the startY and height here. These don't > > > actually affect anything, do they? > > > > No they don't. They are ignored by SkSwizzler. The base class implementation > > of startScanlineDecode() requires that they are (0, height), since we don't > > need/want to specify vertical subsets here. > > Maybe add a comment? Done. https://codereview.chromium.org/1719073002/diff/80001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1719073002/diff/80001/dm/DM.cpp#newcode374 dm/DM.cpp:374: colorTypes.push_back(CodecSrc::kGetFromCanvas_DstColorType); On 2016/02/22 18:46:47, scroggo wrote: > nit: You could include this once outside of the switch statement, and then skip > it in the switch statement. Done. https://codereview.chromium.org/1719073002/diff/80001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1719073002/diff/80001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:529: // align with the jpeg block sizes and it will sometimes not. This allows us On 2016/02/22 18:46:47, scroggo wrote: > Is this because it is not a multiple of 8? (But for after the second tile we'll > have a multiple of 8?) > > Doesn't this mean that we'll always end up expanding the width (and every other > tile expanding the start) and only cover the case where we needed the swizzler? The block size is generally 8 or 16. And the requirement from libjpeg-turbo is that startX be aligned to the block size. There is no alignment requirement for the width - I think this wasn't clear. So we should have: startX = 0: Aligned no swizzler startX = 36: Not-aligned startX = 72: Maybe aligned (depending on if block size is 8 or 16) startX = 108: Not-aligned startX = 144: Aligned no swizzler ...
LGTM at patch set 5 https://codereview.chromium.org/1719073002/diff/80001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1719073002/diff/80001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:529: // align with the jpeg block sizes and it will sometimes not. This allows us On 2016/02/22 19:07:43, msarett wrote: > On 2016/02/22 18:46:47, scroggo wrote: > > Is this because it is not a multiple of 8? (But for after the second tile > we'll > > have a multiple of 8?) > > > > Doesn't this mean that we'll always end up expanding the width (and every > other > > tile expanding the start) and only cover the case where we needed the > swizzler? > > The block size is generally 8 or 16. And the requirement from libjpeg-turbo is > that startX be aligned to the block size. > > There is no alignment requirement for the width - I think this wasn't clear. > > So we should have: > startX = 0: Aligned no swizzler > startX = 36: Not-aligned > startX = 72: Maybe aligned (depending on if block size is 8 or 16) > startX = 108: Not-aligned > startX = 144: Aligned no swizzler > ... Ah, so if width changes, it will be to add originalStartX - alignedStartX. I just looked back at the code in onStartScanlineDecode, and you actually do make this clear in your comments before calling jpeg_crop_scanline https://codereview.chromium.org/1719073002/diff/100001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1719073002/diff/100001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:380: SkASSERT(!fSwizzlerSubset.isEmpty()); Come to think of it, this only catches an error the first time. Maybe this should also assert that options.fSubset contains fSwizzlerSubset?
https://codereview.chromium.org/1719073002/diff/100001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1719073002/diff/100001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:380: SkASSERT(!fSwizzlerSubset.isEmpty()); On 2016/02/22 19:20:20, scroggo wrote: > Come to think of it, this only catches an error the first time. Maybe this > should also assert that options.fSubset contains fSwizzlerSubset? Adding to the assert. We can't assert "contains" because the left offset might shift quite a bit. But we can assert that the widths are equal and that the left offset should stay the same or get smaller.
LGTM patch set 6 https://codereview.chromium.org/1719073002/diff/100001/src/codec/SkJpegCodec.cpp File src/codec/SkJpegCodec.cpp (right): https://codereview.chromium.org/1719073002/diff/100001/src/codec/SkJpegCodec.... src/codec/SkJpegCodec.cpp:380: SkASSERT(!fSwizzlerSubset.isEmpty()); On 2016/02/22 19:32:51, msarett wrote: > On 2016/02/22 19:20:20, scroggo wrote: > > Come to think of it, this only catches an error the first time. Maybe this > > should also assert that options.fSubset contains fSwizzlerSubset? > > Adding to the assert. > > We can't assert "contains" because the left offset might shift quite a bit. > > But we can assert that the widths are equal and that the left offset should stay > the same or get smaller. sgtm
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1719073002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1719073002/120001
Message was sent while issue was closed.
Description was changed from ========== Use new jpeg_crop_scanlines() API to optimize jpeg subset decodes This was adapted from: https://codereview.chromium.org/1530933003 Subset Decode Runtime (Original / Optimized) on Nexus 6P TopLeft 0.51x TopRight 0.56x Middle 0.71x BottomLeft 0.79x BottomRight 0.79x BUG=skia:4256 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Use new jpeg_crop_scanlines() API to optimize jpeg subset decodes This was adapted from: https://codereview.chromium.org/1530933003 Subset Decode Runtime (Original / Optimized) on Nexus 6P TopLeft 0.51x TopRight 0.56x Middle 0.71x BottomLeft 0.79x BottomRight 0.79x BUG=skia:4256 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/91c22b2ea6bd13a31321ead01645467f21858cd0 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://skia.googlesource.com/skia/+/91c22b2ea6bd13a31321ead01645467f21858cd0 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
