Closed Bug 1240478 Opened 4 years ago Closed 4 years ago

MediaStream in video element doesn't guarantee video size on "loadedmetadata"

Categories

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

43 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox43 --- wontfix
firefox44 --- wontfix
firefox45 + fixed
firefox46 --- fixed
firefox47 --- fixed
Blocking Flags:

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

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.
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → 43 Branch
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?
Flags: needinfo?(mreavy)
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.
Flags: needinfo?(roc)
Flags: needinfo?(pehrsons)
Flags: needinfo?(padenot)
Flags: needinfo?(mreavy)
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".
Flags: needinfo?(pehrsons)
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.)
backlog: --- → webrtc/webaudio+
Rank: 10
Flags: needinfo?(roc)
Flags: needinfo?(padenot)
Priority: -- → P1
OK, thanks. Looks like we want to track it then!
When I tested I just didn't get "loadedmetadata" without play()ing. It's bad, but not as bad as we feared.
Assignee: nobody → pehrsons
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
Attachment #8710385 - Flags: review?(jib) → review+
Attachment #8710384 - Flags: review?(roc) → review?(rjesup)
Attachment #8710385 - Flags: review?(roc) → review?(rjesup)
Attachment #8710386 - Flags: review?(roc) → review?(rjesup)
Attachment #8710387 - Flags: review?(roc) → review?(rjesup)
Attachment #8710384 - Flags: review?(rjesup) → review+
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
Attachment #8710385 - Flags: review?(rjesup) → review+
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
Attachment #8710386 - Flags: review?(rjesup) → review+
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
Attachment #8710387 - Flags: review?(rjesup) → review+
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/
Attachment #8710384 - Attachment description: MozReview Request: Bug 1240478 - Remove Mutex from MediaStreamSizeListener. r?roc → MozReview Request: Bug 1240478 - Remove Mutex from MediaStreamSizeListener. r?jesup
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/
Attachment #8710385 - Attachment description: MozReview Request: Bug 1240478 - Add test for video size on 'loadedmetadata'. r?roc,jib → MozReview Request: Bug 1240478 - Add test for video size on 'loadedmetadata'. r?jesup,jib
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/
Attachment #8710386 - Attachment description: MozReview Request: Bug 1240478 - Assert that we know the video size on 'loadedmetadata'. r?roc → MozReview Request: Bug 1240478 - Assert that we know the video size on 'loadedmetadata'. r?jesup
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/
Attachment #8710387 - Attachment description: MozReview Request: Bug 1240478 - Have MediaStreamSizeListener active at all times, not only when playing. r?roc → MozReview Request: Bug 1240478 - Have MediaStreamSizeListener active at all times, not only when playing. r?jesup
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.
Depends on: 1242085
roc, do you by any chance have a clue to how I could have caused these bizarre failures?
Flags: needinfo?(roc)
No idea. You could try bisecting the patches to see which one appears to cause the regression.
Flags: needinfo?(roc)
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
Attachment #8710387 - Attachment is obsolete: true
Attachment #8713495 - Flags: review?(rjesup)
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+
Attachment #8713495 - Flags: review?(rjesup) → review+
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.
Attachment #8713495 - Flags: approval-mozilla-beta?
Attachment #8713495 - Flags: approval-mozilla-aurora?
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.
Attachment #8713495 - Flags: approval-mozilla-beta?
Attachment #8713495 - Flags: approval-mozilla-beta+
Attachment #8713495 - Flags: approval-mozilla-aurora?
Attachment #8713495 - Flags: approval-mozilla-aurora+
No longer depends on: 1242085
You need to log in before you can comment on or make changes to this bug.