Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846693002:
Add UMA metrics for VideoCodec.MP4 and VideoCodec.WebM (Closed)
DescriptionAdd UMA metrics for Media.{SRC,MSE}.VideoCodec.{MP4,WebM}
BUG=715161
TEST=Manually tested with SRC/MSE playback (then checked about:://histograms)
Review-Url: https://codereview.chromium.org/2846693002
Cr-Commit-Position: refs/heads/master@{#469469}
Committed: https://chromium.googlesource.com/chromium/src/+/02b14ba085a0bb98b8d87cd713fb13b9c6d2821a
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address comment #1 #
Total comments: 3
Patch Set 3 : Get container name from ffmpeg_glue #
Total comments: 5
Patch Set 4 : Address comment #
Total comments: 2
Patch Set 5 : Add DCHECK(open_called_) #
Total comments: 2
Patch Set 6 : rename to Media.SRC #
Total comments: 7
Patch Set 7 : Address comments #
Messages
Total messages: 45 (18 generated)
The CQ bit was checked by kqyang@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...
Description was changed from ========== Add UMA metrics for VideoCodec.MP4 and VideoCodec.WebM BUG=715161 ========== to ========== Add UMA metrics for VideoCodec.MP4 and VideoCodec.WebM BUG=715161 ==========
kqyang@chromium.org changed reviewers: + wolenetz@chromium.org, xhwang@chromium.org
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...)
Thanks! Just a few comments/questions. https://codereview.chromium.org/2846693002/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2846693002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:163: static void RecordVideoCodecStats(const std::string& format_names, Can you add a comment about what "format_names" should look like? https://codereview.chromium.org/2846693002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:172: if (format_names.find("mp4") != std::string::npos) { What will happen if we have a file named mp4.webm? https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27828: +<histogram name="Media.MSE.VideoCodec.MP4" enum="MSECodec"> OOC, is there any reason you can't just use enum="VideoCodec" here? https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27829: + <owner>kqyang@chromium.org</owner> The new recommendation is to use media-dev@chromium.org as the owner. https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:28650: + Video codec used in HTML5 media if the media container is MP4. This makes people think that it's for all HTML5 playback, including MSE and SRC, but from the implementation this is only for SRC. We should probably either add "SRC" into this UMA name, or if we prefer the current name, report to this UMA from all demuxers.
Thanks for reviewing. PTAL. https://codereview.chromium.org/2846693002/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2846693002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:163: static void RecordVideoCodecStats(const std::string& format_names, On 2017/04/27 16:32:50, xhwang_slow wrote: > Can you add a comment about what "format_names" should look like? Done. https://codereview.chromium.org/2846693002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:172: if (format_names.find("mp4") != std::string::npos) { On 2017/04/27 16:32:50, xhwang_slow wrote: > What will happen if we have a file named mp4.webm? |format_names| only contains format names :) Hopefully it will be clearer with the comments above. Also changed the code to pass in "const char*" to avoid an extra copying in string construction. https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27828: +<histogram name="Media.MSE.VideoCodec.MP4" enum="MSECodec"> On 2017/04/27 16:32:50, xhwang_slow wrote: > OOC, is there any reason you can't just use enum="VideoCodec" here? Unfortunately the enums used by SRC and MSE are different: https://cs.chromium.org/chromium/src/media/base/video_codecs.h?rcl=e052a49fd7... https://cs.chromium.org/chromium/src/media/filters/stream_parser_factory.cc?r... https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27829: + <owner>kqyang@chromium.org</owner> On 2017/04/27 16:32:50, xhwang_slow wrote: > The new recommendation is to use mailto:media-dev@chromium.org as the owner. Done. https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:28650: + Video codec used in HTML5 media if the media container is MP4. On 2017/04/27 16:32:50, xhwang_slow wrote: > This makes people think that it's for all HTML5 playback, including MSE and SRC, > but from the implementation this is only for SRC. We should probably either add > "SRC" into this UMA name, or if we prefer the current name, report to this UMA > from all demuxers. I think we want to track MSE and SRC separately. I can change the name here to Media.SRC.VideoCodec.MP4/WebM, but it will make the names inconsistent to the existing "Media.VideoCodec" definition. Are you ok with that?
https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27828: +<histogram name="Media.MSE.VideoCodec.MP4" enum="MSECodec"> On 2017/04/27 18:12:53, kqyang wrote: > On 2017/04/27 16:32:50, xhwang_slow wrote: > > OOC, is there any reason you can't just use enum="VideoCodec" here? > > Unfortunately the enums used by SRC and MSE are different: > https://cs.chromium.org/chromium/src/media/base/video_codecs.h?rcl=e052a49fd7... > https://cs.chromium.org/chromium/src/media/filters/stream_parser_factory.cc?r... hmm... good find, I didn't know about this wolenetz: Do you know the history about MSECodec? Does it make sense to convert this to VideoCodec and AudioCodec so we only have one set of codec UMA enums? Currently we have MP3 listed as a bucket in Media.MSE.VideoCodec, which seems odd. https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:28650: + Video codec used in HTML5 media if the media container is MP4. On 2017/04/27 18:12:53, kqyang wrote: > On 2017/04/27 16:32:50, xhwang_slow wrote: > > This makes people think that it's for all HTML5 playback, including MSE and > SRC, > > but from the implementation this is only for SRC. We should probably either > add > > "SRC" into this UMA name, or if we prefer the current name, report to this UMA > > from all demuxers. > > I think we want to track MSE and SRC separately. > > I can change the name here to Media.SRC.VideoCodec.MP4/WebM, but it will make > the names inconsistent to the existing "Media.VideoCodec" definition. Are you ok > with that? Media.VideoCodec was probably added before MSE existed. We should probably update it to include MSE as well.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
https://codereview.chromium.org/2846693002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2846693002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:174: if (strstr(format_names, "mp4")) { ffmpeg_glue has copies of these names too. I think we should find a way to dedupe them. Probably we should make constants out of the exact names and put them in ffmpeg_Common. ffmpeg_common could actually declare the proper extern to access the ff_mov_demuxer.name value too if we wanted.
https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2846693002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:28650: + Video codec used in HTML5 media if the media container is MP4. On 2017/04/27 19:00:53, xhwang_slow wrote: > On 2017/04/27 18:12:53, kqyang wrote: > > On 2017/04/27 16:32:50, xhwang_slow wrote: > > > This makes people think that it's for all HTML5 playback, including MSE and > > SRC, > > > but from the implementation this is only for SRC. We should probably either > > add > > > "SRC" into this UMA name, or if we prefer the current name, report to this > UMA > > > from all demuxers. > > > > I think we want to track MSE and SRC separately. > > > > I can change the name here to Media.SRC.VideoCodec.MP4/WebM, but it will make > > the names inconsistent to the existing "Media.VideoCodec" definition. Are you > ok > > with that? > > Media.VideoCodec was probably added before MSE existed. We should probably > update it to include MSE as well. I did some code search. Surprisingly, this does not just affect one or two UMA metrics, instead, it affects more than >15 metrics. You can search for "UMA_HISTOGRAM" in "src/media/filters/ffmpeg_demuxer.cc", none of these metrics track MSE playbacks. We could fix them by either renaming the metric names to something like: Media.SRC.VideoXxx or extends them to cover MSE as well (this will take some time). In any case, I don't think it is a good idea to fix them in this CL. I have opened a separate bug to track this: crbug/716183 https://codereview.chromium.org/2846693002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2846693002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:174: if (strstr(format_names, "mp4")) { On 2017/04/27 19:05:28, DaleCurtis wrote: > ffmpeg_glue has copies of these names too. I think we should find a way to > dedupe them. Probably we should make constants out of the exact names and put > them in ffmpeg_Common. ffmpeg_common could actually declare the proper extern to > access the ff_mov_demuxer.name value too if we wanted. How about make container[1] a member variable of ffmpeg_glue? We can then access the container from here? container should have already been filled in as RecordVideoCodecStats is done after glue->OpenContext. [1] https://cs.chromium.org/chromium/src/media/filters/ffmpeg_glue.cc?rcl=94ad18d...
https://codereview.chromium.org/2846693002/diff/20001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2846693002/diff/20001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:174: if (strstr(format_names, "mp4")) { On 2017/04/27 at 21:07:59, kqyang wrote: > On 2017/04/27 19:05:28, DaleCurtis wrote: > > ffmpeg_glue has copies of these names too. I think we should find a way to > > dedupe them. Probably we should make constants out of the exact names and put > > them in ffmpeg_Common. ffmpeg_common could actually declare the proper extern to > > access the ff_mov_demuxer.name value too if we wanted. > > How about make container[1] a member variable of ffmpeg_glue? We can then access the container from here? > > container should have already been filled in as RecordVideoCodecStats is done after glue->OpenContext. > > [1] https://cs.chromium.org/chromium/src/media/filters/ffmpeg_glue.cc?rcl=94ad18d... Good suggestion, sgtm.
On 2017/04/27 21:10:43, DaleCurtis wrote: > https://codereview.chromium.org/2846693002/diff/20001/media/filters/ffmpeg_de... > File media/filters/ffmpeg_demuxer.cc (right): > > https://codereview.chromium.org/2846693002/diff/20001/media/filters/ffmpeg_de... > media/filters/ffmpeg_demuxer.cc:174: if (strstr(format_names, "mp4")) { > On 2017/04/27 at 21:07:59, kqyang wrote: > > On 2017/04/27 19:05:28, DaleCurtis wrote: > > > ffmpeg_glue has copies of these names too. I think we should find a way to > > > dedupe them. Probably we should make constants out of the exact names and > put > > > them in ffmpeg_Common. ffmpeg_common could actually declare the proper > extern to > > > access the ff_mov_demuxer.name value too if we wanted. > > > > How about make container[1] a member variable of ffmpeg_glue? We can then > access the container from here? > > > > container should have already been filled in as RecordVideoCodecStats is done > after glue->OpenContext. > > > > [1] > https://cs.chromium.org/chromium/src/media/filters/ffmpeg_glue.cc?rcl=94ad18d... > > Good suggestion, sgtm. Done. PTAL
https://codereview.chromium.org/2846693002/diff/40001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2846693002/diff/40001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:172: if (container == container_names::CONTAINER_MOV) { There are a few other usages of the iformat.name in this file, want to fix them up at the same time? https://codereview.chromium.org/2846693002/diff/40001/media/filters/ffmpeg_gl... File media/filters/ffmpeg_glue.cc (right): https://codereview.chromium.org/2846693002/diff/40001/media/filters/ffmpeg_gl... media/filters/ffmpeg_glue.cc:188: else No need since default is unknown?
PTAL https://codereview.chromium.org/2846693002/diff/40001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2846693002/diff/40001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:172: if (container == container_names::CONTAINER_MOV) { On 2017/04/27 21:51:02, DaleCurtis wrote: > There are a few other usages of the iformat.name in this file, want to fix them > up at the same time? Done. https://codereview.chromium.org/2846693002/diff/40001/media/filters/ffmpeg_gl... File media/filters/ffmpeg_glue.cc (right): https://codereview.chromium.org/2846693002/diff/40001/media/filters/ffmpeg_gl... media/filters/ffmpeg_glue.cc:188: else On 2017/04/27 21:51:02, DaleCurtis wrote: > No need since default is unknown? Sure. (I am more comfortable to have an extra guarantee. I wasn't sure how FfmpegGlue would be used, e.g. would the same object be used on multiple different files?)
glue changes lgtm, defer to xhwang@ for the rest. https://codereview.chromium.org/2846693002/diff/40001/media/filters/ffmpeg_gl... File media/filters/ffmpeg_glue.cc (right): https://codereview.chromium.org/2846693002/diff/40001/media/filters/ffmpeg_gl... media/filters/ffmpeg_glue.cc:188: else On 2017/04/27 at 22:04:57, kqyang wrote: > On 2017/04/27 21:51:02, DaleCurtis wrote: > > No need since default is unknown? > > Sure. > > (I am more comfortable to have an extra guarantee. I wasn't sure how FfmpegGlue would be used, e.g. would the same object be used on multiple different files?) This can only be called once and UNKNOWN should never be a valid value here. https://codereview.chromium.org/2846693002/diff/60001/media/filters/ffmpeg_gl... File media/filters/ffmpeg_glue.h (right): https://codereview.chromium.org/2846693002/diff/60001/media/filters/ffmpeg_gl... media/filters/ffmpeg_glue.h:75: // Returns the container name. Note that it is only available after Line break. DCHECK(open_called_); ?
+wolenetz The change lg. But I'd like to get wolenetz's opinion on potentially consolidating MseCodecs and Audio/Video Codecs for UMA reporting.
Thanks for reviewing. https://codereview.chromium.org/2846693002/diff/60001/media/filters/ffmpeg_gl... File media/filters/ffmpeg_glue.h (right): https://codereview.chromium.org/2846693002/diff/60001/media/filters/ffmpeg_gl... media/filters/ffmpeg_glue.h:75: // Returns the container name. Note that it is only available after On 2017/04/27 22:10:49, DaleCurtis wrote: > Line break. DCHECK(open_called_); ? Done.
lgtm w.r.t. consolidating histograms for MSE and SRC= codecs, that can be done separately from this CL. I'll file crbug to track that now.
https://codereview.chromium.org/2846693002/diff/80001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2846693002/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:174: UMA_HISTOGRAM_ENUMERATION("Media.VideoCodec.MP4", video_config.codec(), Shall we just name this Media.SRC.VideoCodec.MP4 to avoid confusion? Then probably add a TODO to fix other UMAs in this file and link the bug you filed.
https://codereview.chromium.org/2846693002/diff/80001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2846693002/diff/80001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:174: UMA_HISTOGRAM_ENUMERATION("Media.VideoCodec.MP4", video_config.codec(), On 2017/05/03 05:33:45, xhwang wrote: > Shall we just name this Media.SRC.VideoCodec.MP4 to avoid confusion? Then > probably add a TODO to fix other UMAs in this file and link the bug you filed. I don't recall the previous reason why we didn't do this already, but I think it was discussed years ago. Maybe it was related to dashboard queries with the old (no "SRC") bucket names or just generally low pri? I agree with your suggestion; kqyang@ please modify these SRC= container histograms to be Media.SRC.*.* and add TODOs around l.171 and l.146 in this file, with a link to https://crbug.com/717199 in the TODOs and CL description. xhwang@, using "SRC" to mean a non-MSE, non-MediaStream playback is a bit of tribal knowledge. I can't think of a better name, so SRC SGTM (just might confuse folks unaware that "MediaSource Extensions" is *not* "SRC" :)
On 2017/05/03 20:48:24, wolenetz wrote: > https://codereview.chromium.org/2846693002/diff/80001/media/filters/ffmpeg_de... > File media/filters/ffmpeg_demuxer.cc (right): > > https://codereview.chromium.org/2846693002/diff/80001/media/filters/ffmpeg_de... > media/filters/ffmpeg_demuxer.cc:174: > UMA_HISTOGRAM_ENUMERATION("Media.VideoCodec.MP4", video_config.codec(), > On 2017/05/03 05:33:45, xhwang wrote: > > Shall we just name this Media.SRC.VideoCodec.MP4 to avoid confusion? Then > > probably add a TODO to fix other UMAs in this file and link the bug you filed. > > I don't recall the previous reason why we didn't do this already, but I think it > was discussed years ago. Maybe it was related to dashboard queries with the old > (no "SRC") bucket names or just generally low pri? I agree with your suggestion; > kqyang@ please modify these SRC= container histograms to be Media.SRC.*.* and > add TODOs around l.171 and l.146 in this file, with a link to > https://crbug.com/717199 in the TODOs and CL description. > > xhwang@, using "SRC" to mean a non-MSE, non-MediaStream playback is a bit of > tribal knowledge. I can't think of a better name, so SRC SGTM (just might > confuse folks unaware that "MediaSource Extensions" is *not* "SRC" :) We already have MSE/SRC/EME in a bunch of media UMAs (e.g. WatchTime), so this is not new :) We could use Media.VideoCodec.MP4 to all playbacks (MSE+SRC), but today MSE and SRC are using two different codec enums so that needs to be solved separately.
On 2017/05/03 21:36:56, xhwang wrote: > On 2017/05/03 20:48:24, wolenetz wrote: > > > https://codereview.chromium.org/2846693002/diff/80001/media/filters/ffmpeg_de... > > File media/filters/ffmpeg_demuxer.cc (right): > > > > > https://codereview.chromium.org/2846693002/diff/80001/media/filters/ffmpeg_de... > > media/filters/ffmpeg_demuxer.cc:174: > > UMA_HISTOGRAM_ENUMERATION("Media.VideoCodec.MP4", video_config.codec(), > > On 2017/05/03 05:33:45, xhwang wrote: > > > Shall we just name this Media.SRC.VideoCodec.MP4 to avoid confusion? Then > > > probably add a TODO to fix other UMAs in this file and link the bug you > filed. > > > > I don't recall the previous reason why we didn't do this already, but I think > it > > was discussed years ago. Maybe it was related to dashboard queries with the > old > > (no "SRC") bucket names or just generally low pri? I agree with your > suggestion; > > kqyang@ please modify these SRC= container histograms to be Media.SRC.*.* and > > add TODOs around l.171 and l.146 in this file, with a link to > > https://crbug.com/717199 in the TODOs and CL description. > > > > xhwang@, using "SRC" to mean a non-MSE, non-MediaStream playback is a bit of > > tribal knowledge. I can't think of a better name, so SRC SGTM (just might > > confuse folks unaware that "MediaSource Extensions" is *not* "SRC" :) > > We already have MSE/SRC/EME in a bunch of media UMAs (e.g. WatchTime), so this > is not new :) > > We could use Media.VideoCodec.MP4 to all playbacks (MSE+SRC), but today MSE and > SRC are using two different codec enums so that needs to be solved separately. Done. PTAL. (Btw, anyone know how can I test this feature? Thanks.)
LGTM % nits For testing, there's no automated tests for UMA AFAIK. You can manually test it and check about://histograms to see whether you see the histograms you added. https://codereview.chromium.org/2846693002/diff/100001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2846693002/diff/100001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:171: // TODO(crbug/716183): Fix these misleading metric names. They should be nit: TODO(ldap) .... See http://crbug.com/716183 https://codereview.chromium.org/2846693002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2846693002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:28213: + Video codec used in HTML5 media if the media container is MP4. nit: Mention this is only for SRC playback. https://codereview.chromium.org/2846693002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:28220: + Video codec used in HTML5 media if the media container is WebM. ditto
lgtm % nit (duplicating some of xhwang@'s I think) https://codereview.chromium.org/2846693002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2846693002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:28220: + Video codec used in HTML5 media if the media container is WebM. nit here and in the .SRC., above: I think the forthcoming Media.ALL.* (see bug 716183) should have this text, and the .SRC. metrics added here should be more specific. xhwang@, WDYT about "Video codec used in plain src= (not MSE) HTML5 media if the media container is {WebM,MP4}" ? We'll also (eventually) want to be even more clear about which metrics might count things in multiple places (ALL and SRC/MSE and EME, and maybe EME and SRC/MSE) -- this further clarification can and should happen as part of fixing crbug 716183 IMHO.
On 2017/05/04 18:46:00, wolenetz wrote: > lgtm % nit (duplicating some of xhwang@'s I think) > > https://codereview.chromium.org/2846693002/diff/100001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2846693002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:28220: + Video codec used in HTML5 > media if the media container is WebM. > nit here and in the .SRC., above: I think the forthcoming Media.ALL.* (see bug > 716183) should have this text, and the .SRC. metrics added here should be more > specific. > > xhwang@, WDYT about "Video codec used in plain src= (not MSE) HTML5 media if the > media container is {WebM,MP4}" ? > > We'll also (eventually) want to be even more clear about which metrics might > count things in multiple places (ALL and SRC/MSE and EME, and maybe EME and > SRC/MSE) -- this further clarification can and should happen as part of fixing > crbug 716183 IMHO. Also, please update the CL description and title --> Add UMA metrics for Media.VideoCodec.{SRC,MSE}.{MP4,WebM}
Description was changed from
==========
Add UMA metrics for VideoCodec.MP4 and VideoCodec.WebM
BUG=715161
==========
to
==========
Add UMA metrics for
Media.{SRC,MSE}.VideoCodec.{MP4,WebM}
BUG=715161
==========
Done. Thanks for reviewing! https://codereview.chromium.org/2846693002/diff/100001/media/filters/ffmpeg_d... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2846693002/diff/100001/media/filters/ffmpeg_d... media/filters/ffmpeg_demuxer.cc:171: // TODO(crbug/716183): Fix these misleading metric names. They should be On 2017/05/04 18:34:53, xhwang wrote: > nit: TODO(ldap) .... See http://crbug.com/716183 Done. Put your name inside TODO as the bug is assigned to you. Hope you don't mind :) https://codereview.chromium.org/2846693002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2846693002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:28213: + Video codec used in HTML5 media if the media container is MP4. On 2017/05/04 18:34:53, xhwang wrote: > nit: Mention this is only for SRC playback. Done. https://codereview.chromium.org/2846693002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:28220: + Video codec used in HTML5 media if the media container is WebM. On 2017/05/04 18:46:00, wolenetz wrote: > nit here and in the .SRC., above: I think the forthcoming Media.ALL.* (see bug > 716183) should have this text, and the .SRC. metrics added here should be more > specific. > > xhwang@, WDYT about "Video codec used in plain src= (not MSE) HTML5 media if the > media container is {WebM,MP4}" ? > > We'll also (eventually) want to be even more clear about which metrics might > count things in multiple places (ALL and SRC/MSE and EME, and maybe EME and > SRC/MSE) -- this further clarification can and should happen as part of fixing > crbug 716183 IMHO. Done.
Description was changed from
==========
Add UMA metrics for
Media.{SRC,MSE}.VideoCodec.{MP4,WebM}
BUG=715161
==========
to
==========
Add UMA metrics for Media.{SRC,MSE}.VideoCodec.{MP4,WebM}
BUG=715161
TEST=Manually tested with SRC/MSE playback (then checked about:://histograms)
==========
The CQ bit was checked by kqyang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, xhwang@chromium.org, wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/2846693002/#ps120001 (title: "Address comments")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
kqyang@chromium.org changed reviewers: + asvitkine@chromium.org
kqyang@chromium.org changed reviewers: + holte@google.com
Added asvitkine@ and holte@ for histograms.xml approval. Please review. Thanks.
holte@chromium.org changed reviewers: + holte@chromium.org
lgtm
The CQ bit was checked by kqyang@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": 120001, "attempt_start_ts": 1493929933230430,
"parent_rev": "d817ddec78c9de78881a1661fa7f03e077d32d36", "commit_rev":
"02b14ba085a0bb98b8d87cd713fb13b9c6d2821a"}
Message was sent while issue was closed.
Description was changed from
==========
Add UMA metrics for Media.{SRC,MSE}.VideoCodec.{MP4,WebM}
BUG=715161
TEST=Manually tested with SRC/MSE playback (then checked about:://histograms)
==========
to
==========
Add UMA metrics for Media.{SRC,MSE}.VideoCodec.{MP4,WebM}
BUG=715161
TEST=Manually tested with SRC/MSE playback (then checked about:://histograms)
Review-Url: https://codereview.chromium.org/2846693002
Cr-Commit-Position: refs/heads/master@{#469469}
Committed:
https://chromium.googlesource.com/chromium/src/+/02b14ba085a0bb98b8d87cd713fb...
==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/02b14ba085a0bb98b8d87cd713fb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
