Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
404613003:
Update supported key system info from browser process if needed. (Closed)
|
Created:
6 years, 5 months ago by xhwang Modified:
6 years, 5 months ago Reviewers:
ddorwin CC:
chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, eme-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionUpdate supported key system info from browser process if needed.
BUG=381484
TEST=Tested on Windows.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284587
Patch Set 1 #
Total comments: 1
Patch Set 2 : rebase only #
Total comments: 10
Patch Set 3 : comments addressed #
Total comments: 2
Patch Set 4 : comments addressed #Messages
Total messages: 16 (0 generated)
Tested on Windows and the canPlayType() result is updated after the CDM update. The plugin still doesn't load though, which I'll address in a separate CL. https://codereview.chromium.org/404613003/diff/1/content/renderer/media/crypt... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/404613003/diff/1/content/renderer/media/crypt... content/renderer/media/crypto/key_systems.cc:224: DVLOG(2) << __FUNCTION__; I'll remove this.
rebase only
https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:177: g_key_systems.Get().UpdateIfNeeded(); Get() is not "free": https://code.google.com/p/chromium/codesearch#chromium/src/base/lazy_instance... So, a local var may actually be a good idea here. https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:207: if (IsConcreteSupportedKeySystem(kWidevineKeySystem)) Technically, we don't need to do this unless the request is for Widevine. Practically, this has the desired behavior. For this reason and because IsConcreteSupportedKeySystem() requires some amount of CPU cycles, maybe we should instead have a member variable that tracks whether all delay-installed CDMs are present. (This might not make sense if there were multiple, but it is sufficient in practice.) The member variable could be set in UpdateSupportedKeySystems() and checked here. https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:219: UpdateSupportedKeySystems(); How does this affect the UMAs? https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:225: concrete_key_system_map_.clear(); Calling this function outside the constructor (as at l219) could cause a race condition. LazyInstance ensures synchronized creation, which includes initializing the object's values as we do at l198. We probably need some type of read/write lock to prevent a conflict between use and updating. Alternatively, maybe all these calls occur on the main renderer thread. In that case, some DCHECKs would ensure we don't run into the race condition.
comments addressed
PTAL again. https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:177: g_key_systems.Get().UpdateIfNeeded(); On 2014/07/18 19:57:21, ddorwin wrote: > Get() is not "free": > https://code.google.com/p/chromium/codesearch#chromium/src/base/lazy_instance... > > So, a local var may actually be a good idea here. Done. https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:207: if (IsConcreteSupportedKeySystem(kWidevineKeySystem)) On 2014/07/18 19:57:21, ddorwin wrote: > Technically, we don't need to do this unless the request is for Widevine. > Practically, this has the desired behavior. > > For this reason and because IsConcreteSupportedKeySystem() requires some amount > of CPU cycles, maybe we should instead have a member variable that tracks > whether all delay-installed CDMs are present. (This might not make sense if > there were multiple, but it is sufficient in practice.) > > The member variable could be set in UpdateSupportedKeySystems() and checked > here. Done. https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:219: UpdateSupportedKeySystems(); On 2014/07/18 19:57:21, ddorwin wrote: > How does this affect the UMAs? UMA is reported in IsSupportedKeySystemWithMediaMimeType(), which is orthogonal to this CL. This CL makes the KeySystems info more up-to-date. It should also make the UMA stats more accurate. https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:225: concrete_key_system_map_.clear(); On 2014/07/18 19:57:21, ddorwin wrote: > Calling this function outside the constructor (as at l219) could cause a race > condition. LazyInstance ensures synchronized creation, which includes > initializing the object's values as we do at l198. > > We probably need some type of read/write lock to prevent a conflict between use > and updating. Alternatively, maybe all these calls occur on the main renderer > thread. In that case, some DCHECKs would ensure we don't run into the race > condition. All KeySystems calls are made on the render main thread. I added a thread checker to check this.
lgtm with nit https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:219: UpdateSupportedKeySystems(); On 2014/07/19 00:58:08, xhwang wrote: > On 2014/07/18 19:57:21, ddorwin wrote: > > How does this affect the UMAs? > > UMA is reported in IsSupportedKeySystemWithMediaMimeType(), which is orthogonal > to this CL. This CL makes the KeySystems info more up-to-date. It should also > make the UMA stats more accurate. But we'll get a UMA for failed and later succeeded? I guess that's a function of the update and could have happened in a separate tab already. https://codereview.chromium.org/404613003/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/404613003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:158: extra line
comments addressed
https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/404613003/diff/20001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:219: UpdateSupportedKeySystems(); On 2014/07/20 15:47:25, ddorwin wrote: > On 2014/07/19 00:58:08, xhwang wrote: > > On 2014/07/18 19:57:21, ddorwin wrote: > > > How does this affect the UMAs? > > > > UMA is reported in IsSupportedKeySystemWithMediaMimeType(), which is > orthogonal > > to this CL. This CL makes the KeySystems info more up-to-date. It should also > > make the UMA stats more accurate. > > But we'll get a UMA for failed and later succeeded? I guess that's a function of > the update and could have happened in a separate tab already. That's right. key_systems_support_uma_ could report false and true later. https://codereview.chromium.org/404613003/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/404613003/diff/40001/content/renderer/media/c... content/renderer/media/crypto/key_systems.cc:158: On 2014/07/20 15:47:26, ddorwin wrote: > extra line Done.
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/404613003/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/404613003/60001
Message was sent while issue was closed.
Change committed as 284587 |
