Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2743773002:
Port part of media.tough_video_cases to TBMv2 (Closed)
DescriptionReplace battor.tough_video_cases with media.tough_video_cases_tbmv2
This is part of the effort to port media.tough_video_cases to use
the new TBMv2 framework, and to add support for BattOr. Existing
measurements of CPU percentage and power consumption have been
moved to the new benchmark, and battor.tough_video_cases benchmark
has been deleted.
R=nednguyen@google.com
BUG=700160
Review-Url: https://codereview.chromium.org/2743773002
Cr-Commit-Position: refs/heads/master@{#456953}
Committed: https://chromium.googlesource.com/chromium/src/+/0d7277979d62a3b3efc707fb52d3b45b727a04a4
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add BUG=700160 #Patch Set 3 : Rename class Media2 to Media_TBMv2 #
Total comments: 6
Patch Set 4 : Combine battor.tough_video_cases with new benchmark #
Total comments: 4
Patch Set 5 : Improve how category filter is set up #
Total comments: 2
Patch Set 6 : Add Owner to new benchmark #Patch Set 7 : Rebase and resolve merge conflicts #Patch Set 8 : Rebase #
Messages
Total messages: 60 (26 generated)
Description was changed from ========== Port part of media.tough_video_cases to TBMv2 This is the first step of porting media.tough_video_cases to use the new TBMv2 framework, and to eventually add support for BattOr. Currently only CPU time percentage metrics are recorded. Other metrics will be added later. R=nednguyen@google.com ========== to ========== Port part of media.tough_video_cases to TBMv2 This is the first step of porting media.tough_video_cases to use the new TBMv2 framework, and to eventually add support for BattOr. Currently only CPU time percentage metrics are recorded. Other metrics will be added later. R=nednguyen@google.com ==========
johnchen@chromium.org changed reviewers: + benjhayden@chromium.org, crouleau@chromium.org
PTAL
Great to see progress here! Also great to see small, readable CLs as well. https://codereview.chromium.org/2743773002/diff/1/tools/perf/benchmarks/media.py File tools/perf/benchmarks/media.py (right): https://codereview.chromium.org/2743773002/diff/1/tools/perf/benchmarks/media... tools/perf/benchmarks/media.py:55: class Media2(perf_benchmark.PerfBenchmark): How about Media_TBMv2 similar to my comment below? https://codereview.chromium.org/2743773002/diff/1/tools/perf/benchmarks/media... tools/perf/benchmarks/media.py:72: return 'media.tough_video_cases2' I think it is worth naming this 'media.tough_video_cases_tbmv2' similar to 'v8.infinite_scroll_tbmv2' https://cs.chromium.org/chromium/src/tools/perf/benchmarks/v8.py?l=172 Once the current media.tough_video_cases is removed, we can rename 'media.tough_video_cases_tbmv2' to 'media.tough_video_cases' I think this will make the code clearly. Please feel free to push back if you have a different opinion. :)
https://codereview.chromium.org/2743773002/diff/1/tools/perf/benchmarks/media.py File tools/perf/benchmarks/media.py (right): https://codereview.chromium.org/2743773002/diff/1/tools/perf/benchmarks/media... tools/perf/benchmarks/media.py:67: options.SetTimelineBasedMetrics( ['cpuTimeMetric']) Remove extra space
Description was changed from ========== Port part of media.tough_video_cases to TBMv2 This is the first step of porting media.tough_video_cases to use the new TBMv2 framework, and to eventually add support for BattOr. Currently only CPU time percentage metrics are recorded. Other metrics will be added later. R=nednguyen@google.com ========== to ========== Port part of media.tough_video_cases to TBMv2 This is the first step of porting media.tough_video_cases to use the new TBMv2 framework, and to eventually add support for BattOr. Currently only CPU time percentage metrics are recorded. Other metrics will be added later. R=nednguyen@google.com BUG=700160 ==========
On 2017/03/09 21:58:39, crouleau wrote: > Great to see progress here! > > Also great to see small, readable CLs as well. > > https://codereview.chromium.org/2743773002/diff/1/tools/perf/benchmarks/media.py > File tools/perf/benchmarks/media.py (right): > > https://codereview.chromium.org/2743773002/diff/1/tools/perf/benchmarks/media... > tools/perf/benchmarks/media.py:55: class Media2(perf_benchmark.PerfBenchmark): > How about Media_TBMv2 similar to my comment below? > > https://codereview.chromium.org/2743773002/diff/1/tools/perf/benchmarks/media... > tools/perf/benchmarks/media.py:72: return 'media.tough_video_cases2' > I think it is worth naming this > > 'media.tough_video_cases_tbmv2' > > similar to 'v8.infinite_scroll_tbmv2' > https://cs.chromium.org/chromium/src/tools/perf/benchmarks/v8.py?l=172 > > Once the current media.tough_video_cases is removed, we can rename > 'media.tough_video_cases_tbmv2' to 'media.tough_video_cases' > > I think this will make the code clearly. Please feel free to push back if you > have a different opinion. :) Made the suggested changes.
LGTM from high-level perspective. benjhayden and nednguyen can help with the details and best practices around TBMv2 hopefully.
nednguyen@google.com changed reviewers: + charliea@chromium.org
On 2017/03/09 22:28:04, crouleau wrote: > LGTM from high-level perspective. benjhayden and nednguyen can help with the > details and best practices around TBMv2 hopefully. +Charlie, I remember you may have already use tough_video_cases in a battor benchmark. I think we should unify that benchmark with this one?
The CQ bit was checked by johnchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/03/10 13:42:31, nednguyen wrote: > On 2017/03/09 22:28:04, crouleau wrote: > > LGTM from high-level perspective. benjhayden and nednguyen can help with the > > details and best practices around TBMv2 hopefully. > > +Charlie, I remember you may have already use tough_video_cases in a battor > benchmark. I think we should unify that benchmark with this one? My understanding is that the battor.* benchmarks are more like meta-benchmarks to make sure the battor is working correctly. If you visit https://chromeperf.appspot.com/report and view the subtests for battor.tough_video_cases, there are many metrics there that look like they are for diagnosing battor issues. We need permanent media.* tbmv2 benchmarks so we can replace the current media.* legacy benchmarks. After this changelist, John will start adding other metrics including media-specific metrics listed at https://docs.google.com/document/d/1vNdlHEIuLVfhaALaOsQmJYh8ipJ3FA0KTg_uPPPO9...
My understanding is that dproy's thread times metric will replace the cpuTimeMetric. Would it be ok to wait for that metric to land? There's an email thread for coordinating the timing and design.
Just to clarify: based on our offline discussion on Friday, the next step in this CL is to include the deletion of battor.tough_video_cases in this CL.
charliea@chromium.org changed reviewers: + zhenw@chromium.org
https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... File tools/perf/benchmarks/media.py (right): https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:45: class Media(perf_benchmark.PerfBenchmark): I think this should technically be MediaToughVideoCases. nednguyen@google.com, changing this class name doesn't actually affect anything, so we should be able to change it without consequences of affecting data continuity, right? https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:55: class Media_TBMv2(perf_benchmark.PerfBenchmark): I think MediaToughVideoCasesTBMv2 would be a more idiomatic name https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:61: # For cpuTimeMetric: nit: you can put this on the same line as 'toplevel', e.g.: categories = [ 'toplevel', # Required by cpuTimeMetric ] https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:65: ','.join(['-*'] + categories)) Yikes: this filter string is confusing. Looking at it in detail: ','.join(...): join each of the items in the inner list with a comma. Okay, this isn't bad. ['-*'] + categories: Prepend the list with '-*' as the first item. Presumably this removes all categories except those explicitly specified. This should be the same behavior as: = chrome_trace_category_filter.ChromeTraceCategoryFilter(','.join(categories)) I think the rules for trace category filters are: - If nothing is specified, use the default filters - If something is specified, use only that filter zhenw@chromium.org, can you confirm this?
https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... File tools/perf/benchmarks/media.py (right): https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:45: class Media(perf_benchmark.PerfBenchmark): On 2017/03/13 16:21:52, charliea wrote: > I think this should technically be MediaToughVideoCases. > > mailto:nednguyen@google.com, changing this class name doesn't actually affect anything, > so we should be able to change it without consequences of affecting data > continuity, right? correct. Class name doesn't effect the data which the benchmark produces.
https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... File tools/perf/benchmarks/media.py (right): https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:65: ','.join(['-*'] + categories)) On 2017/03/13 16:21:52, charliea wrote: > Yikes: this filter string is confusing. > > Looking at it in detail: > > ','.join(...): join each of the items in the inner list with a comma. Okay, this > isn't bad. > > ['-*'] + categories: Prepend the list with '-*' as the first item. Presumably > this removes all categories except those explicitly specified. This should be > the same behavior as: > > = chrome_trace_category_filter.ChromeTraceCategoryFilter(','.join(categories)) > > I think the rules for trace category filters are: > > - If nothing is specified, use the default filters > - If something is specified, use only that filter > > mailto:zhenw@chromium.org, can you confirm this? As far as I remember, this is correct. I don't really remember why people were adding "-*" before.
I have deleted battor.tough_video_cases, as suggested in offline discussion. The power measurement in battor.tough_video_cases is now moved to the new media.tough_video_cases_tbmv2 benchmark. The clock sync latency measurement isn't moved, since it still exists in other BattOr benchmarks and it isn't necessary to measure it again in the media benchmark.
On 2017/03/13 17:30:41, Zhen Wang wrote: > https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... > File tools/perf/benchmarks/media.py (right): > > https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... > tools/perf/benchmarks/media.py:65: ','.join(['-*'] + categories)) > On 2017/03/13 16:21:52, charliea wrote: > > Yikes: this filter string is confusing. > > > > Looking at it in detail: > > > > ','.join(...): join each of the items in the inner list with a comma. Okay, > this > > isn't bad. > > > > ['-*'] + categories: Prepend the list with '-*' as the first item. Presumably > > this removes all categories except those explicitly specified. This should be > > the same behavior as: > > > > = chrome_trace_category_filter.ChromeTraceCategoryFilter(','.join(categories)) > > > > I think the rules for trace category filters are: > > > > - If nothing is specified, use the default filters > > - If something is specified, use only that filter > > > > mailto:zhenw@chromium.org, can you confirm this? > > As far as I remember, this is correct. I don't really remember why people were > adding "-*" before. I have copied over the equivalent code from battor.tough_video_cases, which is much simpler.
Description was changed from ========== Port part of media.tough_video_cases to TBMv2 This is the first step of porting media.tough_video_cases to use the new TBMv2 framework, and to eventually add support for BattOr. Currently only CPU time percentage metrics are recorded. Other metrics will be added later. R=nednguyen@google.com BUG=700160 ========== to ========== Replace battor.tough_video_cases with media.tough_video_cases_tbmv2 This is part of the effort to port media.tough_video_cases to use the new TBMv2 framework, and to add support for BattOr. Existing measurements of CPU percentage and power consumption have been moved to the new benchmark, and battor.tough_video_cases benchmark has been deleted. R=nednguyen@google.com BUG=700160 ==========
On 2017/03/13 17:55:24, johnchen wrote: > On 2017/03/13 17:30:41, Zhen Wang wrote: > > > https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... > > File tools/perf/benchmarks/media.py (right): > > > > > https://codereview.chromium.org/2743773002/diff/40001/tools/perf/benchmarks/m... > > tools/perf/benchmarks/media.py:65: ','.join(['-*'] + categories)) > > On 2017/03/13 16:21:52, charliea wrote: > > > Yikes: this filter string is confusing. > > > > > > Looking at it in detail: > > > > > > ','.join(...): join each of the items in the inner list with a comma. Okay, > > this > > > isn't bad. > > > > > > ['-*'] + categories: Prepend the list with '-*' as the first item. > Presumably > > > this removes all categories except those explicitly specified. This should > be > > > the same behavior as: > > > > > > = > chrome_trace_category_filter.ChromeTraceCategoryFilter(','.join(categories)) > > > > > > I think the rules for trace category filters are: > > > > > > - If nothing is specified, use the default filters > > > - If something is specified, use only that filter > > > > > > mailto:zhenw@chromium.org, can you confirm this? > > > > As far as I remember, this is correct. I don't really remember why people > were > > adding "-*" before. > > I have copied over the equivalent code from battor.tough_video_cases, which is > much simpler. lgtm but please wait for Charlie
lgtm but please wait for Charlie
LGTM
watk@chromium.org changed reviewers: + watk@chromium.org
Nice! I only had one minor comment https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... File tools/perf/benchmarks/media.py (right): https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:56: class MediaToughVideoCasesTBMv2(perf_benchmark.PerfBenchmark): Not a comment for this CL, just a naming bikeshed :) for when we eventually rename this: I'd prefer if we called this either just Media, ToughMediaCases because it isn't only for video. There are audio stories too. https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:64: options.config.chrome_trace_config.category_filter.AddFilterString('rail') nit: it's weird that you add "toplevel" above and then "rail" here with a gap. Can you group all the category filters into one block so it's easy to see? Take a look at the v8 link below for an example. Also I think it's really useful to have a comment indicating why the category is enabled. It can be quite difficult to find out why a category is enabled from looking at the metrics. e.g., I just had a look at powerMetric.html and it doesn't say which categories it it needs. So if we later change the metrics it's difficult to know which categories we should change. I like the way it's done for some v8 ones: https://cs.chromium.org/chromium/src/tools/perf/benchmarks/v8.py?l=262&rcl=95...
https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... File tools/perf/benchmarks/media.py (right): https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:56: class MediaToughVideoCasesTBMv2(perf_benchmark.PerfBenchmark): On 2017/03/14 00:08:45, watk wrote: > Not a comment for this CL, just a naming bikeshed :) for when we eventually > rename this: I'd prefer if we called this either just Media, ToughMediaCases > because it isn't only for video. There are audio stories too. John and I already discussed this. Our current idea is that after we remove legacy media.tough_video_cases, we will rename TBMv2 tough_media_cases to media.src_cases. This is because it will be parallel with media.mse_cases and (not yet written) media.eme_cases. Then that breaks down media use cases into src, eme, and mse, which I think is most close to how the actual code works. WDYT?
On 2017/03/14 00:12:46, crouleau wrote: > https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... > File tools/perf/benchmarks/media.py (right): > > https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... > tools/perf/benchmarks/media.py:56: class > MediaToughVideoCasesTBMv2(perf_benchmark.PerfBenchmark): > On 2017/03/14 00:08:45, watk wrote: > > Not a comment for this CL, just a naming bikeshed :) for when we eventually > > rename this: I'd prefer if we called this either just Media, ToughMediaCases > > because it isn't only for video. There are audio stories too. > > John and I already discussed this. Our current idea is that after we remove > legacy media.tough_video_cases, we will rename TBMv2 tough_media_cases to > media.src_cases. This is because it will be parallel with media.mse_cases and > (not yet written) media.eme_cases. Then that breaks down media use cases into > src, eme, and mse, which I think is most close to how the actual code works. > WDYT? Nice, sgtm!
On 2017/03/14 00:08:45, watk wrote: > https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... > tools/perf/benchmarks/media.py:64: > options.config.chrome_trace_config.category_filter.AddFilterString('rail') > nit: it's weird that you add "toplevel" above and then "rail" here with a gap. > Can you group all the category filters into one block so it's easy to see? Take > a look at the v8 link below for an example. > > Also I think it's really useful to have a comment indicating why the category is > enabled. It can be quite difficult to find out why a category is enabled from > looking at the metrics. e.g., I just had a look at powerMetric.html and it > doesn't say which categories it it needs. > > So if we later change the metrics it's difficult to know which categories we > should change. > > I like the way it's done for some v8 ones: > https://cs.chromium.org/chromium/src/tools/perf/benchmarks/v8.py?l=262&rcl=95... Thanks for the suggestion. I've updated the code.
On 2017/03/14 01:36:10, johnchen wrote: > On 2017/03/14 00:08:45, watk wrote: > > > https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... > > tools/perf/benchmarks/media.py:64: > > options.config.chrome_trace_config.category_filter.AddFilterString('rail')ply > > nit: it's weird that you add "toplevel" above and then "rail" here with a gap. > > Can you group all the category filters into one block so it's easy to see? > Take > > a look at the v8 link below for an example. > > > > Also I think it's really useful to have a comment indicating why the category > is > > enabled. It can be quite difficult to find out why a category is enabled from > > looking at the metrics. e.g., I just had a look at powerMetric.html and it > > doesn't say which categories it it needs. > > > > So if we later change the metrics it's difficult to know which categories we > > should change. > > > > I like the way it's done for some v8 ones: > > > https://cs.chromium.org/chromium/src/tools/perf/benchmarks/v8.py?l=262&rcl=95... > > Thanks for the suggestion. I've updated the code. lgtm thanks!
lgtm w/ comment https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... File tools/perf/benchmarks/media.py (right): https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:56: class MediaToughVideoCasesTBMv2(perf_benchmark.PerfBenchmark): nednguyen@google.com: if I understand your long-term naming suggestions, you want one benchmark per team with the user stories separated by tags, correct? so there's just two benchmarks, media.desktop and media.mobile, and there's multiple tag sets within those benchmarks ("mse" might be one tag set and "src" might be another). Is this right? I'm not sure how much we want to publicize this long-term plan now given that so little of it is implemented. Regardless, I don't think it affects this CL. https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... File tools/perf/benchmarks/media.py (right): https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:57: """Obtains media metrics using TBMv2. Will eventually replace Media class.""" I think "Media" class should now be "MediaToughVideoCases" class
On 2017/03/14 19:04:21, charliea wrote: > lgtm w/ comment > > https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... > File tools/perf/benchmarks/media.py (right): > > https://codereview.chromium.org/2743773002/diff/60001/tools/perf/benchmarks/m... > tools/perf/benchmarks/media.py:56: class > MediaToughVideoCasesTBMv2(perf_benchmark.PerfBenchmark): > mailto:nednguyen@google.com: if I understand your long-term naming suggestions, you > want one benchmark per team with the user stories separated by tags, correct? so > there's just two benchmarks, media.desktop and media.mobile, and there's > multiple tag sets within those benchmarks ("mse" might be one tag set and "src" > might be another). Is this right? Yes, correct. I think the plan should be: 1) Have an old legacy media benchmark migrated to tbmv2. 2) Verify that the new one is as good as the old ones in term of catching regressions 3) Create a new unify benchmarks (media.desktop & media.mobile) in TBMv2 4) Delete all the old benchmark + temporary TBMv2 one in (1). To see how you can unify the benchmarks, please take a look at: https://docs.google.com/document/d/1t8JG5988sB8ncznEjjQNfurRl6V8B2xML-kX07cQX... > > I'm not sure how much we want to publicize this long-term plan now given that so > little of it is implemented. Regardless, I don't think it affects this CL. > > https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... > File tools/perf/benchmarks/media.py (right): > > https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... > tools/perf/benchmarks/media.py:57: """Obtains media metrics using TBMv2. Will > eventually replace Media class.""" > I think "Media" class should now be "MediaToughVideoCases" class
https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... File tools/perf/benchmarks/media.py (right): https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... tools/perf/benchmarks/media.py:55: @benchmark.Disabled('android') Can you add @benchmark.Owner(...) before landing this?
On 2017/03/14 19:18:41, nednguyen wrote: > https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... > File tools/perf/benchmarks/media.py (right): > > https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... > tools/perf/benchmarks/media.py:55: @benchmark.Disabled('android') > Can you add @benchmark.Owner(...) before landing this? If Owner can be a team, we would want it to be videostack-eng@google.com
On 2017/03/14 19:38:01, crouleau wrote: > On 2017/03/14 19:18:41, nednguyen wrote: > > > https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... > > File tools/perf/benchmarks/media.py (right): > > > > > https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... > > tools/perf/benchmarks/media.py:55: @benchmark.Disabled('android') > > Can you add @benchmark.Owner(...) before landing this? > > If Owner can be a team, we would want it to be mailto:videostack-eng@google.com I cannot find any result in https://bugs.chromium.org/p/chromium/issues/list?can=2&q=owner%3Avideostack-e.... Do media team has a chromium account for the whole team? Owner(...) also support "component=..." which translates to crbug component, but we need a single owner to help triage the bug to when benchmark break.
On 2017/03/14 19:42:33, nednguyen wrote: > On 2017/03/14 19:38:01, crouleau wrote: > > On 2017/03/14 19:18:41, nednguyen wrote: > > > > > > https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... > > > File tools/perf/benchmarks/media.py (right): > > > > > > > > > https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... > > > tools/perf/benchmarks/media.py:55: @benchmark.Disabled('android') > > > Can you add @benchmark.Owner(...) before landing this? > > > > If Owner can be a team, we would want it to be > mailto:videostack-eng@google.com > > I cannot find any result in > https://bugs.chromium.org/p/chromium/issues/list?can=2&q=owner%3Avideostack-e.... > Do media team has a chromium account for the whole team? > > Owner(...) also support "component=..." which translates to crbug component, but > we need a single owner to help triage the bug to when benchmark break. I can put myself as the owner for now, especially since I'll be continue working on it. What would be the right component name for this benchmark?
On 2017/03/14 19:53:56, johnchen wrote: > On 2017/03/14 19:42:33, nednguyen wrote: > > On 2017/03/14 19:38:01, crouleau wrote: > > > On 2017/03/14 19:18:41, nednguyen wrote: > > > > > > > > > > https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... > > > > File tools/perf/benchmarks/media.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2743773002/diff/80001/tools/perf/benchmarks/m... > > > > tools/perf/benchmarks/media.py:55: @benchmark.Disabled('android') > > > > Can you add @benchmark.Owner(...) before landing this? > > > > > > If Owner can be a team, we would want it to be > > mailto:videostack-eng@google.com > > > > I cannot find any result in > > > https://bugs.chromium.org/p/chromium/issues/list?can=2&q=owner%3Avideostack-e.... > > Do media team has a chromium account for the whole team? > > > > Owner(...) also support "component=..." which translates to crbug component, > but > > we need a single owner to help triage the bug to when benchmark break. > > I can put myself as the owner for now, especially since I'll be continue working > on it. > What would be the right component name for this benchmark? Please add me as owner as well. The right component is Internals>Media
Added @benchmark.Owner decoration, and fixed comment.
lgtm
The CQ bit was checked by johnchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by johnchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org, watk@chromium.org, charliea@chromium.org, nednguyen@google.com, crouleau@chromium.org Link to the patchset: https://codereview.chromium.org/2743773002/#ps140001 (title: "Rebase")
The CQ bit was checked by johnchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by johnchen@chromium.org
The CQ bit was unchecked by johnchen@chromium.org
The CQ bit was checked by johnchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489543722216360,
"parent_rev": "db539b8aaf2c0a5e3687f92dfba14a7036aa808f", "commit_rev":
"0d7277979d62a3b3efc707fb52d3b45b727a04a4"}
Message was sent while issue was closed.
Description was changed from ========== Replace battor.tough_video_cases with media.tough_video_cases_tbmv2 This is part of the effort to port media.tough_video_cases to use the new TBMv2 framework, and to add support for BattOr. Existing measurements of CPU percentage and power consumption have been moved to the new benchmark, and battor.tough_video_cases benchmark has been deleted. R=nednguyen@google.com BUG=700160 ========== to ========== Replace battor.tough_video_cases with media.tough_video_cases_tbmv2 This is part of the effort to port media.tough_video_cases to use the new TBMv2 framework, and to add support for BattOr. Existing measurements of CPU percentage and power consumption have been moved to the new benchmark, and battor.tough_video_cases benchmark has been deleted. R=nednguyen@google.com BUG=700160 Review-Url: https://codereview.chromium.org/2743773002 Cr-Commit-Position: refs/heads/master@{#456953} Committed: https://chromium.googlesource.com/chromium/src/+/0d7277979d62a3b3efc707fb52d3... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/0d7277979d62a3b3efc707fb52d3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
