Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1175993005:
DM testing for skipping and scaling (Closed)
DescriptionDM testing for skipping and scaling
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/0a24297be4737f5fe23b3b918b193260e64ab32d
Patch Set 1 : #
Total comments: 10
Patch Set 2 : kStripe_Mode is a separate mode, scaling can be tested for all modes #
Total comments: 14
Patch Set 3 : Added error checking #
Messages
Total messages: 12 (5 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
Added kScale_Mode to CodecSrc in DM. Performing the decode in "stripes" allows me to test that skip() works at various image offsets. Adding fScale allows me to test that decoding and skipping both work for scaled decodes. Emmalee mentioned that it would be a good idea to benchmark scaled, subset decodes. I would agree, especially if there are potential users of this capability. For now, I just need the correctness test before I attempt to upstream to turbo.
On 2015/06/10 20:41:20, msarett wrote: > Added kScale_Mode to CodecSrc in DM. > > Performing the decode in "stripes" allows me to test that skip() works at > various image offsets. > > Adding fScale allows me to test that decoding and skipping both work for scaled > decodes. > I have made some comments inline, but as an overview: I think scaling and stripes are both interesting, but I think they should be separate modes. (Or at least, stripes should be its own mode. Scaling could arguably be done in kNormal_Mode, if we don't need to go to the fallback path.) If the decoder does not give us the exact scales we need, we should do some sort of fallback. (We've talked about doing this with the scanline decoder.) It might also be interesting to do scaling with SkImageDecoder. > Emmalee mentioned that it would be a good idea to benchmark scaled, subset > decodes. I would agree, especially if there are potential users of this > capability. For now, I just need the correctness test before I attempt to > upstream to turbo. Agreed. I also think it makes sense to check in this first, since the nanobench version should look similar, so we might as well be happy with this version before writing the other. https://codereview.chromium.org/1175993005/diff/40001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1175993005/diff/40001/dm/DM.cpp#newcode207 dm/DM.cpp:207: const float scales[] = { 0.125f, 0.25f, 0.5f, 1.0f, 2.0f, 4.0f }; It looks we'll use 1.0f twice - once in kScale_Mode and once in kNormal_Mode (oh, and a third time in kScanline_Mode, although that may make sense...) Is this intentional? Do we do something interestingly different in kScale_Mode with 1.0f? Also, I'm not sure that scaling up is interesting. We can always allow the drawing code to scale up. The really interesting part is scaling down, when we can save memory (and potentially go faster). How did you choose the downscales? If we want to compare with SkImageDecoder, we should choose scales that can be used by it. We should perhaps do some that require the fallback path as well. https://codereview.chromium.org/1175993005/diff/40001/dm/DM.cpp#newcode246 dm/DM.cpp:246: push_src("image", "scanline", new CodecSrc(path, CodecSrc::kScanline_Mode, I could imagine that it might be useful to test scanline mode with a scale. (At least in the case where the decoder supports it; if we have to fallback when the decoder does not, I think we'll be using the scanline decoder anyway.) https://codereview.chromium.org/1175993005/diff/40001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1175993005/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:267: // dimensions using stripes. It is curious to me that we combine scaling with stripes. Maybe stripes should be a separate mode? On a related note, I can imagine a couple of ways to do scaling: - use the dimensions from getScaledDimensions - jpeg will support scaling (although maybe not to the exact dimensions we want) - others may not (e.g. png). we should have some sort of fallback (which can also be used if jpeg is not exact) - I don't know whether the fallback should be a separate case or not. https://codereview.chromium.org/1175993005/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:325: SkISize size = codec->getScaledDimensions(fScale); For png (for example) this will return the original size. Not sure this is useful. https://codereview.chromium.org/1175993005/diff/40001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1175993005/diff/40001/dm/DMSrcSink.h#newcode108 dm/DMSrcSink.h:108: CodecSrc(Path, Mode, DstColorType, float); Maybe we should introduce a separate constructor for kScale_Mode: CodecSrc(Path, DstColorType, float); Since we always use 1.0f when not using kScale_Mode.
I created kStripe_Mode as a separate mode and enabled scaling to be tested for all modes. I think this makes a lot more sense. Additionally, any tests where the image won't scale will return Error::Nonfatal, which will prevent us from outputting tons of duplicate images. This test is only adequate for testing scaling with onGetScaledDimensions. Any fallback scaling implementation using the scanline decoder will need to be integrated into these tests later. Hopefully, it will become clear how this should be done once we have begun to implement fallback scaling. Scaling tests for SkImageDecoder are left as a TODO. It is my understanding that gold will compare images with the same names, so when this is implemented we just need to make sure that the naming conventions stay the same. (Ex: if the downscale by 2, we should append 0.500 to the image name.) https://codereview.chromium.org/1175993005/diff/40001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1175993005/diff/40001/dm/DM.cpp#newcode207 dm/DM.cpp:207: const float scales[] = { 0.125f, 0.25f, 0.5f, 1.0f, 2.0f, 4.0f }; On 2015/06/11 15:50:02, scroggo wrote: > It looks we'll use 1.0f twice - once in kScale_Mode and once in kNormal_Mode > (oh, and a third time in kScanline_Mode, although that may make sense...) Is > this intentional? Do we do something interestingly different in kScale_Mode with > 1.0f? > > Also, I'm not sure that scaling up is interesting. We can always allow the > drawing code to scale up. The really interesting part is scaling down, when we > can save memory (and potentially go faster). > > How did you choose the downscales? If we want to compare with SkImageDecoder, we > should choose scales that can be used by it. We should perhaps do some that > require the fallback path as well. In the new design, scale is not a mode. Rather, each mode will be tested for all of the input scales. I agree that scaling up is not interesting. These have been removed. The downscales are known to work well with the current scaling implementation for jpeg (powers of 2). These are also integer downscales, so they will be able to be compared to scaling on SkImageDecoder (when we implement a scaling test for SkImageDecoder). More scales can (and should) be added once our scaling capabilities are expanded. To my knowledge, only JpegCodec (powers of 2) and IcoCodec (depends on which images are embedded) support scaling. https://codereview.chromium.org/1175993005/diff/40001/dm/DM.cpp#newcode246 dm/DM.cpp:246: push_src("image", "scanline", new CodecSrc(path, CodecSrc::kScanline_Mode, On 2015/06/11 15:50:02, scroggo wrote: > I could imagine that it might be useful to test scanline mode with a scale. (At > least in the case where the decoder supports it; if we have to fallback when the > decoder does not, I think we'll be using the scanline decoder anyway.) I agree. The new patch tests scaling for all of the modes. As far as fallbacks, we are choosing not to test the cases where onGetScaledDimensions fails to scale and returns the original dimensions. As we discussed, in the case that this occurs, the long term plan is to use the scanline decoder to implement scaling. I think how we might test this capability will become more clear once we have a better idea of the implementation plan. https://codereview.chromium.org/1175993005/diff/40001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1175993005/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:267: // dimensions using stripes. On 2015/06/11 15:50:02, scroggo wrote: > It is curious to me that we combine scaling with stripes. Maybe stripes should > be a separate mode? > > On a related note, I can imagine a couple of ways to do scaling: > - use the dimensions from getScaledDimensions > - jpeg will support scaling (although maybe not to the exact dimensions we > want) > - others may not (e.g. png). we should have some sort of fallback (which can > also be used if jpeg is not exact) > - I don't know whether the fallback should be a separate case or not. Yes this was an odd decision that didn't make much sense. This has been redesigned. https://codereview.chromium.org/1175993005/diff/40001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:325: SkISize size = codec->getScaledDimensions(fScale); On 2015/06/11 15:50:02, scroggo wrote: > For png (for example) this will return the original size. Not sure this is > useful. We are now returning Error::Nonfatal for tests where fScale is not 1.0 and the scaled dims are not different from the original dims. I hope that clears up this concern. https://codereview.chromium.org/1175993005/diff/40001/dm/DMSrcSink.h File dm/DMSrcSink.h (right): https://codereview.chromium.org/1175993005/diff/40001/dm/DMSrcSink.h#newcode108 dm/DMSrcSink.h:108: CodecSrc(Path, Mode, DstColorType, float); On 2015/06/11 15:50:02, scroggo wrote: > Maybe we should introduce a separate constructor for kScale_Mode: > > CodecSrc(Path, DstColorType, float); > > Since we always use 1.0f when not using kScale_Mode. Agreed, however, now all of the modes use a scale, so this constructor makes sense.
lgtm https://codereview.chromium.org/1175993005/diff/60001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1175993005/diff/60001/dm/DM.cpp#newcode225 dm/DM.cpp:225: push_src("image", "scale_kGray8", new CodecSrc(path, CodecSrc::kStripe_Mode, stripe_kGray8? https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:273: // This mode tests the skipping of scanlines. Maybe the description belongs in the header, by the declaration of kStripe_Mode? https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:275: int width = decodeInfo.width(); Can these be const? https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:297: const int linesToSkip = SkTMin(stripeHeight, height - i * stripeHeight); Will this ever be less than stripeHeight? It seems like we would either have a full stripe to skip, or there is less than a full stripe to skip, in which case we won't have anything to read down below. Ok, I think I see: down below, we might read 0 scanlines? https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:298: decoder->skipScanlines(linesToSkip); I think we want to return an error if this or getScanlines fails. https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:301: const int startY = SkTMin((i + 1) * stripeHeight, height - 1); When will we use height - 1? It seems like if the first value is too large, we've gone past the end. Or do we special case so that we'll read the last line? Is that so the final skip was meaningful? Maybe add a comment; this seems a little tricky to me. https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:307: decoder = codec->getScanlineDecoder(decodeInfo, NULL, colorPtr, colorCountPtr); I think we want to return an error if decoder is null. (Since it should always succeed the second time).
https://codereview.chromium.org/1175993005/diff/60001/dm/DM.cpp File dm/DM.cpp (right): https://codereview.chromium.org/1175993005/diff/60001/dm/DM.cpp#newcode225 dm/DM.cpp:225: push_src("image", "scale_kGray8", new CodecSrc(path, CodecSrc::kStripe_Mode, On 2015/06/11 20:12:48, scroggo wrote: > stripe_kGray8? Yup! https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:273: // This mode tests the skipping of scanlines. On 2015/06/11 20:12:48, scroggo wrote: > Maybe the description belongs in the header, by the declaration of kStripe_Mode? Done. https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:275: int width = decodeInfo.width(); On 2015/06/11 20:12:48, scroggo wrote: > Can these be const? Done. https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:297: const int linesToSkip = SkTMin(stripeHeight, height - i * stripeHeight); On 2015/06/11 20:12:48, scroggo wrote: > Will this ever be less than stripeHeight? It seems like we would either have a > full stripe to skip, or there is less than a full stripe to skip, in which case > we won't have anything to read down below. > > Ok, I think I see: down below, we might read 0 scanlines? Yes, we might read 0 scanlines below. https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:298: decoder->skipScanlines(linesToSkip); On 2015/06/11 20:12:48, scroggo wrote: > I think we want to return an error if this or getScanlines fails. Done. https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:301: const int startY = SkTMin((i + 1) * stripeHeight, height - 1); On 2015/06/11 20:12:48, scroggo wrote: > When will we use height - 1? It seems like if the first value is too large, > we've gone past the end. Or do we special case so that we'll read the last line? > Is that so the final skip was meaningful? Maybe add a comment; this seems a > little tricky to me. Yes this is tricky and a bug. The original code was: const int startY = (i + 1) * stripeHeight; Which can go past the end of the image, but works with the code below because linesToRead would then be 0. I added SkTMin becuase bitmap.getAddr() will fail in Debug mode if startY >= height. But now this will cause us to always read at least one line below, which we don't want. The fix should clear things up. https://codereview.chromium.org/1175993005/diff/60001/dm/DMSrcSink.cpp#newcod... dm/DMSrcSink.cpp:307: decoder = codec->getScanlineDecoder(decodeInfo, NULL, colorPtr, colorCountPtr); On 2015/06/11 20:12:48, scroggo wrote: > I think we want to return an error if decoder is null. (Since it should always > succeed the second time). Done.
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/1175993005/#ps80001 (title: "Added error checking")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1175993005/80001
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://skia.googlesource.com/skia/+/0a24297be4737f5fe23b3b918b193260e64ab32d |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
