Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
423633002:
Make HTMLMediaElement.setMediaKeys() asynchronous. (Closed)
|
Created:
6 years, 5 months ago by jrummell Modified:
6 years, 3 months ago CC:
abarth-chromium, blink-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, jamesr, philipj_slow Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionMake HTMLMediaElement.setMediaKeys() asynchronous.
BUG=358271
TEST=Updated EME layouts tests pass
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182644
Patch Set 1 #
Total comments: 10
Patch Set 2 : sync method #
Total comments: 54
Patch Set 3 : No more states #
Total comments: 16
Patch Set 4 : Changes #
Total comments: 6
Patch Set 5 : make private #
Total comments: 4
Patch Set 6 : Callback #
Total comments: 4
Patch Set 7 : Remove clearing (+rebase) #
Total comments: 2
Patch Set 8 : expand comment #
Total comments: 4
Patch Set 9 : lowercase #Patch Set 10 : rebase #Patch Set 11 : Move initial_cdm to constructor #
Total comments: 2
Patch Set 12 : rebase + add comment #Messages
Total messages: 41 (5 generated)
PTAL. Chromium side changes in https://codereview.chromium.org/416333011/ need to be submitted first.
Initial feedback on C++. https://codereview.chromium.org/423633002/diff/1/Source/modules/encryptedmedi... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h (right): https://codereview.chromium.org/423633002/diff/1/Source/modules/encryptedmedi... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:57: friend class SetMediaKeysWorker; "Worker" has specific JS-facing meanings. Is there a better name? https://codereview.chromium.org/423633002/diff/1/Source/web/WebMediaPlayerCli... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/423633002/diff/1/Source/web/WebMediaPlayerCli... Source/web/WebMediaPlayerClientImpl.cpp:57: // This happens asynchronously. Since there is no way to report an error, This is the scenario xhwang was referring to in the other CL. Maybe we should DCHECK that it doesn't fail. https://codereview.chromium.org/423633002/diff/1/Source/web/WebMediaPlayerCli... Source/web/WebMediaPlayerClientImpl.cpp:59: class IgnoreResponseContentDecryptionModuleResult FINAL : public ContentDecryptionModuleResult { We should come up with a better name, especially if we make changes related to the comments above and below. https://codereview.chromium.org/423633002/diff/1/Source/web/WebMediaPlayerCli... Source/web/WebMediaPlayerClientImpl.cpp:240: m_webMediaPlayer->setContentDecryptionModule(HTMLMediaElementEncryptedMedia::contentDecryptionModule(mediaElement()), result->result()); Hmm, I wonder if this needs to be synchronous due to the load call that follows. They at least need to be serialized. Maybe the new Result class above should call load() in complete(). That would be the safest solution. HOWEVER, load() is probably expected to be synchronous, so we might need to block here or something. Hmm. Alternatively, maybe we should keep (and rename) the synchronous version of setContentDecryptionModule for this very scenario. https://codereview.chromium.org/423633002/diff/1/public/platform/WebMediaPlay... File public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/423633002/diff/1/public/platform/WebMediaPlay... public/platform/WebMediaPlayer.h:158: virtual void setContentDecryptionModule(WebContentDecryptionModule* cdm) { } FIXME: Remove.
Updated. Includes a rebase (sorry) that mostly removes some code, but should be easy to tell. https://codereview.chromium.org/423633002/diff/1/Source/modules/encryptedmedi... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h (right): https://codereview.chromium.org/423633002/diff/1/Source/modules/encryptedmedi... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:57: friend class SetMediaKeysWorker; On 2014/07/31 00:56:36, ddorwin wrote: > "Worker" has specific JS-facing meanings. Is there a better name? Looked like "Implementer" and "Handler" are common suffixes. Picked "Handler". https://codereview.chromium.org/423633002/diff/1/Source/web/WebMediaPlayerCli... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/423633002/diff/1/Source/web/WebMediaPlayerCli... Source/web/WebMediaPlayerClientImpl.cpp:57: // This happens asynchronously. Since there is no way to report an error, On 2014/07/31 00:56:36, ddorwin wrote: > This is the scenario xhwang was referring to in the other CL. > Maybe we should DCHECK that it doesn't fail. Remove this in favor of having a synchronous method. https://codereview.chromium.org/423633002/diff/1/Source/web/WebMediaPlayerCli... Source/web/WebMediaPlayerClientImpl.cpp:59: class IgnoreResponseContentDecryptionModuleResult FINAL : public ContentDecryptionModuleResult { On 2014/07/31 00:56:36, ddorwin wrote: > We should come up with a better name, especially if we make changes related to > the comments above and below. Gone. https://codereview.chromium.org/423633002/diff/1/Source/web/WebMediaPlayerCli... Source/web/WebMediaPlayerClientImpl.cpp:240: m_webMediaPlayer->setContentDecryptionModule(HTMLMediaElementEncryptedMedia::contentDecryptionModule(mediaElement()), result->result()); On 2014/07/31 00:56:36, ddorwin wrote: > Hmm, I wonder if this needs to be synchronous due to the load call that follows. > They at least need to be serialized. > > Maybe the new Result class above should call load() in complete(). That would be > the safest solution. HOWEVER, load() is probably expected to be synchronous, so > we might need to block here or something. Hmm. > > Alternatively, maybe we should keep (and rename) the synchronous version of > setContentDecryptionModule for this very scenario. Done. https://codereview.chromium.org/423633002/diff/1/public/platform/WebMediaPlay... File public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/423633002/diff/1/public/platform/WebMediaPlay... public/platform/WebMediaPlayer.h:158: virtual void setContentDecryptionModule(WebContentDecryptionModule* cdm) { } On 2014/07/31 00:56:36, ddorwin wrote: > FIXME: Remove. Renaming, which will work as long a Chromium changes go in first.
https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-playback-setmediakeys-after-src.html (right): https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-setmediakeys-after-src.html:73: waitForEventAndRunStep('needkey', video, onNeedKey, test); Why were these moved out in the previous files but not these? https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html (right): https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html:43: video.src = '../content/test-encrypted.webm'; Is this necessary? https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html:78: test.done(); If we return, we can still run the null test at l84. Even if replacement is not supported, setting to null may be. We might also consider doing what's right for Chrome for now and modifying the test if necessary when submitting it. We want to make sure Chrome does what we expect. Alternatively, we could do that in a browser test. Note that since we are not playing (and without line34, it is not loaded, most implementations will probably succeed here. Hmm, maybe we should run some of these again AFTER setting the source and AFTER playing. (We can do this in a separate CL.) https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html:80: remove https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-unprefixed-after-prefixed.html (right): https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-unprefixed-after-prefixed.html:28: test.done(); check the error https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:55: void completeWithDOMException(ExceptionCode, const String& errorMessage); Is completeWithDOMException the terminology used everywhere. Why not rejectWith...? https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:74: // Represents the result used when setContentDecryptionModule() is called. It seems there must be a cleaner way of handling this. Maybe an object that calls a callback (proceedToNextStage) provided at construction time and rejects the promise on failure. For the final call, can the result just be provided directly without using this class? https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:76: // multiple times, and not resolve the promise until the very end. Any errors What does until the very end mean? Each setMK should be resolved when it completes. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:77: // that happen will reject the promise immediately. But not if a worker is not provided. I think this is a problem with this design. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:79: // This class can be used without specifying a worker to call. Useful when Replace "worker". https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:80: // calling setContentDecryptionModule() while tearing down a player. *Not specifying is* useful when... ? Do we even need this class in that case? Is there a base class we could use instead? https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:84: : m_worker(worker) change var names too https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:129: // 3. Complete setting this asynchronously. nit: Run the remaining steps asynchronously? https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:145: proceedToNextStage(); I assume this can only be called in Starting from this location. Should that just be a separate function instead of a stage? That might simplify things. I still need to understand the other stages and whether we could then eliminate them https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:158: // "QuotaExceededError". (Currently no restrictions on using the same What is the purpose of the statement in ()? https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:169: if (m_element->isPlaying()) { Note that "playback" could also mean load has occurred. If !playing is enough for us, greeat. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:170: completeWithDOMException(InvalidStateError, "The existing MediaKeys object cannot be removed at this time."); ... removed while playing? https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:184: // Don't do anything more until SetContentDecryptionModuleResult ...until |result| is resolved? https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:195: thisElement.m_mediaKeys.clear(); Technically, the JS attribute is not supposed to be cleared yet. We're just supposed to run line 224 in all cases except failing (l203). I'm open to changing that. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:199: if (m_mediaKeys) { This name is confusing (see 195). Maybe m_newMediaKeys. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:200: // 3.3.1 Associate the CDM instance represented by mediaKeys with the These are large blocks, which would be good in functions. OTOH, it's also nice to have all the steps together in order. Getting rid of Stage would probably be motivation enough, though, and we can still have the steps in order in different functions. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:203: // 3.3.2.1 Set the mediaKeys attribute to null. This is handled above. We should note that. But maybe we should do this in the promise instead (see also the comment at l195). Of course, the failure at 182 would need to not clear the attribute. It would be nice if we could catch/wrap the failures and put the failure text and code there. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:214: // Don't do anything more until SetContentDecryptionModuleResult ditto https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:287: // 2. Let promise be a new promise. Remaining steps done in worker. handler https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:540: if (element.webMediaPlayer()) { Won't this always be false in this function? (I see this was copied from setMediaKeysInternal(), but it may not have applied here.) https://codereview.chromium.org/423633002/diff/40001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/423633002/diff/40001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:16: #include "platform/ContentDecryptionModuleResult.h" not needed https://codereview.chromium.org/423633002/diff/40001/public/platform/WebMedia... File public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/423633002/diff/40001/public/platform/WebMedia... public/platform/WebMediaPlayer.h:158: virtual void setContentDecryptionModuleSync(WebContentDecryptionModule* cdm) { } This probably warrants a comment. Also, since it should be used in only one specific case, it should probably come after the one below.
Updated. https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-playback-setmediakeys-after-src.html (right): https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-setmediakeys-after-src.html:73: waitForEventAndRunStep('needkey', video, onNeedKey, test); On 2014/08/08 02:50:50, ddorwin wrote: > Why were these moved out in the previous files but not these? Probably while testing I tried different things, and din't put the code back to the original form. Updated the other tests to minimize that changes. https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html (right): https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html:43: video.src = '../content/test-encrypted.webm'; On 2014/08/08 02:50:50, ddorwin wrote: > Is this necessary? Nope. Removed. https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html:78: test.done(); On 2014/08/08 02:50:50, ddorwin wrote: > If we return, we can still run the null test at l84. Even if replacement is not > supported, setting to null may be. > > We might also consider doing what's right for Chrome for now and modifying the > test if necessary when submitting it. We want to make sure Chrome does what we > expect. Alternatively, we could do that in a browser test. > > Note that since we are not playing (and without line34, it is not loaded, most > implementations will probably succeed here. Hmm, maybe we should run some of > these again AFTER setting the source and AFTER playing. (We can do this in a > separate CL.) Updated test to fail, along with a FIXME as a reminder. https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html:80: On 2014/08/08 02:50:50, ddorwin wrote: > remove Done. https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-unprefixed-after-prefixed.html (right): https://codereview.chromium.org/423633002/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-unprefixed-after-prefixed.html:28: test.done(); On 2014/08/08 02:50:51, ddorwin wrote: > check the error Done. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:55: void completeWithDOMException(ExceptionCode, const String& errorMessage); On 2014/08/08 02:50:51, ddorwin wrote: > Is completeWithDOMException the terminology used everywhere. Why not > rejectWith...? ContentDecryptionModuleResult uses completeWith..., based on WebCryptoResult. ScriptPromise uses rejectWithDOMException. Gone now. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:74: // Represents the result used when setContentDecryptionModule() is called. On 2014/08/08 02:50:52, ddorwin wrote: > It seems there must be a cleaner way of handling this. Maybe an object that > calls a callback (proceedToNextStage) provided at construction time and rejects > the promise on failure. For the final call, can the result just be provided > directly without using this class? How about this? https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:76: // multiple times, and not resolve the promise until the very end. Any errors On 2014/08/08 02:50:51, ddorwin wrote: > What does until the very end mean? Each setMK should be resolved when it > completes. Removed. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:77: // that happen will reject the promise immediately. On 2014/08/08 02:50:51, ddorwin wrote: > But not if a worker is not provided. I think this is a problem with this design. Acknowledged. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:79: // This class can be used without specifying a worker to call. Useful when On 2014/08/08 02:50:51, ddorwin wrote: > Replace "worker". Done. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:80: // calling setContentDecryptionModule() while tearing down a player. On 2014/08/08 02:50:51, ddorwin wrote: > *Not specifying is* useful when... ? > Do we even need this class in that case? Is there a base class we could use > instead? Removed. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:84: : m_worker(worker) On 2014/08/08 02:50:51, ddorwin wrote: > change var names too Done. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:129: // 3. Complete setting this asynchronously. On 2014/08/08 02:50:51, ddorwin wrote: > nit: Run the remaining steps asynchronously? Done. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:145: proceedToNextStage(); On 2014/08/08 02:50:51, ddorwin wrote: > I assume this can only be called in Starting from this location. Should that > just be a separate function instead of a stage? That might simplify things. I > still need to understand the other stages and whether we could then eliminate > them Done. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:158: // "QuotaExceededError". (Currently no restrictions on using the same On 2014/08/08 02:50:52, ddorwin wrote: > What is the purpose of the statement in ()? Just to note why there is no code for this step. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:169: if (m_element->isPlaying()) { On 2014/08/08 02:50:51, ddorwin wrote: > Note that "playback" could also mean load has occurred. If !playing is enough > for us, greeat. isPlaying() is the only function I could find. However, if the load has occurred, then it's possible the pipeline has been initialized, so checking load status would be better. However, load status is private. Not sure if there is a better test. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:170: completeWithDOMException(InvalidStateError, "The existing MediaKeys object cannot be removed at this time."); On 2014/08/08 02:50:51, ddorwin wrote: > ... removed while playing? Sure. I think the error message is from the older spec where the messages were specified. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:184: // Don't do anything more until SetContentDecryptionModuleResult On 2014/08/08 02:50:51, ddorwin wrote: > ...until |result| is resolved? Done. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:195: thisElement.m_mediaKeys.clear(); On 2014/08/08 02:50:51, ddorwin wrote: > Technically, the JS attribute is not supposed to be cleared yet. We're just > supposed to run line 224 in all cases except failing (l203). > I'm open to changing that. Done this way in case setContentDecryptionModule() call below fails. Fixed. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:199: if (m_mediaKeys) { On 2014/08/08 02:50:51, ddorwin wrote: > This name is confusing (see 195). Maybe m_newMediaKeys. Done. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:200: // 3.3.1 Associate the CDM instance represented by mediaKeys with the On 2014/08/08 02:50:51, ddorwin wrote: > These are large blocks, which would be good in functions. OTOH, it's also nice > to have all the steps together in order. > Getting rid of Stage would probably be motivation enough, though, and we can > still have the steps in order in different functions. Acknowledged. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:203: // 3.3.2.1 Set the mediaKeys attribute to null. On 2014/08/08 02:50:52, ddorwin wrote: > This is handled above. We should note that. But maybe we should do this in the > promise instead (see also the comment at l195). Of course, the failure at 182 > would need to not clear the attribute. It would be nice if we could catch/wrap > the failures and put the failure text and code there. Acknowledged. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:214: // Don't do anything more until SetContentDecryptionModuleResult On 2014/08/08 02:50:52, ddorwin wrote: > ditto Done. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:287: // 2. Let promise be a new promise. Remaining steps done in worker. On 2014/08/08 02:50:52, ddorwin wrote: > handler Done. https://codereview.chromium.org/423633002/diff/40001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:540: if (element.webMediaPlayer()) { On 2014/08/08 02:50:51, ddorwin wrote: > Won't this always be false in this function? (I see this was copied from > setMediaKeysInternal(), but it may not have applied here.) Converted to an ASSERT, which doesn't fire in any of the tests. https://codereview.chromium.org/423633002/diff/40001/Source/web/WebMediaPlaye... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/423633002/diff/40001/Source/web/WebMediaPlaye... Source/web/WebMediaPlayerClientImpl.cpp:16: #include "platform/ContentDecryptionModuleResult.h" On 2014/08/08 02:50:52, ddorwin wrote: > not needed Done. https://codereview.chromium.org/423633002/diff/40001/public/platform/WebMedia... File public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/423633002/diff/40001/public/platform/WebMedia... public/platform/WebMediaPlayer.h:158: virtual void setContentDecryptionModuleSync(WebContentDecryptionModule* cdm) { } On 2014/08/08 02:50:52, ddorwin wrote: > This probably warrants a comment. Also, since it should be used in only one > specific case, it should probably come after the one below. Done.
I like this better! Thanks. https://codereview.chromium.org/423633002/diff/50001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html (right): https://codereview.chromium.org/423633002/diff/50001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html:76: // FIXME: Enable before submitting to W3C. Allow this to pass as necessary... ? https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:60: void ClearFailed(ExceptionCode, const String& errorMessage); Unless this is a callback (On...), it should be an action. ReportClearFailed? Ditto below. https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:130: void SetMediaKeysHandler::ClearExistingMediaKeys(Timer<SetMediaKeysHandler>*) It's a bit weird that this action is tied to a Timer. I wonder if we should have a timer handler (DoSetMK, SetMKInternal) that just calls this. It's one more function, but it might be clearer. Also, line 118 is a bit weird: "The timer calls ClearExistingMediaKeys - What? Why?" WDYT? https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:139: // (Currently no restrictions on using the same MediaKeys with multiple Chrome doesn't have any such limitations? I think Pepper D&D and Android will. For now, let's fail (unless I'm missing something). I guess that would require more changes, though, so FIXME? https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:149: if (m_element->isPlaying()) { On 2014/08/08 23:15:12, jrummell wrote: > On 2014/08/08 02:50:51, ddorwin wrote: > > Note that "playback" could also mean load has occurred. If !playing is enough > > for us, greeat. > > isPlaying() is the only function I could find. However, if the load has > occurred, then it's possible the pipeline has been initialized, so checking load > status would be better. However, load status is private. Not sure if there is a > better test. m_element->webMediaPlayer() == is loaded? It's at least what we care about, right? We'd need to update the message below too. https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:150: ClearFailed(InvalidStateError, "The existing MediaKeys object cannot be removed while playing."); Because this function includes a step that doesn't belong here and it's just 1 line, I think we should call reject directly. https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:187: // sessions). Handled by Chromium? https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:546: // If a player is connected, tell it that the CDM has changed. // There is no player to inform.
Updated. https://codereview.chromium.org/423633002/diff/50001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html (right): https://codereview.chromium.org/423633002/diff/50001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html:76: // FIXME: Enable before submitting to W3C. On 2014/08/09 00:36:52, ddorwin wrote: > Allow this to pass as necessary... ? Done. https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:60: void ClearFailed(ExceptionCode, const String& errorMessage); On 2014/08/09 00:36:53, ddorwin wrote: > Unless this is a callback (On...), it should be an action. ReportClearFailed? > Ditto below. Done. https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:130: void SetMediaKeysHandler::ClearExistingMediaKeys(Timer<SetMediaKeysHandler>*) On 2014/08/09 00:36:53, ddorwin wrote: > It's a bit weird that this action is tied to a Timer. I wonder if we should have > a timer handler (DoSetMK, SetMKInternal) that just calls this. > > It's one more function, but it might be clearer. Also, line 118 is a bit weird: > "The timer calls ClearExistingMediaKeys - What? Why?" > > WDYT? Done. https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:139: // (Currently no restrictions on using the same MediaKeys with multiple On 2014/08/09 00:36:52, ddorwin wrote: > Chrome doesn't have any such limitations? I think Pepper D&D and Android will. > For now, let's fail (unless I'm missing something). > > I guess that would require more changes, though, so FIXME? Done. https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:149: if (m_element->isPlaying()) { On 2014/08/09 00:36:52, ddorwin wrote: > On 2014/08/08 23:15:12, jrummell wrote: > > On 2014/08/08 02:50:51, ddorwin wrote: > > > Note that "playback" could also mean load has occurred. If !playing is > enough > > > for us, greeat. > > > > isPlaying() is the only function I could find. However, if the load has > > occurred, then it's possible the pipeline has been initialized, so checking > load > > status would be better. However, load status is private. Not sure if there is > a > > better test. > > m_element->webMediaPlayer() == is loaded? > It's at least what we care about, right? > We'd need to update the message below too. Done. https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:150: ClearFailed(InvalidStateError, "The existing MediaKeys object cannot be removed while playing."); On 2014/08/09 00:36:53, ddorwin wrote: > Because this function includes a step that doesn't belong here and it's just 1 > line, I think we should call reject directly. Done. https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:187: // sessions). On 2014/08/09 00:36:52, ddorwin wrote: > Handled by Chromium? Done. https://codereview.chromium.org/423633002/diff/50001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:546: // If a player is connected, tell it that the CDM has changed. On 2014/08/09 00:36:52, ddorwin wrote: > // There is no player to inform. Removed since this is called right after m_webMediaPlayer.clear(); (and the procedure name implies that the player has already been destroyed).
Thanks. LGTM with comments. https://codereview.chromium.org/423633002/diff/70001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html (right): https://codereview.chromium.org/423633002/diff/70001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html:76: // FIXME: Enable before submitting to W3C to allow this I meant to reword the whole thing since "Enable" is ambiguous. Allow this to pass as necessary before submitting to W3C. https://codereview.chromium.org/423633002/diff/70001/Source/modules/encrypted... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/70001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:58: void ClearExistingMediaKeys(); Can the rest be private? https://codereview.chromium.org/423633002/diff/70001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:157: reject(DOMException::create(InvalidStateError, "The existing MediaKeys object cannot be removed.")); ... while loaded?
+acolwell@ & abarth@ for OWNERS review https://codereview.chromium.org/423633002/diff/70001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html (right): https://codereview.chromium.org/423633002/diff/70001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-setmediakeys.html:76: // FIXME: Enable before submitting to W3C to allow this On 2014/08/11 21:42:55, ddorwin wrote: > I meant to reword the whole thing since "Enable" is ambiguous. > > Allow this to pass as necessary before submitting to W3C. Done. https://codereview.chromium.org/423633002/diff/70001/Source/modules/encrypted... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/70001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:58: void ClearExistingMediaKeys(); On 2014/08/11 21:42:55, ddorwin wrote: > Can the rest be private? Yup. https://codereview.chromium.org/423633002/diff/70001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:157: reject(DOMException::create(InvalidStateError, "The existing MediaKeys object cannot be removed.")); On 2014/08/11 21:42:55, ddorwin wrote: > ... while loaded? Done.
https://codereview.chromium.org/423633002/diff/90001/Source/modules/encrypted... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/90001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:72: typedef Function<void()> SuccessCB; SuccessCB -> SuccessCallback ? Please use complete words https://codereview.chromium.org/423633002/diff/90001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:73: typedef Function<void(ExceptionCode, const String&)> FailureCB; ditto
I'm not really the right person to review this CL. Removing myself from the reviewers list.
Updated. https://codereview.chromium.org/423633002/diff/90001/Source/modules/encrypted... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/90001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:72: typedef Function<void()> SuccessCB; On 2014/08/11 22:45:19, abarth wrote: > SuccessCB -> SuccessCallback ? Please use complete words Done. https://codereview.chromium.org/423633002/diff/90001/Source/modules/encrypted... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:73: typedef Function<void(ExceptionCode, const String&)> FailureCB; On 2014/08/11 22:45:19, abarth wrote: > ditto Done.
https://codereview.chromium.org/423633002/diff/110001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (left): https://codereview.chromium.org/423633002/diff/110001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:66: exceptionState.throwDOMException(InvalidStateError, "Mixed use of EME prefixed and unprefixed API not allowed."); Why was this change made? It seems like having this in one place is better than duplicating the string in a variety of places. https://codereview.chromium.org/423633002/diff/110001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/110001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:165: if (m_element->webMediaPlayer()) { How can you get here? Doesn't the early return above prevent the code in this block from ever running?
Updated. Changes in files other than HTMLMediaElementEncryptedMedia.cpp are just part of a rebase. https://codereview.chromium.org/423633002/diff/110001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (left): https://codereview.chromium.org/423633002/diff/110001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:66: exceptionState.throwDOMException(InvalidStateError, "Mixed use of EME prefixed and unprefixed API not allowed."); On 2014/08/13 22:04:43, acolwell wrote: > Why was this change made? It seems like having this in one place is better than > duplicating the string in a variety of places. For promises there is no exception -- it becomes a failed promise. I could add setEmeMode w/exception back in as a second method. WDYT? As well, it will all go away when we get rid of prefixed EME :) https://codereview.chromium.org/423633002/diff/110001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/110001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:165: if (m_element->webMediaPlayer()) { On 2014/08/13 22:04:43, acolwell wrote: > How can you get here? Doesn't the early return above prevent the code in this > block from ever running? Done. The test on 155 used to be more specific (IsPlaying()).
https://codereview.chromium.org/423633002/diff/130001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/130001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:164: // 3.2.4 If the preceding step failed, ... You might as well include the full text since it's not anywhere else.
lgtm
+abarth for OWNERS review of Source/web and public/platform. Changes there are pretty trivial. https://codereview.chromium.org/423633002/diff/130001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/130001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:164: // 3.2.4 If the preceding step failed, ... On 2014/08/14 01:26:34, ddorwin wrote: > You might as well include the full text since it's not anywhere else. Done.
I'm sorry, but I don't have the bandwith to review this change.
+jamesr@ for OWNERS review of Source/web and public/platform.
https://codereview.chromium.org/423633002/diff/150001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/150001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:135: void SetMediaKeysHandler::ClearExistingMediaKeys() blink style is for function names to start with a lowercase letter, why aren't these doing that? https://codereview.chromium.org/423633002/diff/150001/Source/web/WebMediaPlay... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/423633002/diff/150001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:210: m_webMediaPlayer->setContentDecryptionModuleSync(HTMLMediaElementEncryptedMedia::contentDecryptionModule(mediaElement())); why does this call have to be synchronous? will the load behave differently if it isn't?
Updated. https://codereview.chromium.org/423633002/diff/150001/Source/modules/encrypte... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right): https://codereview.chromium.org/423633002/diff/150001/Source/modules/encrypte... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:135: void SetMediaKeysHandler::ClearExistingMediaKeys() On 2014/08/15 00:56:43, jamesr wrote: > blink style is for function names to start with a lowercase letter, why aren't > these doing that? Done. https://codereview.chromium.org/423633002/diff/150001/Source/web/WebMediaPlay... File Source/web/WebMediaPlayerClientImpl.cpp (right): https://codereview.chromium.org/423633002/diff/150001/Source/web/WebMediaPlay... Source/web/WebMediaPlayerClientImpl.cpp:210: m_webMediaPlayer->setContentDecryptionModuleSync(HTMLMediaElementEncryptedMedia::contentDecryptionModule(mediaElement())); On 2014/08/15 00:56:43, jamesr wrote: > why does this call have to be synchronous? will the load behave differently if > it isn't? The current HTML standard for resource fetching is synchronous. In this state (newly created player) setContentDecryptionModuleSync() and setContentDecryptionModule() do the same thing. Rather than relying on setContentDecryptionModule() to complete immediately (or even worse, adding a wait), I added this method to make it clear it needs to be synchronous. The Chromium side checks that it can be synchronous (https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...).
I can't figure out from the spec or code where the async step is in here that might fail. How would a web author who wanted the same behavior as what you're doing here get it?
Most of the encrypted media enhancements to HTMLMediaElement are now asynchronous and use promises (spec is still evolving). This CL is to have setMediaKeys return a promise, rather than executing synchronously. Why execute asynchronously? The actual decrypting is done on the media thread, while all this happens on the main thread. The synchronous call could return before all the work was done (since part runs on the media thread, the call succeeded as soon as the request was posted). Failures on the media thread were ignored. If you want to change the value after it's set, we need to wait until the media thread is done with the current decryptor before changing to the new one. In the past with the synchronous API we had crashes if you tried to change it, as the call would return and then the previous MediaKeys object would get freed, causing the media thread to crash. So we disabled changing it once set (which this CL doesn't change, but will in the future).
OK, so what does this ..Sync call enforce for you on the main thread? What
state can you observe that would be different based on whether the call is sync
or not? Do other APIs behave differently?
I.e. if I do
foo.setMediaKeys();
foo.something();
for what value of 'something' will this behave differently than if I did
foo.setMediaKeys().then(function() { foo.something(); });
and how does that map to what this C++ code is doing?
naively I would expect the setMediaKeys to post a task to the media thread to do
something and any calls made after that would post another task. Since tasks
aren't reordered, it seems that they would evaluate in the same order on the
media thread and have the same behavior in the end. No?
The ...Sync() version was added to fit into an existing sync api as best we can (see the change in WebMediaPlayerClientImpl.cpp). It checks that no call is need to the media thread. It is possible to use the async version (see WebMediaPlayerClientImpl.cpp in PS1). However, other reviewers found that confusing (see comments there).
The initial comments seem to be saying that it's unclear if setMediaKeys() and load() calls are serialized or not. Are they? Where is this documented in the spec? It should be pretty clear one way or the other.
setMediaKeys() is specified as asynchronous in current EME spec (https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-...). load() is synchronous in the HTML5 spec (http://www.w3.org/TR/2011/WD-html5-20110113/video.html#htmlmediaelement). load() can also be triggered by setting the src attribute. The actual media player object isn't created until load() is called. So if MediaKeys is set before load() is called, the value is simply saved until the player is constructed. If MediaKeys is set after load() is called, then the media decryptors need to be reset (as it defaulted to non-encrypted instances). We could pass the object as part of the media player constructor, or part of the load() call. We could rename ...Sync to be more explicit about it's intended usage, something like ...BeforeLoad(), as it really is a special case.
Patchset #10 (id:190001) has been deleted
PS11 gets rid of setContentDecryptionModuleSync() and passes the initial CDM when constructing the media player. If this is accepted, I'll have to change the Chromium side first before submitting this (to handle the extra parameter when creating the media player).
acolwell@chromium.org changed reviewers: - acolwell@chromium.org
jamesr@: friendly ping for OWNERS review
public/ lgtm. Did not look at the rest.
https://codereview.chromium.org/423633002/diff/230001/public/web/WebFrameClie... File public/web/WebFrameClient.h (right): https://codereview.chromium.org/423633002/diff/230001/public/web/WebFrameClie... public/web/WebFrameClient.h:108: // May return null. Add a comment that WebContentDecryptionModule* may be null if one has not yet been set yet?
Thanks for the reviews. https://codereview.chromium.org/423633002/diff/230001/public/web/WebFrameClie... File public/web/WebFrameClient.h (right): https://codereview.chromium.org/423633002/diff/230001/public/web/WebFrameClie... public/web/WebFrameClient.h:108: // May return null. On 2014/09/23 23:18:05, ddorwin wrote: > Add a comment that WebContentDecryptionModule* may be null if one has not yet > been set yet? Done.
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/423633002/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/423633002/250001
Message was sent while issue was closed.
Committed patchset #12 (id:250001) as 182644 |
