Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
539103002:
Seeking media fragment URI before loadeddata event (Closed)
|
Created:
6 years, 3 months ago by amogh.bihani Modified:
6 years, 3 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@HAVE_NOTHING Project:
blink Visibility:
Public. |
DescriptionSeeking media fragment URI before loadeddata event
As per the spec, media fragment URI must seek to start time
before loadeddata event. This patch makes media element seek
before the loaddeddata event and thereby removes the
m_fragmentStartTime.
It also introduces the jumped state which avoids this seek if
currentTime has been set when readyState is HAVE_NOTHING.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182134
Patch Set 1 #
Total comments: 2
Patch Set 2 : Ready for review #
Total comments: 8
Patch Set 3 : Seeking media fragments if they have media controller #
Total comments: 18
Patch Set 4 : Addressing comments #
Total comments: 11
Patch Set 5 : Renaming file and addressing more comments #Patch Set 6 : updating layout tests #
Total comments: 1
Patch Set 7 : Rebase #
Total comments: 11
Patch Set 8 : renaming test and addressing comments #Messages
Total messages: 29 (7 generated)
amogh.bihani@samsung.com changed reviewers: + philipj@opera.com
https://codereview.chromium.org/539103002/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/539103002/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:1849: applyMediaFragmentURI(); Could you help me understand why this is here? I mean in the spec this is step 10, before loadeddata event. If we move this up then it would be easy to remove m_fragmentStartTime as well, otherwise I guess it is better to evaluate it before hand when we evalute m_fragmentEndtime in prepareMediaFragmentURI.
This doesn't quite seem right. In the spec, jumped is just a local variable used within the steps for "Once enough of the media data has been fetched to determine the duration of the media resource, its dimensions, and other metadata" We should implement those steps as closely as possible. We will have to change when the media fragment is parsed. At the step "If either the media resource or the address of the current media resource indicate a particular start time, then set the initial playback position to that time and, if jumped is still false, seek to that time and let jumped be true." we should do the equivalent of prepareMediaFragmentURI() and get the start time for immediate use. m_fragmentStartTime can then be removed, but m_fragmentEndTime will have to be kept even though it isn't per spec and I'd like to remove it: https://www.w3.org/Bugs/Public/show_bug.cgi?id=24910
https://codereview.chromium.org/539103002/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/539103002/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:1849: applyMediaFragmentURI(); On 2014/09/04 13:44:18, amogh.bihani wrote: > Could you help me understand why this is here? I mean in the spec this is step > 10, before loadeddata event. If we move this up then it would be easy to remove > m_fragmentStartTime as well, otherwise I guess it is better to evaluate it > before hand when we evalute m_fragmentEndtime in prepareMediaFragmentURI. Sorry, I didn't see this comment at first. You're right, we shouldn't have a split prepareMediaFragmentURI() and applyMediaFragmentURI(), it should all go in a single place.
amogh.bihani@samsung.com changed reviewers: + acolwell@chromium.org
PTAL. https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:1822: double end = fragmentParser.endTime(); Does end==0 needs to be taken care of? As per line 2430 (which uses >= for comparison) I don't think so, but wanted to confirm first. If it is not needed then we can also remove 'double end' and save one more variable. :) something like m_fragmentEndtime = fragmentParser.endTime(); if (m_fragmentEndTime != invalidTime()) m_fraggmentEndTime = std::min((std::max(0, m_fraggmentEndTime), duration); https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:446: void prepareMediaFragmentURI(); Sorry, I forgot these, I'll remove them in next PS.
https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:1822: double end = fragmentParser.endTime(); On 2014/09/05 09:05:43, amogh.bihani wrote: > Does end==0 needs to be taken care of? As per line 2430 (which uses >= for > comparison) I don't think so, but wanted to confirm first. If it is not needed > then we can also remove 'double end' and save one more variable. :) > > something like > m_fragmentEndtime = fragmentParser.endTime(); > if (m_fragmentEndTime != invalidTime()) > m_fraggmentEndTime = std::min((std::max(0, m_fraggmentEndTime), duration); At this point it seems like we should just set m_fragmentEndTime = fragmentParser.endTime() and it seems like HTMLMediaElement::playbackProgressTimerFired will behave just fine. Clamping to the duration seems pretty useless, I'm not sure why that was done originally. https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:1841: if (start !=MediaPlayer::invalidTime() && start > 0) { Missing whitespace after != https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:1845: jumped = true; Don't need to set jumped, the spec doesn't AFAICT.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:1822: double end = fragmentParser.endTime(); On 2014/09/05 22:49:11, philipj wrote: > On 2014/09/05 09:05:43, amogh.bihani wrote: > > Does end==0 needs to be taken care of? As per line 2430 (which uses >= for > > comparison) I don't think so, but wanted to confirm first. If it is not needed > > then we can also remove 'double end' and save one more variable. :) > > > > something like > > m_fragmentEndtime = fragmentParser.endTime(); > > if (m_fragmentEndTime != invalidTime()) > > m_fraggmentEndTime = std::min((std::max(0, m_fraggmentEndTime), duration); > > At this point it seems like we should just set m_fragmentEndTime = > fragmentParser.endTime() and it seems like > HTMLMediaElement::playbackProgressTimerFired will behave just fine. Clamping to > the duration seems pretty useless, I'm not sure why that was done originally. Done. https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:1845: jumped = true; On 2014/09/05 22:49:11, philipj wrote: > Don't need to set jumped, the spec doesn't AFAICT. Step 10 says "if jumped is still false, seek to that time and let jumped be true." I had not implemented step 13 as I thought it is a big change and should be in a new CL, although it turns out it wasn't so I am adding that in next PS.
https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/539103002/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:1845: jumped = true; On 2014/09/08 09:33:45, amogh.bihani wrote: > On 2014/09/05 22:49:11, philipj wrote: > > Don't need to set jumped, the spec doesn't AFAICT. > > Step 10 says "if jumped is still false, seek to that time and let jumped be > true." > > I had not implemented step 13 as I thought it is a big change and should be in a > new CL, although it turns out it wasn't so I am adding that in next PS. Oh sorry, sloppy spec reading on my part. That the use of jumped wasn't implemented was indeed why I missed this.
When the implementation is updated, another test will be needed as well. Step 13 has two branches that need testing, at least. https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/media-... File LayoutTests/media/media-controller-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/media-... LayoutTests/media/media-controller-media-fragment-uri.html:7: var video2; Don't need these two vars, you need them to end up on window anyway. https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/media-... LayoutTests/media/media-controller-media-fragment-uri.html:11: video = document.getElementById('video1'); I'd get rid of the ids and just use videos = getElementsByTagName('video') https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/media-... LayoutTests/media/media-controller-media-fragment-uri.html:13: run('controller = video.controller'); why run? https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/media-... LayoutTests/media/media-controller-media-fragment-uri.html:28: waitForEvent('seeked', function() waitForEventAndEnd("seeked") https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/media-... LayoutTests/media/media-controller-media-fragment-uri.html:34: window.addEventListener('load', onWindowLoad, false); Many tests have a runTest function, I think that's nicer than onWindowLoad. Note that you could also just put the script after the videos and not wait for the load event at all, if you want. https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/video-... File LayoutTests/media/video-currentTime-before-loadmetadata-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-currentTime-before-loadmetadata-media-fragment-uri.html:31: <p>Test currentTime values when setting while HAVE_NOTHING for media fragment URI.</p> Please name the two related tests similarly, maybe before-loadedmetadata in both? https://codereview.chromium.org/539103002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/539103002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:1845: if (jumped && m_mediaController && m_mediaController->currentTime() < currentTime()) This bit would be clearer if the initial playback position were made explicit. I've filed a spec bug pointing out that it's only used locally in these steps: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26771 I suggest a double initialPlaybackPosition locally in these steps. The structure should be something like: if (m_mediaController) { if (jumped && initialPlaybackPosition > m_mediaController->currentTime()) m_mediaController->setCurrentTime(initialPlaybackPosition); else seek(m_mediaController->currentTime()); } Writing that out, it's clear that this step is trying to get the controller and media element in sync, which is relevant to the bug you filed. Maybe there is no bug in the spec?
Oh right, you need to update the description to wrap the lines at 72 columns, and put back the title in a line of its own. Otherwise the resulting commit is going to be missing its title, which I've done myself: https://codereview.chromium.org/18181010/
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Sorry for the late reply. I was busy with other thing :( https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/media-... File LayoutTests/media/media-controller-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/media-... LayoutTests/media/media-controller-media-fragment-uri.html:7: var video2; On 2014/09/10 12:25:48, philipj wrote: > Don't need these two vars, you need them to end up on window anyway. Done. https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/media-... LayoutTests/media/media-controller-media-fragment-uri.html:11: video = document.getElementById('video1'); On 2014/09/10 12:25:48, philipj wrote: > I'd get rid of the ids and just use videos = getElementsByTagName('video') Done. https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/media-... LayoutTests/media/media-controller-media-fragment-uri.html:13: run('controller = video.controller'); On 2014/09/10 12:25:48, philipj wrote: > why run? Dont know... it was getting timeout without this :/ https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/media-... LayoutTests/media/media-controller-media-fragment-uri.html:28: waitForEvent('seeked', function() On 2014/09/10 12:25:48, philipj wrote: > waitForEventAndEnd("seeked") Done. https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/media-... LayoutTests/media/media-controller-media-fragment-uri.html:34: window.addEventListener('load', onWindowLoad, false); On 2014/09/10 12:25:48, philipj wrote: > Many tests have a runTest function, I think that's nicer than onWindowLoad. Note > that you could also just put the script after the videos and not wait for the > load event at all, if you want. Done. https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/video-... File LayoutTests/media/video-currentTime-before-loadmetadata-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-currentTime-before-loadmetadata-media-fragment-uri.html:31: <p>Test currentTime values when setting while HAVE_NOTHING for media fragment URI.</p> On 2014/09/10 12:25:48, philipj wrote: > Please name the two related tests similarly, maybe before-loadedmetadata in > both? But the other file has nothing to do with before loadedmetadata... it is the event I am looking at to check the currentTime. Here we are setting currentTime before HAVE_NOTHING. https://codereview.chromium.org/539103002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/539103002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:1845: if (jumped && m_mediaController && m_mediaController->currentTime() < currentTime()) On 2014/09/10 12:25:48, philipj wrote: > This bit would be clearer if the initial playback position were made explicit. > I've filed a spec bug pointing out that it's only used locally in these steps: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=26771 I guess then it would be going away from media element load algorithm (step 8)... right? > I suggest a double initialPlaybackPosition locally in these steps. The structure > should be something like: > > if (m_mediaController) { > if (jumped && initialPlaybackPosition > m_mediaController->currentTime()) > m_mediaController->setCurrentTime(initialPlaybackPosition); > else > seek(m_mediaController->currentTime()); > } > > Writing that out, it's clear that this step is trying to get the controller and > media element in sync, which is relevant to the bug you filed. Maybe there is no > bug in the spec? Ah yes! It works as intended after this. :)
OK, this looks almost ready now, but the test coverage is probably incomplete. For each of the four places that seek the media element or the media controller in the modified code, can you try commenting out that seek or seeking to the wrong place, and checking that some test fails? In particular the case of seeking a MediaController and then attaching a HTMLMediaElement which is in readyState HAVE_NOTHING is probably not tested. https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/video-... File LayoutTests/media/video-currentTime-before-loadmetadata-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-currentTime-before-loadmetadata-media-fragment-uri.html:31: <p>Test currentTime values when setting while HAVE_NOTHING for media fragment URI.</p> On 2014/09/11 13:35:59, amogh.bihani wrote: > On 2014/09/10 12:25:48, philipj wrote: > > Please name the two related tests similarly, maybe before-loadedmetadata in > > both? > > But the other file has nothing to do with before loadedmetadata... it is the > event I am looking at to check the currentTime. Here we are setting currentTime > before HAVE_NOTHING. Sorry, I meant video-currentTime-before-have-metadata.html, that showed up in the diff for patch set 3 because (I guess) it was copied from there. At least initially, those two tests were very similar. https://codereview.chromium.org/539103002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/539103002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:1845: if (jumped && m_mediaController && m_mediaController->currentTime() < currentTime()) On 2014/09/11 13:35:59, amogh.bihani wrote: > On 2014/09/10 12:25:48, philipj wrote: > > This bit would be clearer if the initial playback position were made explicit. > > I've filed a spec bug pointing out that it's only used locally in these steps: > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=26771 > > I guess then it would be going away from media element load algorithm (step > 8)... right? Yes, AFAICT that step doesn't amount to anything that couldn't be kept local. > > I suggest a double initialPlaybackPosition locally in these steps. The > structure > > should be something like: > > > > if (m_mediaController) { > > if (jumped && initialPlaybackPosition > m_mediaController->currentTime()) > > m_mediaController->setCurrentTime(initialPlaybackPosition); > > else > > seek(m_mediaController->currentTime()); > > } > > > > Writing that out, it's clear that this step is trying to get the controller > and > > media element in sync, which is relevant to the bug you filed. Maybe there is > no > > bug in the spec? > > Ah yes! It works as intended after this. :) Should I close the spec bug then? https://codereview.chromium.org/539103002/diff/120001/LayoutTests/media/media... File LayoutTests/media/media-controller-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/120001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri.html:3: <head></head> You don't need an empty <head>. If you like, don't need <html> or <body> either, see e.g. video-buffered.html https://codereview.chromium.org/539103002/diff/120001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri.html:28: waitForEvent('seeked'); This test is timing out, should use waitForEventAndEnd. https://codereview.chromium.org/539103002/diff/120001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri.html:30: <p>Test currentTime of media controller when one of the element has initial start time greater than 0.</p> Please move this description to the top, or <title> since it doesn't really matter if it's in the expectations file. https://codereview.chromium.org/539103002/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/539103002/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:1834: if (initialPlaybackPosition > 0) { Can you invert the order of these two conditions or merge them, so that it more obviously does what the spec says? If you like you can merge everything into an if (fragmentParser.startTime() != MediaPlayer::invalidTime()). https://codereview.chromium.org/539103002/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:1840: // FIXME: Add support for selecting tracks by ID with the Media Fragments track dimension. Maybe it's time to drop or move this FIXME, because this isn't where it should be fixed.
Patchset #5 (id:140001) has been deleted
Comments in PS 3 and 4. Ill get back with the results of commenting seek at varioud places. I'm getting build error at some unrelated places, I guess I pulled when the tree was closed. https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/video-... File LayoutTests/media/video-currentTime-before-loadmetadata-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/60001/LayoutTests/media/video-... LayoutTests/media/video-currentTime-before-loadmetadata-media-fragment-uri.html:31: <p>Test currentTime values when setting while HAVE_NOTHING for media fragment URI.</p> On 2014/09/11 14:38:25, philipj wrote: > On 2014/09/11 13:35:59, amogh.bihani wrote: > > On 2014/09/10 12:25:48, philipj wrote: > > > Please name the two related tests similarly, maybe before-loadedmetadata in > > > both? > > > > But the other file has nothing to do with before loadedmetadata... it is the > > event I am looking at to check the currentTime. Here we are setting > currentTime > > before HAVE_NOTHING. > > Sorry, I meant video-currentTime-before-have-metadata.html, that showed up in > the diff for patch set 3 because (I guess) it was copied from there. At least > initially, those two tests were very similar. Ah yes... this branch was made on top of that... Although after that patch merged I rebased it on top of master, the base URL is still showing <path>/blink.git@HAVE_NOTHING. (HAVE_NOTHING) was name of my local branch for that CL. https://codereview.chromium.org/539103002/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/539103002/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:1845: if (jumped && m_mediaController && m_mediaController->currentTime() < currentTime()) On 2014/09/11 14:38:26, philipj wrote: > On 2014/09/11 13:35:59, amogh.bihani wrote: > > On 2014/09/10 12:25:48, philipj wrote: > > > This bit would be clearer if the initial playback position were made > explicit. > > > I've filed a spec bug pointing out that it's only used locally in these > steps: > > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=26771 > > > > I guess then it would be going away from media element load algorithm (step > > 8)... right? > > Yes, AFAICT that step doesn't amount to anything that couldn't be kept local. > > > > I suggest a double initialPlaybackPosition locally in these steps. The > > structure > > > should be something like: > > > > > > if (m_mediaController) { > > > if (jumped && initialPlaybackPosition > > m_mediaController->currentTime()) > > > m_mediaController->setCurrentTime(initialPlaybackPosition); > > > else > > > seek(m_mediaController->currentTime()); > > > } > > > > > > Writing that out, it's clear that this step is trying to get the controller > > and > > > media element in sync, which is relevant to the bug you filed. Maybe there > is > > no > > > bug in the spec? > > > > Ah yes! It works as intended after this. :) > > Should I close the spec bug then? Yes sure :) https://codereview.chromium.org/539103002/diff/120001/LayoutTests/media/media... File LayoutTests/media/media-controller-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/120001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri.html:3: <head></head> On 2014/09/11 14:38:26, philipj wrote: > You don't need an empty <head>. If you like, don't need <html> or <body> either, > see e.g. video-buffered.html Ummm... I was advised by srirama.m@ to add the <html> tag because he was onced asked in one of his CLs https://codereview.chromium.org/539103002/diff/120001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri.html:28: waitForEvent('seeked'); On 2014/09/11 14:38:26, philipj wrote: > This test is timing out, should use waitForEventAndEnd. Done. https://codereview.chromium.org/539103002/diff/120001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri.html:30: <p>Test currentTime of media controller when one of the element has initial start time greater than 0.</p> On 2014/09/11 14:38:28, philipj wrote: > Please move this description to the top, or <title> since it doesn't really > matter if it's in the expectations file. Done. https://codereview.chromium.org/539103002/diff/120001/Source/core/html/HTMLMe... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/539103002/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:1834: if (initialPlaybackPosition > 0) { On 2014/09/11 14:38:28, philipj wrote: > Can you invert the order of these two conditions or merge them, so that it more > obviously does what the spec says? Done. > If you like you can merge everything into an > if (fragmentParser.startTime() != MediaPlayer::invalidTime()). Pardon me if I got it wrong, but then I guess we wont be needing line 1830. That would cause a problem on line 1844 if we do not set it to 0. https://codereview.chromium.org/539103002/diff/120001/Source/core/html/HTMLMe... Source/core/html/HTMLMediaElement.cpp:1840: // FIXME: Add support for selecting tracks by ID with the Media Fragments track dimension. On 2014/09/11 14:38:28, philipj wrote: > Maybe it's time to drop or move this FIXME, because this isn't where it should > be fixed. Done.
On 2014/09/11 14:38:28, philipj wrote: > OK, this looks almost ready now, but the test coverage is probably incomplete. > For each of the four places that seek the media element or the media controller > in the modified code, can you try commenting out that seek or seeking to the > wrong place, and checking that some test fails? Tests failures: *disabling seek for m_defaultPlaybackStartPosition (line1824): video-currentTime-before-have-metadata.html video-currentTime-before-have-metadata-media-fragment-uri.html *disabling setCurrentTime when jumped (line1842): media-controller-media-fragment-uri-consecutive-append.html media-controller-media-fragment-uri.html *disabling seek to mediacontroller currentTime when not jumped (line1844): media-controller-media-fragment-uri-conseutive-append.html *disabling seek when not jumped and intialPlaybackPosition (line1836): /media-fragments/TC0005.html /media-fragments/TC0006.html /media-fragments/TC0009.html /media-fragments/TC0011.html /media-fragments/TC0014.html /media-fragments/TC0024.html /media-fragments/TC0035.html /media-fragments/TC0036.html /media-fragments/TC0037.html /media-fragments/TC0038.html /media-fragments/TC0039.html /media-fragments/TC0051.html /media-fragments/TC0052.html /media-fragments/TC0053.html /media-fragments/TC0054.html /media-fragments/TC0055.html /media-fragments/TC0059.html /media-fragments/TC0072.html /media-fragments/TC0078.html /media-fragments/TC0079.html /media-fragments/TC0085.html /media-fragments/TC0086.html /media-fragments/TC0087.html /media-fragments/TC0088.html /media-fragments/TC0089.html /media-fragments/TC0090.html /media-fragments/TC0091.html
https://codereview.chromium.org/539103002/diff/180001/LayoutTests/media/video... File LayoutTests/media/video-currentTime-before-have-metadata-media-fragment-uri-expected.txt (right): https://codereview.chromium.org/539103002/diff/180001/LayoutTests/media/video... LayoutTests/media/video-currentTime-before-have-metadata-media-fragment-uri-expected.txt:1: This test is failing because of this whitespace. I will remove this in the next PS.
On 2014/09/12 13:37:04, amogh.bihani wrote: > https://codereview.chromium.org/539103002/diff/180001/LayoutTests/media/video... > File > LayoutTests/media/video-currentTime-before-have-metadata-media-fragment-uri-expected.txt > (right): > > https://codereview.chromium.org/539103002/diff/180001/LayoutTests/media/video... > LayoutTests/media/video-currentTime-before-have-metadata-media-fragment-uri-expected.txt:1: > > This test is failing because of this whitespace. I will remove this in the next > PS. Ping!
On 2014/09/12 13:31:31, amogh.bihani wrote: > On 2014/09/11 14:38:28, philipj wrote: > > OK, this looks almost ready now, but the test coverage is probably incomplete. > > For each of the four places that seek the media element or the media > controller > > in the modified code, can you try commenting out that seek or seeking to the > > wrong place, and checking that some test fails? > > Tests failures: > *disabling seek for m_defaultPlaybackStartPosition (line1824): > video-currentTime-before-have-metadata.html > video-currentTime-before-have-metadata-media-fragment-uri.html > > *disabling setCurrentTime when jumped (line1842): > media-controller-media-fragment-uri-consecutive-append.html > media-controller-media-fragment-uri.html > > *disabling seek to mediacontroller currentTime when not jumped (line1844): > media-controller-media-fragment-uri-conseutive-append.html > > *disabling seek when not jumped and intialPlaybackPosition (line1836): > /media-fragments/TC0005.html > /media-fragments/TC0006.html > /media-fragments/TC0009.html > /media-fragments/TC0011.html > /media-fragments/TC0014.html > /media-fragments/TC0024.html > /media-fragments/TC0035.html > /media-fragments/TC0036.html > /media-fragments/TC0037.html > /media-fragments/TC0038.html > /media-fragments/TC0039.html > /media-fragments/TC0051.html > /media-fragments/TC0052.html > /media-fragments/TC0053.html > /media-fragments/TC0054.html > /media-fragments/TC0055.html > /media-fragments/TC0059.html > /media-fragments/TC0072.html > /media-fragments/TC0078.html > /media-fragments/TC0079.html > /media-fragments/TC0085.html > /media-fragments/TC0086.html > /media-fragments/TC0087.html > /media-fragments/TC0088.html > /media-fragments/TC0089.html > /media-fragments/TC0090.html > /media-fragments/TC0091.html Thanks, it sounds like there is already sufficient test coverage here.
My apologies for letting this sit unattended. The code looks good to me, but when I took a second look at the test I noticed that I didn't understand one of them. https://codereview.chromium.org/539103002/diff/120001/LayoutTests/media/media... File LayoutTests/media/media-controller-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/120001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri.html:3: <head></head> On 2014/09/12 11:02:23, amogh.bihani wrote: > On 2014/09/11 14:38:26, philipj wrote: > > You don't need an empty <head>. If you like, don't need <html> or <body> > either, > > see e.g. video-buffered.html > > Ummm... I was advised by srirama.m@ to add the <html> tag because he was onced > asked in one of his CLs In the style of an existing test is OK. I happen to prefer minimalist tests using testharness.js, but am not going to enforce it. https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... File LayoutTests/media/media-controller-media-fragment-uri-consecutive-append.html (right): https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri-consecutive-append.html:4: <title>Test currentTime for media controller when media fragments are added consicutively.</title> Media fragments aren't being added consecutively in this test, because there's only one media fragment added anywhere. I think what this was intended to test is adding a media element which hasn't even reached HAVE_METADATA yet to a media controller with a currentTime > 0 and seeing that the new media element ends up in sync with the media controller. If this is the intention, the test doesn't need to involve media fragments at all. Instead, create a video and set currentTime to 2. In the seeked event, create a media controller and attach it to that element. Then create a second video element and attach it to the media controller. Wait for the seeked event on the second media element and pass the test if in that seeked event all currentTimes are 2. If this was intended to test something else, please rename or otherwise clarify. Typo: consecutively. https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri-consecutive-append.html:7: <video controls></video> The controls attribute is probably not needed here. https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri-consecutive-append.html:22: setTimeout(function() { In general, never use setTimeout unless there's no other way, it's a good way to create flaky tests. In this case I think you want to wait until the first video is loaded and has seeked, in which case waiting for the seeked event is more direct. https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... File LayoutTests/media/media-controller-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri.html:7: <video controls mediaGroup="group"></video> The controls attribute is probably not needed here. https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/video... File LayoutTests/media/video-currentTime-before-have-metadata-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/video... LayoutTests/media/video-currentTime-before-have-metadata-media-fragment-uri.html:7: <video controls id="video"></video> Probably don't need controls.
https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... File LayoutTests/media/media-controller-media-fragment-uri-consecutive-append.html (right): https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri-consecutive-append.html:4: <title>Test currentTime for media controller when media fragments are added consicutively.</title> On 2014/09/16 12:27:15, philipj wrote: > Media fragments aren't being added consecutively in this test, because there's > only one media fragment added anywhere. I think what this was intended to test > is adding a media element which hasn't even reached HAVE_METADATA yet to a media > controller with a currentTime > 0 and seeing that the new media element ends up > in sync with the media controller. If this is the intention, the test doesn't > need to involve media fragments at all. Instead, create a video and set > currentTime to 2. In the seeked event, create a media controller and attach it > to that element. Then create a second video element and attach it to the media > controller. Wait for the seeked event on the second media element and pass the > test if in that seeked event all currentTimes are 2. > > If this was intended to test something else, please rename or otherwise clarify. > > Typo: consecutively. I wanted to test whether the new media element seeks to the media controller's time when added after a media fragment URI. (Earlier, I had a confusion regarding this in the spec, hence had created the test at that time.) So this test basically is for intitial playback position. Should we have another test for seek in media group as well? https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri-consecutive-append.html:7: <video controls></video> On 2014/09/16 12:27:15, philipj wrote: > The controls attribute is probably not needed here. Done. https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri-consecutive-append.html:22: setTimeout(function() { On 2014/09/16 12:27:15, philipj wrote: > In general, never use setTimeout unless there's no other way, it's a good way to > create flaky tests. In this case I think you want to wait until the first video > is loaded and has seeked, in which case waiting for the seeked event is more > direct. Done. https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... File LayoutTests/media/media-controller-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri.html:7: <video controls mediaGroup="group"></video> On 2014/09/16 12:27:15, philipj wrote: > The controls attribute is probably not needed here. Done. https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/video... File LayoutTests/media/video-currentTime-before-have-metadata-media-fragment-uri.html (right): https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/video... LayoutTests/media/video-currentTime-before-have-metadata-media-fragment-uri.html:7: <video controls id="video"></video> On 2014/09/16 12:27:15, philipj wrote: > Probably don't need controls. Done.
LGTM to land with or without an additional test. https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... File LayoutTests/media/media-controller-media-fragment-uri-consecutive-append.html (right): https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... LayoutTests/media/media-controller-media-fragment-uri-consecutive-append.html:4: <title>Test currentTime for media controller when media fragments are added consicutively.</title> On 2014/09/16 13:46:01, amogh.bihani wrote: > On 2014/09/16 12:27:15, philipj wrote: > > Media fragments aren't being added consecutively in this test, because there's > > only one media fragment added anywhere. I think what this was intended to test > > is adding a media element which hasn't even reached HAVE_METADATA yet to a > media > > controller with a currentTime > 0 and seeing that the new media element ends > up > > in sync with the media controller. If this is the intention, the test doesn't > > need to involve media fragments at all. Instead, create a video and set > > currentTime to 2. In the seeked event, create a media controller and attach it > > to that element. Then create a second video element and attach it to the media > > controller. Wait for the seeked event on the second media element and pass the > > test if in that seeked event all currentTimes are 2. > > > > If this was intended to test something else, please rename or otherwise > clarify. > > > > Typo: consecutively. > > I wanted to test whether the new media element seeks to the media controller's > time when added after a media fragment URI. (Earlier, I had a confusion > regarding this in the spec, hence had created the test at that time.) > > So this test basically is for intitial playback position. > > Should we have another test for seek in media group as well? Thanks, the updated test is easier to understand. Since you tested that all of the seeks were covered by some test it's OK by me, but if there is some directed test that you think would add value go ahead.
On 2014/09/16 19:18:15, philipj wrote: > LGTM to land with or without an additional test. > > https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... > File > LayoutTests/media/media-controller-media-fragment-uri-consecutive-append.html > (right): > > https://codereview.chromium.org/539103002/diff/200001/LayoutTests/media/media... > LayoutTests/media/media-controller-media-fragment-uri-consecutive-append.html:4: > <title>Test currentTime for media controller when media fragments are added > consicutively.</title> > On 2014/09/16 13:46:01, amogh.bihani wrote: > > On 2014/09/16 12:27:15, philipj wrote: > > > Media fragments aren't being added consecutively in this test, because > there's > > > only one media fragment added anywhere. I think what this was intended to > test > > > is adding a media element which hasn't even reached HAVE_METADATA yet to a > > media > > > controller with a currentTime > 0 and seeing that the new media element ends > > up > > > in sync with the media controller. If this is the intention, the test > doesn't > > > need to involve media fragments at all. Instead, create a video and set > > > currentTime to 2. In the seeked event, create a media controller and attach > it > > > to that element. Then create a second video element and attach it to the > media > > > controller. Wait for the seeked event on the second media element and pass > the > > > test if in that seeked event all currentTimes are 2. > > > > > > If this was intended to test something else, please rename or otherwise > > clarify. > > > > > > Typo: consecutively. > > > > I wanted to test whether the new media element seeks to the media controller's > > time when added after a media fragment URI. (Earlier, I had a confusion > > regarding this in the spec, hence had created the test at that time.) > > > > So this test basically is for intitial playback position. > > > > Should we have another test for seek in media group as well? > > Thanks, the updated test is easier to understand. Since you tested that all of > the seeks were covered by some test it's OK by me, but if there is some directed > test that you think would add value go ahead. Thanks :) I think we have ample test coverage.
The CQ bit was checked by amogh.bihani@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/539103002/220001
Message was sent while issue was closed.
Committed patchset #8 (id:220001) as 182134 |
