Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1288963002:
Provides multiple implementations of Android's SkBitmapRegionDecoder (Closed)
DescriptionProvides various implementations of Android's SkBitmapRegionDecoder.
Implements testing in DM for these implementations.
nanobench testing will follow after this.
TBR=scroggo
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/76f755e6d54a32f9887ad254ce59a3a62f28bde4
Committed: https://skia.googlesource.com/skia/+/a5783aeff042ccaf517e50dee3660a4925f5f694
Patch Set 1 : #
Total comments: 53
Patch Set 2 : Added kSrc_Mode and setFilterQuality #Patch Set 3 : Response to Leon's comments #
Total comments: 14
Patch Set 4 : Rebase #Patch Set 5 : Response to comments from patch set 3 #Patch Set 6 : Use nullptr instead of NULL #
Total comments: 20
Patch Set 7 : Rebase #Patch Set 8 : Response to comments #
Total comments: 42
Patch Set 9 : Response to comments #
Total comments: 10
Patch Set 10 : Rebase #Patch Set 11 : Share set_subset_region code #Patch Set 12 : Acknowledged a subtle bug in the test #
Total comments: 10
Patch Set 13 : Final response to comments #Patch Set 14 : Changed name to SkBitmapRegionDecoderInterface to avoid conflict with Android #Patch Set 15 : Win compile fix #Patch Set 16 : Fix more win errors #Patch Set 17 : Move BRD code to tools #Patch Set 18 : Disable png subsets for SkImageDecoder #
Depends on Patchset: Messages
Total messages: 48 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
msarett@google.com changed reviewers: + emmaleer@google.com, scroggo@google.com
https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:112: // Use a border to test what happens when we ask for subsets that extend outside the image This leads to outputs that are a bit strange for 565. We zero the part of the bitmap rect that is outside the image. For 565, this is black. When we eventually draw on the canvas, some scaling modes will have averaged some of these black pixels with pixels on the edge of the image. For very small scaled images, ~1-5 pixels, black edges are noticeable. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:111: bitmap->eraseColor(0); This was necessary for images with transparency. Without this line, partially transparent images are blended with the uninitialized memory already in the bitmap. I plan to explore if there is some mode I can change to make it overwrite the bitmap completely. I also need to figure out how to adjust the quality modes on drawBitmapRect.
Could you add some more explanation in your description? Something like provides various implementations for Android's SkBitmapRegionDecoder? https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcode67 dm/DMSrcSink.cpp:67: BRDSrc::BRDSrc(Path path, SkBitmapRegionDecoder::Strategy strategy, Mode mode, SkColorType dstColorType, uint32_t sampleSize) nit: too many chars https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcode76 dm/DMSrcSink.cpp:76: // It is interesting to test different dstColorTypes instead of different sinks Huh? https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcode93 dm/DMSrcSink.cpp:93: return "Error: Could not CreateBRD.\n"; nit: I don't think "Error: " is necessary here. I think if it returns anything other than "Error::Nonfatal" it is considered an error. (We might want to include the path though, as in CodecSrc.) https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:104: SkAutoTDelete<SkBitmapRegionDecoder> brd( It looks like these two are the same? Why not create this outside of the switch statement, and save duplicate code? https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:112: // Use a border to test what happens when we ask for subsets that extend outside the image On 2015/08/13 15:16:34, msarett wrote: > This leads to outputs that are a bit strange for 565. We zero the part of the > bitmap rect that is outside the image. For 565, this is black. When we > eventually draw on the canvas, some scaling modes will have averaged some of > these black pixels with pixels on the edge of the image. For very small scaled > images, ~1-5 pixels, black edges are noticeable. Wait, so if the subset is bigger than 5 pixels, these edges are not noticeable? I hope no one is creating 5 pixel subsets! That said, if we do have some sort of problem, it seems like we could do the following: - only draw the subset that intersects with the actual image - if the requested subset is goes outside the bounds of the image, fill the destination with black (rather than filling the pre-scaled destination with black) - then draw the subset That is probably better anyway, since it means: - we filled less - we drew a smaller temporary bitmap to the final output https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:161: if (NULL != codec) { nit: checking !condition seems odd to me, if there's going to be an else. i.e. if (!condition) { // condition was false } else { // condition was true } Seems less straightforward than if (condition) { // condition was true } else { // condition was false } If you want to have the condition where we had a codec first, you could make that: if (codec) { // ... } else { // ... } (I just looked at the style guide and was reminded that if (codec) is the "slightly preferred" way to check for the existence of codec.) https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:164: } else { This else is unnecessary, since the earlier condition returned. https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:170: // We will try to replicate the names used by CodecSrc so that images can Try to? Do we ever fail? https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.h#newcode104 dm/DMSrcSink.h:104: class BRDSrc : public Src { Maybe add a description for what this is for? https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.h#newcode107 dm/DMSrcSink.h:107: kNormal_Mode, What do these modes mean? Isn't BRD always used for doing subsets? https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:34: SkColorType colorType) { nit: Maybe this should be dstColorType? https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:81: case SkCodec::kIncompleteInput: It seems like if we got incomplete at this point, we have failed? https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:111: bitmap->eraseColor(0); On 2015/08/13 15:16:34, msarett wrote: > This was necessary for images with transparency. Without this line, partially > transparent images are blended with the uninitialized memory already in the > bitmap. Seems like a good candidate for a FIXME. > > I plan to explore if there is some mode I can change to make it overwrite the > bitmap completely. How about SkXfermode::kSrc_Mode? > > I also need to figure out how to adjust the quality modes on drawBitmapRect. Do you mean the SkFilterQuality on the SkPaint? https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionCanvas.h (right): https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.h:2: * Copyright (C) 2015 The Android Open Source Project This is the wrong header. If this is new code in Skia, I think you want the one used in the other files? Is this a modified version of a file that is in Android? https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.h:22: * This class implements SkBitmapRegionDecoder using SkCanvas. by drawing to an SkCanvas? https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.h:28: * This has several key differences from the Android version: I think it's probably good to contrast with the android version, but it would be helpful to explain what the parameters mean. (That said, maybe that is in the base class description, which I should look at to understand this file?) https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.h:32: SkBitmap* decodeRegion(int start_x, int start_y, int width, int height, Out of curiosity, why does this return a pointer to an SkBitmap? It seems like it could return an SkBitmap (its operator= method is relatively cheap) or take a pointer to an SkBitmap as a parameter? Ah, I see that you return NULL on failure. The latter method could also return a bool, like SkImageDecoder does. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.h:35: SkBitmapRegionCanvas(SkScanlineDecoder* decoder); nit: We usually put constructors first, I believe. Also, comments explaining ownership? https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.h:37: ~SkBitmapRegionCanvas() {} Is this necessary? I think the compiler will generate this for you? https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionDecoder.h (right): https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionDecoder.h:23: kSample_Strategy, So this uses SkImageDecoder? https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionDecoder.h:30: virtual SkBitmap* decodeRegion(int start_x, int start_y, int width, comments? https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionDecoder.h:47: int fWidth; Can these be const? https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionSampler.cpp (right): https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionSampler.cpp:37: fDecoder->setDitherImage(true); Why did you choose these settings? https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionSampler.cpp:41: SkIRect region; nit: a blank line before this line would be helpful
Would this change break Android if it lands? I'm wondering because there are now two definitions of SkBitmapRegionDecoder. https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcode67 dm/DMSrcSink.cpp:67: BRDSrc::BRDSrc(Path path, SkBitmapRegionDecoder::Strategy strategy, Mode mode, SkColorType dstColorType, uint32_t sampleSize) On 2015/08/13 16:53:06, scroggo wrote: > nit: too many chars Done. https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcode76 dm/DMSrcSink.cpp:76: // It is interesting to test different dstColorTypes instead of different sinks On 2015/08/13 16:53:06, scroggo wrote: > Huh? I changed this comment. The original comment was an attempt to say that we want to test on different SkColorTypes rather than different backends. https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcode93 dm/DMSrcSink.cpp:93: return "Error: Could not CreateBRD.\n"; On 2015/08/13 16:53:06, scroggo wrote: > nit: I don't think "Error: " is necessary here. I think if it returns anything > other than "Error::Nonfatal" it is considered an error. (We might want to > include the path though, as in CodecSrc.) Done. https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:104: SkAutoTDelete<SkBitmapRegionDecoder> brd( On 2015/08/13 16:53:06, scroggo wrote: > It looks like these two are the same? Why not create this outside of the switch > statement, and save duplicate code? Done. https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:112: // Use a border to test what happens when we ask for subsets that extend outside the image On 2015/08/13 16:53:06, scroggo wrote: > On 2015/08/13 15:16:34, msarett wrote: > > This leads to outputs that are a bit strange for 565. We zero the part of the > > bitmap rect that is outside the image. For 565, this is black. When we > > eventually draw on the canvas, some scaling modes will have averaged some of > > these black pixels with pixels on the edge of the image. For very small > scaled > > images, ~1-5 pixels, black edges are noticeable. > > Wait, so if the subset is bigger than 5 pixels, these edges are not noticeable? > I hope no one is creating 5 pixel subsets! > > That said, if we do have some sort of problem, it seems like we could do the > following: > - only draw the subset that intersects with the actual image > - if the requested subset is goes outside the bounds of the image, fill the > destination with black (rather than filling the pre-scaled destination with > black) > - then draw the subset > > That is probably better anyway, since it means: > - we filled less > - we drew a smaller temporary bitmap to the final output So I think my comment was completely wrong. We are already doing what you suggest. I think that a few of the really small outputs have black pixels on the edges in 565 due to "an implementation bug that I don't understand yet". I know we don't really care about these subsets (the reason I have the tests is because I want to make sure we don't crash), but I should probably figure out what is actually going on. https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:161: if (NULL != codec) { On 2015/08/13 16:53:06, scroggo wrote: > nit: checking !condition seems odd to me, if there's going to be an else. i.e. > > if (!condition) { > // condition was false > } else { > // condition was true > } > > Seems less straightforward than > > if (condition) { > // condition was true > } else { > // condition was false > } > > If you want to have the condition where we had a codec first, you could make > that: > > if (codec) { > // ... > } else { > // ... > } > > (I just looked at the style guide and was reminded that if (codec) is the > "slightly preferred" way to check for the existence of codec.) Done. https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:164: } else { On 2015/08/13 16:53:06, scroggo wrote: > This else is unnecessary, since the earlier condition returned. Done. https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:170: // We will try to replicate the names used by CodecSrc so that images can On 2015/08/13 16:53:06, scroggo wrote: > Try to? Do we ever fail? Nope :) https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.h#newcode104 dm/DMSrcSink.h:104: class BRDSrc : public Src { On 2015/08/13 16:53:07, scroggo wrote: > Maybe add a description for what this is for? Done. https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.h#newcode107 dm/DMSrcSink.h:107: kNormal_Mode, On 2015/08/13 16:53:07, scroggo wrote: > What do these modes mean? Isn't BRD always used for doing subsets? Maybe we don't need both of these tests or these aren't the only interesting tests? I think kFullImage_Mode, kDivisor_Mode are more accurate. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:34: SkColorType colorType) { On 2015/08/13 16:53:07, scroggo wrote: > nit: Maybe this should be dstColorType? Done. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:81: case SkCodec::kIncompleteInput: On 2015/08/13 16:53:07, scroggo wrote: > It seems like if we got incomplete at this point, we have failed? Agreed. Changing this. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:111: bitmap->eraseColor(0); On 2015/08/13 16:53:07, scroggo wrote: > On 2015/08/13 15:16:34, msarett wrote: > > This was necessary for images with transparency. Without this line, partially > > transparent images are blended with the uninitialized memory already in the > > bitmap. > > Seems like a good candidate for a FIXME. > > > > > I plan to explore if there is some mode I can change to make it overwrite the > > bitmap completely. > > How about SkXfermode::kSrc_Mode? > > > > > I also need to figure out how to adjust the quality modes on drawBitmapRect. > > Do you mean the SkFilterQuality on the SkPaint? Yes on all counts. This has been added. SkFilterQuality defaults to None but we may want to test all of them at some point. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionCanvas.h (right): https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.h:2: * Copyright (C) 2015 The Android Open Source Project On 2015/08/13 16:53:07, scroggo wrote: > This is the wrong header. If this is new code in Skia, I think you want the one > used in the other files? > > Is this a modified version of a file that is in Android? You're right. BitmapRegionSampler needs the Android header I think, but this should have the regular skia header. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.h:22: * This class implements SkBitmapRegionDecoder using SkCanvas. On 2015/08/13 16:53:07, scroggo wrote: > by drawing to an SkCanvas? Done. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.h:28: * This has several key differences from the Android version: On 2015/08/13 16:53:07, scroggo wrote: > I think it's probably good to contrast with the android version, but it would be > helpful to explain what the parameters mean. > > (That said, maybe that is in the base class description, which I should look at > to understand this file?) Added comments on the parameters to the base class. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.h:32: SkBitmap* decodeRegion(int start_x, int start_y, int width, int height, On 2015/08/13 16:53:07, scroggo wrote: > Out of curiosity, why does this return a pointer to an SkBitmap? It seems like > it could return an SkBitmap (its operator= method is relatively cheap) or take a > pointer to an SkBitmap as a parameter? > > Ah, I see that you return NULL on failure. The latter method could also return a > bool, like SkImageDecoder does. The reason this returns an SkBitmap* is that I was going for the closest match to the Android API. decodeRegion (Java) calls nativeDecodeRegion (C++) which essentially returns a pointer to an Android bitmap (which I think is basically an SkBitmap). Maybe it's not so important to match this? It seems logical to me - especially since we will be moving one of these implementations into Android. FYI, nativeDecodeRegion (C++) calls SkBitmapRegionDecoder::decodeRegion (C++) passing in a pointer to the SkBitmap and returns a bool. I combined these two functions and matched the nativeDecodeRegion API. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.h:35: SkBitmapRegionCanvas(SkScanlineDecoder* decoder); On 2015/08/13 16:53:07, scroggo wrote: > nit: > > We usually put constructors first, I believe. Also, comments explaining > ownership? Done. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.h:37: ~SkBitmapRegionCanvas() {} On 2015/08/13 16:53:07, scroggo wrote: > Is this necessary? I think the compiler will generate this for you? Done. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionDecoder.h (right): https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionDecoder.h:23: kSample_Strategy, On 2015/08/13 16:53:07, scroggo wrote: > So this uses SkImageDecoder? Yeah - I think there will be a Sample strategy for ImageDecoder and Codec once Emmalee's change lands. I will go ahead and change the name now. Maybe something like Baseline or Original - so we can compare all of the Codec versions to the old version. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionDecoder.h:30: virtual SkBitmap* decodeRegion(int start_x, int start_y, int width, On 2015/08/13 16:53:07, scroggo wrote: > comments? Adding comments! https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionDecoder.h:47: int fWidth; On 2015/08/13 16:53:07, scroggo wrote: > Can these be const? Yes! https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionSampler.cpp (right): https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionSampler.cpp:37: fDecoder->setDitherImage(true); On 2015/08/13 16:53:07, scroggo wrote: > Why did you choose these settings? These are Android's default settings. Adding a comment. https://codereview.chromium.org/1288963002/diff/40001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionSampler.cpp:41: SkIRect region; On 2015/08/13 16:53:07, scroggo wrote: > nit: a blank line before this line would be helpful Done.
msarett@google.com changed reviewers: + reed@google.com
Mike, SkBitmapRegionCanvas may be interesting to you.
https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:112: // Use a border to test what happens when we ask for subsets that extend outside the image On 2015/08/13 18:10:22, msarett wrote: > On 2015/08/13 16:53:06, scroggo wrote: > > On 2015/08/13 15:16:34, msarett wrote: > > > This leads to outputs that are a bit strange for 565. We zero the part of > the > > > bitmap rect that is outside the image. For 565, this is black. When we > > > eventually draw on the canvas, some scaling modes will have averaged some of > > > these black pixels with pixels on the edge of the image. For very small > > scaled > > > images, ~1-5 pixels, black edges are noticeable. > > > > Wait, so if the subset is bigger than 5 pixels, these edges are not > noticeable? > > I hope no one is creating 5 pixel subsets! > > > > That said, if we do have some sort of problem, it seems like we could do the > > following: > > - only draw the subset that intersects with the actual image > > - if the requested subset is goes outside the bounds of the image, fill the > > destination with black (rather than filling the pre-scaled destination with > > black) > > - then draw the subset > > > > That is probably better anyway, since it means: > > - we filled less > > - we drew a smaller temporary bitmap to the final output > > So I think my comment was completely wrong. > > We are already doing what you suggest. > > I think that a few of the really small outputs have black pixels on the edges in > 565 due to "an implementation bug that I don't understand yet". > > I know we don't really care about these subsets (the reason I have the tests is > because I want to make sure we don't crash), but I should probably figure out > what is actually going on. +1 https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.h#newcode107 dm/DMSrcSink.h:107: kNormal_Mode, On 2015/08/13 18:10:23, msarett wrote: > On 2015/08/13 16:53:07, scroggo wrote: > > What do these modes mean? Isn't BRD always used for doing subsets? > > Maybe we don't need both of these tests or these aren't the only interesting > tests? The FullImage mode seems weird. Does it show us something unique, interesting, or analogous to how clients use it? Divisor definitely seems interesting. Or maybe these should track the uses in your various benchmarks? > > I think kFullImage_Mode, kDivisor_Mode are more accurate. Agreed. https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:69: fDecoder->skipScanlines(top); nit: could fit on one line? https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:71: case SkCodec::kSuccess: Perhaps this can be if (fDecoder->skipScanlines(top) != kSuccess) { return NULL; } https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:108: // isn't critical for clients who call this API with Why not? Don't clients actually use it in this way? Or will we convert the clients to not use it that way? https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:116: bitmap->eraseColor(0); Why zero? I thought you said we should draw black where the subset is outside the original image? https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:127: // Set the scaling quality, kNone is the default // TODO: Use other qualities? It seems like we can skip this if we just use the default. https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionDecoder.h (right): https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionDecoder.h:28: * @param stream Encoded image stream Takes ownership? https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionDecoder.h:29: * @param strategy Method uses for scaling and subsetting Strategy used* ?
Patchset #4 (id:100001) has been deleted
msarett@google.com changed reviewers: - emmaleer@google.com
https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:112: // Use a border to test what happens when we ask for subsets that extend outside the image On 2015/08/28 13:49:43, scroggo wrote: > On 2015/08/13 18:10:22, msarett wrote: > > On 2015/08/13 16:53:06, scroggo wrote: > > > On 2015/08/13 15:16:34, msarett wrote: > > > > This leads to outputs that are a bit strange for 565. We zero the part of > > the > > > > bitmap rect that is outside the image. For 565, this is black. When we > > > > eventually draw on the canvas, some scaling modes will have averaged some > of > > > > these black pixels with pixels on the edge of the image. For very small > > > scaled > > > > images, ~1-5 pixels, black edges are noticeable. > > > > > > Wait, so if the subset is bigger than 5 pixels, these edges are not > > noticeable? > > > I hope no one is creating 5 pixel subsets! > > > > > > That said, if we do have some sort of problem, it seems like we could do the > > > following: > > > - only draw the subset that intersects with the actual image > > > - if the requested subset is goes outside the bounds of the image, fill the > > > destination with black (rather than filling the pre-scaled destination with > > > black) > > > - then draw the subset > > > > > > That is probably better anyway, since it means: > > > - we filled less > > > - we drew a smaller temporary bitmap to the final output > > > > So I think my comment was completely wrong. > > > > We are already doing what you suggest. > > > > I think that a few of the really small outputs have black pixels on the edges > in > > 565 due to "an implementation bug that I don't understand yet". > > > > I know we don't really care about these subsets (the reason I have the tests > is > > because I want to make sure we don't crash), but I should probably figure out > > what is actually going on. > > +1 I've fixed this. I had an off by one error that was caused when the border was larger than the image. I think that the issue is with this test, not the implementation. So I've set a limit on the border size and added a comment. If you're interested in what was actually going on, I'm happy to explain in person but I think it's a little tricky to explain here. https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:69: fDecoder->skipScanlines(top); On 2015/08/28 13:49:43, scroggo wrote: > nit: could fit on one line? Done. https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:71: case SkCodec::kSuccess: On 2015/08/28 13:49:44, scroggo wrote: > Perhaps this can be > > if (fDecoder->skipScanlines(top) != kSuccess) { > return NULL; > } Done. https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:108: // isn't critical for clients who call this API with On 2015/08/28 13:49:44, scroggo wrote: > Why not? Don't clients actually use it in this way? Or will we convert the > clients to not use it that way? Revising this comment. https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:116: bitmap->eraseColor(0); On 2015/08/28 13:49:43, scroggo wrote: > Why zero? I thought you said we should draw black where the subset is outside > the original image? Yeah I did say that. I thought I saw a comment in SkImageDecoder about that. But I've since not been able to find that comment anywhere and running tests shows that SkImageDecoder actually leaves the memory uninitialized. This may not be a huge issue if the client is using a zero initialized bitmap. I think for us, zero is the right choice (transparent for kN32, black for 565) in case anybody was depending on zero init bitmaps. In looking at use cases, I don't think I saw anybody intentionally reading outside of the image, but I wouldn't rule it out. I do think my comment is a bit harsh. We should try to zero the bitmap efficiently - I will word things more appropriately :). https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionCanvas.cpp:127: // Set the scaling quality, kNone is the default On 2015/08/28 13:49:44, scroggo wrote: > // TODO: Use other qualities? > > It seems like we can skip this if we just use the default. Yep this is unnecessary. I think I was just teaching myself how to change the quality haha. https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... File src/utils/SkBitmapRegionDecoder.h (right): https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionDecoder.h:28: * @param stream Encoded image stream On 2015/08/28 13:49:44, scroggo wrote: > Takes ownership? Done. https://codereview.chromium.org/1288963002/diff/80001/src/utils/SkBitmapRegio... src/utils/SkBitmapRegionDecoder.h:29: * @param strategy Method uses for scaling and subsetting On 2015/08/28 13:49:44, scroggo wrote: > Strategy used* ? Done.
https://codereview.chromium.org/1288963002/diff/160001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1288963002/diff/160001/dm/DM.cpp#newcode294 dm/DM.cpp:294: push_src("image", "brd_canvas_kN32", new BRDSrc(path, I think this can be cleaned up to behave similarly to your other DM change. https://codereview.chromium.org/1288963002/diff/160001/dm/DM.cpp#newcode336 dm/DM.cpp:336: static bool codec_supported(const char* ext) { I was glancing at this earlier - do we still need this? Doesn't this cover all the types? You could repurpose it to be used for BRD, where I think we should only support PNG, JPG and WEBP. https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:84: return Error::Nonfatal("Testing to multiple backends is uninteresting."); More importantly, I think we never run this for 565, right? (Or did I misread an earlier comment?) So we would be decoding an 8888 bitmap and trying to draw it to 8888. https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:89: SkNEW_ARGS(SkMemoryStream, (SkData::NewFromFileName(fPath.c_str()))), nit: no need for SkNEW anymore https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:161: return SkISize::Make(SkTMax(1, codec->getInfo().width() / (int) fSampleSize), It seems a little weird that we calculate this using a codec divided by sample size, but this class does not necessarily use a codec, right? Maybe this should create a BRD? https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:174: return SkStringPrintf("%s_%.3f", SkOSPath::Basename(fPath.c_str()).c_str(), scale); Maybe this should be in a helper function? https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.h#newcode107 dm/DMSrcSink.h:107: kFullImage_Mode, Maybe add some comments explaining what these mean? https://codereview.chromium.org/1288963002/diff/160001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/160001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:74: SkCodec::Result result = fDecoder->getScanlines(tmp.getAddr(0, 0), height, So this won't work for all BMP, for example, but that's okay, since it only needs to work for the big three, right? https://codereview.chromium.org/1288963002/diff/160001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.h (right): https://codereview.chromium.org/1288963002/diff/160001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.h:13: * This class implements SkBitmapRegionDecoder by drawing to an SkCanvas. I'm not sure this captures what is interesting about this version - it does the *scaling* by drawing to a canvas, right?
https://codereview.chromium.org/1288963002/diff/160001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1288963002/diff/160001/dm/DM.cpp#newcode294 dm/DM.cpp:294: push_src("image", "brd_canvas_kN32", new BRDSrc(path, On 2015/09/01 22:05:28, scroggo wrote: > I think this can be cleaned up to behave similarly to your other DM change. Yes it can! Done. https://codereview.chromium.org/1288963002/diff/160001/dm/DM.cpp#newcode336 dm/DM.cpp:336: static bool codec_supported(const char* ext) { On 2015/09/01 22:05:28, scroggo wrote: > I was glancing at this earlier - do we still need this? Doesn't this cover all > the types? You could repurpose it to be used for BRD, where I think we should > only support PNG, JPG and WEBP. You are correct that it's no longer needed. I will repurpose it. https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:84: return Error::Nonfatal("Testing to multiple backends is uninteresting."); On 2015/09/01 22:05:28, scroggo wrote: > More importantly, I think we never run this for 565, right? (Or did I misread an > earlier comment?) So we would be decoding an 8888 bitmap and trying to draw it > to 8888. Sorry, forgot about this. Otherwise I might have proposed a similar change to CodecSrc. I don't really like how CodecSrc decodes to 8888 (and maybe other color types) when the backend is 8888 and decodes to 565 (and has to check for and disable other color types) when the backend is 565. For BRDSrc, I decided to only allow one backend (8888). This is tested to all of the color types that we want to decode to (8888, 565, Index8 etc). Maybe this is just as confusing as what we do in CodecSrc? Or maybe it's confusing because it's different than what we do in CodecSrc? https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:89: SkNEW_ARGS(SkMemoryStream, (SkData::NewFromFileName(fPath.c_str()))), On 2015/09/01 22:05:28, scroggo wrote: > nit: no need for SkNEW anymore Done. https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:161: return SkISize::Make(SkTMax(1, codec->getInfo().width() / (int) fSampleSize), On 2015/09/01 22:05:28, scroggo wrote: > It seems a little weird that we calculate this using a codec divided by sample > size, but this class does not necessarily use a codec, right? Maybe this should > create a BRD? Agreed. Fixed to use a BRD. https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:174: return SkStringPrintf("%s_%.3f", SkOSPath::Basename(fPath.c_str()).c_str(), scale); On 2015/09/01 22:05:28, scroggo wrote: > Maybe this should be in a helper function? Yeah I think so. https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.h#newcode107 dm/DMSrcSink.h:107: kFullImage_Mode, On 2015/09/01 22:05:28, scroggo wrote: > Maybe add some comments explaining what these mean? Yes that would be helpful. Done. https://codereview.chromium.org/1288963002/diff/160001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/160001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:74: SkCodec::Result result = fDecoder->getScanlines(tmp.getAddr(0, 0), height, On 2015/09/01 22:05:28, scroggo wrote: > So this won't work for all BMP, for example, but that's okay, since it only > needs to work for the big three, right? Correct. This was written before we had kBottomUp, kOutOfOrder etc. Theoretically, we could rewrite this to support all of the modes, but there's not reason to unless we want to start supporting BMP, GIF, etc. I'm adding an SkASSERT(kTopDown). https://codereview.chromium.org/1288963002/diff/160001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.h (right): https://codereview.chromium.org/1288963002/diff/160001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.h:13: * This class implements SkBitmapRegionDecoder by drawing to an SkCanvas. On 2015/09/01 22:05:28, scroggo wrote: > I'm not sure this captures what is interesting about this version - it does the > *scaling* by drawing to a canvas, right? It also does some subsetting. Since we can only decode full rows right now, it subsets in the x-dimension and scales by drawing to an SkCanvas. I will improve the comment. I'm planning on adding the ability to get partial scanlines for SkScaledCodec - when this lands, we should update this class.
https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:84: return Error::Nonfatal("Testing to multiple backends is uninteresting."); On 2015/09/02 18:02:23, msarett wrote: > On 2015/09/01 22:05:28, scroggo wrote: > > More importantly, I think we never run this for 565, right? (Or did I misread > an > > earlier comment?) So we would be decoding an 8888 bitmap and trying to draw it > > to 8888. > > Sorry, forgot about this. Otherwise I might have proposed a similar change to > CodecSrc. > > I don't really like how CodecSrc decodes to 8888 (and maybe other color types) > when the backend is 8888 and decodes to 565 (and has to check for and disable > other color types) when the backend is 565. I agree that it is a little awkward. I assume your dislike of it is our combination of two different ideas: 1) decode to the color type of the canvas This allows us to take advantage of the fact that DM is already set up to choose a color type (even though it's treated a little differently by the image tests). It allows us to separate the images into different folders (565, 8888). This also allows us to compare the images to each other. 2) for index8 and gray, decode to the 8888 canvas This is because we *cannot* draw to an index8/gray backend. This is sort of a workaround. It means that these images all go into the 8888 folder, but are separated by other folders We could switch everything to (2). I think the disadvantage is that all our images go into the 8888 folder, and we need to mark their subfolders with their types (as we already do for index8 and gray). Or maybe we just need to think harder about how we do this? It occurs to me that "image" is the only type that uses TaggedSrc::options; maybe options should be more than one level deep? Right now, if I look in my dm results folder, I see the following structure: 565 codec codec_subset decode scanline scanline_subset stripe subset 8888 <all the same ones> codec_kGray8 codec_kIndex8 <etc...> Maybe instead it should be something like: 8888 codec 8888 565 kIndex8 kGray8 scanline <same config list> <etc> It would be a little bit weird, because we have to drill down into the 8888 folder to find 565, although we already have to do so to get to kIndex8 and kGray8, so maybe it's not so bad after all? The other thing to consider is how do we make Gold separate these? The other nice thing about relying on the canvas config is that Gold correctly shows 565 images as having the config 565 (but obviously it's wrong for kIndex8 and kGray8...). This would be relegated to options, which may not be so bad. (We would need to update Gold to handle our new options structure.) > > For BRDSrc, I decided to only allow one backend (8888). This is tested to all > of the color types that we want to decode to (8888, 565, Index8 etc). Maybe > this is just as confusing as what we do in CodecSrc? > > Or maybe it's confusing because it's different than what we do in CodecSrc? It's probably confusing because it's different. I think we should stick with the same decision for both. https://codereview.chromium.org/1288963002/diff/200001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:120: uint32_t width = brd->width(); nit: These could have been defined above, so you only access brd->width() once. Also, can any of these fields be const? https://codereview.chromium.org/1288963002/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:141: SkAutoTDelete<SkBitmap> bitmap(brd->decodeRegion(decodeLeft, What should we do about the fact that WEBP gives us a different subset in the case of an odd left or top? A FIXME? Handle it inside the BRD? https://codereview.chromium.org/1288963002/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:171: static SkString get_scaled_name(Path path, float scale) { Copying SkStrings is cheap, but const &ing them is cheaper! https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:33: int left = SkTMax(0, input_x); This is because BRD supports subsets that are not truly subsets? (i.e. they lie partially outside the bounds?) https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:38: int height = SkTMin(this->height() - top, input_h - topOffset); Calculating width and height are very similar. What do you think about a helper function? https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:44: // Create the image info the decode for* the decode? https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:46: if (kUnpremul_SkAlphaType == dstAlphaType) { This is a downside to this version of BRD - we cannot support unpremultiplied (or index8, gray8). Maybe we should note this somewhere? Android cares a lot about backwards compatibility. It seems like we cannot use this method if someone was relying on this? https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:87: int outWidth = SkTMax(1, input_w / sampleSize); It surprised me that this is input_w and not width. I guess this is the width of the whole thing, including the part that is outside the original image? https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:91: SkAutoTDelete<SkBitmap> bitmap(SkNEW(SkBitmap)); nit: new https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:111: // Use a canvas to crop and scale to the destination bitmap Shouldn't we be using SkSurface? I guess we cannot because we need an SkBitmap at the end? I suppose we could use SkImage::asLegacyBitmap to convert to SkBitmap, but maybe that's convoluted? https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:112: SkAutoTDelete<SkCanvas> canvas(SkNEW_ARGS(SkCanvas, (*bitmap))); nit: new Actually, wait, can you put this on the stack? https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:115: SkTMax(1, width / sampleSize), SkTMax(1, height / sampleSize)); SkTMax(1, N / sampleSize) This pattern gets used a lot. Maybe write a helper function? https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:116: SkPaint* paint = new SkPaint(); This gets leaked. Why not just put it on the stack? https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.h (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.h:26: * This has several key differences from the Android version: "several", or two? Are there others? https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionDecoder.cpp (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionDecoder.cpp:26: SkDELETE(decoder); nit: delete https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionDecoder.cpp:29: return SkNEW_ARGS(SkBitmapRegionSampler, (decoder, width, height)); nit: new https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionDecoder.cpp:37: return SkNEW_ARGS(SkBitmapRegionCanvas, (decoder)); nit: new https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionDecoder.h (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionDecoder.h:39: * @param start_x X-coordinate of upper-left corner of region. So if I understand correctly, these can be negative? https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionSampler.cpp (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionSampler.cpp:2: * Copyright (C) 2010 The Android Open Source Project I think we already talked about this, so sorry if I'm repeating myself. Was this pulled directly from Android? I'm guessing the code has changed a fair bit, so I think this should have a different copyright header? https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionSampler.h (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionSampler.h:26: * This has several key differences from the Android version: "several" or three?
https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/160001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:84: return Error::Nonfatal("Testing to multiple backends is uninteresting."); On 2015/09/02 21:32:24, scroggo wrote: > On 2015/09/02 18:02:23, msarett wrote: > > On 2015/09/01 22:05:28, scroggo wrote: > > > More importantly, I think we never run this for 565, right? (Or did I > misread > > an > > > earlier comment?) So we would be decoding an 8888 bitmap and trying to draw > it > > > to 8888. > > > > Sorry, forgot about this. Otherwise I might have proposed a similar change to > > CodecSrc. > > > > I don't really like how CodecSrc decodes to 8888 (and maybe other color types) > > when the backend is 8888 and decodes to 565 (and has to check for and disable > > other color types) when the backend is 565. > > I agree that it is a little awkward. I assume your dislike of it is our > combination of two different ideas: > 1) decode to the color type of the canvas > This allows us to take advantage of the fact that DM is already set up to choose > a color type (even though it's treated a little differently by the image tests). > It allows us to separate the images into different folders (565, 8888). This > also allows us to compare the images to each other. > 2) for index8 and gray, decode to the 8888 canvas > This is because we *cannot* draw to an index8/gray backend. This is sort of a > workaround. It means that these images all go into the 8888 folder, but are > separated by other folders > > We could switch everything to (2). I think the disadvantage is that all our > images go into the 8888 folder, and we need to mark their subfolders with their > types (as we already do for index8 and gray). > > Or maybe we just need to think harder about how we do this? It occurs to me that > "image" is the only type that uses TaggedSrc::options; maybe options should be > more than one level deep? > > Right now, if I look in my dm results folder, I see the following structure: > > 565 > codec codec_subset decode scanline scanline_subset stripe subset > 8888 > <all the same ones> codec_kGray8 codec_kIndex8 <etc...> > > Maybe instead it should be something like: > > 8888 > codec > 8888 565 kIndex8 kGray8 > scanline > <same config list> > <etc> > I like this idea. > It would be a little bit weird, because we have to drill down into the 8888 > folder to find 565, although we already have to do so to get to kIndex8 and > kGray8, so maybe it's not so bad after all? > > The other thing to consider is how do we make Gold separate these? The other > nice thing about relying on the canvas config is that Gold correctly shows 565 > images as having the config 565 (but obviously it's wrong for kIndex8 and > kGray8...). This would be relegated to options, which may not be so bad. (We > would need to update Gold to handle our new options structure.) > > > > > For BRDSrc, I decided to only allow one backend (8888). This is tested to all > > of the color types that we want to decode to (8888, 565, Index8 etc). Maybe > > this is just as confusing as what we do in CodecSrc? > > > > Or maybe it's confusing because it's different than what we do in CodecSrc? > > It's probably confusing because it's different. I think we should stick with the > same decision for both. I'm going to change this to act like CodecSrc for two reasons: (1) It's simple. (2) I think you make a good point about Gold. I like the separation of 8888 and 565 and don't want to lose that. https://codereview.chromium.org/1288963002/diff/200001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:120: uint32_t width = brd->width(); On 2015/09/02 21:32:24, scroggo wrote: > nit: These could have been defined above, so you only access brd->width() once. > > Also, can any of these fields be const? Good point! Pretty sure lots of them can be const. https://codereview.chromium.org/1288963002/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:141: SkAutoTDelete<SkBitmap> bitmap(brd->decodeRegion(decodeLeft, On 2015/09/02 21:32:24, scroggo wrote: > What should we do about the fact that WEBP gives us a different subset in the > case of an odd left or top? A FIXME? Handle it inside the BRD? I think it should be handled in BRD. We want BRD to handle everything, since I think we will either call it from Android code or simply use it to replicate and test (as close as we can) the code that we will have in Android. Having said that, it should be a FIXME since we aren't currently handling it in BRD. The BRD implementation that uses SkScaledCodec (WIP, not in this CL) already handles it partly (fails when the subsets aren't on even boundaries). https://codereview.chromium.org/1288963002/diff/200001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:171: static SkString get_scaled_name(Path path, float scale) { On 2015/09/02 21:32:24, scroggo wrote: > Copying SkStrings is cheap, but const &ing them is cheaper! :) https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:33: int left = SkTMax(0, input_x); On 2015/09/02 21:32:24, scroggo wrote: > This is because BRD supports subsets that are not truly subsets? (i.e. they lie > partially outside the bounds?) Yes :( https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:38: int height = SkTMin(this->height() - top, input_h - topOffset); On 2015/09/02 21:32:24, scroggo wrote: > Calculating width and height are very similar. What do you think about a helper > function? Seems like a good idea. Will do. https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:44: // Create the image info the decode On 2015/09/02 21:32:24, scroggo wrote: > for* the decode? Done. https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:46: if (kUnpremul_SkAlphaType == dstAlphaType) { On 2015/09/02 21:32:24, scroggo wrote: > This is a downside to this version of BRD - we cannot support unpremultiplied > (or index8, gray8). Maybe we should note this somewhere? > > Android cares a lot about backwards compatibility. It seems like we cannot use > this method if someone was relying on this? Yeah agreed. I will file a bug for comparing/choosing an implementation of BRD. Honestly, I didn't write this implementation because I think we're going to use it. I wrote this because the performance/quality comparison will hopefully validate the creation of SkScaledCodec. You could still argue that if this turns out to be the superior implementation, we will use it when we can, and use another as a fallback if we can't. https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:87: int outWidth = SkTMax(1, input_w / sampleSize); On 2015/09/02 21:32:24, scroggo wrote: > It surprised me that this is input_w and not width. I guess this is the width of > the whole thing, including the part that is outside the original image? Yes. No matter what (for backward compatibility) we need to return a bitmap that is as big as the requested region. If only a small portion of the requested region contains the image, we will be returning a bitmap with a lot of empty space. https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:91: SkAutoTDelete<SkBitmap> bitmap(SkNEW(SkBitmap)); On 2015/09/02 21:32:24, scroggo wrote: > nit: new Done. https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:111: // Use a canvas to crop and scale to the destination bitmap On 2015/09/02 21:32:24, scroggo wrote: > Shouldn't we be using SkSurface? I guess we cannot because we need an SkBitmap > at the end? I suppose we could use SkImage::asLegacyBitmap to convert to > SkBitmap, but maybe that's convoluted? I don't really know anything about SkSurface vs SkCanvas etc. I'm guessing SkSurface is newer and SkCanvas is being phased out? https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:112: SkAutoTDelete<SkCanvas> canvas(SkNEW_ARGS(SkCanvas, (*bitmap))); On 2015/09/02 21:32:24, scroggo wrote: > nit: new > > Actually, wait, can you put this on the stack? Yes I believe so! https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:115: SkTMax(1, width / sampleSize), SkTMax(1, height / sampleSize)); On 2015/09/02 21:32:24, scroggo wrote: > SkTMax(1, N / sampleSize) > > This pattern gets used a lot. Maybe write a helper function? Yes! https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:116: SkPaint* paint = new SkPaint(); On 2015/09/02 21:32:24, scroggo wrote: > This gets leaked. Why not just put it on the stack? Putting on stack! https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.h (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.h:26: * This has several key differences from the Android version: On 2015/09/02 21:32:24, scroggo wrote: > "several", or two? Are there others? Yeah I think this covers the important ones, will reword. https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionDecoder.cpp (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionDecoder.cpp:26: SkDELETE(decoder); On 2015/09/02 21:32:24, scroggo wrote: > nit: delete Done. https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionDecoder.cpp:29: return SkNEW_ARGS(SkBitmapRegionSampler, (decoder, width, height)); On 2015/09/02 21:32:24, scroggo wrote: > nit: new Done. https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionDecoder.cpp:37: return SkNEW_ARGS(SkBitmapRegionCanvas, (decoder)); On 2015/09/02 21:32:24, scroggo wrote: > nit: new Done. https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionDecoder.h (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionDecoder.h:39: * @param start_x X-coordinate of upper-left corner of region. On 2015/09/02 21:32:24, scroggo wrote: > So if I understand correctly, these can be negative? Yes. This is unfortunate, but it's currently allowed in Android. https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionSampler.cpp (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionSampler.cpp:2: * Copyright (C) 2010 The Android Open Source Project On 2015/09/02 21:32:24, scroggo wrote: > I think we already talked about this, so sorry if I'm repeating myself. Was this > pulled directly from Android? I'm guessing the code has changed a fair bit, so I > think this should have a different copyright header? Yeah I was sticking with the original header since it was originally pulled from Android. I think you're right, enough has changed that I should change the copyright header. https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionSampler.h (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionSampler.h:26: * This has several key differences from the Android version: On 2015/09/02 21:32:25, scroggo wrote: > "several" or three? Acknowledged.
https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:111: // Use a canvas to crop and scale to the destination bitmap On 2015/09/03 19:20:52, msarett wrote: > On 2015/09/02 21:32:24, scroggo wrote: > > Shouldn't we be using SkSurface? I guess we cannot because we need an SkBitmap > > at the end? I suppose we could use SkImage::asLegacyBitmap to convert to > > SkBitmap, but maybe that's convoluted? > > I don't really know anything about SkSurface vs SkCanvas etc. I'm guessing > SkSurface is newer Yes. > and SkCanvas is being phased out? No. SkBitmap is being phased out; at least from the public API. It is being replaced by SkImage. You still draw to a surface using a canvas (see SkSurface::getCanvas()). But then when you want the result, you call SkSurface::newImageSnapshot(). https://codereview.chromium.org/1288963002/diff/220001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/220001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:96: if (kRGB_565_SkColorType == colorType) { Instead of repeating this three times, can we move it outside the switch statement? if (kRGB_565_SkColorType == colorType && fDstColorType != CodecSrc::kGetFromCanvas_DstColorType) { return Error::Nonfatal(...) } https://codereview.chromium.org/1288963002/diff/220001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1288963002/diff/220001/dm/DMSrcSink.h#newcode118 dm/DMSrcSink.h:118: kAlpha_Always_DstColorType, Alpha8 is used as a hack for Grayscale. Maybe we should reuse Grayscale, and make the BRD translate? https://codereview.chromium.org/1288963002/diff/220001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/220001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:21: static int get_safe_subset_dimension(int imageOffset, int imageDim, int regionOffset, These names are pretty opaque to me. They describe what gets passed to this function, but not really what's done with them? Really you just want to know the smaller of two differences? Looking back at my comment, I see I said "Calculating width and height are very similar. What do you think about a helper function?" But it's more than just width and height - leftOffset and topOffset are parallels, along with left and top. I was thinking a function to compute all of that. Maybe that would end up being too messy though. https://codereview.chromium.org/1288963002/diff/220001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:29: return SkTMax(1, dimension / sampleSize); It seems like this is logically the same as SkScaledCodec.cpp's get_scaled_dimension? Can we share? I guess the tricky part is finding the right place for it?
https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/200001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:111: // Use a canvas to crop and scale to the destination bitmap On 2015/09/03 20:07:41, scroggo wrote: > On 2015/09/03 19:20:52, msarett wrote: > > On 2015/09/02 21:32:24, scroggo wrote: > > > Shouldn't we be using SkSurface? I guess we cannot because we need an > SkBitmap > > > at the end? I suppose we could use SkImage::asLegacyBitmap to convert to > > > SkBitmap, but maybe that's convoluted? > > > > I don't really know anything about SkSurface vs SkCanvas etc. I'm guessing > > SkSurface is newer > > Yes. > > > and SkCanvas is being phased out? > > No. SkBitmap is being phased out; at least from the public API. It is being > replaced by SkImage. You still draw to a surface using a canvas (see > SkSurface::getCanvas()). But then when you want the result, you call > SkSurface::newImageSnapshot(). Ok that makes sense. Since we are returning an SkBitmap* anyway, I think it makes sense to keep as is. https://codereview.chromium.org/1288963002/diff/220001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/220001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:96: if (kRGB_565_SkColorType == colorType) { On 2015/09/03 20:07:41, scroggo wrote: > Instead of repeating this three times, can we move it outside the switch > statement? > > if (kRGB_565_SkColorType == colorType && fDstColorType != > CodecSrc::kGetFromCanvas_DstColorType) { > return Error::Nonfatal(...) > } Yes this is better. https://codereview.chromium.org/1288963002/diff/220001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1288963002/diff/220001/dm/DMSrcSink.h#newcode118 dm/DMSrcSink.h:118: kAlpha_Always_DstColorType, On 2015/09/03 20:07:41, scroggo wrote: > Alpha8 is used as a hack for Grayscale. Maybe we should reuse Grayscale, and > make the BRD translate? Yes I think this makes sense. https://codereview.chromium.org/1288963002/diff/220001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/220001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:21: static int get_safe_subset_dimension(int imageOffset, int imageDim, int regionOffset, On 2015/09/03 20:07:41, scroggo wrote: > These names are pretty opaque to me. They describe what gets passed to this > function, but not really what's done with them? > > Really you just want to know the smaller of two differences? > > Looking back at my comment, I see I said "Calculating width and height are very > similar. What do you think about a helper function?" But it's more than just > width and height - leftOffset and topOffset are parallels, along with left and > top. I was thinking a function to compute all of that. Maybe that would end up > being too messy though. I'll add comments, I was hoping the names would make it more clear. My original plan was to share more than this one line - but you'll notice that left, leftOffset, top, topOffset are all used below. I suppose we could pass the address of local variable to a helper function, but I'm not sure that's an improvement. Edit: Tried sharing more code, let me know what you think. https://codereview.chromium.org/1288963002/diff/220001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:29: return SkTMax(1, dimension / sampleSize); On 2015/09/03 20:07:41, scroggo wrote: > It seems like this is logically the same as SkScaledCodec.cpp's > get_scaled_dimension? Can we share? I guess the tricky part is finding the right > place for it? Yeah I tried to share it in SkCodecPriv.h but that doesn't work. I'm not aware of a good location - maybe there is one somewhere? I'm not sure it's worth creating a new file for?
Patchset #11 (id:260001) has been deleted
lgtm with some comments https://codereview.chromium.org/1288963002/diff/220001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/220001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:29: return SkTMax(1, dimension / sampleSize); On 2015/09/03 22:10:30, msarett wrote: > On 2015/09/03 20:07:41, scroggo wrote: > > It seems like this is logically the same as SkScaledCodec.cpp's > > get_scaled_dimension? Can we share? I guess the tricky part is finding the > right > > place for it? > > Yeah I tried to share it in SkCodecPriv.h but that doesn't work. > > I'm not aware of a good location - maybe there is one somewhere? I'm not sure > it's worth creating a new file for? Ah, because this is in utils? maybe declare it as an extern function? Either way, it seems like we should decided on an implementation (not sure which is better, but one does a divide no matter what, and then compares it against one, and the other first compares the two numbers. I'd lean towards doing the check first, but I could certainly be wrong. If it's not an extern function, maybe add TODOs/FIXMEs to combine them? https://codereview.chromium.org/1288963002/diff/300001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/300001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:161: // The output image should be 97x97. I think it's okay that the final image is less than 97 x 97. The Java client will supply a Bitmap for each quadrant (assuming this use case), and we'll return four Bitmaps of 48 x 48. If someone is using our native API, they will specify the scaling and subset they want, and we'll return the size of the output (I think I need to add that API, actually - just added skbug.com/4302). For SkImageDecoder, we provide the output bitmap, so we'll just allocate the memory we need. https://codereview.chromium.org/1288963002/diff/300001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/300001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:47: SkBitmap* SkBitmapRegionCanvas::decodeRegion(int regionLeft, int regionTop, I know Android started using this term "region", but I don't think it provides any meaningful information. Maybe it fits because the name of this function is "decodeRegion", but I liked the "input" names better (although they should be camelCase e.g. inputX etc). https://codereview.chromium.org/1288963002/diff/300001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:57: // The client may not necessarily request a region that is fully within I find these comments very helpful :) https://codereview.chromium.org/1288963002/diff/300001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:64: // The size of the output bitmap is determined by the size of the nit: blank line between code and large comment block: int imageSubsetLeft; // Comment block https://codereview.chromium.org/1288963002/diff/300001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:69: int bitmapLeft; Maybe this would be better named outLeft? Something that tells the reader this applies to the output bitmap?
https://codereview.chromium.org/1288963002/diff/220001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/220001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:29: return SkTMax(1, dimension / sampleSize); On 2015/09/04 17:55:06, scroggo wrote: > On 2015/09/03 22:10:30, msarett wrote: > > On 2015/09/03 20:07:41, scroggo wrote: > > > It seems like this is logically the same as SkScaledCodec.cpp's > > > get_scaled_dimension? Can we share? I guess the tricky part is finding the > > right > > > place for it? > > > > Yeah I tried to share it in SkCodecPriv.h but that doesn't work. > > > > I'm not aware of a good location - maybe there is one somewhere? I'm not sure > > it's worth creating a new file for? > > Ah, because this is in utils? maybe declare it as an extern function? Either > way, it seems like we should decided on an implementation (not sure which is > better, but one does a divide no matter what, and then compares it against one, > and the other first compares the two numbers. I'd lean towards doing the check > first, but I could certainly be wrong. > > If it's not an extern function, maybe add TODOs/FIXMEs to combine them? I'll go with the original implementation in SkScaledCodec (that may soon be moved to SkCodecPriv in another CL). Actually I was able to include SkCodecPriv.h in this file by changing utils.gyp, but then the compiler complained that all of the other functions in the SkCodecPriv.h were defined but not used. I have added a TODO to combine this with the other implementation. https://codereview.chromium.org/1288963002/diff/300001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1288963002/diff/300001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:161: // The output image should be 97x97. On 2015/09/04 17:55:06, scroggo wrote: > I think it's okay that the final image is less than 97 x 97. The Java client > will supply a Bitmap for each quadrant (assuming this use case), and we'll > return four Bitmaps of 48 x 48. If someone is using our native API, they will > specify the scaling and subset they want, and we'll return the size of the > output (I think I need to add that API, actually - just added skbug.com/4302). > > For SkImageDecoder, we provide the output bitmap, so we'll just allocate the > memory we need. Yeah I agree with that assessment. BRD is behaving as we want it to - it's just a bit strange when combining regions for the purposes of testing. https://codereview.chromium.org/1288963002/diff/300001/src/utils/SkBitmapRegi... File src/utils/SkBitmapRegionCanvas.cpp (right): https://codereview.chromium.org/1288963002/diff/300001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:47: SkBitmap* SkBitmapRegionCanvas::decodeRegion(int regionLeft, int regionTop, On 2015/09/04 17:55:06, scroggo wrote: > I know Android started using this term "region", but I don't think it provides > any meaningful information. Maybe it fits because the name of this function is > "decodeRegion", but I liked the "input" names better (although they should be > camelCase e.g. inputX etc). Will do. And I'm replacing Left and Top in all of the names with X and Y. Skia uses the XYWH convention with SkRect. https://codereview.chromium.org/1288963002/diff/300001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:57: // The client may not necessarily request a region that is fully within On 2015/09/04 17:55:07, scroggo wrote: > I find these comments very helpful :) Great! Third try is the charm haha. https://codereview.chromium.org/1288963002/diff/300001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:64: // The size of the output bitmap is determined by the size of the On 2015/09/04 17:55:07, scroggo wrote: > nit: blank line between code and large comment block: > > int imageSubsetLeft; > > // Comment block Done. https://codereview.chromium.org/1288963002/diff/300001/src/utils/SkBitmapRegi... src/utils/SkBitmapRegionCanvas.cpp:69: int bitmapLeft; On 2015/09/04 17:55:06, scroggo wrote: > Maybe this would be better named outLeft? Something that tells the reader this > applies to the output bitmap? Yes 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/1288963002/#ps330001 (title: "Changed name to SkBitmapRegionDecoderInterface to avoid conflict with Android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288963002/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288963002/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
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/1288963002/#ps340012 (title: "Win compile fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288963002/340012 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288963002/340012
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
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/1288963002/#ps360001 (title: "Fix more win errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288963002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288963002/360001
Message was sent while issue was closed.
Committed patchset #16 (id:360001) as https://skia.googlesource.com/skia/+/76f755e6d54a32f9887ad254ce59a3a62f28bde4
Message was sent while issue was closed.
On 2015/09/04 at 20:00:54, commit-bot wrote: > Committed patchset #16 (id:360001) as https://skia.googlesource.com/skia/+/76f755e6d54a32f9887ad254ce59a3a62f28bde4 This change seems to be breaking DM on several bots: https://uberchromegw.corp.google.com/i/client.skia.android/builders/Test-Andr... ( 207/843 MB 70906) 93.3ms 8888 image brd_sample_0.333 webp_test.webp_0.333Error: Failed to create decoder. Error: Failed to create decoder.
Message was sent while issue was closed.
Sorry, you're right, this needs a revert and a manual roll. Will do when I get to the office.
Message was sent while issue was closed.
On 2015/09/08 12:02:07, msarett wrote: > Sorry, you're right, this needs a revert and a manual roll. Will do when I get > to the office. Looks like it's also running the Test-Android-*-CPU-* bots out of RAM when running DM. Seeing a lot of "killed (exit code 137)", i.e. kill -9.
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:360001) has been created in https://codereview.chromium.org/1322773004/ by msarett@google.com. The reason for reverting is: Breaking Android bots Bad use of the utils folder.
Patchset #18 (id:400001) has been deleted
Patchset #18 (id:420001) has been deleted
(1) Moved code to tools directory. (2) Disabled the testing of pngs using SkImageDecoder. I tried to run png and jpeg partial decodes on my Linux machine in order to figure out what was causing the issues on our bots. I never succeeded with jpeg - just got seg faults. Maybe I just messed up with changing the Android specific memory manager etc. With png, I saw significant memory leaks in our forked copy of libpng. I disabled testing of pngs in this mode for now and will try to recommit. Fingers crossed that everything works, but I think the bots will tell us if there are remaining issues.
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/1288963002/#ps440001 (title: "Disable png subsets for SkImageDecoder")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288963002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288963002/440001
Message was sent while issue was closed.
Committed patchset #18 (id:440001) as https://skia.googlesource.com/skia/+/a5783aeff042ccaf517e50dee3660a4925f5f694
Message was sent while issue was closed.
On 2015/09/08 22:29:13, msarett wrote: > (1) Moved code to tools directory. > > (2) Disabled the testing of pngs using SkImageDecoder. > > I tried to run png and jpeg partial decodes on my Linux > machine in order to figure out what was causing the issues > on our bots. > > I never succeeded with jpeg - just got seg faults. > Maybe I just messed up with changing the Android > specific memory manager etc. What was the case where you got segfaults with jpeg? Is this a problem? > > With png, I saw significant memory leaks in our forked > copy of libpng. I disabled testing of pngs in this mode > for now and will try to recommit. Fingers crossed that > everything works, but I think the bots will tell us if > there are remaining issues.
Message was sent while issue was closed.
On 2015/09/18 17:58:08, scroggo wrote: > On 2015/09/08 22:29:13, msarett wrote: > > (1) Moved code to tools directory. > > > > (2) Disabled the testing of pngs using SkImageDecoder. > > > > I tried to run png and jpeg partial decodes on my Linux > > machine in order to figure out what was causing the issues > > on our bots. > > > > I never succeeded with jpeg - just got seg faults. > > Maybe I just messed up with changing the Android > > specific memory manager etc. > > What was the case where you got segfaults with jpeg? Is this a problem? > Sorry this comment was misleading. The seg faults indicate that I failed to duplicate our Android jpeg subset decoding on my desktop machine. I'm not sure why I failed, I gave up because I found the bug I was looking for in png. Certainly if the seg faults were occurring on our Android devices, our tests would crash and we would know. I thought it was relevant to mention that I failed to rule out memory leaks in jpeg subset decoding, especially since I wasn't sure at the time if the png fix would be sufficient. Since removing the png leak fixed the Android bots, I assumed that the jpeg decodes were ok. Maybe it's worth noting that our SkImageDecoder png subset test leaks memory and has been running (and continues to run) on the Android bots with no problem since I've been here. So it is possible that there are issues with jpeg that we don't know about. I think that's ok, since we intend to replace it anyway. It will be nice when we no longer have code that we only test on Android, since valgrind/ASAN etc. will expose these types of issues. > > > > With png, I saw significant memory leaks in our forked > > copy of libpng. I disabled testing of pngs in this mode > > for now and will try to recommit. Fingers crossed that > > everything works, but I think the bots will tell us if > > there are remaining issues. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
