Closed Bug 1165203 Opened 9 years ago Closed 9 years ago

Video doesn't start on nicovideo.jp because we don't send a loadedmetadata event in response to video.load()

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Version:
29 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
p11 + ---
firefox41 --- fixed

People

(Reporter: hsteen, Assigned: birtles)

References

(
URL
)

Details

(Keywords: site-compat)

Attachments

(1 file, 1 obsolete file)

Make calls to load() upgrade preload=none to preload=metadata
8.61 KB, patch
cpearce
: review+
Details | Diff | Splinter Review 
The site http://sp.nicovideo.jp has several problems. There's some backend browser sniffing which we're contacting them about in https://webcompat.com/issues/1074 - but if we work around that, another issue appears. Now the video is inserted into the page, but it doesn't start automatically. Nor does it start on first tap/click - it takes three taps to get it going.

It seems the reason is that Firefox doesn't fire the loadedmetadata event when the page expects. This is why it doesn't start auto-playing and needs a couple of extra taps.

Spec:
https://html.spec.whatwg.org/multipage/embedded-content.html#event-media-loadedmetadata

Demo: go to
http://www.w3.org/2010/05/video/mediaevents.html
and click/tap the "load()" button. In other browsers, note how the counter for loadedmetadata goes up to 1 and the square turns green after a few seconds of loading. In Firefox this counter doesn't move until you click the play() button.

This causes the problem when nicovideo's watch/html5.min.js script does

    this.video_element.addEventListener("loadedmetadata", function() {
                    a.videoLoadeddata()
                })

and

            }, a.prototype.videoLoadeddata = function() {
                this.video_loaded = !0, this.checkLoadFinish()
            }

which is supposed to start the video.
(The workaround for the backend problem is to add an Android version number in the UA string - for example make sure it includes "Android 4.4.4". I forgot to mention that this site also requires login to play video - contact me for test account if you wish to test a fix on the real site.)
Screwed the formatting in the previous comment. Here we go again.


I'm trying to read the spec to see what's expected here as the preload attribute is "none".

If I set preload to "metadata" for instance, it works as expected. Try hitting the load() button after doing
> document.getElementsByTagName('video')[0].preload = "metadata"

Well, on to the spec; from https://html.spec.whatwg.org/multipage/embedded-content.html#loading-the-media-resource:attr-media-preload
> The resource fetch algorithm for a media element and a given absolute URL or media provider object is as follows:
> 
>     1. If the algorithm was invoked with a URL, then let mode be remote, otherwise let mode be local.
> 
>     2. If mode is local, then let the current media resource be the resource given by the absolute URL passed to this algorithm; otherwise, let the current media resource be the resource given by the media provider object. Either way, the current media resource is now the element's media resource.
> 
>     3. Remove all media-resource-specific text tracks from the media element's list of pending text tracks, if any.
> 
>     4. Run the appropriate steps from the following list:
> 
>     -> If mode is remote
> 
> See this -> 1. Optionally, run the following substeps. This is the expected behaviour if the user agent intends to not attempt to fetch the resource until the user requests it explicitly (e.g. as a way to implement the preload attribute's none keyword).
> 
>                 1. Set the networkState to NETWORK_IDLE.
> 
>                 2. Queue a task to fire a simple event named suspend at the element.
> 
>                 3. Queue a task to set the element's delaying-the-load-event flag to false. This stops delaying the load event.
> 
>                 4. Wait for the task to be run.
> 
>                 5. Wait for an implementation-defined event (e.g. the user requesting that the media element begin playback).
> 
>                 6. Set the element's delaying-the-load-event flag back to true (this delays the load event again, in case it hasn't been fired yet).
> 
>                 7. Set the networkState to NETWORK_LOADING.


And code at https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#883
>       if (mPreloadAction == HTMLMediaElement::PRELOAD_NONE &&
>           !IsMediaStreamURI(mLoadingSrc)) {
>         // preload:none media, suspend the load here before we make any
>         // network requests.
>         SuspendLoad();
>         return;
>       }

So I'm not convinced Firefox is doing the wrong thing here. roc, perhaps you have some light to shed (or know who does)?
Hm.. You're saying that if there's no "preload" attribute, the load() method should basically be a no-op? That sounds odd. I'd expect the "load" method to mean "figure out what source we can use and start loading it".

FWIW, both IE and Chrome choose to fire the event as a consequence of load(). Presto doesn't..
That's not what I'm saying. I'm saying that if preload is set to "none" it should behave as observed in the description. According to http://dev.w3.org/html5/spec-preview/media-elements.html#attr-media-preload, if it's not set it's interpreted as "auto", causing different behavior. The demo page you linked (very nice, btw) indicated preload "none". Perhaps the problem is that we are interpreting no preload as "none" instead? I didn't check that but we should definitely verify.
OK, let me double-check what the page does. I think it may have no preload attribute..
Flags: needinfo?(roc)
At least the demo page you linked to explicitly mentions preload "none". I also tested real quick to create a video element in the devtools console and it's ready state goes to HAVE_METADATA as expected when preload is unset (aka "").

Let's see what the page does.
I can also confirm that in Chrome 42, load() with preload "none" on the demo page progresses to HAVE_METADATA.

I'm not an authority when it comes to spec work by any means. I'll also note that it said "Optionally, run the following substeps" in the spec (per comment 3). I'll categorize that as 'interesting' ;-)
I'm pretty sure that our behavior is spec-compliant. load() basically just says "cancel playback of the current resource and run the selection algorithm again", there's nothing to force us to load data or progress to metadataloaded. However, I think we should be open to changing our behavior, and possibly tightening up the spec to match whatever's needed for Web compatibility.

It seems to me we have three options:
-- Keep our current behavior.
-- If the current resource load was triggered by load(), force the element to progress to
Flags: needinfo?(cpearce)
Flags: needinfo?(cajbir.bugzilla)
oops!

It seems to me we have three options:
1) Keep our current behavior.
2) If the current resource load was triggered by load(), treat PRELOAD_NONE as PRELOAD_METADATA.
3) Always treat PRELOAD_NONE as PRELOAD_METADATA.

I kinda don't like #3 since it's nice to support the case where a page has many media elements with preload="none" and we don't generate any traffic for them until the user (or a script) plays them. However, if IE and Chrome have behavior #3, maybe we just have to do it (and change the spec to eliminate the None preload value), since there's clearly an interop hazard here.
On platforms where the number of instances of hardware codec are limited, loading metadata means to allocate hardware codec and will result in failures of other media elements that fail to acquire hardware resources. Therefore, I would also oppose #3.
I'd  like to not do (3) since preload="none" plus a poster is good for pages with large numbers of videos. Option (2) seems reasonable.
Flags: needinfo?(cajbir.bugzilla)
Option #2 sounds reasonable to me as well.
Flags: needinfo?(cpearce)
The nicovideo.jp site does *not* use preload=none - turns out I was fooled by the preload attribute in the W3C demo into thinking that the same problem also exists on desktop (but as a side effect I found an interesting incompatibility issue we should also fix - I also agree with suggestion 2 above and will report a spin-off bug).

This test shows the nicovideo problem is a Firefox for Android-specific issue:
http://hallvord.com/temp/moz/bug/nicovideo.htm

In Firefox desktop and Chrome on Android, the test will show "PASSED" after a few seconds. In Firefox on Android it won't.
See Also: → 1167097
https://hg.mozilla.org/mozilla-central/file/8d8df22fe72d/mobile/android/app/mobile.js#l553
Default preload is 'none' on mobile platforms which means if you don't have the 'preload' attribute, 'none' will be assumed internally.
Aha.. then it *is* the same issue after all. OK, someone please implement Roc's suggestion so Nicovideo.jp starts working! :)
tracking-p11: --- → +
Attached patch WIP patch (obsolete) — — Splinter Review 
Chris, does this patch make any sense at all?

Specifically:

1) Is it bad to update mPreloadAction like this? Should we just, instead, detect the condition in SelectResource() and call ResumeLoad from there as necessary?

2) What tests to I need to run to check this change?

3) I didn't make the patch clear mIsDoingExplicitLoad in AbortExistingLoads() since that gets called at the start of DoLoad() and would clear the state after we just set it. Does that make sense?
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Flags: needinfo?(cpearce)
Attachment #8617124 - Flags: feedback?(cpearce)
Flags: needinfo?(cpearce)
FYI, seems we hit the same issue on BBC World: https://webcompat.com/issues/882
Chris, what do you think of this? (comment 18)
Flags: needinfo?(cpearce)
Comment on attachment 8617124 [details] [diff] [review]
WIP patch

Review of attachment 8617124 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brian Birtles (:birtles) from comment #18)
> Created attachment 8617124 [details] [diff] [review]
> 1) Is it bad to update mPreloadAction like this? Should we just, instead,
> detect the condition in SelectResource() and call ResumeLoad from there as
> necessary?

Updating mPreloadAction like this should be fine... But the mochitests will confirm that!

SelectResource() calls UpdatePreloadAction(), so I don't think it's a bug deal whether you detect in SelectResource() or in UpdatePreloadAction(), though I think it might be clearer doing it in UpdatePreloadAction().


> 2) What tests to I need to run to check this change?

m2 and m3 on treeherder.

`./mach mochitest dom/media/test/test*load*` will probably get you most of the way locally.


> 3) I didn't make the patch clear mIsDoingExplicitLoad in
> AbortExistingLoads() since that gets called at the start of DoLoad() and
> would clear the state after we just set it. Does that make sense?

Yes.
Attachment #8617124 - Flags: feedback?(cpearce) → feedback+
Thanks for taking this on Brian. Sorry for slow response.
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #21)
> > 2) What tests to I need to run to check this change?
> 
> m2 and m3 on treeherder.
> 
> `./mach mochitest dom/media/test/test*load*` will probably get you most of
> the way locally.

Great, thanks Chris! The tests seem to pass locally so I'll add another test and put this up for review.
Attachment #8617124 - Attachment is obsolete: true
I'll send a message to whatwg tomorrow about the suggested spec changes.
Comment on attachment 8622930 [details] [diff] [review]
Make calls to load() upgrade preload=none to preload=metadata

Review of attachment 8622930 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!
Attachment #8622930 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/34335f888c30
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: