Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
918673002:
Adding new benchmark to test image decoding performance. (Closed)
DescriptionAdding new benchmark to test image decoding performance.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/95f192d19938b98a45dd1fa4112d965f60d10516
Patch Set 1 #
Total comments: 26
Patch Set 2 : Image decode benchmarking code (revision 2) #
Total comments: 19
Patch Set 3 : Finer grained benchmarking and subset decode benchmarking #Patch Set 4 : Missed a few minor comments (sorry!) #
Total comments: 11
Patch Set 5 : Fail less loudly, subset error message only printed once. #
Total comments: 2
Patch Set 6 : Better version of fine-graining #Patch Set 7 : Build tile failing is not an error #
Total comments: 4
Patch Set 8 : Duplicate stream before calling bTI #
Total comments: 1
Patch Set 9 : Removed unused variable #Patch Set 10 : Added include #
Messages
Total messages: 32 (9 generated)
msarett@google.com changed reviewers: + djsollen@google.com, mtklein@google.com, reed@android.com, scroggo@google.com
Performance Benchmark for Image Decoding - Runs a benchmark to test the decode performance for all files in the resource path. - Performs the decode test using three different color options.
https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp File bench/SKDBench.cpp (right): https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp#newcode28 bench/SKDBench.cpp:28: return backend == kNonRendering_Backend; Seems like we might want to say this is suitable for kRaster_Backend. Then we can let nanobench iterate over the backend types for us (--config, e.g. 565, 8888) , instead of explicitly looping over color types specially for image decoding. In onDraw(), you'd call canvas->imageInfo().colorType() to get the color type to draw. Whether or not you actually draw it into the canvas doesn't really matter. You may want to draw it once (with canvas->drawBitmap()) at the end outside the loop, just so 'nanobench -w' will work. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.h File bench/SKDBench.h (right): https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.h#newcode17 bench/SKDBench.h:17: class SKDBench : public Benchmark { Let's call it DecodingBench? D is a little vague, and we don't really need the Sk. This isn't part of the Skia library (no one outside of tools we write is expected to link in one of these). (If you were modeling it of SKPBench, I can see how that's confusing. We write SkPictures to files with an .skp extension, so SKPBench is sort of the natural name for that.) https://codereview.chromium.org/918673002/diff/1/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/1/bench/nanobench.cpp#newcode483 bench/nanobench.cpp:483: fFileIter = *(new SkOSFile::Iter(fResourceDir.c_str())); This pattern seems odd. It's allocating on the heap, copying to the member, and then leaking the one on the heap? Let's instead try adding , fFileIter(GetResourcePath().c_str()) to the initializer list? https://codereview.chromium.org/918673002/diff/1/bench/nanobench.cpp#newcode489 bench/nanobench.cpp:489: fColorTypes = *(new SkTArray<SkColorType>(colorTypes, 3)); If this sticks, I think you might want to write this as: SkColorType colorTypes[] = { kN32_SkColorType, kRGB_565_SkColorType, kAlpha_8_SkColorType }; fColorTypes.push_back_n(SK_ARRAY_COUNT(colorTypes), colorTypes); Otherwise, again we're both hitting the heap pointlessly, and leaking the allocation we make there.
https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp File bench/SKDBench.cpp (right): https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp#newcode19 bench/SKDBench.cpp:19: , fColorType(colorType) { I don't think we've made it official, but we've been discussing placing this brace on its own line, so it differentiates the initialization list from the body of the constructor. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp#newcode20 bench/SKDBench.cpp:20: fName.printf("%s_%s_%u", name, path.c_str(), colorType); We use indents of 4 spaces. It looks like for decoding "resources/CMYK.jpg", we'll end up with a name like SKD_resources/CMYK.jpg_3 ? Maybe a better name would be something like Decode_CMYK.jpg_kN32_SkColorType (or possibly replace ".jpg" with "_jpg") See SkOSFile.h for a method that will get the basename from the full name. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp#newcode28 bench/SKDBench.cpp:28: return backend == kNonRendering_Backend; Constants should go on the left side of == (see https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...). https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp#newcode28 bench/SKDBench.cpp:28: return backend == kNonRendering_Backend; On 2015/02/11 19:15:36, mtklein wrote: > In onDraw(), you'd call canvas->imageInfo().colorType() to get the color type to > draw. That's what I did in DM, but I realized that I want to test decoding to kAlpha_8 (for some images). We don't (and can't) have an alpha 8 canvas. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.h File bench/SKDBench.h (right): https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.h#newcode9 bench/SKDBench.h:9: #include "SkImageDecoder.h" I don't think you need to include SkImageDecoder.h, but you should include others, e.g. SkString.h SkImageInfo.h https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.h#newcode17 bench/SKDBench.h:17: class SKDBench : public Benchmark { On 2015/02/11 19:15:36, mtklein wrote: > Let's call it DecodingBench? D is a little vague, and we don't really need the > Sk. This isn't part of the Skia library (no one outside of tools we write is > expected to link in one of these). > > (If you were modeling it of SKPBench, I can see how that's confusing. We write > SkPictures to files with an .skp extension, so SKPBench is sort of the natural > name for that.) +1 I also think we should remove the existing DecodeBench and ImageDecodeBench. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.h#newcode19 bench/SKDBench.h:19: SKDBench(const char* name, SkString path, SkColorType colorType); What is "name"? It looks like we always use "SKD"? https://codereview.chromium.org/918673002/diff/1/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/1/bench/nanobench.cpp#newcode477 bench/nanobench.cpp:477: fResourceDir = GetResourcePath(); Sorry, I think I mislead you in conversation; I think it would be better to have a separate flag for images, just like we do with skps. Using the resource path means you'll attempt to decode anything that ends up in that folder (which is sort of a catch all). If we use a separate flag, we can choose any folder, and still use the resources for general purposes. https://codereview.chromium.org/918673002/diff/1/bench/nanobench.cpp#newcode588 bench/nanobench.cpp:588: return SkNEW_ARGS(SKDBench, ("SKD", path, As with SKPs and images in DM, I think we should check to make sure the image decodes before creating a benchmark.
I made the edits suggested by mtklein and scroggo. I think there are some significant improvements. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp File bench/SKDBench.cpp (right): https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp#newcode19 bench/SKDBench.cpp:19: , fColorType(colorType) { On 2015/02/11 19:38:10, scroggo wrote: > I don't think we've made it official, but we've been discussing placing this > brace on its own line, so it differentiates the initialization list from the > body of the constructor. Done. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp#newcode20 bench/SKDBench.cpp:20: fName.printf("%s_%s_%u", name, path.c_str(), colorType); On 2015/02/11 19:38:10, scroggo wrote: > We use indents of 4 spaces. > > It looks like for decoding "resources/CMYK.jpg", we'll end up with a name like > > SKD_resources/CMYK.jpg_3 > > ? > > Maybe a better name would be something like > > Decode_CMYK.jpg_kN32_SkColorType (or possibly replace ".jpg" with "_jpg") > > See SkOSFile.h for a method that will get the basename from the full name. Done. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp#newcode28 bench/SKDBench.cpp:28: return backend == kNonRendering_Backend; On 2015/02/11 19:15:36, mtklein wrote: > Seems like we might want to say this is suitable for kRaster_Backend. Then we > can let nanobench iterate over the backend types for us (--config, e.g. 565, > 8888) , instead of explicitly looping over color types specially for image > decoding. > > In onDraw(), you'd call canvas->imageInfo().colorType() to get the color type to > draw. > > Whether or not you actually draw it into the canvas doesn't really matter. You > may want to draw it once (with canvas->drawBitmap()) at the end outside the > loop, just so 'nanobench -w' will work. As discussed, we are not making this edit in order to be able to test on Alpha8 images. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp#newcode28 bench/SKDBench.cpp:28: return backend == kNonRendering_Backend; On 2015/02/11 19:38:10, scroggo wrote: > On 2015/02/11 19:15:36, mtklein wrote: > > > In onDraw(), you'd call canvas->imageInfo().colorType() to get the color type > to > > draw. > > That's what I did in DM, but I realized that I want to test decoding to kAlpha_8 > (for some images). We don't (and can't) have an alpha 8 canvas. Done. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.cpp#newcode28 bench/SKDBench.cpp:28: return backend == kNonRendering_Backend; On 2015/02/11 19:38:10, scroggo wrote: > Constants should go on the left side of == (see > https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...). Done. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.h File bench/SKDBench.h (right): https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.h#newcode9 bench/SKDBench.h:9: #include "SkImageDecoder.h" On 2015/02/11 19:38:10, scroggo wrote: > I don't think you need to include SkImageDecoder.h, but you should include > others, e.g. > > SkString.h > SkImageInfo.h Done. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.h#newcode17 bench/SKDBench.h:17: class SKDBench : public Benchmark { On 2015/02/11 19:15:36, mtklein wrote: > Let's call it DecodingBench? D is a little vague, and we don't really need the > Sk. This isn't part of the Skia library (no one outside of tools we write is > expected to link in one of these). > > (If you were modeling it of SKPBench, I can see how that's confusing. We write > SkPictures to files with an .skp extension, so SKPBench is sort of the natural > name for that.) Done. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.h#newcode17 bench/SKDBench.h:17: class SKDBench : public Benchmark { On 2015/02/11 19:38:10, scroggo wrote: > On 2015/02/11 19:15:36, mtklein wrote: > > Let's call it DecodingBench? D is a little vague, and we don't really need > the > > Sk. This isn't part of the Skia library (no one outside of tools we write is > > expected to link in one of these). > > > > (If you were modeling it of SKPBench, I can see how that's confusing. We > write > > SkPictures to files with an .skp extension, so SKPBench is sort of the natural > > name for that.) > > +1 > > I also think we should remove the existing DecodeBench and ImageDecodeBench. Done. https://codereview.chromium.org/918673002/diff/1/bench/SKDBench.h#newcode19 bench/SKDBench.h:19: SKDBench(const char* name, SkString path, SkColorType colorType); On 2015/02/11 19:38:10, scroggo wrote: > What is "name"? It looks like we always use "SKD"? Done. https://codereview.chromium.org/918673002/diff/1/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/1/bench/nanobench.cpp#newcode477 bench/nanobench.cpp:477: fResourceDir = GetResourcePath(); On 2015/02/11 19:38:10, scroggo wrote: > Sorry, I think I mislead you in conversation; I think it would be better to have > a separate flag for images, just like we do with skps. > > Using the resource path means you'll attempt to decode anything that ends up in > that folder (which is sort of a catch all). If we use a separate flag, we can > choose any folder, and still use the resources for general purposes. Done. https://codereview.chromium.org/918673002/diff/1/bench/nanobench.cpp#newcode483 bench/nanobench.cpp:483: fFileIter = *(new SkOSFile::Iter(fResourceDir.c_str())); On 2015/02/11 19:15:36, mtklein wrote: > This pattern seems odd. It's allocating on the heap, copying to the member, and > then leaking the one on the heap? > > Let's instead try adding > , fFileIter(GetResourcePath().c_str()) > to the initializer list? Done. https://codereview.chromium.org/918673002/diff/1/bench/nanobench.cpp#newcode489 bench/nanobench.cpp:489: fColorTypes = *(new SkTArray<SkColorType>(colorTypes, 3)); On 2015/02/11 19:15:36, mtklein wrote: > If this sticks, I think you might want to write this as: > > SkColorType colorTypes[] = > { kN32_SkColorType, kRGB_565_SkColorType, kAlpha_8_SkColorType }; > fColorTypes.push_back_n(SK_ARRAY_COUNT(colorTypes), colorTypes); > > Otherwise, again we're both hitting the heap pointlessly, and leaking the > allocation we make there. Done. https://codereview.chromium.org/918673002/diff/1/bench/nanobench.cpp#newcode588 bench/nanobench.cpp:588: return SkNEW_ARGS(SKDBench, ("SKD", path, On 2015/02/11 19:38:10, scroggo wrote: > As with SKPs and images in DM, I think we should check to make sure the image > decodes before creating a benchmark. Done. https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp#newc... bench/nanobench.cpp:35: I will remove this. I was using it to debug and it looks like I forgot about it.
lgtm Please make sure Leon's happy with it too! https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp#newc... bench/nanobench.cpp:35: On 2015/02/11 21:57:01, msarett wrote: > I will remove this. I was using it to debug and it looks like I forgot about > it. One day you will find yourself typing SkDebugf accidentally instead of printf...
https://codereview.chromium.org/918673002/diff/20001/bench/DecodingBench.cpp File bench/DecodingBench.cpp (right): https://codereview.chromium.org/918673002/diff/20001/bench/DecodingBench.cpp#... bench/DecodingBench.cpp:26: case kN32_SkColorType: Skia coding style dictates this should be indented from switch. https://codereview.chromium.org/918673002/diff/20001/bench/DecodingBench.cpp#... bench/DecodingBench.cpp:55: } Didn't Mike suggest drawing to the canvas? Oh, I think that doesn't work since we're only running this on the nonrendering backend. I guess that's a tradeoff :( https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp#newc... bench/nanobench.cpp:491: else if (sk_exists(flag)) { This should go on the same line as the closing brace. https://codereview.chromium.org/918673002/diff/20001/gyp/bench.gyp File gyp/bench.gyp (right): https://codereview.chromium.org/918673002/diff/20001/gyp/bench.gyp#newcode12 gyp/bench.gyp:12: '../bench/DecodingBench.cpp', Nit: I know this was not alphabetized before, but the old order had its own consistency (bench/ was grouped together). So long as we're looking at these sources, why are they here and not in bench.gypi? The result is that any gyp file which includes bench.gypi needs to also include these files. For example, iOSShell.gypi also lists these sources. This CL does not add DecodingBench.cpp to the iOS build, meaning it will now fail to compile (since nanobench.cpp attempts to use it).
https://codereview.chromium.org/918673002/diff/20001/bench/DecodingBench.cpp File bench/DecodingBench.cpp (right): https://codereview.chromium.org/918673002/diff/20001/bench/DecodingBench.cpp#... bench/DecodingBench.cpp:55: } On 2015/02/12 14:19:02, scroggo wrote: > Didn't Mike suggest drawing to the canvas? Oh, I think that doesn't work since > we're only running this on the nonrendering backend. I guess that's a tradeoff > :( Yeah, I think in this case you'll find canvas is always NULL. Don't draw on that. :P https://codereview.chromium.org/918673002/diff/20001/gyp/bench.gyp File gyp/bench.gyp (right): https://codereview.chromium.org/918673002/diff/20001/gyp/bench.gyp#newcode12 gyp/bench.gyp:12: '../bench/DecodingBench.cpp', On 2015/02/12 14:19:02, scroggo wrote: > Nit: I know this was not alphabetized before, but the old order had its own > consistency (bench/ was grouped together). > > So long as we're looking at these sources, why are they here and not in > bench.gypi? > > The result is that any gyp file which includes bench.gypi needs to also include > these files. For example, iOSShell.gypi also lists these sources. This CL does > not add DecodingBench.cpp to the iOS build, meaning it will now fail to compile > (since nanobench.cpp attempts to use it). If we do move them, let's keep them together in one distinct clump in bench.gypi. These are the core files needed to build nanobench, as opposed to files which provide benchmarks through registration. These are required, the others are optional.
scroggo@google.com changed reviewers: + caryclark@google.com
Adding Cary for insight into iOSShell.gyp. https://codereview.chromium.org/918673002/diff/20001/gyp/bench.gyp File gyp/bench.gyp (right): https://codereview.chromium.org/918673002/diff/20001/gyp/bench.gyp#newcode12 gyp/bench.gyp:12: '../bench/DecodingBench.cpp', On 2015/02/12 14:41:38, mtklein wrote: > On 2015/02/12 14:19:02, scroggo wrote: > > Nit: I know this was not alphabetized before, but the old order had its own > > consistency (bench/ was grouped together). > > > > So long as we're looking at these sources, why are they here and not in > > bench.gypi? > > > > The result is that any gyp file which includes bench.gypi needs to also > include > > these files. For example, iOSShell.gypi also lists these sources. This CL does > > not add DecodingBench.cpp to the iOS build, meaning it will now fail to > compile > > (since nanobench.cpp attempts to use it). > > If we do move them, let's keep them together in one distinct clump in > bench.gypi. These are the core files needed to build nanobench, as opposed to > files which provide benchmarks through registration. These are required, the > others are optional. Maybe move them into their own gypi? bench_core.gypi? That seems a little odd though; isn't this the core of bench? Maybe the real question is, why does iOS not include this target? I think it's because the iOS build just has one executable for nanobench and dm (although I'm not sure why), so it just recreates this file inside iOSShell.gyp. Maybe there's a better way to share the code? Not to say that decision needs to hold this CL up. I'd be fine with adding DecodingBench.cpp to iOSShell.gyp to land this, and then dealing with this duplication in another CL.
caryclark@chromium.org changed reviewers: + caryclark@chromium.org
https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp#newc... bench/nanobench.cpp:498: { kN32_SkColorType, kRGB_565_SkColorType, kAlpha_8_SkColorType }; make this const https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp#newc... bench/nanobench.cpp:597: SkString path = fImages[fCurrentImage]; const SkString& path https://codereview.chromium.org/918673002/diff/20001/gyp/bench.gyp File gyp/bench.gyp (right): https://codereview.chromium.org/918673002/diff/20001/gyp/bench.gyp#newcode12 gyp/bench.gyp:12: '../bench/DecodingBench.cpp', ioSShell.gyp exists because multiple iOS applications do not play well together. Creating a single application with an additional flag to choose whether the bot calls nanobench or dm was the simplest way to prototype a bot-driven iOS test environment. That said, the bots are building but not running iOS to my knowledge so that decision could be revisited. This CL should maintain iOSShell.gyp by minimally adding DecodingBench.cpp to it as well.
New patchsets have been uploaded after l-g-t-m from mtklein@google.com
Patch 4 Includes: Edits from suggestions in patch 2. Finer grained benchmarking of the decode (we don't want to measure reading from disk). Benchmarking for decode subset. https://codereview.chromium.org/918673002/diff/20001/bench/DecodingBench.cpp File bench/DecodingBench.cpp (right): https://codereview.chromium.org/918673002/diff/20001/bench/DecodingBench.cpp#... bench/DecodingBench.cpp:26: case kN32_SkColorType: On 2015/02/12 14:19:02, scroggo wrote: > Skia coding style dictates this should be indented from switch. Done. https://codereview.chromium.org/918673002/diff/20001/bench/DecodingBench.cpp#... bench/DecodingBench.cpp:55: } On 2015/02/12 14:19:02, scroggo wrote: > Didn't Mike suggest drawing to the canvas? Oh, I think that doesn't work since > we're only running this on the nonrendering backend. I guess that's a tradeoff > :( We discussed and decided it's best to use the non-rendering backend and not draw. https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp#newc... bench/nanobench.cpp:35: On 2015/02/12 00:29:34, mtklein wrote: > On 2015/02/11 21:57:01, msarett wrote: > > I will remove this. I was using it to debug and it looks like I forgot about > > it. > > One day you will find yourself typing SkDebugf accidentally instead of printf... :) https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp#newc... bench/nanobench.cpp:491: else if (sk_exists(flag)) { On 2015/02/12 14:19:02, scroggo wrote: > This should go on the same line as the closing brace. Done. https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp#newc... bench/nanobench.cpp:498: { kN32_SkColorType, kRGB_565_SkColorType, kAlpha_8_SkColorType }; On 2015/02/12 15:18:05, _cary wrote: > make this const Done. https://codereview.chromium.org/918673002/diff/20001/bench/nanobench.cpp#newc... bench/nanobench.cpp:597: SkString path = fImages[fCurrentImage]; On 2015/02/12 15:18:05, _cary wrote: > const SkString& path Done. https://codereview.chromium.org/918673002/diff/20001/gyp/bench.gyp File gyp/bench.gyp (right): https://codereview.chromium.org/918673002/diff/20001/gyp/bench.gyp#newcode12 gyp/bench.gyp:12: '../bench/DecodingBench.cpp', On 2015/02/12 15:18:05, _cary wrote: > ioSShell.gyp exists because multiple iOS applications do not play well together. > Creating a single application with an additional flag to choose whether the bot > calls nanobench or dm was the simplest way to prototype a bot-driven iOS test > environment. That said, the bots are building but not running iOS to my > knowledge so that decision could be revisited. > > This CL should maintain iOSShell.gyp by minimally adding DecodingBench.cpp to it > as well. Done.
https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp#newc... bench/nanobench.cpp:632: SkDebugf("Subset decoding is not supported for %s\n", Forgot to mention: We may want to consider failing silently here. This error message is printed many times if the images in the specified directory aren't webp.
https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp#newc... bench/nanobench.cpp:632: SkDebugf("Subset decoding is not supported for %s\n", On 2015/02/12 18:57:06, msarett wrote: > Forgot to mention: We may want to consider failing silently here. This error > message is printed many times if the images in the specified directory aren't > webp. Ah. We can certainly fail less loudly. Maybe only print this the first time? The nanobench driver is singlethreaded, so this can be a global bool or something... expedient hacks work fine.
Fail less loudly, subset error message only printed once. https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp#newc... bench/nanobench.cpp:632: SkDebugf("Subset decoding is not supported for %s\n", On 2015/02/12 19:28:56, mtklein wrote: > On 2015/02/12 18:57:06, msarett wrote: > > Forgot to mention: We may want to consider failing silently here. This error > > message is printed many times if the images in the specified directory aren't > > webp. > > Ah. We can certainly fail less loudly. Maybe only print this the first time? > The nanobench driver is singlethreaded, so this can be a global bool or > something... expedient hacks work fine. Done.
https://codereview.chromium.org/918673002/diff/60001/bench/DecodingBench.cpp File bench/DecodingBench.cpp (right): https://codereview.chromium.org/918673002/diff/60001/bench/DecodingBench.cpp#... bench/DecodingBench.cpp:52: SkAutoTDelete<SkStreamRewindable> stream( I was thinking more like what DM does: Read the file into an SkData (using SkData::NewFromFileName(fPath.c_str())). Then read from an SkMemoryStream. Then we don't include reading from the file system in the loops. (We could even create the SkData in the constructor in in predraw.) https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp#newc... bench/nanobench.cpp:625: SkDebugf("Cannot find decoder for %s\n", path.c_str()); Why do we make sure decoding succeeds for subset images, but not for regular decoding? https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp#newc... bench/nanobench.cpp:644: for (int y = 0; y < h; y += sH) { At first it seemed odd to me that we make sure all subsets succeed, but if one of the subset decodes failed we could get a misleading answer, so this probably makes sense. We now have this code in 3 places. I think it would be better to consolidate it somewhere. (This also means we'll be able to update it more easily.) https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp#newc... bench/nanobench.cpp:654: return SkNEW_ARGS(DecodingSubsetBench, Any reason you're going back and forth on "new" versus "SkNEW"? Typically inside the Skia library we use SkNEW. I think our tests are inconsistent, but I thought we had decided we didn't need it there? https://codereview.chromium.org/918673002/diff/80001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/80001/bench/nanobench.cpp#newc... bench/nanobench.cpp:636: fSubsetFailSilent = true; I think this should likely just be silent all the time. Not supporting buildTileIndex is not an error. Perhaps we should have a check somewhere that all the calls to buildTileIndex work if we expect them to. I don't think that should be a part of nanobench though.
Feels like we are getting closer. I think the big outstanding question is the possible refactoring of the repetitive subset decode code. https://codereview.chromium.org/918673002/diff/60001/bench/DecodingBench.cpp File bench/DecodingBench.cpp (right): https://codereview.chromium.org/918673002/diff/60001/bench/DecodingBench.cpp#... bench/DecodingBench.cpp:52: SkAutoTDelete<SkStreamRewindable> stream( On 2015/02/12 19:53:28, scroggo wrote: > I was thinking more like what DM does: Read the file into an SkData (using > SkData::NewFromFileName(fPath.c_str())). Then read from an SkMemoryStream. Then > we don't include reading from the file system in the loops. (We could even > create the SkData in the constructor in in predraw.) Yes this makes significantly more sense. https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp#newc... bench/nanobench.cpp:625: SkDebugf("Cannot find decoder for %s\n", path.c_str()); On 2015/02/12 19:53:28, scroggo wrote: > Why do we make sure decoding succeeds for subset images, but not for regular > decoding? We do in fact make the check for both. The check is much shorter for decode because it is encapsulated by DecodeFile. https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp#newc... bench/nanobench.cpp:644: for (int y = 0; y < h; y += sH) { On 2015/02/12 19:53:28, scroggo wrote: > At first it seemed odd to me that we make sure all subsets succeed, but if one > of the subset decodes failed we could get a misleading answer, so this probably > makes sense. > > We now have this code in 3 places. I think it would be better to consolidate it > somewhere. (This also means we'll be able to update it more easily.) It is true that we have similar code in 3 places which is unfortunate. The code in nanobench.cpp creates a stream, builds the tile index, and decodes subsets. It prints any error and moves to the next benchmark. The code in DMSrcSick.cpp performs the same tasks, however it returns any error as a string. The code in DecodingSubsetBench.cpp performs these tasks as well, however it has some key differences from the first two. There is no error checking because we assume any errors affecting the benchmark would have been caught in nanobench (and we don't want to include error checking in the benchmark timing). Additionally, it splits up the tasks between the constructor and onDraw, since we only want to measure the performance of the decode. I don't think it makes sense to share code between DecodingSubsetBench.cpp and the first two. It seems to me that there are too many differences. I think that we could share between nanobench.cpp and DMSrcSink.cpp by adding an additional API to SkImageDecoder.cpp (called DecodeMemorySubset or something) that returns some kind of error code that could be printed in the case of nanobench and returned in the case of DMSrcSink.cpp. However, I'm not sure how much sense this makes given that we are in the process of changing APIs anyway. I think this would be worthwhile to discuss. https://codereview.chromium.org/918673002/diff/60001/bench/nanobench.cpp#newc... bench/nanobench.cpp:654: return SkNEW_ARGS(DecodingSubsetBench, On 2015/02/12 19:53:28, scroggo wrote: > Any reason you're going back and forth on "new" versus "SkNEW"? Typically inside > the Skia library we use SkNEW. I think our tests are inconsistent, but I thought > we had decided we didn't need it there? I am happy to not use SkNew! https://codereview.chromium.org/918673002/diff/80001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/80001/bench/nanobench.cpp#newc... bench/nanobench.cpp:636: fSubsetFailSilent = true; On 2015/02/12 19:53:29, scroggo wrote: > I think this should likely just be silent all the time. Not supporting > buildTileIndex is not an error. > > Perhaps we should have a check somewhere that all the calls to buildTileIndex > work if we expect them to. I don't think that should be a part of nanobench > though. Sounds good.
On 2015/02/12 22:05:46, msarett wrote: > It is true that we have similar code in 3 places which is unfortunate. > > The code in nanobench.cpp creates a stream, builds the tile index, and decodes > subsets. It prints any error and moves to the next benchmark. > > The code in DMSrcSick.cpp performs the same tasks, however it returns any error > as a string. > > The code in DecodingSubsetBench.cpp performs these tasks as well, however it has > some key differences from the first two. There is no error checking because we > assume any errors affecting the benchmark would have been caught in nanobench > (and we don't want to include error checking in the benchmark timing). > Additionally, it splits up the tasks between the constructor and onDraw, since > we only want to measure the performance of the decode. > > I don't think it makes sense to share code between DecodingSubsetBench.cpp and > the first two. It seems to me that there are too many differences. That may be true. We probably could share code, but it might take some more convoluted code... > > I think that we could share between nanobench.cpp and DMSrcSink.cpp by adding an > additional API to SkImageDecoder.cpp (called DecodeMemorySubset or something) > that returns some kind of error code that could be printed in the case of > nanobench and returned in the case of DMSrcSink.cpp. > > However, I'm not sure how much sense this makes given that we are in the process > of changing APIs anyway. I think this would be worthwhile to discuss. Consider SkImageDecoder deprecated. We won't be adding any APIs to make it easier to test or use. We've already decided we don't like the API, so we're replacing it completely. The only reason we're setting up this testing is so we can get a baseline when we write the new code. If our new implementation is incorrect, we'll be able to see it more easily, and if it is slower, we have evidence that we can do a better job. https://codereview.chromium.org/918673002/diff/120001/bench/DecodingSubsetBen... File bench/DecodingSubsetBench.cpp (right): https://codereview.chromium.org/918673002/diff/120001/bench/DecodingSubsetBen... bench/DecodingSubsetBench.cpp:62: fDecoder->buildTileIndex(fStream, &w, &h); At this point, buildTileIndex takes ownership of the stream, so we shouldn't be holding on to a pointer to it and using it later. Since the decoder now owns the stream, it will delete it when you set a new one. In this case, it happens to be the same stream, but buildTileIndex does not check for that. (The main client of this feature, Android, does not reuse the decoder in this way, so there is no need for it to handle this case in practice.) I think the correct behavior here will be to keep fStream in an SkAutoTDelete, and pass fStream->duplicate() to this method. You can then remove the call to rewind(), since the original will remain at the beginning. https://codereview.chromium.org/918673002/diff/120001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/120001/bench/nanobench.cpp#new... bench/nanobench.cpp:654: fDivisor); This should line up with path (although I think it might fit on the previous line).
Fixed comments from scroggo https://codereview.chromium.org/918673002/diff/120001/bench/DecodingSubsetBen... File bench/DecodingSubsetBench.cpp (right): https://codereview.chromium.org/918673002/diff/120001/bench/DecodingSubsetBen... bench/DecodingSubsetBench.cpp:62: fDecoder->buildTileIndex(fStream, &w, &h); On 2015/02/13 13:41:31, scroggo wrote: > At this point, buildTileIndex takes ownership of the stream, so we shouldn't be > holding on to a pointer to it and using it later. > > Since the decoder now owns the stream, it will delete it when you set a new one. > In this case, it happens to be the same stream, but buildTileIndex does not > check for that. (The main client of this feature, Android, does not reuse the > decoder in this way, so there is no need for it to handle this case in > practice.) > > I think the correct behavior here will be to keep fStream in an SkAutoTDelete, > and pass fStream->duplicate() to this method. You can then remove the call to > rewind(), since the original will remain at the beginning. That explains a few bugs I was running into. Thanks! https://codereview.chromium.org/918673002/diff/120001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/120001/bench/nanobench.cpp#new... bench/nanobench.cpp:654: fDivisor); On 2015/02/13 13:41:31, scroggo wrote: > This should line up with path (although I think it might fit on the previous > line). Done.
lgtm https://codereview.chromium.org/918673002/diff/140001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/918673002/diff/140001/bench/nanobench.cpp#new... bench/nanobench.cpp:593: SkString file; This variable is unused.
New patchsets have been uploaded after l-g-t-m from scroggo@google.com
On 2015/02/13 14:33:56, scroggo wrote: > lgtm > > https://codereview.chromium.org/918673002/diff/140001/bench/nanobench.cpp > File bench/nanobench.cpp (right): > > https://codereview.chromium.org/918673002/diff/140001/bench/nanobench.cpp#new... > bench/nanobench.cpp:593: SkString file; > This variable is unused. lgtm
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/918673002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...) Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)
New patchsets have been uploaded after l-g-t-m from scroggo@google.com
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/918673002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/95f192d19938b98a45dd1fa4112d965f60d10516 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
