Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814613002:
Organize tough_video_cases by adding tags. (Closed)
|
Created:
3 years, 8 months ago by CalebRouleau Modified:
3 years, 8 months ago CC:
chromium-reviews, posciak+watch_chromium.org, sullivan, telemetry-reviews_chromium.org, yihongg1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionOrganize tough_video_cases by adding tags describing media file format.
This is the first change. Later changes will remove unnecessary pages.
Design doc:
https://docs.google.com/document/d/1TvUWPl6diK3DTJdSWMOcdz-6qnKC6KEHAZqoBBD8lIc/edit#heading=h.j6gogx2lkhfb
BUG=710253
Review-Url: https://codereview.chromium.org/2814613002
Cr-Commit-Position: refs/heads/master@{#463836}
Committed: https://chromium.googlesource.com/chromium/src/+/2ae7c8c0e3d01a2d73df388ffce7fb8045bcf0b0
Patch Set 1 #Patch Set 2 : fix syntax #
Total comments: 4
Patch Set 3 : 'pcm_s16le' => 'pcm' #
Total comments: 2
Patch Set 4 : Add tags list to check for invalid values #
Total comments: 13
Patch Set 5 : Style stuff. #Patch Set 6 : Fix syntax + missed codecs. #Patch Set 7 : Fix missed tag. #Patch Set 8 : Fix tag problems, and test this time. #Messages
Total messages: 32 (12 generated)
Description was changed from ========== Organize tough_video_cases by adding tags. This is the first change. Later changes will remove unneccessary pages. Design doc: https://docs.google.com/document/d/1TvUWPl6diK3DTJdSWMOcdz-6qnKC6KEHAZqoBBD8l... BUG=710253 ========== to ========== Organize tough_video_cases by adding tags describing media file format. This is the first change. Later changes will remove unnecessary pages. Design doc: https://docs.google.com/document/d/1TvUWPl6diK3DTJdSWMOcdz-6qnKC6KEHAZqoBBD8l... BUG=710253 ==========
crouleau@chromium.org changed reviewers: + dalecurtis@chromium.org, johnchen@chromium.org, nednguyen@google.com
PTAL
https://codereview.chromium.org/2814613002/diff/20001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_video_cases.py (right): https://codereview.chromium.org/2814613002/diff/20001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:49: tags=['pcm_s16le', 'audio_only']) Just pcm? Don't think we care to go through the full matrix of pcm formats. https://codereview.chromium.org/2814613002/diff/20001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:63: tags=['vorbis', 'audio_only']) Is it enough to tag with codecs? audio_only vs audio+video vs video_only is then inferred.
Thanks for review comments so far. I addressed them. https://codereview.chromium.org/2814613002/diff/20001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_video_cases.py (right): https://codereview.chromium.org/2814613002/diff/20001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:49: tags=['pcm_s16le', 'audio_only']) On 2017/04/10 23:25:34, DaleCurtis wrote: > Just pcm? Don't think we care to go through the full matrix of pcm formats. Done. https://codereview.chromium.org/2814613002/diff/20001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:63: tags=['vorbis', 'audio_only']) On 2017/04/10 23:25:34, DaleCurtis wrote: > Is it enough to tag with codecs? audio_only vs audio+video vs video_only is then > inferred. Yes, it is true that you could infer that information from the other two tags, but the problem is that you would have trouble filtering for it with a flag since filtering normally only allows simple logic like "contains tag x" or "does not contain tag y". It would be difficult and overly complex to write something like "contains both an audio codec (audio codecs are in this list) and a video codec (video codecs are in this list)". I doubt the filtering that Ned mentioned supports this, and I don't think it should.
https://codereview.chromium.org/2814613002/diff/40001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_video_cases.py (right): https://codereview.chromium.org/2814613002/diff/40001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:12: url=url, page_set=page_set, tags=tags) To avoid typo, I suggest you make a list of all acceptable tags used by media stories & add assertion here that tags should be a subset of that list.
Thanks for the helpful comments! https://codereview.chromium.org/2814613002/diff/40001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_video_cases.py (right): https://codereview.chromium.org/2814613002/diff/40001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:12: url=url, page_set=page_set, tags=tags) On 2017/04/10 23:43:08, nednguyen wrote: > To avoid typo, I suggest you make a list of all acceptable tags used by media > stories & add assertion here that tags should be a subset of that list. Good idea. Done.
lgtm if other folks are happy https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_video_cases.py (right): https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:33: for tag in tags: nits: for t in tags I usually try to avoid variable names that are only different by a single suffix since that can easily cause typo (especially in dynamic language like python).
https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_video_cases.py (right): https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:24: 'is_50fps', Probably tags like 720p60, 2160p60, etc would be more descriptive. Long term I don't think it's worth having a random 50fps test case here.
Thanks for the comments! https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_video_cases.py (right): https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:24: 'is_50fps', On 2017/04/11 00:08:01, DaleCurtis wrote: > Probably tags like 720p60, 2160p60, etc would be more descriptive. Long term I > don't think it's worth having a random 50fps test case here. Agreed. I'm just organizing stuff up in this CL without removing anything. I will delete this tag later on, but right now the benchmark file is depending on it to work correctly. The idea of having 720p60 as a tag is interesting. Would that be better than two separate tags '720p' and '60fps'? Then you should be able to filter for tests that are both 720p and 60fps... this is perhaps better discussed in the design doc since I will not implement this in this CL. https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:33: for tag in tags: On 2017/04/11 00:04:02, nednguyen wrote: > nits: for t in tags > > I usually try to avoid variable names that are only different by a single suffix > since that can easily cause typo (especially in dynamic language like python). I disagree in general with using single character names, but I will change if you feel strongly about it.
https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_video_cases.py (right): https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:24: 'is_50fps', On 2017/04/11 at 00:26:50, crouleau wrote: > On 2017/04/11 00:08:01, DaleCurtis wrote: > > Probably tags like 720p60, 2160p60, etc would be more descriptive. Long term I > > don't think it's worth having a random 50fps test case here. > > Agreed. I'm just organizing stuff up in this CL without removing anything. I will delete this tag later on, but right now the benchmark file is depending on it to work correctly. > > The idea of having 720p60 as a tag is interesting. Would that be better than two separate tags '720p' and '60fps'? Then you should be able to filter for tests that are both 720p and 60fps... this is perhaps better discussed in the design doc since I will not implement this in this CL. Depends on if tag filtering is a regex or not. If you can say something like 720p* or *p60$, then it doesn't matter.
https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_video_cases.py (right): https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:33: for tag in tags: On 2017/04/11 00:26:50, crouleau wrote: > On 2017/04/11 00:04:02, nednguyen wrote: > > nits: for t in tags > > > > I usually try to avoid variable names that are only different by a single > suffix > > since that can easily cause typo (especially in dynamic language like python). > > I disagree in general with using single character names, but I will change if > you feel strongly about it. Using single character for counter or iterator is ok (https://google.github.io/styleguide/pyguide.html?showone=Naming#Naming). I would prefer doing so to avoid possible typo.
https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_video_cases.py (right): https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:7: PAGE_TAGS_LIST = [ Do we expect other modules to need access to this list? If not, the variable name should start with "_". https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:32: if tags: Probably makes more sense to make tags required now.
Thanks for the reviews. https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_video_cases.py (right): https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:7: PAGE_TAGS_LIST = [ On 2017/04/11 00:50:49, johnchen wrote: > Do we expect other modules to need access to this list? If not, the variable > name should start with "_". Done. https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:7: PAGE_TAGS_LIST = [ On 2017/04/11 00:50:49, johnchen wrote: > Do we expect other modules to need access to this list? If not, the variable > name should start with "_". Done. https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:24: 'is_50fps', On 2017/04/11 00:34:08, DaleCurtis wrote: > On 2017/04/11 at 00:26:50, crouleau wrote: > > On 2017/04/11 00:08:01, DaleCurtis wrote: > > > Probably tags like 720p60, 2160p60, etc would be more descriptive. Long term > I > > > don't think it's worth having a random 50fps test case here. > > > > Agreed. I'm just organizing stuff up in this CL without removing anything. I > will delete this tag later on, but right now the benchmark file is depending on > it to work correctly. > > > > The idea of having 720p60 as a tag is interesting. Would that be better than > two separate tags '720p' and '60fps'? Then you should be able to filter for > tests that are both 720p and 60fps... this is perhaps better discussed in the > design doc since I will not implement this in this CL. > > Depends on if tag filtering is a regex or not. If you can say something like > 720p* or *p60$, then it doesn't matter. Yeah, that's true. I'll look into it further. https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:32: if tags: On 2017/04/11 00:50:49, johnchen wrote: > Probably makes more sense to make tags required now. Done. https://codereview.chromium.org/2814613002/diff/60001/tools/perf/page_sets/to... tools/perf/page_sets/tough_video_cases.py:33: for tag in tags: On 2017/04/11 00:40:24, nednguyen wrote: > On 2017/04/11 00:26:50, crouleau wrote: > > On 2017/04/11 00:04:02, nednguyen wrote: > > > nits: for t in tags > > > > > > I usually try to avoid variable names that are only different by a single > > suffix > > > since that can easily cause typo (especially in dynamic language like > python). > > > > I disagree in general with using single character names, but I will change if > > you feel strongly about it. > > Using single character for counter or iterator is ok > (https://google.github.io/styleguide/pyguide.html?showone=Naming#Naming). I > would prefer doing so to avoid possible typo. Just because they say you don't have to avoid it doesn't mean it is recommended. I still disagree, but this isn't worth arguing over. I changed it to t.
LGTM
lgtm provided we fix the is_4k, is_50fps tags in a followup.
The CQ bit was checked by crouleau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, johnchen@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2814613002/#ps100001 (title: "Fix syntax + missed codecs.")
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
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 crouleau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnchen@chromium.org, nednguyen@google.com, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2814613002/#ps70002 (title: "Fix missed tag.")
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
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 crouleau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnchen@chromium.org, nednguyen@google.com, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2814613002/#ps130001 (title: "Fix tag problems, and test this time.")
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": 130001, "attempt_start_ts": 1491949290200240,
"parent_rev": "46f77da2781539ca0cefc84e7bf86b138bea7bc0", "commit_rev":
"2ae7c8c0e3d01a2d73df388ffce7fb8045bcf0b0"}
Message was sent while issue was closed.
Description was changed from ========== Organize tough_video_cases by adding tags describing media file format. This is the first change. Later changes will remove unnecessary pages. Design doc: https://docs.google.com/document/d/1TvUWPl6diK3DTJdSWMOcdz-6qnKC6KEHAZqoBBD8l... BUG=710253 ========== to ========== Organize tough_video_cases by adding tags describing media file format. This is the first change. Later changes will remove unnecessary pages. Design doc: https://docs.google.com/document/d/1TvUWPl6diK3DTJdSWMOcdz-6qnKC6KEHAZqoBBD8l... BUG=710253 Review-Url: https://codereview.chromium.org/2814613002 Cr-Commit-Position: refs/heads/master@{#463836} Committed: https://chromium.googlesource.com/chromium/src/+/2ae7c8c0e3d01a2d73df388ffce7... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001) as https://chromium.googlesource.com/chromium/src/+/2ae7c8c0e3d01a2d73df388ffce7... |
