RESOLVED FIXED 116817
[GStreamer] support preload="metadata"
https://bugs.webkit.org/show_bug.cgi?id=116817
Summary [GStreamer] support preload="metadata"
Balazs Kelemen
Reported 2013-05-27 03:33:10 PDT
Add appropriate behavior.
Attachments
Patch (5.42 KB, patch)
2013-05-27 03:51 PDT, Balazs Kelemen
no flags
Patch (5.59 KB, patch)
2013-06-27 04:52 PDT, Balazs Kelemen
no flags
Patch (6.29 KB, patch)
2013-06-27 07:49 PDT, Balazs Kelemen
no flags
Archive of layout-test-results from APPLE-EWS-3 for win-future (1.07 MB, application/zip)
2013-06-27 23:40 PDT, Build Bot
no flags
Patch (6.14 KB, patch)
2013-07-03 06:22 PDT, Balazs Kelemen
no flags
Patch (6.84 KB, patch)
2013-07-03 15:09 PDT, Balazs Kelemen
no flags
Balazs Kelemen
Comment 1 2013-05-27 03:51:12 PDT
Philippe Normand
Comment 2 2013-05-27 03:59:46 PDT
Can it be rebased against ToT please? Would be nice to probe the new test on mac.
Balazs Kelemen
Comment 3 2013-05-27 04:03:12 PDT
(In reply to comment #2) > Can it be rebased against ToT please? Would be nice to probe the new test on mac. It has a dependency. I will reupload after bug 116642 is resolved.
Philippe Normand
Comment 4 2013-06-27 03:44:26 PDT
So this patch can be rebased now? :)
Balazs Kelemen
Comment 5 2013-06-27 04:33:06 PDT
(In reply to comment #4) > So this patch can be rebased now? :) Ah, sure, I was forgetting my promise :)
Balazs Kelemen
Comment 6 2013-06-27 04:52:47 PDT
Philippe Normand
Comment 7 2013-06-27 05:46:06 PDT
Comment on attachment 205588 [details] Patch No test results?
Balazs Kelemen
Comment 8 2013-06-27 07:49:57 PDT
Balazs Kelemen
Comment 9 2013-06-27 07:50:27 PDT
(In reply to comment #7) > (From update of attachment 205588 [details]) > No test results? Fixed, sorry...
Philippe Normand
Comment 10 2013-06-27 08:26:20 PDT
Comment on attachment 205607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205607&action=review Looks good, I only have a couple of questions about the test > LayoutTests/media/video-load-preload-metadata.html:22 > + setTimeout(function () { Why doing this with a timer? > LayoutTests/media/video-load-preload-metadata.html:24 > + playbackStarted = true; Perhaps this can be set when the play event is received.
Eric Carlson
Comment 11 2013-06-27 09:14:08 PDT
Comment on attachment 205607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205607&action=review > LayoutTests/media/video-load-preload-metadata.html:17 > + function start() > + { > + findMediaElement(); > + video.src = findMediaFile("video", "content/test"); > + > + testExpected("video.preload", "metadata"); > + testExpected("video.readyState", HTMLMediaElement.HAVE_NOTHING); > + waitForEvent('loadedmetadata', onLoadedMetadata); > + waitForEvent('canplaythrough', onCanPlayThrough); > + failTestIn(3000); > + } I am afraid this test will be flakey because there is no guarantee that 'load' will fire before the media engine starts loading data. If you want to test that readyState is HAVE_NOTHING, I think the only way to be sure is to have "preload=none" on the element and set preload to "metadata" in the load handler. >> LayoutTests/media/video-load-preload-metadata.html:22 >> + setTimeout(function () { > > Why doing this with a timer? Agreed, no timers unless absolutely necessary please.
Build Bot
Comment 12 2013-06-27 23:40:04 PDT
Comment on attachment 205607 [details] Patch Attachment 205607 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/965922 New failing tests: media/video-load-preload-metadata.html
Build Bot
Comment 13 2013-06-27 23:40:06 PDT
Created attachment 205671 [details] Archive of layout-test-results from APPLE-EWS-3 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: APPLE-EWS-3 Port: win-future Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit
Balazs Kelemen
Comment 14 2013-06-28 09:17:24 PDT
(In reply to comment #11) > (From update of attachment 205607 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205607&action=review > > > LayoutTests/media/video-load-preload-metadata.html:17 > > + function start() > > + { > > + findMediaElement(); > > + video.src = findMediaFile("video", "content/test"); > > + > > + testExpected("video.preload", "metadata"); > > + testExpected("video.readyState", HTMLMediaElement.HAVE_NOTHING); > > + waitForEvent('loadedmetadata', onLoadedMetadata); > > + waitForEvent('canplaythrough', onCanPlayThrough); > > + failTestIn(3000); > > + } > > I am afraid this test will be flakey because there is no guarantee that 'load' will fire before the media engine starts loading data. > > If you want to test that readyState is HAVE_NOTHING, I think the only way to be sure is to have "preload=none" on the element and set preload to "metadata" in the load handler. > HAVE_NOTHING is not so important in this test, the important part is |testExpected("video.readyState", HTMLMediaElement.HAVE_ENOUGH_DATA, "<")| before playback is started. I will remove HAVE_NOTHING check.
Balazs Kelemen
Comment 15 2013-06-28 09:27:48 PDT
(In reply to comment #10) > (From update of attachment 205607 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205607&action=review > > Looks good, I only have a couple of questions about the test > > > LayoutTests/media/video-load-preload-metadata.html:22 > > + setTimeout(function () { > > Why doing this with a timer? Without the timer we can't be sure that canplaythrough not fires too early. Basically the problem is that there is no way to test that smg _not_ fires without a timer. > > > LayoutTests/media/video-load-preload-metadata.html:24 > > + playbackStarted = true; > > Perhaps this can be set when the play event is received. We need it in the canPlayThrough callback and canplaythrough fires earlier than play. If you have a good idea how to avoid the timer without loosing testing completeness I'm open to that, but currently I would rather keep it.
Balazs Kelemen
Comment 16 2013-07-03 06:22:01 PDT
Balazs Kelemen
Comment 17 2013-07-03 06:22:31 PDT
(In reply to comment #14) > (In reply to comment #11) > > (From update of attachment 205607 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=205607&action=review > > > > > LayoutTests/media/video-load-preload-metadata.html:17 > > > + function start() > > > + { > > > + findMediaElement(); > > > + video.src = findMediaFile("video", "content/test"); > > > + > > > + testExpected("video.preload", "metadata"); > > > + testExpected("video.readyState", HTMLMediaElement.HAVE_NOTHING); > > > + waitForEvent('loadedmetadata', onLoadedMetadata); > > > + waitForEvent('canplaythrough', onCanPlayThrough); > > > + failTestIn(3000); > > > + } > > > > I am afraid this test will be flakey because there is no guarantee that 'load' will fire before the media engine starts loading data. > > > > If you want to test that readyState is HAVE_NOTHING, I think the only way to be sure is to have "preload=none" on the element and set preload to "metadata" in the load handler. > > > > HAVE_NOTHING is not so important in this test, the important part is |testExpected("video.readyState", HTMLMediaElement.HAVE_ENOUGH_DATA, "<")| before playback is started. I will remove HAVE_NOTHING check. I removed this, otherwise it's the same patch.
Eric Carlson
Comment 18 2013-07-03 09:48:58 PDT
Comment on attachment 205999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205999&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1523 > + bool shouldDownload = !isLiveStream() && m_preload == MediaPlayer::Auto; > if (shouldDownload) { Do you really want to clear the GST_PLAY_FLAG_DOWNLOAD flag when m_preload changes from "auto" to "metadata" or "none" (eg. after downloading has already started)?
Balazs Kelemen
Comment 19 2013-07-03 15:07:50 PDT
(In reply to comment #18) > (From update of attachment 205999 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205999&action=review > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1523 > > + bool shouldDownload = !isLiveStream() && m_preload == MediaPlayer::Auto; > > if (shouldDownload) { > > Do you really want to clear the GST_PLAY_FLAG_DOWNLOAD flag when m_preload changes from "auto" to "metadata" or "none" (eg. after downloading has already started)? Good point. It needs a bit of additional logic so I think it's better to do one more round.
Balazs Kelemen
Comment 20 2013-07-03 15:09:11 PDT
Balazs Kelemen
Comment 21 2013-07-04 00:45:02 PDT
Comment on attachment 206028 [details] Patch Clearing flags on attachment: 206028 Committed r152391: <http://trac.webkit.org/changeset/152391>
Balazs Kelemen
Comment 22 2013-07-04 00:45:09 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.