Bug 114357 – [GStreamer] Media attribute preload="none" is not honored

• Component: Media
• Status: RESOLVED FIXED

[Balazs Kelemen]

Test: in the try it yourself page, add preload="none" attribute and submit code.
Expected: Video does not start to load (slider not filled)

[Balazs Kelemen]

Created attachment 197267 [details]
Patch

[Balazs Kelemen]

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.

[Build Bot]

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

[Build Bot]

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

[Gustavo Noronha (kov)]

Comment on attachment 197267 [details]
Patch

Looks sane to me, would be good to understand why the test didn't catch it indeed, though.

[Balazs Kelemen]

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).

[Balazs Kelemen]

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.

[Balazs Kelemen]

Created attachment 197860 [details]
Patch

[Eric Carlson]

(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

[Balazs Kelemen]

(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).

[Eric Carlson]

(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.

[Balazs Kelemen]

(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.)

[Eric Carlson]

(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 ;-)

[Philippe Normand]

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()

[Balazs Kelemen]

(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.

[Philippe Normand]

(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.

[Eric Carlson]

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.

[Balazs Kelemen]

Created attachment 198946 [details]
Patch

[Balazs Kelemen]

(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.

[Philippe Normand]

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

[Balazs Kelemen]

Committed r148840: <http://trac.webkit.org/changeset/148840>