Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2265893003:
media: Add UMA to report CDM interface version being used (Closed)
|
Created:
4 years, 4 months ago by xhwang Modified:
4 years, 4 months ago CC:
chromium-reviews, eme-reviews_chromium.org, asvitkine+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Add UMA to report CDM interface version being used
This will help us understand what CDM interface versions are being
used. This can help us decide whether the support of an old CDM
interface version can be dropped.
BUG=570216
TEST=Check that the histogram is successfully generated.
Committed: https://crrev.com/b46705d568877857541375eaba4767abcfd6a329
Cr-Commit-Position: refs/heads/master@{#413988}
Patch Set 1 #
Total comments: 15
Patch Set 2 : comments addressed #
Total comments: 2
Patch Set 3 : comments addressed #
Messages
Total messages: 30 (11 generated)
xhwang@chromium.org changed reviewers: + ddorwin@chromium.org, isherman@chromium.org
This is a small CL adding one UMA. ddorwin: Please review from media/EME's perspective. isherman: Please review from UMA's perspective.
https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/ppapi/ppap... File media/cdm/ppapi/ppapi_cdm_adapter.cc (right): https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/ppapi/ppap... media/cdm/ppapi/ppapi_cdm_adapter.cc:362: cdm::ContentDecryptionModule::kVersion + 1); Just FYI: cdm::ContentDecryptionModule::kVersion is 8 for now. cdm->GetVersion() can return 7 or 8. In the future, cdm::ContentDecryptionModule::kVersion could grow, e.g. to 9, and cdm->GetVersion() can return some integer that is <= 9.
Description comments: * The BUG probably doesn't provide any context. Maybe just reference the decision we need to make in the comment thread. * nit: add a comma before "which" https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/cdm_wrapper.h File media/cdm/cdm_wrapper.h (right): https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/cdm_wrappe... media/cdm/cdm_wrapper.h:64: virtual int GetVersion() = 0; Get*API*Version or something like that. (In theory, the Host and CDM APIs could diverge, but I guess we don't need to worry about that.) Also, update the CL description. https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/ppapi/ppap... File media/cdm/ppapi/ppapi_cdm_adapter.cc (right): https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/ppapi/ppap... media/cdm/ppapi/ppapi_cdm_adapter.cc:361: "Media.EME.CdmVersion", cdm->GetVersion(), Update this name too. https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/ppapi/ppap... media/cdm/ppapi/ppapi_cdm_adapter.cc:362: cdm::ContentDecryptionModule::kVersion + 1); On 2016/08/22 17:59:38, xhwang (slow) wrote: > Just FYI: cdm::ContentDecryptionModule::kVersion is 8 for now. cdm->GetVersion() > can return 7 or 8. In the future, cdm::ContentDecryptionModule::kVersion could > grow, e.g. to 9, and cdm->GetVersion() can return some integer that is <= 9. What is the +1 doing here? It might be worth commenting this. Currently, it is set to the max version supported by the adapter/Chrome + 1. Why is that? https://chromiumcodereview.appspot.com/2265893003/diff/1/tools/metrics/histog... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/2265893003/diff/1/tools/metrics/histog... tools/metrics/histograms/histograms.xml:22305: +<histogram name="Media.EME.CdmVersion"> Update name https://chromiumcodereview.appspot.com/2265893003/diff/1/tools/metrics/histog... tools/metrics/histograms/histograms.xml:22308: + Version of the CDM interface that the created CDM uses. This is recorded "the created CDM" is unclear in this context (UMA dashboard).
https://codereview.chromium.org/2265893003/diff/1/media/cdm/ppapi/ppapi_cdm_a... File media/cdm/ppapi/ppapi_cdm_adapter.cc (right): https://codereview.chromium.org/2265893003/diff/1/media/cdm/ppapi/ppapi_cdm_a... media/cdm/ppapi/ppapi_cdm_adapter.cc:362: cdm::ContentDecryptionModule::kVersion + 1); Could this be recorded as a sparse histogram?
https://codereview.chromium.org/2265893003/diff/1/media/cdm/ppapi/ppapi_cdm_a... File media/cdm/ppapi/ppapi_cdm_adapter.cc (right): https://codereview.chromium.org/2265893003/diff/1/media/cdm/ppapi/ppapi_cdm_a... media/cdm/ppapi/ppapi_cdm_adapter.cc:362: cdm::ContentDecryptionModule::kVersion + 1); On 2016/08/22 20:53:36, Ilya Sherman wrote: > Could this be recorded as a sparse histogram? This will be a very limited set. For now, it'll only be 7 and 8. In the future, it'll be something like 8 and 9, then 9 and 10 etc. Do you suggest using a sparse histogram for this case? BTW, I am using the pepper interface to report UMA. It seems like this interface doesn't support sparse histogram yet :(
https://codereview.chromium.org/2265893003/diff/1/media/cdm/ppapi/ppapi_cdm_a... File media/cdm/ppapi/ppapi_cdm_adapter.cc (right): https://codereview.chromium.org/2265893003/diff/1/media/cdm/ppapi/ppapi_cdm_a... media/cdm/ppapi/ppapi_cdm_adapter.cc:362: cdm::ContentDecryptionModule::kVersion + 1); On 2016/08/22 21:51:22, xhwang (slow) wrote: > On 2016/08/22 20:53:36, Ilya Sherman wrote: > > Could this be recorded as a sparse histogram? > > This will be a very limited set. For now, it'll only be 7 and 8. In the future, > it'll be something like 8 and 9, then 9 and 10 etc. Do you suggest using a > sparse histogram for this case? > > BTW, I am using the pepper interface to report UMA. It seems like this interface > doesn't support sparse histogram yet :( It's okay to use a dense histogram as long as the values are small. If you prefer that approach, could you please add an assertion to verify that the version number is not very large, and a comment to document why that assertion is in place?
Description was changed from ========== media: Add UMA to report CDM version being used This will help us understand what CDM versions are being used which can help us decide whether the support of an old CDM version can be dropped. BUG=570216 TEST=Check that the histogram is successfully generated. ========== to ========== media: Add UMA to report CDM interface version being used This will help us understand what CDM interface versions are being used. This can help us decide whether the support of an old CDM interface version can be dropped. BUG=570216 TEST=Check that the histogram is successfully generated. ==========
Patchset #2 (id:20001) has been deleted
comments addressed
PTAL again https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/cdm_wrapper.h File media/cdm/cdm_wrapper.h (right): https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/cdm_wrappe... media/cdm/cdm_wrapper.h:64: virtual int GetVersion() = 0; On 2016/08/22 20:38:15, ddorwin wrote: > Get*API*Version or something like that. > (In theory, the Host and CDM APIs could diverge, but I guess we don't need to > worry about that.) > > Also, update the CL description. Done. https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/ppapi/ppap... File media/cdm/ppapi/ppapi_cdm_adapter.cc (right): https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/ppapi/ppap... media/cdm/ppapi/ppapi_cdm_adapter.cc:361: "Media.EME.CdmVersion", cdm->GetVersion(), On 2016/08/22 20:38:15, ddorwin wrote: > Update this name too. Done. https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/ppapi/ppap... media/cdm/ppapi/ppapi_cdm_adapter.cc:362: cdm::ContentDecryptionModule::kVersion + 1); On 2016/08/22 20:38:15, ddorwin wrote: > On 2016/08/22 17:59:38, xhwang (slow) wrote: > > Just FYI: cdm::ContentDecryptionModule::kVersion is 8 for now. > cdm->GetVersion() > > can return 7 or 8. In the future, cdm::ContentDecryptionModule::kVersion could > > grow, e.g. to 9, and cdm->GetVersion() can return some integer that is <= 9. > > What is the +1 doing here? It might be worth commenting this. > > Currently, it is set to the max version supported by the adapter/Chrome + 1. Why > is that? This part is a bit confusing. The doc says that the "value" should be strictly less than the "boundary", hence the +1: https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?rcl=0&l=253 I added a comment here. https://chromiumcodereview.appspot.com/2265893003/diff/1/media/cdm/ppapi/ppap... media/cdm/ppapi/ppapi_cdm_adapter.cc:362: cdm::ContentDecryptionModule::kVersion + 1); On 2016/08/22 22:10:38, Ilya Sherman wrote: > On 2016/08/22 21:51:22, xhwang (slow) wrote: > > On 2016/08/22 20:53:36, Ilya Sherman wrote: > > > Could this be recorded as a sparse histogram? > > > > This will be a very limited set. For now, it'll only be 7 and 8. In the > future, > > it'll be something like 8 and 9, then 9 and 10 etc. Do you suggest using a > > sparse histogram for this case? > > > > BTW, I am using the pepper interface to report UMA. It seems like this > interface > > doesn't support sparse histogram yet :( > > It's okay to use a dense histogram as long as the values are small. If you > prefer that approach, could you please add an assertion to verify that the > version number is not very large, and a comment to document why that assertion > is in place? Done. https://chromiumcodereview.appspot.com/2265893003/diff/1/tools/metrics/histog... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/2265893003/diff/1/tools/metrics/histog... tools/metrics/histograms/histograms.xml:22305: +<histogram name="Media.EME.CdmVersion"> On 2016/08/22 20:38:15, ddorwin wrote: > Update name Done. https://chromiumcodereview.appspot.com/2265893003/diff/1/tools/metrics/histog... tools/metrics/histograms/histograms.xml:22308: + Version of the CDM interface that the created CDM uses. This is recorded On 2016/08/22 20:38:15, ddorwin wrote: > "the created CDM" is unclear in this context (UMA dashboard). Done.
Metrics lgtm % a nit: https://chromiumcodereview.appspot.com/2265893003/diff/40001/media/cdm/ppapi/... File media/cdm/ppapi/ppapi_cdm_adapter.cc (right): https://chromiumcodereview.appspot.com/2265893003/diff/40001/media/cdm/ppapi/... media/cdm/ppapi/ppapi_cdm_adapter.cc:363: PP_DCHECK(cdm->GetInterfaceVersion() <= 100); Please set the upper bound to something closer to 20.
lgtm
comments addressed
https://chromiumcodereview.appspot.com/2265893003/diff/40001/media/cdm/ppapi/... File media/cdm/ppapi/ppapi_cdm_adapter.cc (right): https://chromiumcodereview.appspot.com/2265893003/diff/40001/media/cdm/ppapi/... media/cdm/ppapi/ppapi_cdm_adapter.cc:363: PP_DCHECK(cdm->GetInterfaceVersion() <= 100); On 2016/08/23 02:35:32, Ilya Sherman wrote: > Please set the upper bound to something closer to 20. Done with 30, which will be enough for several years.
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, ddorwin@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2265893003/#ps60001 (title: "comments addressed")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by xhwang@chromium.org
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== media: Add UMA to report CDM interface version being used This will help us understand what CDM interface versions are being used. This can help us decide whether the support of an old CDM interface version can be dropped. BUG=570216 TEST=Check that the histogram is successfully generated. ========== to ========== media: Add UMA to report CDM interface version being used This will help us understand what CDM interface versions are being used. This can help us decide whether the support of an old CDM interface version can be dropped. BUG=570216 TEST=Check that the histogram is successfully generated. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== media: Add UMA to report CDM interface version being used This will help us understand what CDM interface versions are being used. This can help us decide whether the support of an old CDM interface version can be dropped. BUG=570216 TEST=Check that the histogram is successfully generated. ========== to ========== media: Add UMA to report CDM interface version being used This will help us understand what CDM interface versions are being used. This can help us decide whether the support of an old CDM interface version can be dropped. BUG=570216 TEST=Check that the histogram is successfully generated. Committed: https://crrev.com/b46705d568877857541375eaba4767abcfd6a329 Cr-Commit-Position: refs/heads/master@{#413988} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b46705d568877857541375eaba4767abcfd6a329 Cr-Commit-Position: refs/heads/master@{#413988} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
