Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1685693003:
Add AndroidCodecBench to time scaled decodes (Closed)
DescriptionAdd AndroidCodecBench to time scaled decodes
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1685693003
Committed: https://skia.googlesource.com/skia/+/84451024bfe06d138629dd7c27cf2ec0f9774dbe
Patch Set 1 : #
Total comments: 15
Patch Set 2 : Fix dates #
Messages
Total messages: 13 (5 generated)
Description was changed from ========== Add AndroidCodecBench to time scaled decodes BUG=skia: ========== to ========== Add AndroidCodecBench to time scaled decodes BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #1 (id:1) has been deleted
msarett@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/1685693003/diff/20001/bench/nanobench.cpp File bench/nanobench.cpp (left): https://codereview.chromium.org/1685693003/diff/20001/bench/nanobench.cpp#old... bench/nanobench.cpp:819: // JPEG decodes using kOriginal_Strategy are broken for non-powers of two. This code has been deleted! https://codereview.chromium.org/1685693003/diff/20001/bench/nanobench.cpp#old... bench/nanobench.cpp:824: const uint32_t sampleSizes[] = { 1, 2, 4, 8, 16, 32, 64 }; We don't have any test images that are actually large enough to be scaled by sampleSize=64 and still be larger than minOutputSize. I'm deleting 32 as well because I don't think anybody cares (or it's interestingly different that 16). Please correct me if I'm wrong. https://codereview.chromium.org/1685693003/diff/20001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/1685693003/diff/20001/bench/nanobench.cpp#new... bench/nanobench.cpp:804: const int sampleSizes[] = { 2, 4, 8 }; Being conservative to only add the benches that I know we need right now. nanobench already runs for quite a long time... We can extend this later if we want more (might make sense to extend this after I separate dm tests images from nanobench test images sometime in the future).
lgtm https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... File bench/AndroidCodecBench.cpp (right): https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... bench/AndroidCodecBench.cpp:2: * Copyright 2015 Google Inc. nit 2016 https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... bench/AndroidCodecBench.cpp:20: fName.printf("AndroidCodec_%s_SampleSize%d", baseName.c_str(), sampleSize); This just occurred to me - should we print the benchtype when print each line? Then we could remove "AndroidCodec" from the name and rely on the type for that info. https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... bench/AndroidCodecBench.cpp:36: .makeColorType(kN32_SkColorType); why only n32, and only premul? - edit: looking down below, it looks like this is to save time. sgt https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench.h File bench/AndroidCodecBench.h (right): https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... bench/AndroidCodecBench.h:2: * Copyright 2015 Google Inc. nit 2016 https://codereview.chromium.org/1685693003/diff/20001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/1685693003/diff/20001/bench/nanobench.cpp#new... bench/nanobench.cpp:804: const int sampleSizes[] = { 2, 4, 8 }; On 2016/02/09 23:38:09, msarett wrote: > Being conservative to only add the benches that I know we need right now. > nanobench already runs for quite a long time... > > We can extend this later if we want more (might make sense to extend this after > I separate dm tests images from nanobench test images sometime in the future). This probably answers my question about why only test n32 and premul.
https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... File bench/AndroidCodecBench.cpp (right): https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... bench/AndroidCodecBench.cpp:20: fName.printf("AndroidCodec_%s_SampleSize%d", baseName.c_str(), sampleSize); On 2016/02/10 16:29:49, scroggo wrote: > This just occurred to me - should we print the benchtype when print each line? > Then we could remove "AndroidCodec" from the name and rely on the type for that > info. Do you think I should change the name to match the benchType or that we should pass the benchType as a parameter to CodecBench, AndroidCodecBench, BRDBench (and then use that to set fName)? https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... bench/AndroidCodecBench.cpp:36: .makeColorType(kN32_SkColorType); On 2016/02/10 16:29:49, scroggo wrote: > why only n32, and only premul? > > - edit: looking down below, it looks like this is to save time. sgt Yep that's right
https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... File bench/AndroidCodecBench.cpp (right): https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... bench/AndroidCodecBench.cpp:20: fName.printf("AndroidCodec_%s_SampleSize%d", baseName.c_str(), sampleSize); On 2016/02/10 20:06:45, msarett wrote: > On 2016/02/10 16:29:49, scroggo wrote: > > This just occurred to me - should we print the benchtype when print each line? > > Then we could remove "AndroidCodec" from the name and rely on the type for > that > > info. > > Do you think I should change the name to match the benchType or that we should > pass the benchType as a parameter to CodecBench, AndroidCodecBench, BRDBench > (and then use that to set fName)? My idea was that the name did not include the type at all - that would be stored in the benchtype, which we would print on the same line as the results. Passing it as a parameter seems unnecessary (it'll always be set to the same value). But I'm also fine with leaving as is.
https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... File bench/AndroidCodecBench.cpp (right): https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... bench/AndroidCodecBench.cpp:2: * Copyright 2015 Google Inc. On 2016/02/10 16:29:49, scroggo wrote: > nit 2016 Done. https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... bench/AndroidCodecBench.cpp:20: fName.printf("AndroidCodec_%s_SampleSize%d", baseName.c_str(), sampleSize); On 2016/02/10 21:13:54, scroggo wrote: > On 2016/02/10 20:06:45, msarett wrote: > > On 2016/02/10 16:29:49, scroggo wrote: > > > This just occurred to me - should we print the benchtype when print each > line? > > > Then we could remove "AndroidCodec" from the name and rely on the type for > > that > > > info. > > > > Do you think I should change the name to match the benchType or that we should > > pass the benchType as a parameter to CodecBench, AndroidCodecBench, BRDBench > > (and then use that to set fName)? > > My idea was that the name did not include the type at all - that would be stored > in the benchtype, which we would print on the same line as the results. > > Passing it as a parameter seems unnecessary (it'll always be set to the same > value). > > But I'm also fine with leaving as is. Ahh ok I understand. I think it's a good idea, but I'd like to avoid making that a part of this CL. https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench.h File bench/AndroidCodecBench.h (right): https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... bench/AndroidCodecBench.h:2: * Copyright 2015 Google Inc. On 2016/02/10 16:29:49, scroggo wrote: > nit 2016 Done.
lgtm https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... File bench/AndroidCodecBench.cpp (right): https://codereview.chromium.org/1685693003/diff/20001/bench/AndroidCodecBench... bench/AndroidCodecBench.cpp:20: fName.printf("AndroidCodec_%s_SampleSize%d", baseName.c_str(), sampleSize); On 2016/02/11 14:07:15, msarett wrote: > On 2016/02/10 21:13:54, scroggo wrote: > > On 2016/02/10 20:06:45, msarett wrote: > > > On 2016/02/10 16:29:49, scroggo wrote: > > > > This just occurred to me - should we print the benchtype when print each > > line? > > > > Then we could remove "AndroidCodec" from the name and rely on the type for > > > that > > > > info. > > > > > > Do you think I should change the name to match the benchType or that we > should > > > pass the benchType as a parameter to CodecBench, AndroidCodecBench, BRDBench > > > (and then use that to set fName)? > > > > My idea was that the name did not include the type at all - that would be > stored > > in the benchtype, which we would print on the same line as the results. > > > > Passing it as a parameter seems unnecessary (it'll always be set to the same > > value). > > > > But I'm also fine with leaving as is. > > Ahh ok I understand. I think it's a good idea, but I'd like to avoid making > that a part of this CL. sgtm
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1685693003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1685693003/40001
Message was sent while issue was closed.
Description was changed from ========== Add AndroidCodecBench to time scaled decodes BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add AndroidCodecBench to time scaled decodes BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/84451024bfe06d138629dd7c27cf2ec0f9774dbe ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://skia.googlesource.com/skia/+/84451024bfe06d138629dd7c27cf2ec0f9774dbe |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
