Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
203323006:
Add layout test for EME WD using multiple sessions (Closed)
|
Created:
6 years, 9 months ago by jrummell Modified:
6 years, 9 months ago CC:
blink-reviews, feature-media-reviews_chromium.org, philipj_slow, eric.carlson_apple.com Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd layout test for EME WD using multiple sessions
BUG=308704
TEST=new test runs
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169796
Patch Set 1 #
Total comments: 10
Patch Set 2 : New test file #
Total comments: 21
Patch Set 3 : remove extra asserts #
Total comments: 2
Patch Set 4 : assert_true added #
Total comments: 4
Patch Set 5 : key provided test #
Total comments: 6
Patch Set 6 : remove onReady() #
Total comments: 1
Patch Set 7 : Remove webM file #
Total comments: 2
Messages
Total messages: 21 (0 generated)
PTAL.
https://codereview.chromium.org/203323006/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:20: var mediaKeyVideoSession = null; nit: audioMediaKeySession/videoMediaKeySession ? https://codereview.chromium.org/203323006/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:78: } It seems we can combine onAudioMessage and onVideoMessage to reduce some duplicate codes. https://codereview.chromium.org/203323006/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:94: } ditto for combining methods to reduce duplicate codes. https://codereview.chromium.org/203323006/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:107: return; If audio key is provided but video key is not provided, will playback still proceed and this test still pass? It's at least worth checking. https://codereview.chromium.org/203323006/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:115: video.src = "../content/test-encrypted-2-keys.webm"; This file is a bit confusing. It's called 2-keys but really it's 1-key-for-2-keyIds. Can we actually get a file that uses two different keys for audio/video streams?
Updated. https://codereview.chromium.org/203323006/diff/1/LayoutTests/media/encrypted-... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:20: var mediaKeyVideoSession = null; On 2014/03/19 01:22:00, xhwang wrote: > nit: audioMediaKeySession/videoMediaKeySession ? Done. https://codereview.chromium.org/203323006/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:78: } On 2014/03/19 01:22:00, xhwang wrote: > It seems we can combine onAudioMessage and onVideoMessage to reduce some > duplicate codes. It doesn't really save that much duplication (most lines have audio/video specific variables), but done to make it clearer they are for basically the same event. https://codereview.chromium.org/203323006/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:94: } On 2014/03/19 01:22:00, xhwang wrote: > ditto for combining methods to reduce duplicate codes. Done. https://codereview.chromium.org/203323006/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:107: return; On 2014/03/19 01:22:00, xhwang wrote: > If audio key is provided but video key is not provided, will playback still > proceed and this test still pass? It's at least worth checking. If I change onVideoMessage() to do nothing, the Playing event never happens and the test timeouts. https://codereview.chromium.org/203323006/diff/1/LayoutTests/media/encrypted-... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:115: video.src = "../content/test-encrypted-2-keys.webm"; On 2014/03/19 01:22:00, xhwang wrote: > This file is a bit confusing. It's called 2-keys but really it's > 1-key-for-2-keyIds. Can we actually get a file that uses two different keys for > audio/video streams? Done.
Thanks. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:15: // Timeout for all tests to run. It would be nice to have a single line for this timeout. The comment should fit on the line fine, though I'm not sure what exactly it means or that it's necessary. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:16: setup({ timeout: 60000 }); Oh, maybe this is from other files that ran multiple tests. I don't think we need it here. (We already have line 112.) https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:37: assert_equals(event.target, video); We're not specifically testing the events, so we probably don't need these. WDYT? https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:41: // Ideally we could just look at event.contentType, but it This comment is unnecessary - the spec says you can't depend on contentType. It seems pretty straightforward that we want to figure out which key ID this is. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:47: videoMediaKeySession = mediaKeys.createSession(event.contentType, event.initData); nit: We could share a bit more code by creating the session above with a local var then assigning it here and waiting for an event after the else. WDYT? https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:89: function onPlaying(event) OOC, could we just register the timeupdate event directly? https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:99: if (event.target.currentTime < 0.2) FYI: I think this only works as a test if the player stops when either audio or video are blocked. I don't think Chrome currently has that behavior. We should have tests that verify the Chrome behavior is fixed, including EME waiting tests. Any thoughts on how to verify both keys are being used in this test? A better test would be a file that uses one key for .1s then another key for the rest of the file. We can address that later, though. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:108: video.src = "../content/test-encrypted-2-keys.webm"; nit: We should name it different-av-keys or something like that that indicates that the a and v have different keys.
Updated. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:15: // Timeout for all tests to run. On 2014/03/21 06:00:20, ddorwin wrote: > It would be nice to have a single line for this timeout. The comment should fit > on the line fine, though I'm not sure what exactly it means or that it's > necessary. Done. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:16: setup({ timeout: 60000 }); On 2014/03/21 06:00:20, ddorwin wrote: > Oh, maybe this is from other files that ran multiple tests. I don't think we > need it here. (We already have line 112.) The testharness has 2 timeouts -- 1 for the timeout of each test (default 2s), and 1 for the total time of all tests in the file (default 5s). We typically only have 1 test per file, although encrypted-media-v2-syntax.html has several. Without this line this test would timeout after 5 seconds, regardless of the bigger timeout value specified on line 112. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:37: assert_equals(event.target, video); On 2014/03/21 06:00:20, ddorwin wrote: > We're not specifically testing the events, so we probably don't need these. > WDYT? Sure. I just copied the regular playback test and added support for 2 keys. I didn't do any other cleanup. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:41: // Ideally we could just look at event.contentType, but it On 2014/03/21 06:00:20, ddorwin wrote: > This comment is unnecessary - the spec says you can't depend on contentType. It > seems pretty straightforward that we want to figure out which key ID this is. Done. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:47: videoMediaKeySession = mediaKeys.createSession(event.contentType, event.initData); On 2014/03/21 06:00:20, ddorwin wrote: > nit: We could share a bit more code by creating the session above with a local > var then assigning it here and waiting for an event after the else. WDYT? Done. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:89: function onPlaying(event) On 2014/03/21 06:00:20, ddorwin wrote: > OOC, could we just register the timeupdate event directly? I'm not sure it makes any difference. We still want to see the playing event so we verify that playing doesn't start until after both keys are provided. We won't get timeupdate events until playing starts. Leaving it as is to continue to match what is in the other encrypted-media-playback-* tests. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:99: if (event.target.currentTime < 0.2) On 2014/03/21 06:00:20, ddorwin wrote: > FYI: I think this only works as a test if the player stops when either audio or > video are blocked. I don't think Chrome currently has that behavior. > We should have tests that verify the Chrome behavior is fixed, including EME > waiting tests. > Any thoughts on how to verify both keys are being used in this test? > A better test would be a file that uses one key for .1s then another key for the > rest of the file. We can address that later, though. If I remove either of the update() calls in onMessage(), the playing event is never received and the test times out. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:108: video.src = "../content/test-encrypted-2-keys.webm"; On 2014/03/21 06:00:20, ddorwin wrote: > nit: We should name it different-av-keys or something like that that indicates > that the a and v have different keys. Done.
Thanks. A few comments are in the previous PS. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:16: setup({ timeout: 60000 }); On 2014/03/21 17:23:01, jrummell wrote: > On 2014/03/21 06:00:20, ddorwin wrote: > > Oh, maybe this is from other files that ran multiple tests. I don't think we > > need it here. (We already have line 112.) > > The testharness has 2 timeouts -- 1 for the timeout of each test (default 2s), > and 1 for the total time of all tests in the file (default 5s). We typically > only have 1 test per file, although encrypted-media-v2-syntax.html has several. > Without this line this test would timeout after 5 seconds, regardless of the > bigger timeout value specified on line 112. :( Thanks for the explanation. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:89: function onPlaying(event) On 2014/03/21 17:23:01, jrummell wrote: > On 2014/03/21 06:00:20, ddorwin wrote: > > OOC, could we just register the timeupdate event directly? > > I'm not sure it makes any difference. We still want to see the playing event so > we verify that playing doesn't start until after both keys are provided. We > won't get timeupdate events until playing starts. Leaving it as is to continue > to match what is in the other encrypted-media-playback-* tests. Do we really not get a playing event until the keys are provided? If so, we should verify that by asserting that update() has been called on videoMediaKeySession and audioMediaKeySession. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:99: if (event.target.currentTime < 0.2) On 2014/03/21 17:23:01, jrummell wrote: > On 2014/03/21 06:00:20, ddorwin wrote: > > FYI: I think this only works as a test if the player stops when either audio > or > > video are blocked. I don't think Chrome currently has that behavior. > > We should have tests that verify the Chrome behavior is fixed, including EME > > waiting tests. > > Any thoughts on how to verify both keys are being used in this test? > > A better test would be a file that uses one key for .1s then another key for > the > > rest of the file. We can address that later, though. > > If I remove either of the update() calls in onMessage(), the playing event is > never received and the test times out. Okay. We should have a test like this when we start testing waiting. https://codereview.chromium.org/203323006/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:41: waitForEventAndRunStep("message", videoMediaKeySession, onMessage, test); This line can go after the line 47 if we replace video/audio with newSession (and delete line 46).
Updated. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:89: function onPlaying(event) On 2014/03/21 17:36:49, ddorwin wrote: > On 2014/03/21 17:23:01, jrummell wrote: > > On 2014/03/21 06:00:20, ddorwin wrote: > > > OOC, could we just register the timeupdate event directly? > > > > I'm not sure it makes any difference. We still want to see the playing event > so > > we verify that playing doesn't start until after both keys are provided. We > > won't get timeupdate events until playing starts. Leaving it as is to continue > > to match what is in the other encrypted-media-playback-* tests. > > Do we really not get a playing event until the keys are provided? If so, we > should verify that by asserting that update() has been called on > videoMediaKeySession and audioMediaKeySession. Test that Ready received for both audio and video done in onTimeUpdate(). Converted to asserts here. https://codereview.chromium.org/203323006/diff/20001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:99: if (event.target.currentTime < 0.2) On 2014/03/21 17:36:49, ddorwin wrote: > On 2014/03/21 17:23:01, jrummell wrote: > > On 2014/03/21 06:00:20, ddorwin wrote: > > > FYI: I think this only works as a test if the player stops when either audio > > or > > > video are blocked. I don't think Chrome currently has that behavior. > > > We should have tests that verify the Chrome behavior is fixed, including EME > > > waiting tests. > > > Any thoughts on how to verify both keys are being used in this test? > > > A better test would be a file that uses one key for .1s then another key for > > the > > > rest of the file. We can address that later, though. > > > > If I remove either of the update() calls in onMessage(), the playing event is > > never received and the test times out. > > Okay. We should have a test like this when we start testing waiting. Done. https://codereview.chromium.org/203323006/diff/40001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/40001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:41: waitForEventAndRunStep("message", videoMediaKeySession, onMessage, test); On 2014/03/21 17:36:49, ddorwin wrote: > This line can go after the line 47 if we replace video/audio with newSession > (and delete line 46). Done.
https://codereview.chromium.org/203323006/diff/60001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:78: assert_true(audioSessionReadyReceived); I'm not sure if there is a guarantee that we will receive ready before playing. I started to look up the ordering in the spec, but then I remembered that the "ready" event is being removed from the spec, so this won't be sufficient anyway. That's why I proposed that we should assertin that update() has been called on videoMediaKeySession and audioMediaKeySession. You might also comment that playback should not have started before both keys are received. https://codereview.chromium.org/203323006/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:80: assert_true(event.target.currentTime < 0.2); I don't think this is necessary. In theory, the event could be handled much later. What did you intend this to check?
Updated. https://codereview.chromium.org/203323006/diff/60001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:78: assert_true(audioSessionReadyReceived); On 2014/03/21 18:16:38, ddorwin wrote: > I'm not sure if there is a guarantee that we will receive ready before playing. > I started to look up the ordering in the spec, but then I remembered that the > "ready" event is being removed from the spec, so this won't be sufficient > anyway. That's why I proposed that we should assertin that update() has been > called on videoMediaKeySession and audioMediaKeySession. > > You might also comment that playback should not have started before both keys > are received. Done. https://codereview.chromium.org/203323006/diff/60001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:80: assert_true(event.target.currentTime < 0.2); On 2014/03/21 18:16:38, ddorwin wrote: > I don't think this is necessary. In theory, the event could be handled much > later. What did you intend this to check? Checking that the video did not play. However, it doesn't make much sense in onPlaying() since this indicates that playback has just started. Removed.
https://codereview.chromium.org/203323006/diff/80001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/80001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:57: waitForEventAndRunStep("ready", videoMediaKeySession, onReady, test); nit: This line could be shared by using event.target for the session. There's probably some other optimizations as well. I don't know whether that makes it more (less code) or less (slightly more complex) readable, though. It does seem we should try to be consistent with onNeedKey, though. WDYT? Or we can just remove this wait since we don't do anything in the handler. https://codereview.chromium.org/203323006/diff/80001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:68: function onReady(event) Remove then? https://codereview.chromium.org/203323006/diff/80001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:76: assert_true(audioKeyProvided); nit: All other functions process video then audio. It's nice to be consistent. Also, update the comment.
Removed onReady(). https://codereview.chromium.org/203323006/diff/80001/LayoutTests/media/encryp... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/80001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:57: waitForEventAndRunStep("ready", videoMediaKeySession, onReady, test); On 2014/03/21 19:04:23, ddorwin wrote: > nit: This line could be shared by using event.target for the session. There's > probably some other optimizations as well. > > I don't know whether that makes it more (less code) or less (slightly more > complex) readable, though. It does seem we should try to be consistent with > onNeedKey, though. WDYT? > > Or we can just remove this wait since we don't do anything in the handler. Removed. https://codereview.chromium.org/203323006/diff/80001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:68: function onReady(event) On 2014/03/21 19:04:23, ddorwin wrote: > Remove then? Done. https://codereview.chromium.org/203323006/diff/80001/LayoutTests/media/encryp... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:76: assert_true(audioKeyProvided); On 2014/03/21 19:04:23, ddorwin wrote: > nit: All other functions process video then audio. It's nice to be consistent. > Also, update the comment. Done.
lgtm Thanks! https://codereview.chromium.org/203323006/diff/100001/LayoutTests/media/encry... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/100001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:73: // Not using waitForEventAndRunStep() to avoid too many This is fine, but note that there's no line length limit in Blink.
lgtm with one question. https://codereview.chromium.org/203323006/diff/120001/LayoutTests/media/encry... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/120001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:69: // provided. Did you verify this? It used to be the case if audio key is provided, audio will start playing, even though the video decoding isn't progressing.
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrummell@chromium.org/203323006/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on mac_blink_compile_dbg
Thanks for the reviews. https://codereview.chromium.org/203323006/diff/120001/LayoutTests/media/encry... File LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html (right): https://codereview.chromium.org/203323006/diff/120001/LayoutTests/media/encry... LayoutTests/media/encrypted-media/encrypted-media-playback-multiple-sessions.html:69: // provided. On 2014/03/22 00:56:49, xhwang wrote: > Did you verify this? It used to be the case if audio key is provided, audio will > start playing, even though the video decoding isn't progressing. I did. If you comment out either of the 2 update() calls above, then the playback never starts.
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jrummell@chromium.org/203323006/120001
Message was sent while issue was closed.
Change committed as 169796 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
