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

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

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.

Comment 1

[Andreas Pehrson [:pehrsons]]

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.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

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?

Comment 3

[Maire Reavy [: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.

Comment 4

[Andreas Pehrson [:pehrsons]]

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

Comment 5

[Maire Reavy [:mreavy]]

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

Comment 6

[Sylvestre Ledru [:Sylvestre]]

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

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1240478 - Remove Mutex from MediaStreamSizeListener. r?jesup

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

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1240478 - Add test for video size on 'loadedmetadata'. r?jesup,jib

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

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1240478 - Assert that we know the video size on 'loadedmetadata'. r?jesup

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

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1240478 - Have MediaStreamSizeListener active at all times, not only when playing. r?jesup

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

Comment 11

[Andreas Pehrson [:pehrsons]]

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

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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 13

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

Comment on attachment 8710384 [details]
MozReview Request: Bug 1240478 - Remove Mutex from MediaStreamSizeListener. r?jesup

https://reviewboard.mozilla.org/r/31735/#review28609

Comment 14

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

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 15

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

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 16

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

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 17

[Andreas Pehrson [:pehrsons]]

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 18

[Andreas Pehrson [:pehrsons]]

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 19

[Andreas Pehrson [:pehrsons]]

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 20

[Andreas Pehrson [:pehrsons]]

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/

Comment 21

[Andreas Pehrson [:pehrsons]]

I rebased and fixed a detail in the last patch.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3bf507b23aa

Comment 22

[Pulsebot]

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

Comment 23

[Phil Ringnalda (:philor)]

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.

Comment 24

[Andreas Pehrson [:pehrsons]]

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

Comment 25

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

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

Comment 26

[Andreas Pehrson [:pehrsons]]

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.

Comment 27

[Andreas Pehrson [:pehrsons]]

Attachment: Have MediaStreamSizeListener active immediately, until the initial size is known

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 28

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

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+

Comment 29

[Pulsebot]

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

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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 31

[Andreas Pehrson [:pehrsons]]

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 32

[Andreas Pehrson [:pehrsons]]

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 33

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b88b0338f3b4

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/94425f94b457