See bug 1097260 comment 12, > Not sure this is properly fixed for WebRTC at least. > > When attaching a mediaStream to video.srcObject the loadedmetadata event is fired however the video.videoWidth and video.videoHeight are 0 until the first frame is received which happens on play. > > According to the spec[1] the loadedmetadata event should be fired when: > The user agent has just determined the duration and dimensions of the media resource and the text tracks are ready. > > http://dev.w3.org/html5/spec-preview/media-elements.html#event-media-loadedmetadata We have a special MediaStreamSizeListener in HTMLMediaElement that should cover exactly this case, but I think it broke when roc removed blocking because that listener is now basically added to the playback stream on play(). So, in addition to fixing this, we desperately need basic tests that cover this case.

If my analysis is correct, this is broken in 43 and up. [Tracking Requested - why for this release]: That a video element's videoWidth and videoHeight are reliably working when playing streams after its "loadedmetadata" event is a basic feature that we have had to fix before (we got multiple comments requesting it fixed before that). Fixing and uplifting this to Firefox 44 seems too tight, but I think 45 is reasonable.

Maire, do you know who could help with this? I am not capable to evaluate the impact on the release. Andreas, could you bit a more explicit for someone who doesn't know WebRTC?

This is not a major regression, but it is a noticeable, unfortunate annoyance. I agree with Andreas that this is something we want to fix and uplift to Fx45, but not to Fx44. Sylvestre: The higher level summary is that there is a mechanism that allows a web dev to know what the resolution of a video is before receiving the first frame. This broke when Bug 1189506 landed. This regression is not critical (e.g. no one has pinged me or anyone on the team about it until now even though Fx43 is about to be replaced by Fx44 as our release) -- a developer can find out this information later in the sequence when the first frame is received -- but this regression definitely violates the spec (would be annoying to a web dev, especially since it was working previously), and we want to fix it. Pehrsons, Roc, Padenot -- This is not a firedrill, but I'd like to get this fixed sometime in the next 2 weeks and then uplift it early in Fx45. Would one of you like to take this, or shall I find someone else? If you don't want to or can't take this, can you comment on the best way to fix this (if you have opinions/suggestions), especially given we'll want to uplift the fix? Thanks.

I can take it as soon as I free up some brain cells from cloning. I think it's as easy as moving the frame size listener from UpdateSrcMediaStreamPlaying() to Setup/EndSrcMediaStreamPlaying(). I also want to add an NS_ASSERTION to verify that videoWidth and Videoheight have been set as we issue "loadedmetadata".

Thanks, Andreas. I'll plan on your taking it this and getting a fix into Fx45 in early Beta or earlier. (FYI: I think of "early Beta" as the first 2 weeks of Beta -- which for Fx45 would mean on or before Feb 5th.)

OK, thanks. Looks like we want to track it then!

Review commit: https://reviewboard.mozilla.org/r/31735/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31735/

Review commit: https://reviewboard.mozilla.org/r/31737/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31737/

Review commit: https://reviewboard.mozilla.org/r/31739/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31739/

Review commit: https://reviewboard.mozilla.org/r/31741/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31741/

When I tested I just didn't get "loadedmetadata" without play()ing. It's bad, but not as bad as we feared.

Comment on attachment 8710385 [details] MozReview Request: Bug 1240478 - Add test for video size on 'loadedmetadata'. r?jesup,jib https://reviewboard.mozilla.org/r/31737/#review28517

Comment on attachment 8710384 [details] MozReview Request: Bug 1240478 - Remove Mutex from MediaStreamSizeListener. r?jesup https://reviewboard.mozilla.org/r/31735/#review28609

Comment on attachment 8710385 [details] MozReview Request: Bug 1240478 - Add test for video size on 'loadedmetadata'. r?jesup,jib https://reviewboard.mozilla.org/r/31737/#review28611

Comment on attachment 8710386 [details] MozReview Request: Bug 1240478 - Assert that we know the video size on 'loadedmetadata'. r?jesup https://reviewboard.mozilla.org/r/31739/#review28613

Comment on attachment 8710387 [details] MozReview Request: Bug 1240478 - Have MediaStreamSizeListener active at all times, not only when playing. r?jesup https://reviewboard.mozilla.org/r/31741/#review28615

Comment on attachment 8710384 [details] MozReview Request: Bug 1240478 - Remove Mutex from MediaStreamSizeListener. r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31735/diff/1-2/

Comment on attachment 8710385 [details] MozReview Request: Bug 1240478 - Add test for video size on 'loadedmetadata'. r?jesup,jib Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31737/diff/1-2/

Comment on attachment 8710386 [details] MozReview Request: Bug 1240478 - Assert that we know the video size on 'loadedmetadata'. r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31739/diff/1-2/

Comment on attachment 8710387 [details] MozReview Request: Bug 1240478 - Have MediaStreamSizeListener active at all times, not only when playing. r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31741/diff/1-2/

I rebased and fixed a detail in the last patch. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3bf507b23aa

https://hg.mozilla.org/integration/mozilla-inbound/rev/38ef99bddd74 https://hg.mozilla.org/integration/mozilla-inbound/rev/485bd08acb96 https://hg.mozilla.org/integration/mozilla-inbound/rev/bc9a56a067b5 https://hg.mozilla.org/integration/mozilla-inbound/rev/31629671c71c

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/601943e5bee5 https://treeherder.mozilla.org/#/jobs?repo=try&revision=639fc43f725e,ead4a726df58,83425612b633 is, from the bottom up, your parent rev, you, and you backed out. Somehow this media patch causes a 50% failure in a font-matching reftest. I'm trying to imagine a way that that would be a good and reasonable thing, and coming up empty.

roc, do you by any chance have a clue to how I could have caused these bizarre failures?

No idea. You could try bisecting the patches to see which one appears to cause the regression.

It must be the last one, because this is the video size assert and it passes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa1882aaedef And the last one is the actual fix.

This is a workaround for the font reftest issue. It basically makes sure that the stream size listener will have been removed by the time those font tests run (not sure if it was still applied without the workaround, but the original patch definitely changed the timing of when it was removed, while this restores it somewhat). Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=961add745471&selectedJob=16076378

Comment on attachment 8713495 [details] [diff] [review] Have MediaStreamSizeListener active immediately, until the initial size is known Review of attachment 8713495 [details] [diff] [review]: ----------------------------------------------------------------- Still makes absolutely no sense, but r+

https://hg.mozilla.org/integration/mozilla-inbound/rev/16928e1771ca https://hg.mozilla.org/integration/mozilla-inbound/rev/ca4fa9403dc6 https://hg.mozilla.org/integration/mozilla-inbound/rev/b35aa9ae63e8 https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5c4de4b923

https://hg.mozilla.org/mozilla-central/rev/16928e1771ca https://hg.mozilla.org/mozilla-central/rev/ca4fa9403dc6 https://hg.mozilla.org/mozilla-central/rev/b35aa9ae63e8 https://hg.mozilla.org/mozilla-central/rev/ad5c4de4b923

Comment on attachment 8713495 [details] [diff] [review] Have MediaStreamSizeListener active immediately, until the initial size is known Approval Request Comment [Feature/regressing bug #]: 1189506 [User impact if declined]: See comment 3. [Describe test coverage new/current, TreeHerder]: Covered by mochitest in m-c. [Risks and why]: Low risk; small change, has mochitest on m-c. [String/UUID change made/needed]: None.

Comment on attachment 8713495 [details] [diff] [review] Have MediaStreamSizeListener active immediately, until the initial size is known Oh, and only this patch is requested to be uplifted.

Comment on attachment 8713495 [details] [diff] [review] Have MediaStreamSizeListener active immediately, until the initial size is known Fix a regression, taking it. Should be in 45 beta 2.