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

Status: RESOLVED FIXED

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

Description

[Chris Mills [:cmills]]

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

[Alice0775 White]

[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

Comment 2

[Andreas Pehrson [:pehrsons]]

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.

Comment 4

[Paul Adenot (:padenot)]

Noted.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 6

[michael.heuberger]

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

[Liz Henry (:lizzard) (relman/hg->git project)]

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

[michael.heuberger]

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

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 10

[Paul Adenot (:padenot)]

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.

Comment 11

[Maire Reavy [:mreavy]]

(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

[Andreas Pehrson [:pehrsons]]

Attachment: Part 1. Add mochitest

Here's a mochitest that fails for me.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

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)

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I have a WIP fix.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1. Add a listener directly to the unblocked input stream that reports the size of the first non-empty frame seen

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2. Add mochitest

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8589464 [details] [diff] [review]
Part 2. Add mochitest

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

carrying forward r+

Comment 20

[Andreas Pehrson [:pehrsons]]

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.

Comment 21

[michael.heuberger]

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

[Andreas Pehrson [:pehrsons]]

(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

[Sylvestre Ledru [:Sylvestre]]

Recent regression, tracking.

Comment 24

[michael.heuberger]

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?

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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

[michael.heuberger]

It matters not to call play() too early. Every browser behaves differently and W3C recommends so.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8773737a0115
https://hg.mozilla.org/integration/mozilla-inbound/rev/69a1557ce1f8

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 30

[michael.heuberger]

Hmmm, Randell, are you 100% sure about that? And that all browsers behave that way and that it's W3C conform?

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8773737a0115
https://hg.mozilla.org/mozilla-central/rev/69a1557ce1f8

Comment 32

[Sylvestre Ledru [:Sylvestre]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3aa52ed1f8d9
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ad26ed2c2e9

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d46cb3b3ebb3
https://hg.mozilla.org/releases/mozilla-beta/rev/c821f76bf302

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[michael.heuberger]

Ah, wonderful, thanks for the advice. Works fine now on FF! Check out my cool prototype at https://www.videomail.io ;)

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: 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

Comment 39

[Sotaro Ikeda [:sotaro]]

Attachment: 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

Comment 40

[Sotaro Ikeda [:sotaro]]

Attachment: 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

Comment 41

[howie [:howie]]

Triage: blocking.

Hi Robert, can you request approval for approval‑mozilla‑b2g37 ? Thanks

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f7d3d822c197
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5e36d2132f06