[eme] clearkey-mp4-playback-temporary-multikey-sequential-readyState.html playing count fix #3985

cpearce · 2016-10-17T01:16:28Z

Fix clearkey-mp4-playback-temporary-multikey-sequential-readyState.html so that it expects two "playing" events, as the specification calls for firing "playing" every time the readyState was HAVE_CURRENT_DATA or less and the new ready state is HAVE_FUTURE_DATA. The waiting for key algorithm requires us to drop the readyState to HAVE_METADATA, so we should expect to fire two "playing" events.

I have patches for Firefox in the works to make us pass this test with this fix.

Update from master

We should get two "playing" events in this test; one when we first play the media encrypted with the first key, and then once we hit waiting for the second key we should drop back to readyState HAVE_METADATA, and once we receive the second key and play the second segment again, the readyState will raise above HAVE_FUTURE_DATA or more and we should fire another "playing" event. See: https://dev.w3.org/html5/spec-preview/media-elements.html#ready-states

wpt-pr-bot · 2016-10-17T01:16:37Z

Reviewers for this pull request are: @ddorwin.

key, or if we are waiting for a key but the readyState is HAVE_METADATA. The "Waiting for key" algorithm says to change the readyState to HAVE_METADATA. The HTMLMediaElement.readyState spec says "If the previous ready state was HAVE_FUTURE_DATA or more, and the new ready state is HAVE_CURRENT_DATA or less" we should dispatch a "timeupdate" event. So this test can't assert that we don't get a timeupdate event while we're waiting for a key, as we're supposed to. So just assert that we should only get a timeupdate while waiting for a key if we're already in HAVE_METADATA, so that we test that we've transitioned.

ddorwin · 2016-10-17T19:36:28Z

I believe the text @cpearce is referring to is the following from https://html.spec.whatwg.org/multipage/embedded-content.html#ready-states:

If the previous ready state was HAVE_CURRENT_DATA or less, and the new ready state is HAVE_FUTURE_DATA
The user agent must queue a task to fire a simple event named canplay at the element.
If the element's paused attribute is false, the user agent must notify about playing for the element.

Should we also have checks for the first step (canplay)?

ddorwin · 2016-10-17T19:37:01Z

encrypted-media/scripts/playback-temporary-multikey-sequential.js

@@ -62,11 +63,12 @@ function runTest(config,qualifier) {
        }

        function onPlaying(event) {
-            assert_equals(_mediaKeySessions.length, 1, "Playback should start with a single key / session");
+            _playingCount++;
+            assert_equals(_mediaKeySessions.length, _playingCount, "Should get one 'playing' event per key / session played");


Should we also check that _playingCount is <= 2?

Sure. Done.

ddorwin · 2016-10-17T19:38:04Z

encrypted-media/scripts/playback-temporary-multikey-sequential.js

        }

        function onTimeupdate(event) {
-            assert_false(_waitingForKey, "Should not continue playing whilst waiting for a key");
+            assert_true(!_waitingForKey || _video.readyState == _video.HAVE_METADATA, "Should not continue playing whilst waiting for a key");


Maybe explain why we should get a timeupdate when _video.readyState == _video.HAVE_METADATA. Is this at the beginning or on the transition due to waiting for a key?

Yes, we're required to fire 'timeupdate' when we transition from readyState HAVE_FUTURE_DATA or more to HAVE_CURRENT_DATA or less. I've added a comment.

paulbrucecotton · 2016-10-18T15:56:39Z

@cpearce : Can you let us know if you are okay with David's comments?

cpearce · 2016-10-19T03:12:20Z

@paulbrucecotton In principle, yes I agree with those comments, but I'm not sure what readyState behaviour we're targeting until w3c/encrypted-media#336, w3c/encrypted-media#338, and w3c/encrypted-media#339 are resolved. And this test is testing readyState behaviour.

HAVE_METADATA.

ddorwin

Thanks. Looks good except I have questions about one of the asserts.

ddorwin · 2016-10-20T06:01:51Z

encrypted-media/scripts/playback-temporary-multikey-sequential.js

-            assert_false(_waitingForKey, "Should not continue playing whilst waiting for a key");
+            // We should not receive 'timeupdate' events due to playing while waiting for a key, except
+            // when we first start waiting for key we should change the readyState to HAVE_METADATA
+            // which will trigger the "If the previous ready state was HAVE_FUTURE_DATA or more, and


We'll need to update this once w3c/encrypted-media#338 is fixed.

ddorwin · 2016-10-20T06:03:36Z

encrypted-media/scripts/playback-temporary-multikey-sequential.js

+            // when we first start waiting for key we should change the readyState to HAVE_METADATA
+            // which will trigger the "If the previous ready state was HAVE_FUTURE_DATA or more, and
+            // the new ready state is HAVE_CURRENT_DATA or less" case of the readyState change
+            // algorithm which requires a "timeupdate" event be fired.


Don't you only want to see timeupdate once in that case?

It seems that the current logic is likely to always be true even if we get many timeupdate events while waiting.

I've updated the test to count the number of timeupdates we get while waiting, and assert that we only get one. I've not been able to get the test to pass in Firefox yet.

cpearce · 2016-10-25T22:51:34Z

@ddorwin please re-review. Thanks!

ddorwin

This works, but I'd like to see the comments addressed. Feel free to get someone else to merge once you are satisfied.

ddorwin · 2016-10-26T04:02:06Z

encrypted-media/scripts/playback-temporary-multikey-sequential.js

+            _waitingForKey = false,
+            _playingCount = 0,
+            _canplayCount = 0,
+            _timeupdateWhileWaiting = 0;


...Count?

ddorwin · 2016-10-26T04:03:02Z

encrypted-media/scripts/playback-temporary-multikey-sequential.js

@@ -66,11 +69,27 @@ function runTest(config,qualifier) {
        }

        function onPlaying(event) {
-            assert_equals(_mediaKeySessions.length, 1, "Playback should start with a single key / session");
+            _playingCount++;
+            assert_equals(_mediaKeySessions.length, _playingCount, "Should get one 'playing' event per key / session played");


s/played/added/ ?

ddorwin · 2016-10-26T04:03:12Z

encrypted-media/scripts/playback-temporary-multikey-sequential.js

+
+        function onCanPlay(event) {
+            _canplayCount++;
+            assert_equals(_mediaKeySessions.length, _canplayCount, "Should get one 'canplay' event per key / session played");


ditto: s/played/added/ ?

ddorwin · 2016-10-26T04:06:59Z

encrypted-media/scripts/playback-temporary-multikey-sequential.js

+            // the new ready state is HAVE_CURRENT_DATA or less" case of the readyState change
+            // algorithm which requires a "timeupdate" event be fired.
+             if (_waitingForKey) {
+                assert_equals(++_timeupdateWhileWaiting, 1, "Should only receive one timeupdate while waiting for key");


Before calling test.done(), we should assert_equals(_timeupdateWhileWaiting, 1) to ensure we did get that one instance. Currently, it's possible to get 0 and pass.

ddorwin · 2016-10-26T04:07:52Z

encrypted-media/scripts/playback-temporary-multikey-sequential.js

+            // algorithm which requires a "timeupdate" event be fired.
+             if (_waitingForKey) {
+                assert_equals(++_timeupdateWhileWaiting, 1, "Should only receive one timeupdate while waiting for key");
+                assert_equals(_video.readyState, _video.HAVE_CURRENT_DATA, "Should not continue playing whilst waiting for a key");


Does this assert really have the meaning in the string? It seems that the logic above and expecting no more than one accomplishes that now.

Just to be clear, are you suggesting I remove the assert or change the string?

Change the string. This is a little duplication of the other similar assert, but checking that it hasn't changed is not a bad thing.

cpearce · 2016-10-26T20:55:24Z

Review comments addressed.

ddorwin

Thanks. LGTM. One string could be clearer.

ddorwin · 2016-10-26T21:27:01Z

encrypted-media/scripts/playback-temporary-multikey-sequential.js

            }

            if (_video.currentTime > config.duration) {
                assert_equals(_mediaKeySessions.length, config.initData.length, "It should require all keys to reach end of content");
+                assert_equals(_timeupdateWhileWaitingCount, 1, "Should have only received one timeupdate while waiting for key");


nit: ... received exactly one...

The check on line 90 checks no more than one. This checks that we did get that one.

ddorwin · 2016-10-26T21:57:33Z

@cpearce, are you able to squash and merge? I was going to, but it's not clear what the commit message should be since there are many commits and it's not clear that the main ones are still accurate. If you have permission to merge, please do so with the commit message you want. Otherwise, post it here and I'll merge.

cpearce · 2016-10-26T22:26:45Z

I do not have write access, so I cannot perform a squash merge to master.

ddorwin · 2016-10-26T22:38:56Z

What do you want the commit message to be?

cpearce · 2016-10-26T22:41:46Z

"Ensure we receivethe expected number of canplay, playing and timeupdate events, and have the correct readyState, when playback waits for a key."

cpearce and others added 2 commits October 16, 2016 15:33

Merge pull request #1 from w3c/master

a506797

Update from master

wpt-pr-bot added encrypted-media wg-html labels Oct 17, 2016

ddorwin reviewed Oct 17, 2016

View reviewed changes

Chris Pearce added 3 commits October 20, 2016 11:21

Merge remote-tracking branch 'upstream/master' into playing-count-fix

13d090b

Test correct number of 'canplay' and 'playing' events are dispatched.

5994685

Add comment explaining why 'timeupdate' fires while readyState is

67887cd

HAVE_METADATA.

ddorwin changed the title ~~clearkey-mp4-playback-temporary-multikey-sequential-readyState.html playing count fix~~ [eme] clearkey-mp4-playback-temporary-multikey-sequential-readyState.html playing count fix Oct 19, 2016

ddorwin reviewed Oct 20, 2016

View reviewed changes

Chris Pearce added 3 commits October 26, 2016 09:34

Merge remote-tracking branch 'upstream/master' into playing-count-fix

1057699

Merge remote-tracking branch 'upstream/master' into playing-count-fix

c810c5e

Ensure we don't fire more than one 'timeupdate' while waiting for key.

513c060

ddorwin approved these changes Oct 26, 2016

View reviewed changes

Chris Pearce added 2 commits October 27, 2016 08:55

Merge remote-tracking branch 'upstream/master' into playing-count-fix

a230eb0

Update to address review comments.

6d8eddc

ddorwin approved these changes Oct 26, 2016

View reviewed changes

Address review comments exactly.

f222dd3

ddorwin approved these changes Oct 26, 2016

View reviewed changes

ddorwin merged commit 7fb01a2 into web-platform-tests:master Oct 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[eme] clearkey-mp4-playback-temporary-multikey-sequential-readyState.html playing count fix #3985

[eme] clearkey-mp4-playback-temporary-multikey-sequential-readyState.html playing count fix #3985

cpearce commented Oct 17, 2016

wpt-pr-bot commented Oct 17, 2016

ddorwin commented Oct 17, 2016

ddorwin Oct 17, 2016

cpearce Oct 19, 2016

ddorwin Oct 17, 2016

cpearce Oct 19, 2016

paulbrucecotton commented Oct 18, 2016

cpearce commented Oct 19, 2016

ddorwin left a comment

ddorwin Oct 20, 2016 •

edited

Loading

cpearce Oct 25, 2016

ddorwin Oct 20, 2016

cpearce Oct 25, 2016

cpearce commented Oct 25, 2016

ddorwin left a comment

ddorwin Oct 26, 2016

ddorwin Oct 26, 2016

ddorwin Oct 26, 2016

ddorwin Oct 26, 2016

ddorwin Oct 26, 2016

cpearce Oct 26, 2016

ddorwin Oct 26, 2016

cpearce commented Oct 26, 2016

ddorwin left a comment

ddorwin Oct 26, 2016

ddorwin commented Oct 26, 2016

cpearce commented Oct 26, 2016

ddorwin commented Oct 26, 2016

cpearce commented Oct 26, 2016

[eme] clearkey-mp4-playback-temporary-multikey-sequential-readyState.html playing count fix #3985

[eme] clearkey-mp4-playback-temporary-multikey-sequential-readyState.html playing count fix #3985

Conversation

cpearce commented Oct 17, 2016

wpt-pr-bot commented Oct 17, 2016

ddorwin commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulbrucecotton commented Oct 18, 2016

cpearce commented Oct 19, 2016

ddorwin left a comment

Choose a reason for hiding this comment

ddorwin Oct 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpearce commented Oct 25, 2016

ddorwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpearce commented Oct 26, 2016

ddorwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddorwin commented Oct 26, 2016

cpearce commented Oct 26, 2016

ddorwin commented Oct 26, 2016

cpearce commented Oct 26, 2016

ddorwin Oct 20, 2016 •

edited

Loading