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)
Tracking
()
People
(Reporter: cmills, Assigned: roc)
References
()
Details
(Keywords: regression)
Attachments
(3 files, 3 obsolete files)
11.04 KB,
patch
|
pehrsons
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
3.34 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
10.78 KB,
patch
|
jocheng
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
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);
};
Comment 1•10 years ago
|
||
[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
status-firefox37:
--- → affected
status-firefox38:
--- → affected
status-firefox39:
--- → affected
status-firefox40:
--- → affected
tracking-firefox38:
--- → ?
tracking-firefox39:
--- → ?
tracking-firefox40:
--- → ?
Flags: needinfo?(pehrsons)
Keywords: regression
OS: Mac OS X → All
Version: 39 Branch → 37 Branch
Updated•10 years ago
|
Summary: video.onloadedmetadata handler doesn't seem to work in Fx 38/39 → video.onloadedmetadata handler doesn't seem to work since Firefox37
Updated•10 years ago
|
Component: Video/Audio → WebRTC: Audio/Video
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
Is there a workaround for Firefox 37? I have lots of complaints from Firefox users unable to use www.videomail.io at the moment?
Comment 7•10 years ago
|
||
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.
Comment 8•10 years ago
|
||
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.
Comment 9•10 years ago
|
||
(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.
Updated•10 years ago
|
Rank: 10
Priority: -- → P1
Comment 10•10 years ago
|
||
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)
Updated•10 years ago
|
Assignee: nobody → pehrsons
Comment 11•10 years ago
|
||
(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.
Comment 12•10 years ago
|
||
Here's a mochitest that fails for me.
Attachment #8589107 -
Flags: review?(rjesup)
Comment 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
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?
Assignee | ||
Comment 16•10 years ago
|
||
(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.
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8589463 -
Flags: review?(pehrsons)
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
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 20•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8589107 -
Attachment is obsolete: true
Comment 21•10 years ago
|
||
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.
Comment 22•10 years ago
|
||
(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.
Comment 23•10 years ago
|
||
Recent regression, tracking.
Comment 24•10 years ago
|
||
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?
Assignee | ||
Comment 25•10 years ago
|
||
(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.
Comment 26•10 years ago
|
||
It matters not to call play() too early. Every browser behaves differently and W3C recommends so.
Assignee | ||
Comment 27•10 years ago
|
||
Assignee | ||
Comment 28•10 years ago
|
||
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?
Comment 29•10 years ago
|
||
(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
Comment 30•10 years ago
|
||
Hmmm, Randell, are you 100% sure about that? And that all browsers behave that way and that it's W3C conform?
Comment 31•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8773737a0115
https://hg.mozilla.org/mozilla-central/rev/69a1557ce1f8
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 32•10 years ago
|
||
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+
Comment 33•10 years ago
|
||
Comment 34•10 years ago
|
||
Comment 35•10 years ago
|
||
(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.)
Assignee | ||
Comment 36•10 years ago
|
||
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.
Comment 37•10 years ago
|
||
Ah, wonderful, thanks for the advice. Works fine now on FF! Check out my cool prototype at https://www.videomail.io ;)
Comment 38•10 years ago
|
||
Comment 39•10 years ago
|
||
Attachment #8603538 -
Attachment is obsolete: true
Comment 40•10 years ago
|
||
Attachment #8603543 -
Attachment is obsolete: true
Updated•10 years ago
|
blocking-b2g: --- → 2.2?
Comment 41•10 years ago
|
||
Triage: blocking.
Hi Robert, can you request approval for approval‑mozilla‑b2g37 ? Thanks
Assignee | ||
Comment 42•10 years ago
|
||
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?
Updated•10 years ago
|
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → fixed
Updated•10 years ago
|
Attachment #8603545 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Comment 43•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•