Test: in the try it yourself page, add preload="none" attribute and submit code. Expected: Video does not start to load (slider not filled)
Created attachment 197267 [details] Patch
Although I set r?, this is WIP since we should have a test. Btw media/video-preload.html should have detected this bug so I will check why it had not.
Comment on attachment 197267 [details] Patch Attachment 197267 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17643116 New failing tests: fast/repaint/japanese-rl-selection-repaint-in-regions.html
Created attachment 197409 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Comment on attachment 197267 [details] Patch Looks sane to me, would be good to understand why the test didn't catch it indeed, though.
Seems like we are facing with multiple issues here. First, I think the video-preload.html test is wrong: video.src = url; if (movieInfo.current > 0) video.load(); Funny thing is that only the first moveInfo test the behavior of preload="none", and it obviously pass because load is not called for it (changing src itself should not start the load.) Easy thing, just remove the if. But the second problem is that load() does not honor the preload attribute again. It's another bug (or at least not standard compliant behavior), in HTMLMediaElement. So in the current situation I'm not able to fix this test in a way to trigger this and only this bug. I am working on a test that contains the video tag and the source statically, but even with that I have problems, because is seems like the loadedmetadata event triggers before onload. I hope I can make it work somehow though. I also plan to investigate in the other problems (not in this patch ofc).
This bug triggers only if the media is loaded from a server (because the buggy code path I removed is in MediaPlayerPrivateGStreamer::changeDuration and it is only called in that case). So we need a http test.
Created attachment 197860 [details] Patch
(In reply to comment #6) > Seems like we are facing with multiple issues here. > First, I think the video-preload.html test is wrong: > > video.src = url; > if (movieInfo.current > 0) > video.load(); > > Funny thing is that only the first moveInfo test the behavior of preload="none", and it obviously pass because load is not called for it (changing src itself should not start the load.) Changing src *is* supposed to trigger the load algorithm [1]: If a src attribute of a media element is set or changed, the user agent must invoke the media element's media element load algorithm. (Removing the src attribute does not do this, even if there are source elements present.) > Easy thing, just remove the if. But the second problem is that load() does not honor the preload attribute again. It's another bug (or at least not standard compliant behavior), in HTMLMediaElement. I don't think this interpretation is correct. Section 4.8.10.5 of the spec says [2]: media.load() Causes the element to reset and start selecting and loading a new media resource from scratch. @preload is a hint to the user agent about what the initial buffering behavior should be before there is any user interaction or script. Further, the media element load algorithm says nothing about preload [3] [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#attr-media-src [2] http://dev.w3.org/html5/spec-author-view/video.html#loading-the-media-resource [3] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#media-element-load-algorithm
(In reply to comment #9) > (In reply to comment #6) > > Seems like we are facing with multiple issues here. > > First, I think the video-preload.html test is wrong: > > > > video.src = url; > > if (movieInfo.current > 0) > > video.load(); > > > > Funny thing is that only the first moveInfo test the behavior of preload="none", and it obviously pass because load is not called for it (changing src itself should not start the load.) > > Changing src *is* supposed to trigger the load algorithm [1]: > > If a src attribute of a media element is set or changed, the user agent > must invoke the media element's media element load algorithm. (Removing > the src attribute does not do this, even if there are source elements present.) That's right, I realized that the bug does not takes effect for local files, and that is the reason why this test did not triggered the bug. > > > Easy thing, just remove the if. But the second problem is that load() does not honor the preload attribute again. It's another bug (or at least not standard compliant behavior), in HTMLMediaElement. > > I don't think this interpretation is correct. Section 4.8.10.5 of the spec says [2]: > > media.load() > Causes the element to reset and start selecting and loading a new > media resource from scratch. > > @preload is a hint to the user agent about what the initial buffering behavior should be before there is any user interaction or script. Further, the media element load algorithm says nothing about preload [3] > > > [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#attr-media-src > [2] http://dev.w3.org/html5/spec-author-view/video.html#loading-the-media-resource > [3] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#media-element-load-algorithm That's right, but I believe the current behavior is that we not just start the loading but also the decoding and buffering of decoded data. See: void MediaPlayerPrivateQTKit::prepareToPlay() { LOG(Media, "MediaPlayerPrivateQTKit::prepareToPlay(%p)", this); setPreload(MediaPlayer::Auto); } It is called from HTMLMediaElement::load, so load() ignores the preload attribute (this is the case with the GStreamer backend as well).
(In reply to comment #10) > That's right, but I believe the current behavior is that we not just start the loading but also the decoding and buffering of decoded data. See: > > void MediaPlayerPrivateQTKit::prepareToPlay() > { > LOG(Media, "MediaPlayerPrivateQTKit::prepareToPlay(%p)", this); > setPreload(MediaPlayer::Auto); > } > > It is called from HTMLMediaElement::load, so load() ignores the preload attribute (this is the case with the GStreamer backend as well). Yes, and I was trying to say that this is the correct behavior. load(), whether called explicitly from script or implicitly as a part of the media element load algorithm *should* ignore preload. @preload is just a hint the the UA about what to do *before* load() is called.
(In reply to comment #11) > (In reply to comment #10) > > That's right, but I believe the current behavior is that we not just start the loading but also the decoding and buffering of decoded data. See: > > > > void MediaPlayerPrivateQTKit::prepareToPlay() > > { > > LOG(Media, "MediaPlayerPrivateQTKit::prepareToPlay(%p)", this); > > setPreload(MediaPlayer::Auto); > > } > > > > It is called from HTMLMediaElement::load, so load() ignores the preload attribute (this is the case with the GStreamer backend as well). > > Yes, and I was trying to say that this is the correct behavior. load(), whether called explicitly from script or implicitly as a part of the media element load algorithm *should* ignore preload. > > @preload is just a hint the the UA about what to do *before* load() is called. Thanks for clarifying this. (Sorry for arguing without reading the spec, honestly I just read some material form w3schools, etc, and there the explanation, although not very clear, rather says that load() is about kicking off the resource load.)
(In reply to comment #12) > > Thanks for clarifying this. (Sorry for arguing without reading the spec, honestly I just read some material form w3schools, etc, and there the explanation, although not very clear, rather says that load() is about kicking off the resource load.) No worries - I had to go and re-read the spec myself to make sure I was telling the truth ;-)
Comment on attachment 197860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197860&action=review > LayoutTests/http/tests/media/video-preload.html:11 > + setTimeout(function() { Timeouts should be avoided, they tend to make tests flaky. Perhaps you can trigger the test when all loadedmetadata events have been fired. video-paint-test.js might have some functions of interest. > LayoutTests/http/tests/media/video-preload.html:26 > + <source src="resources/test.mp4" type="video/mp4" /> > + <source src="resources/test.ogv" type="video/ogg" /> Are multiple sources a requirement for this test? If not, please use findMediaFile()
(In reply to comment #14) > (From update of attachment 197860 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=197860&action=review > > > LayoutTests/http/tests/media/video-preload.html:11 > > + setTimeout(function() { > > Timeouts should be avoided, they tend to make tests flaky. Perhaps you can trigger the test when all loadedmetadata events have been fired. video-paint-test.js might have some functions of interest. It won't work, I'm testing that loadedmetadata does _not_ firing. > > > LayoutTests/http/tests/media/video-preload.html:26 > > + <source src="resources/test.mp4" type="video/mp4" /> > > + <source src="resources/test.ogv" type="video/ogg" /> > > Are multiple sources a requirement for this test? If not, please use findMediaFile() No, but unfortunately I could not trigger the bug if I set the src via javascript. I added two sources just to make sure it will work on every platform. The test could work with the patch but IMHO it has no value if it does not trigger the bug without the patch.
(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 197860 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=197860&action=review > > > > > LayoutTests/http/tests/media/video-preload.html:11 > > > + setTimeout(function() { > > > > Timeouts should be avoided, they tend to make tests flaky. Perhaps you can trigger the test when all loadedmetadata events have been fired. video-paint-test.js might have some functions of interest. > > It won't work, I'm testing that loadedmetadata does _not_ firing. > Snap! Is there another event the test could rely on? It'd be really nice to avoid that timeout but if there's no other way, so be it.
Comment on attachment 197860 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197860&action=review > LayoutTests/http/tests/media/video-preload.html:7 > + window.event.target.hasMeta = true; This is unusual, why not use the Event parameter passed to the function instead of the legacy "window.event" global?? Nit: a function's initial brace should be on a new line. >>> LayoutTests/http/tests/media/video-preload.html:26 >>> + <source src="resources/test.ogv" type="video/ogg" /> >> >> Are multiple sources a requirement for this test? If not, please use findMediaFile() > > No, but unfortunately I could not trigger the bug if I set the src via javascript. I added two sources just to make sure it will work on every platform. The test could work with the patch but IMHO it has no value if it does not trigger the bug without the patch. Please add a comment to the test explaining this so someone doesn't "optimize" it later.
Created attachment 198946 [details] Patch
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > (From update of attachment 197860 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=197860&action=review > > > > > > > LayoutTests/http/tests/media/video-preload.html:11 > > > > + setTimeout(function() { > > > > > > Timeouts should be avoided, they tend to make tests flaky. Perhaps you can trigger the test when all loadedmetadata events have been fired. video-paint-test.js might have some functions of interest. > > > > It won't work, I'm testing that loadedmetadata does _not_ firing. > > > > Snap! Is there another event the test could rely on? It'd be really nice to avoid that timeout but if there's no other way, so be it. All the event flow is the same without the patch if the content is being set in javascript. Added comment for the test, and improved it.
Comment on attachment 198946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198946&action=review Ok, please fix the little typo before landing :) > LayoutTests/http/tests/media/video-preload.html:24 > + <!-- This test referst to the video content statically because otherwise the original bug could not have been triggered. --> Typo: refers
Committed r148840: <http://trac.webkit.org/changeset/148840>