Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
166273009:
PpapiDecryptor: Call NewKeyCB on OnSessionReady(). (Closed)
|
Created:
6 years, 10 months ago by xhwang Modified:
6 years, 10 months ago Reviewers:
ddorwin CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, tinskip1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPpapiDecryptor: Call NewKeyCB on OnSessionReady().
Also updated the browser test to test the case where media pipeline starts to
ask the CDM to do decryption before the session is fully loaded.
BUG=344651
TEST=Updated test.
R=ddorwin@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251934
Patch Set 1 #
Total comments: 13
Patch Set 2 : comments addressed #Patch Set 3 : comments addressed #
Total comments: 8
Patch Set 4 : comments addressed #Patch Set 5 : comments addressed #
Messages
Total messages: 19 (0 generated)
PTAL https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:549: const int64 kDelayToLoadSessionMs = 500; Ideally we should keep the old test (where the delay is 0). But that requires a new key system to be passed in. I am not sure if it's worth the effort... WDYT?
PTAL
minor stuff https://codereview.chromium.org/166273009/diff/1/content/renderer/media/crypt... File content/renderer/media/crypto/ppapi_decryptor.cc (right): https://codereview.chromium.org/166273009/diff/1/content/renderer/media/crypt... content/renderer/media/crypto/ppapi_decryptor.cc:350: if (!new_audio_key_cb_.is_null()) Step 3.5 of the update() algorithm is: If the associated media element(s) are waiting for a key, queue a task to attempt to resume playback. Fortunately, it also says: The user agent may choose to skip this step if it knows resuming will fail (i.e. no usable key was added). We should add some type of comment about what we're doing AND a TODO to rename this callback (something to the effect of should try decrypting again/[re]new[ed] key available) when "ready" is eliminated. https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:271: std::string heartbeat_message; Should we add this before this line? DCHECK(heartbeat_timer_set_); https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:530: void ClearKeyCdm::UpdateLoadableSession() { I think we need a better name for this. Update() is an implementation detail of how we load/initialize the loadable session. How about LoadLoadableSession()? A bit weird. Could do InitializeLoadableSession(). https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:551: &session_id_for_emulated_loadsession_); It might be nice to clarify somewhere that we're using the address of this member as an indication to call UpdateLoadableSession(). https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:552: // Defer OnSessionCreated until the session is updated with ... until the loadable session is initialized? https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:566: // Ignore the Message when we are waiting to update the loadable session. nit: Is upper case 'M' intentional?
comments addressed
comments addressed
PTAL again https://codereview.chromium.org/166273009/diff/1/content/renderer/media/crypt... File content/renderer/media/crypto/ppapi_decryptor.cc (right): https://codereview.chromium.org/166273009/diff/1/content/renderer/media/crypt... content/renderer/media/crypto/ppapi_decryptor.cc:350: if (!new_audio_key_cb_.is_null()) On 2014/02/19 00:04:09, ddorwin wrote: > Step 3.5 of the update() algorithm is: > If the associated media element(s) are waiting for a key, queue a task to > attempt to resume playback. > > Fortunately, it also says: > The user agent may choose to skip this step if it knows resuming will fail (i.e. > no usable key was added). > > We should add some type of comment about what we're doing AND a TODO to rename > this callback (something to the effect of should try decrypting > again/[re]new[ed] key available) when "ready" is eliminated. Done. https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:271: std::string heartbeat_message; On 2014/02/19 00:04:09, ddorwin wrote: > Should we add this before this line? > DCHECK(heartbeat_timer_set_); Done. https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:530: void ClearKeyCdm::UpdateLoadableSession() { On 2014/02/19 00:04:09, ddorwin wrote: > I think we need a better name for this. Update() is an implementation detail of > how we load/initialize the loadable session. How about LoadLoadableSession()? A > bit weird. Could do InitializeLoadableSession(). Done. https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:551: &session_id_for_emulated_loadsession_); On 2014/02/19 00:04:09, ddorwin wrote: > It might be nice to clarify somewhere that we're using the address of this > member as an indication to call UpdateLoadableSession(). Done. https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:552: // Defer OnSessionCreated until the session is updated with On 2014/02/19 00:04:09, ddorwin wrote: > ... until the loadable session is initialized? Done. https://codereview.chromium.org/166273009/diff/1/media/cdm/ppapi/external_cle... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:566: // Ignore the Message when we are waiting to update the loadable session. On 2014/02/19 00:04:09, ddorwin wrote: > nit: Is upper case 'M' intentional? Done.
LGTM Some minor issues, but they can be addressed in a future CL if you want. https://codereview.chromium.org/166273009/diff/20006/content/renderer/media/c... File content/renderer/media/crypto/ppapi_decryptor.cc (right): https://codereview.chromium.org/166273009/diff/20006/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:350: // Based on the spec, we need to resume playback when a session is updated, or "updated" is a bit ambiguous. It's really when update() completes successfully. https://codereview.chromium.org/166273009/diff/20006/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:351: // when a session is loaded. In both cases, the CDM fires OnSessionReady() same/similar: successfully loaded https://codereview.chromium.org/166273009/diff/20006/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:353: // TODO(xhwang): Rename OnSessionReady callback to OnSessionUpdated. SessionUpdated does not seem to cover the loaded case. Also, since "The user agent may choose to skip this step if it knows resuming will fail (i.e. no usable key was added)", we can be more precise. I really think this callback should indicate that a new key is available or available again. https://codereview.chromium.org/166273009/diff/20006/media/cdm/ppapi/external... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/166273009/diff/20006/media/cdm/ppapi/external... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:556: // kLoadableSessionKey. nit: The fact that it is loaded with this specific key is irrelevant to our decision here, right? Just drop " with kLoadableSessionKey"?
comments addressed
https://codereview.chromium.org/166273009/diff/20006/content/renderer/media/c... File content/renderer/media/crypto/ppapi_decryptor.cc (right): https://codereview.chromium.org/166273009/diff/20006/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:350: // Based on the spec, we need to resume playback when a session is updated, or On 2014/02/19 00:38:13, ddorwin wrote: > "updated" is a bit ambiguous. It's really when update() completes successfully. Done. https://codereview.chromium.org/166273009/diff/20006/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:351: // when a session is loaded. In both cases, the CDM fires OnSessionReady() On 2014/02/19 00:38:13, ddorwin wrote: > same/similar: successfully loaded Done. https://codereview.chromium.org/166273009/diff/20006/content/renderer/media/c... content/renderer/media/crypto/ppapi_decryptor.cc:353: // TODO(xhwang): Rename OnSessionReady callback to OnSessionUpdated. On 2014/02/19 00:38:13, ddorwin wrote: > SessionUpdated does not seem to cover the loaded case. Also, since "The user > agent may choose to skip this step if it knows resuming will fail (i.e. no > usable key was added)", we can be more precise. I really think this callback > should indicate that a new key is available or available again. Done. https://codereview.chromium.org/166273009/diff/20006/media/cdm/ppapi/external... File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right): https://codereview.chromium.org/166273009/diff/20006/media/cdm/ppapi/external... media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:556: // kLoadableSessionKey. On 2014/02/19 00:38:13, ddorwin wrote: > nit: The fact that it is loaded with this specific key is irrelevant to our > decision here, right? Just drop " with kLoadableSessionKey"? 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/166273009/2
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
comments addressed
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/166273009/290001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Message was sent while issue was closed.
Committed patchset #5 manually as r251934 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
