Closed Bug 1149494 Opened 10 years ago Closed 10 years ago

video.onloadedmetadata handler doesn't seem to work with MediaStream input since Firefox37

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

37 Branch
x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla40
blocking-b2g 2.2+
Tracking Status
firefox37 --- wontfix
firefox38 + fixed
firefox39 + fixed
firefox40 + fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: cmills, Assigned: roc)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

The following simple demo uses gUM to grab a video steam and three.js to use it as a texture on the six faces of a cube: http://chrisdavidmills.github.io/threejs-video-cube/ It works fine in Chrome, Opera and Firefox 36. However, on newer versions of Firefox it doesn't work. The part of the code that doesn't seem to work is this: video.onloadedmetadata = function() { video.play(); threeRender(video); };
[Tracking Requested - why for this release]: Regression since Firefox37, but it is worth to fin for next esr38. Regression pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e329c7fe4c4e&tochange=654e15b4dc9b Regressed by: Bug 879717
Blocks: 879717
Flags: needinfo?(pehrsons)
Keywords: regression
OS: Mac OS X → All
Version: 39 Branch → 37 Branch
Summary: video.onloadedmetadata handler doesn't seem to work in Fx 38/39 → video.onloadedmetadata handler doesn't seem to work since Firefox37
Component: Video/Audio → WebRTC: Audio/Video
This is because of how streams work. From bug 879717 we need to have an actual video frame from the stream to get "loadedmetadata" (video dimensions must be known), though the stream must be played to receive video frames. If it's not playing it's blocking internally and that doesn't let anything through (video frames, audio, etc.). Iirc this is not in violation of the spec because MediaStream behavior is not regulated specifically for loadedmetadata. It's different from other video resources in a number of ways (being only realtime, video dimensions can always change, etc.). Roc, do you know more on this? Paul, when you're throwing out the blocking logic from MSG, this is something to consider fixing.
Flags: needinfo?(roc)
Flags: needinfo?(pehrsons)
Flags: needinfo?(padenot)
Noted.
Flags: needinfo?(padenot)
Video resource dimensions can always change even for non-realtime videos, but in practice they don't. This seems like a nasty bug that we'd better fix. I guess removing blocking makes this considerably easier. Paul, are you actively working on that?
Flags: needinfo?(roc) → needinfo?(padenot)
Is there a workaround for Firefox 37? I have lots of complaints from Firefox users unable to use www.videomail.io at the moment?
Tracking for 39+. How widespread do you think this issue is? Is it likely to affect many users? Michael, I'm not sure if the issue you report is the same as the one in this bug. If it is, more info should appear in this bug soon. If it isn't, it will be good to report your issue in a new bug. Either way, it's useful to have your report, and maybe it will help in getting to a fix or in testing.
It is the same bug. Google for "getUserMedia demo" and all of these do fail. Does not matter which site. Really, this is huge and pretty obvious, so no detailed report is needed. I already investigated and found out that nowhere the loadedmetadata event is fired anymore. That's the bug.
(In reply to Liz Henry (:lizzard) from comment #7) > Tracking for 39+. How widespread do you think this issue is? Is it likely to > affect many users? This is a major bug; it all depends on how people coded up the "start playing" part - did they wait on onloadedmetadata, or did they just use autoplay or call .play() preemptively. We *really* need to fix this for 38, even if it's only a wallpaper patch.
Rank: 10
Priority: -- → P1
I'm not working on removing blocking at the moment, but it's like I'll work on it sometime this quarter. I think it's worth it to hacking something quick for now.
Flags: needinfo?(padenot)
Assignee: nobody → pehrsons
(In reply to Paul Adenot (:padenot) from comment #10) > I'm not working on removing blocking at the moment, but it's like I'll work > on it sometime this quarter. I think it's worth it to hacking something > quick for now. I agree with Paul. I said this in irc, but I think it's worth capturing here. We need something we can uplift -- ideally to Fx38 -- and I don't want to rush the implementation for removing blocking and then try to uplift that along with the fix for this bug. I'd like a safe fix (even if it's a hack and/or wallpaper patch) that we can uplift and then consider whether it's worth doing a better/less hacky fix once we're ready to land removing blocking.
Attached patch Part 1. Add mochitest (obsolete) — Splinter Review
Here's a mochitest that fails for me.
Attachment #8589107 - Flags: review?(rjesup)
Comment on attachment 8589107 [details] [diff] [review] Part 1. Add mochitest Review of attachment 8589107 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/tests/mochitest/test_getUserMedia_basicVideo_playAfterLoadedmetadata.html @@ +11,5 @@ > + bug: "1149494" > + }); > + /** > + * Run a test to verify that we will always get 'loadedmetadata' from a video > + * HTMLMediaElement playing a gUM MediaStream. Do we check if onloadedmetadata fires for non-gUM media elements? If not, a test for that should be added to one of the streaming-video tests. @@ +28,5 @@ > + return new Promise(resolve => { > + ok(playback.mediaElement.paused, > + "Media element should be paused before play()ing"); > + video.addEventListener('loadedmetadata', > + () => playback.playMedia(false).then(resolve)); Should we check that the video size is now available? (Another long-standing issue we had before the 8xxxxx bug landed)
Attachment #8589107 - Flags: review?(rjesup) → review+
Comment on attachment 8589107 [details] [diff] [review] Part 1. Add mochitest Review of attachment 8589107 [details] [diff] [review]: ----------------------------------------------------------------- Isn't sonmething supposed to call startMedia here? More generally, it seems like something needs to set mozSrcObject, but nothing does?
I have a WIP fix.
Assignee: pehrsons → roc
(In reply to Randell Jesup [:jesup] from comment #13) > Do we check if onloadedmetadata fires for non-gUM media elements? If not, a > test for that should be added to one of the streaming-video tests. I'm pretty sure we do. > Should we check that the video size is now available? (Another > long-standing issue we had before the 8xxxxx bug landed) I'll add that.
Comment on attachment 8589464 [details] [diff] [review] Part 2. Add mochitest Review of attachment 8589464 [details] [diff] [review]: ----------------------------------------------------------------- carrying forward r+
Attachment #8589464 - Flags: review+
Comment on attachment 8589463 [details] [diff] [review] Part 1. Add a listener directly to the unblocked input stream that reports the size of the first non-empty frame seen Review of attachment 8589463 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. IMO remove the MediaStreamSizeListener once it's figured the initial size out. When we reach HAVE_METADATA maybe? ::: dom/html/HTMLMediaElement.cpp @@ +3096,5 @@ > MediaStream* stream = GetSrcMediaStream(); > if (stream) { > + stream->RemoveListener(mMediaStreamListener); > + } > + if (mSrcStream->GetStream()) { Could mSrcStream be null here? Ditto mMediaStreamSizeListener.
Attachment #8589463 - Flags: review?(pehrsons) → review+
Attachment #8589107 - Attachment is obsolete: true
Great, thanks for quick actions. Wondering if the API has changed? Will this affect the videomail-client code? At https://github.com/binarykitchen/videomail-client/blob/master/src/util/userMedia.js#L113 Feedback welcome. That code doesn't currently work on FF.
(In reply to michael.heuberger from comment #21) > Great, thanks for quick actions. > > Wondering if the API has changed? Will this affect the videomail-client > code? At > https://github.com/binarykitchen/videomail-client/blob/master/src/util/ > userMedia.js#L113 > > Feedback welcome. That code doesn't currently work on FF. The API hasn't changed. This was a bug that went undetected through Aurora and Beta after we fixed another bug. The simplest workaround would be to play() right away rather than wait for 'loadedmetadata' first.
Recent regression, tracking.
I see, thanks for explaining. But do not want to call play() asap without ensuring the user media is fully loaded. Did you also add a new test case with the bugfix together to ensure this won't happen again?
(In reply to michael.heuberger from comment #24) > I see, thanks for explaining. But do not want to call play() asap without > ensuring the user media is fully loaded. I don't think that really matters. > Did you also add a new test case with the bugfix together to ensure this > won't happen again? Yes, there's a test here. (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #20) > Looks good. IMO remove the MediaStreamSizeListener once it's figured the > initial size out. When we reach HAVE_METADATA maybe? I don't think it's a problem to leave it in, and it's simpler to do so. > ::: dom/html/HTMLMediaElement.cpp > @@ +3096,5 @@ > > MediaStream* stream = GetSrcMediaStream(); > > if (stream) { > > + stream->RemoveListener(mMediaStreamListener); > > + } > > + if (mSrcStream->GetStream()) { > > Could mSrcStream be null here? Ditto mMediaStreamSizeListener. mSrcStream can't be null; we unconditionally dereference it just below. Likewise mMediaStreamSizeListener can't be null since it's always non-null if mMediaStreamListener is.
It matters not to call play() too early. Every browser behaves differently and W3C recommends so.
Comment on attachment 8589463 [details] [diff] [review] Part 1. Add a listener directly to the unblocked input stream that reports the size of the first non-empty frame seen Approval Request Comment [Feature/regressing bug #]: 879717 [User impact if declined]: Various sites using getUserMedia are broken [Describe test coverage new/current, TreeHerder]: A number of automated tests exist for these features. We've added a new one for the regression. [Risks and why]: This patch is designed to minimize risk, and seems low risk. [String/UUID change made/needed]: none.
Attachment #8589463 - Flags: approval-mozilla-beta?
Attachment #8589463 - Flags: approval-mozilla-aurora?
(In reply to michael.heuberger from comment #26) > It matters not to call play() too early. Every browser behaves differently > and W3C recommends so. I think that only applies to streaming video, not realtime (which is what got broken here): "However, if the play method has been called explicitly, the playback will start as soon as the HAVE_FUTURE_DATA state is reached." HAVE_FUTURE_DATA/HAVE_ENOUGH_DATA are basically irrelevant to realtime sources. That's to manage buffering/playback behavior for streaming video.
Summary: video.onloadedmetadata handler doesn't seem to work since Firefox37 → video.onloadedmetadata handler doesn't seem to work with MediaStream input since Firefox37
Hmmm, Randell, are you 100% sure about that? And that all browsers behave that way and that it's W3C conform?
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8589463 [details] [diff] [review] Part 1. Add a listener directly to the unblocked input stream that reports the size of the first non-empty frame seen Should be in 38 beta 3
Attachment #8589463 - Flags: approval-mozilla-beta?
Attachment #8589463 - Flags: approval-mozilla-beta+
Attachment #8589463 - Flags: approval-mozilla-aurora?
Attachment #8589463 - Flags: approval-mozilla-aurora+
(In reply to michael.heuberger from comment #30) > Hmmm, Randell, are you 100% sure about that? And that all browsers behave > that way and that it's W3C conform? I'll defer to roc here, as he has much more experience with streaming media (and other than the above issue with playthrough, which if you need to wait for you should wait for that instead of onloadedmetadata). The only issue is that you don't know when the first frame will come, or the size when you say play(). (Until the bug that caused this landed, when onloadedmetadata fired for MediaStreams we didn't always/usually have the size available since we usually didn't have a frame.)
Yeah, calling play() after setting src/srcObject but before any load-related events have fired should work in all browsers per spec. For realtime media, you've really got nothing to lose by doing so.
Ah, wonderful, thanks for the advice. Works fine now on FF! Check out my cool prototype at https://www.videomail.io ;)
Blocks: 1162639
blocking-b2g: --- → 2.2?
Keywords: verifyme
Triage: blocking. Hi Robert, can you request approval for approval‑mozilla‑b2g37 ? Thanks
blocking-b2g: 2.2? → 2.2+
Flags: needinfo?(roc)
Keywords: verifyme
Comment on attachment 8603545 [details] [diff] [review] patch for b2g v2.2 - Part 1. Add a listener directly to the unblocked input stream that reports the size of the first non-empty frame seen NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): 879717 User impact if declined: some getUserMedia usage won't work Testing completed: been on trunk for a long time Risk to taking this patch (and alternatives if risky): low risk String or UUID changes made by this patch: none
Flags: needinfo?(roc)
Attachment #8603545 - Flags: approval-mozilla-b2g37?
Attachment #8603545 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: