1123950 - Intermittent test_video_dimensions.html | gizmo.mp4 (Stream) video width should be set on loadedmetadata - got 0, expected 560 | video height should be set on loadedmetadata - got 0, expected 320

Status: RESOLVED FIXED

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

Description

[Ryan VanderMeulen [:RyanVM]]

10:00:18 INFO - 1515 INFO TEST-PASS | dom/media/test/test_video_dimensions.html | gizmo.mp4 should only fire loadedmetadata once
10:00:18 INFO - 1516 INFO TEST-PASS | dom/media/test/test_video_dimensions.html | gizmo.mp4 video width should be set on loadedmetadata
10:00:18 INFO - 1517 INFO TEST-PASS | dom/media/test/test_video_dimensions.html | gizmo.mp4 video height should be set on loadedmetadata
10:00:18 INFO - 1518 INFO TEST-PASS | dom/media/test/test_video_dimensions.html | gizmo.mp4 (Stream) should only fire loadedmetadata once
10:00:18 INFO - 1519 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_video_dimensions.html | gizmo.mp4 (Stream) video width should be set on loadedmetadata - got 0, expected 560
10:00:19 INFO - 1520 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_video_dimensions.html | gizmo.mp4 (Stream) video height should be set on loadedmetadata - got 0, expected 320
10:00:20 INFO - 1521 INFO TEST-PASS | dom/media/test/test_video_dimensions.html | gizmo.mp4 (Captured) should only fire loadedmetadata once
10:00:20 INFO - 1522 INFO TEST-PASS | dom/media/test/test_video_dimensions.html | gizmo.mp4 (Captured) video width should be set on loadedmetadata
10:00:20 INFO - 1523 INFO TEST-PASS | dom/media/test/test_video_dimensions.html | gizmo.mp4 (Captured) video height should be set on loadedmetadata

Comment 1

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=5660250
repository: mozilla-inbound
start_time: 2015-01-20T08:31:20
who: rvandermeulen[at]mozilla[dot]com
machine: tst-linux64-spot-284
buildname: b2g_emulator_vm mozilla-inbound debug test mochitest-debug-12
revision: 5c93233e6aeb

Can't download from http://tooltool.pvt.build.mozilla.org.proxxy1.srv.releng.use1.mozilla.com/build/sha512/263f4e8796c25543f64ba36e53d5c4ab8ed4d4e919226037ac0988761d34791b038ce96a8ae434f0153f9c2061204086decdbff18bdced42f3849156ae4dc9a4 to /builds/slave/test/build/263f4e8796c25543f64ba36e53d5c4ab8ed4d4e919226037ac0988761d34791b038ce96a8ae434f0153f9c2061204086decdbff18bdced42f3849156ae4dc9a4!
Caught exception: HTTP Error 404: Not Found
Caught exception: HTTP Error 404: Not Found
Caught exception: HTTP Error 404: Not Found
1519 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_video_dimensions.html | gizmo.mp4 (Stream) video width should be set on loadedmetadata - got 0, expected 560
1520 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_video_dimensions.html | gizmo.mp4 (Stream) video height should be set on loadedmetadata - got 0, expected 320
Return code: 1

Comment 2

[Andreas Pehrson [:pehrsons]]

I wonder if this could be from bug 1073406 and how the hints are passed from mSrcStream to mPlaybackStream. Or rather, how the hints are not passed..

Will take a closer look!

Comment 3

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #2)
> I wonder if this could be from bug 1073406 and how the hints are passed from
> mSrcStream to mPlaybackStream. Or rather, how the hints are not passed..

No, that can't be. We're already listening to tracks on mSrcStream so we must be in a state where mSrcStream has gotten an audio track but not a video track.

It must be that mSrcStream sees data on the audio track before the captured media element has SetHintContents() on its DecodedStream.

Comment 4

[Andreas Pehrson [:pehrsons]]

Ah, SetHintContents() (and in turn CreateDOMTrack()) doesn't set mTrackTypesAvailable any longer. So they won't be reported available to HTMLMediaElement until MediaStreamGraph has created them (and called BindDOMTrack()) - which it doesn't do until there is data available.

This hasn't worked for a while but used to. I guess I was working on this bug on a revision prior to that.

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Clean out traces of HTMLMediaElement::mSrcStream->GetStream()

Using this opportunity to clean up a bit after bug 1073406. Not sure if I missed this or if it was bitrot that came while I was fixing blockers.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Use hints to check for a DOMMediaStream's tracks

This checks for which tracks a DOMMediaStream will have by using its hints instead of the list of available tracks.

Hints are set by SetHintContents() and BindDOMTrack() while the track list is only modified by BindDOMTrack().

It should be safe to do this in the old OnTracksAvailableCallback since we at the point of the first track becoming available wouldn't have reached HAVE_METADATA anyway.

Comment 7

[Andreas Pehrson [:pehrsons]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=842cea0b687a

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Use hints to check for a DOMMediaStream's tracks

Should compile better this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dad9e907250

Comment 9

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8552304 [details] [diff] [review]
Clean out traces of HTMLMediaElement::mSrcStream->GetStream()

Andreas, just to double check with you. Does this patch solve or cause any issue wrt bug 1046578?

See bug 1073406 where the behavior changed. In short we now pipe mSrcStream to mPlaybackStream which holds the audio and video outputs. GetSrcMediaStream() returns mPlaybackStream->GetStream().

I tried reproducing bug 1046578 with loop but didn't notice a difference before and after this patch. I also noted that volume was very low, but I'm not sure if it's a regression or not.

Comment 10

[Andreas Pehrson [:pehrsons]]

To be honest I can't notice any difference with bug 1073406 reverted either.

Comment 11

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #9)
> Andreas, just to double check with you.

Sorry Andrea, it's just so deeply rooted in my fingers ;-)

Comment 12

[Andreas Pehrson [:pehrsons]]

Attachment: [b2g37] Use hints to check for a DOMMediaStream's tracks

Just needs a little rebase.

Comment 13

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8552304 [details] [diff] [review]
Clean out traces of HTMLMediaElement::mSrcStream->GetStream()

Please see comments 5 and 6 for patch descriptions.

Comment 14

[Andreas Pehrson [:pehrsons]]

Fresh try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dc6c81e10e4

Comment 15

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

Aren't we trying to remove the reliance on hints to know what tracks an element/stream has?  Or is it ok at this point now?  jib was deep in this part a bit ago to make WebAudio sources and mozCaptureStream sources for for peerconnections; NI him.

Comment 16

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

(In reply to Randell Jesup [:jesup] from comment #15)
> Aren't we trying to remove the reliance on hints to know what tracks an
> element/stream has?

Yes we're removing hints, to support multiple video and audio tracks (real soon), so this patch is going in the wrong direction (the before code looks right to me).

Bug 1070127 added placeholder tracks in DOMMediaStream to solve DOM visibility of tentative tracks, but didn't rip out hints at that time. It probably should have.

Comment 17

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

Comment on attachment 8556807 [details] [diff] [review]
[b2g37] Use hints to check for a DOMMediaStream's tracks

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

Per jib's comments, cancelling the review for now.

Comment 18

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #16)
> (In reply to Randell Jesup [:jesup] from comment #15)
> > Aren't we trying to remove the reliance on hints to know what tracks an
> > element/stream has?
> 
> Yes we're removing hints, to support multiple video and audio tracks (real
> soon), so this patch is going in the wrong direction (the before code looks
> right to me).
> 
> Bug 1070127 added placeholder tracks in DOMMediaStream to solve DOM
> visibility of tentative tracks, but didn't rip out hints at that time. It
> probably should have.

I see. Do we have a bug for the multiple track support?

Comment 19

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

Yes, I was referring to the work on PeerConnection, and Bug 1095218 which landed Wednesday seems to fix up MediaStreamTrack. I've been meaning to ask Byron what's needed but it looks like he took care of it (you could presumably have multiple tracks non-PeerConnection tracks before that as well according to Bug 1003695).

I don't think we have a bug on removing hints, though we probably should since they sort of assume one of a kind tracks, and we now have test_peerConnection_twoVideoStreams.html.

Comment 20

[Andreas Pehrson [:pehrsons]]

This should now be fixed by bug 1129263.

Comment 21

[Sotaro Ikeda [:sotaro]]

attachment 8556807 [details] [diff] [review] becomes necessary for Bug 1162639. But attachment 8556807 [details] [diff] [review] has a problem. Need to fix it.

Comment 22

[Sotaro Ikeda [:sotaro]]

StreamSizeListener is registered to mPlaybackStream instead of mSrcStream. StreamSizeListener needs to be registered to mSrcStream.

Comment 23

[Sotaro Ikeda [:sotaro]]

ReOpen bug for Bug 1162639.

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: patch for b2g v2.2 - Clean out traces of HTMLMediaElement::mSrcStream->GetStream()

Comment 25

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8605431 [details] [diff] [review]
patch for b2g v2.2 - Clean out traces of HTMLMediaElement::mSrcStream->GetStream()

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

I meant that you should take attachment 8556807 [details] [diff] [review] instead. Since v2.2 still have hints we can do it there. It was obsoleted for master because hints were being thrown out (landed in 38, so doesn't affect v2.2).

I'll see if I can rebase it on v2.2.

::: dom/html/HTMLMediaElement.cpp
@@ +3042,5 @@
>  
>    GetSrcMediaStream()->AddListener(mMediaStreamListener);
>    // Listen for an initial image size on mSrcStream so we can get results even
>    // if we block the mPlaybackStream.
> +  mSrcStream->GetStream()->AddListener(mMediaStreamSizeListener);

Does this go cleanly on v2.2?

Comment 26

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8556807 [details] [diff] [review]
[b2g37] Use hints to check for a DOMMediaStream's tracks

It goes cleanly, even.

roc, this is for an uplift to b2g37 where hints are still around.

Sotaro, fyi.

Comment 27

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8605431 [details] [diff] [review]
patch for b2g v2.2 - Clean out traces of HTMLMediaElement::mSrcStream->GetStream()

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

Ah, I see what's going on here now.

But since this never landed on master, we should fork it off to another bug and land it there first, if you need the changes. I'll obsolete it here for now.

::: dom/html/HTMLMediaElement.cpp
@@ +3042,5 @@
>  
>    GetSrcMediaStream()->AddListener(mMediaStreamListener);
>    // Listen for an initial image size on mSrcStream so we can get results even
>    // if we block the mPlaybackStream.
> +  mSrcStream->GetStream()->AddListener(mMediaStreamSizeListener);

Yes, it goes cleanly if it's on top of a patch you're uplifting :-)

Comment 28

[howie [:howie]]

Triage: blocking, as this blocks a blocker.

Comment 29

[Sotaro Ikeda [:sotaro]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #27)
> Comment on attachment 8605431 [details] [diff] [review]
> patch for b2g v2.2 - Clean out traces of
> HTMLMediaElement::mSrcStream->GetStream()
> 
> Review of attachment 8605431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ah, I see what's going on here now.
> 
> But since this never landed on master, we should fork it off to another bug
> and land it there first, if you need the changes. I'll obsolete it here for

Thanks, if your patch attachment 8556807 [details] [diff] [review] is applied to b2g b2.2, attachment 8605431 [details] [diff] [review] is not necessary.

Comment 30

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecb25369c006

Comment 31

[Sotaro Ikeda [:sotaro]]

pehrsons, in Bug 1162639 Comment 50, :josh prefers to gather all patches to uplift to b2g v2.2. What is your preference?

Comment 32

[Andreas Pehrson [:pehrsons]]

I don't really have a preference. Do what works best for you.

Comment 33

[Sotaro Ikeda [:sotaro]]

Ok, thanks!

Comment 34

[Josh Cheng [:josh]]

Hi Andreas,
Per Bug 1162639 Comment 82, since some of the bugs are already uplifted. 
Please also raise 2.2 approval for this patch. Thanks.

Comment 35

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8556807 [details] [diff] [review]
[b2g37] Use hints to check for a DOMMediaStream's tracks

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 #): Bug 1073406, I believe
User impact if declined: Media element events might occur out of order, causing errors in user code.
Testing completed: Manually tested (bug 1162639 comment 76)
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/35cae1a87e70