Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
770423004:
Add dedicated PipelineStatus UMAs for encrypted playback (Closed)
DescriptionAdd dedicated PipelineStatus UMAs for encrypted content to differentiate DecryptingVideoDecoder (DVD) & DecryptingDemuxerStream (DDS) codepath.
Tested the DVD path using netflix/Linux. DDS needs a chromeOS build.
BUG=435374
Committed: https://crrev.com/c481239303dd43278633714d434ec4ff6f9c7a06
Cr-Commit-Position: refs/heads/master@{#307062}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address CL comments #Patch Set 3 : Remove audio_dds #
Total comments: 23
Patch Set 4 : Address CL comments #
Total comments: 10
Patch Set 5 : Address CL comments #
Messages
Total messages: 21 (5 generated)
prabhur@chromium.org changed reviewers: + dalecurtis@chromium.org, isherman@chromium.org
PTAL.
LGTM. However, I'm noticing that this is starting to explode out into a quite a lot of histograms. Please think about whether you need all of these in the long-term; and if not, please come back and clean these up at some point. Thanks.
lgtm https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:341: base::LinearHistogram::FactoryGet( Is this the (new?) hotness for dynamic UMA names?
Thanks for the review. CC xhwang & ddorwin as FYI. https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:341: base::LinearHistogram::FactoryGet( On 2014/12/02 23:21:17, DaleCurtis wrote: > Is this the (new?) hotness for dynamic UMA names? Done. https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:341: base::LinearHistogram::FactoryGet( On 2014/12/02 23:21:17, DaleCurtis wrote: > Is this the (new?) hotness for dynamic UMA names? Yep. I think this API has been around for sometime.
The CQ bit was checked by prabhur@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/770423004/1
The CQ bit was unchecked by xhwang@chromium.org
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
Sorry for the late comments. https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:203: bool audio_dds; nit: typically audio goes before video https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:326: } else if (player_info.video_dds || player_info.audio_dds) { Now AudioVideo.VP8.DDS could be because we use audio DDS, which will be very confusing. Since we are not checking DecryptingAudioDecoder(DAD) either, I suggest we don't check audio_dds as well. https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:327: pipeline_uma_prefix += "DDS"; When we use DDS, we are not checking HW vs SW anymore. Actually it'll be interesting to check how often we use DDS with SW. This should be rare, but if we see that a lot, then something is broken. So is it possible we could have? codec.HW codec.SW codec.DVD codec.DDS.HW codec.DDS.SW https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:334: return pipeline_uma_prefix; Returning a uma prefix seems weird. How about something like: static const char kPipelineUmaPrefix[] = "Media.PipelineStatus.AudioVideo."; std::string uma_name = kPipelineUmaPrefix; uma_name += ... return uma_name; https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:342: GetUMANameForAVStream(player_info), media::PIPELINE_OK, media::PIPELINE_OK is 0, which is not recommended. This is a common pitfall in the base::LinearHistogram::FactoryGet(): https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo...
PTAL #PS3 https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:203: bool audio_dds; On 2014/12/03 21:39:50, xhwang wrote: > nit: typically audio goes before video Done. https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:326: } else if (player_info.video_dds || player_info.audio_dds) { On 2014/12/03 21:39:49, xhwang wrote: > Now AudioVideo.VP8.DDS could be because we use audio DDS, which will be very > confusing. Since we are not checking DecryptingAudioDecoder(DAD) either, I > suggest we don't check audio_dds as well. Done. https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:327: pipeline_uma_prefix += "DDS"; On 2014/12/03 21:39:50, xhwang wrote: > When we use DDS, we are not checking HW vs SW anymore. Actually it'll be > interesting to check how often we use DDS with SW. This should be rare, but if > we see that a lot, then something is broken. > > So is it possible we could have? > codec.HW > codec.SW > codec.DVD > codec.DDS.HW > codec.DDS.SW Done. https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:334: return pipeline_uma_prefix; On 2014/12/03 21:39:50, xhwang wrote: > Returning a uma prefix seems weird. How about something like: > > static const char kPipelineUmaPrefix[] = "Media.PipelineStatus.AudioVideo."; > > std::string uma_name = kPipelineUmaPrefix; > uma_name += ... > > return uma_name; Done. https://codereview.chromium.org/770423004/diff/1/content/browser/media/media_... content/browser/media/media_internals.cc:342: GetUMANameForAVStream(player_info), media::PIPELINE_OK, On 2014/12/03 21:39:50, xhwang wrote: > media::PIPELINE_OK is 0, which is not recommended. This is a common pitfall in > the base::LinearHistogram::FactoryGet(): > > https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... > > https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... > > > Done.
https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:216: // Helper to generate pipelinestatus_UMA_prefix for AudioVideo streams. pipelinestatus_UMA_prefix is not correct https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:223: typedef std::map<int, PlayerInfoMap> RendererPlayerMap; Not related to your change, why a map of PipelineInfo is called PlayerInfoMap? Can we make the name consistent? https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:306: const PipelineInfo& player_info) { ditto about pipelineinfo vs playerinfo https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:318: } nit: add empty line here. https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:321: uma_name += "DVD."; You can return here. DVD.SW is redundant. DVD would be sufficient. https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:322: } else if (player_info.video_dds) { If you return above, you don't need "else" here. https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:324: } nit: add empty line here. https://codereview.chromium.org/770423004/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/770423004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58425: + <suffix name="AudioVideo.VP8.DVD.SW" ditto https://codereview.chromium.org/770423004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58432: + <suffix name="AudioVideo.VP9.DDS" DDS.SW and DDS.HW? https://codereview.chromium.org/770423004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58438: + <suffix name="AudioVideo.VP9.DVD.HW" You can not have DVD.HW. https://codereview.chromium.org/770423004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58453: + <suffix name="AudioVideo.H264.DVD.SW" ditto.
PTAL PS4 https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:216: // Helper to generate pipelinestatus_UMA_prefix for AudioVideo streams. On 2014/12/04 00:37:33, xhwang wrote: > pipelinestatus_UMA_prefix is not correct Done. https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:223: typedef std::map<int, PlayerInfoMap> RendererPlayerMap; On 2014/12/04 00:37:33, xhwang wrote: > Not related to your change, why a map of PipelineInfo is called PlayerInfoMap? > Can we make the name consistent? The map stores some state (pipelineInfo in this case) for every player - keyed by playerId. Let me know if you feel strongly about changing it - I can change it. https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:306: const PipelineInfo& player_info) { On 2014/12/04 00:37:33, xhwang wrote: > ditto about pipelineinfo vs playerinfo see earlier response. https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:318: } On 2014/12/04 00:37:33, xhwang wrote: > nit: add empty line here. Done. https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:321: uma_name += "DVD."; On 2014/12/04 00:37:33, xhwang wrote: > You can return here. DVD.SW is redundant. DVD would be sufficient. Done. https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:322: } else if (player_info.video_dds) { On 2014/12/04 00:37:33, xhwang wrote: > If you return above, you don't need "else" here. Done. https://codereview.chromium.org/770423004/diff/40001/content/browser/media/me... content/browser/media/media_internals.cc:324: } On 2014/12/04 00:37:33, xhwang wrote: > nit: add empty line here. Done. https://codereview.chromium.org/770423004/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/770423004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58425: + <suffix name="AudioVideo.VP8.DVD.SW" On 2014/12/04 00:37:33, xhwang wrote: > ditto Done. https://codereview.chromium.org/770423004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58432: + <suffix name="AudioVideo.VP9.DDS" On 2014/12/04 00:37:33, xhwang wrote: > DDS.SW and DDS.HW? Done. https://codereview.chromium.org/770423004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58432: + <suffix name="AudioVideo.VP9.DDS" On 2014/12/04 00:37:33, xhwang wrote: > DDS.SW and DDS.HW? ah..sorry about that! https://codereview.chromium.org/770423004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58438: + <suffix name="AudioVideo.VP9.DVD.HW" On 2014/12/04 00:37:33, xhwang wrote: > You can not have DVD.HW. Done. https://codereview.chromium.org/770423004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58453: + <suffix name="AudioVideo.H264.DVD.SW" On 2014/12/04 00:37:33, xhwang wrote: > ditto. Done.
The CL title is not a complete sentence. Comment for the CL description: > Add dedicated UMAs for encrypted content to differentiate DecryptingVideoDecoder (DVD) & DecryptingDemuxerStream (DDS) codepath. > Tested the DVD path using netflix/Linux. DSD needs a chromeOS build. s/DSD/DDS You can run this test to try decrypt-only: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...
lgtm % nits and sugesstions for the future https://codereview.chromium.org/770423004/diff/60001/content/browser/media/me... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/770423004/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:216: // Helper to generate pipelinestatus UMA name for AudioVideo streams. pipelinestatus are two words, either "pipeline status" or "PipelineStatus" https://codereview.chromium.org/770423004/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/770423004/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58416: label="PipelineStatus for AV streams with VP8 Software decoder."/> here and below s/Soft/soft https://codereview.chromium.org/770423004/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58424: decoder."/> nit: It seems we have SW before HW in all other places. Reverse the order here as well? https://codereview.chromium.org/770423004/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/770423004/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58417: + <suffix name="AudioVideo.VP8.HW" I chatted with ddorwin on this. For video decoders, we already have SW, HW and DVD. But we only have four real VideoDecoders in total: DVD, GVD, FFmpegVD and VpxVD. Since we already have the name for each, we might just use decoder names in the UMA. For example, instead of VP8.HW, we have VP8.GVD; VP8.SW will be split to VP8.FFmpegVideoDecoder and VP8.VpxVideoDecoder, etc. Some combinations are invalid and we don't need to list it in this file, e.g. H264.VpxVideoDecoder. You don't have to do this now, but we should consider this when we work on the long term plan to improve/clean-up these UMAs. https://codereview.chromium.org/770423004/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58418: + label="PipelineStatus for AV streams with VP8 Hardware decoder."/> here and below s/Hard/hard
On 2014/12/04 21:04:02, xhwang wrote: > The CL title is not a complete sentence. > > Comment for the CL description: > > > Add dedicated UMAs for encrypted content to differentiate > DecryptingVideoDecoder (DVD) & DecryptingDemuxerStream (DDS) codepath. > > > Tested the DVD path using netflix/Linux. DSD needs a chromeOS build. > > s/DSD/DDS > > You can run this test to try decrypt-only: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med... Thanks I fixed it. I ran the browser tests - but not sure how to verify the DDS.* histograms - there is no UI to look at the histograms. Is there a different way ?
Thanks for all the comments! Addressed the final nits & comments except those mentioned as future improvements. https://codereview.chromium.org/770423004/diff/60001/content/browser/media/me... File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/770423004/diff/60001/content/browser/media/me... content/browser/media/media_internals.cc:216: // Helper to generate pipelinestatus UMA name for AudioVideo streams. On 2014/12/04 21:15:22, xhwang wrote: > pipelinestatus are two words, either "pipeline status" or "PipelineStatus" Done. https://codereview.chromium.org/770423004/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/770423004/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58416: label="PipelineStatus for AV streams with VP8 Software decoder."/> On 2014/12/04 21:15:22, xhwang wrote: > here and below s/Soft/soft Done. https://codereview.chromium.org/770423004/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58424: decoder."/> On 2014/12/04 21:15:22, xhwang wrote: > nit: It seems we have SW before HW in all other places. Reverse the order here > as well? Done. https://codereview.chromium.org/770423004/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/770423004/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58417: + <suffix name="AudioVideo.VP8.HW" On 2014/12/04 21:15:22, xhwang wrote: > I chatted with ddorwin on this. For video decoders, we already have SW, HW and > DVD. But we only have four real VideoDecoders in total: DVD, GVD, FFmpegVD and > VpxVD. Since we already have the name for each, we might just use decoder names > in the UMA. For example, instead of VP8.HW, we have VP8.GVD; VP8.SW will be > split to VP8.FFmpegVideoDecoder and VP8.VpxVideoDecoder, etc. > > Some combinations are invalid and we don't need to list it in this file, e.g. > H264.VpxVideoDecoder. > > You don't have to do this now, but we should consider this when we work on the > long term plan to improve/clean-up these UMAs. Acknowledged. https://codereview.chromium.org/770423004/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:58418: + label="PipelineStatus for AV streams with VP8 Hardware decoder."/> On 2014/12/04 21:15:22, xhwang wrote: > here and below s/Hard/hard Done.
The CQ bit was checked by prabhur@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/770423004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c481239303dd43278633714d434ec4ff6f9c7a06 Cr-Commit-Position: refs/heads/master@{#307062} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
