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

RESOLVED FIXED in Firefox 38, Firefox OS v2.2

Status

()

P1
normal
Rank:
10
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cmills, Assigned: roc)

Tracking

({regression})

37 Branch
mozilla40
x86
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox37 wontfix, firefox38+ fixed, firefox39+ fixed, firefox40+ fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(URL)

Attachments

(3 attachments, 3 obsolete attachments)

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

4 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

4 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

4 years ago
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)

Updated

4 years ago
Duplicate of this bug: 1150785
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)

Comment 6

4 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?
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.
tracking-firefox39: ? → +
tracking-firefox40: ? → +

Comment 8

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

4 years ago
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.
Created attachment 8589107 [details] [diff] [review]
Part 1. Add mochitest

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.
Created 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
Attachment #8589463 - Flags: review?(pehrsons)
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

Comment 21

4 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.
(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.
status-firefox37: affected → wontfix
tracking-firefox38: ? → +

Comment 24

4 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?
(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

4 years ago
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

Comment 30

4 years ago
Hmmm, Randell, are you 100% sure about that? And that all browsers behave that way and that it's W3C conform?
https://hg.mozilla.org/mozilla-central/rev/8773737a0115
https://hg.mozilla.org/mozilla-central/rev/69a1557ce1f8
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: affected → fixed
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.

Comment 37

4 years ago
Ah, wonderful, thanks for the advice. Works fine now on FF! Check out my cool prototype at https://www.videomail.io ;)
Created attachment 8603538 [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
Created attachment 8603543 [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
Attachment #8603538 - Attachment is obsolete: true
Created 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
Attachment #8603543 - Attachment is obsolete: true

Updated

4 years ago
Blocks: 1162639

Updated

4 years ago
blocking-b2g: --- → 2.2?

Updated

4 years ago
Keywords: verifyme

Comment 41

4 years ago
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?

Updated

4 years ago
status-b2g-v2.2: --- → affected
status-b2g-master: --- → fixed

Updated

4 years ago
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.