Closed Bug 1123950 Opened 5 years ago Closed 4 years ago

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

Categories

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

37 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
blocking-b2g 2.2+
Tracking Status
firefox37 --- unaffected
firefox38 --- fixed
firefox39 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: RyanVM, Assigned: pehrsons)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 4 obsolete files)

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
Flags: needinfo?(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!
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
Flags: needinfo?(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.
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.
See Also: → 1090293
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.
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 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.
Attachment #8552304 - Flags: feedback?(amarchesini)
To be honest I can't notice any difference with bug 1073406 reverted either.
Attachment #8552304 - Flags: feedback?(amarchesini) → feedback+
(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 ;-)
Just needs a little rebase.
Attachment #8552320 - Attachment is obsolete: true
Attachment #8556807 - Flags: review?(roc)
Comment on attachment 8552304 [details] [diff] [review]
Clean out traces of HTMLMediaElement::mSrcStream->GetStream()

Please see comments 5 and 6 for patch descriptions.
Attachment #8552304 - Flags: review?(roc)
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.
Flags: needinfo?(jib)
(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.
Flags: needinfo?(jib)
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.
Attachment #8556807 - Flags: review?(roc)
(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?
Flags: needinfo?(jib)
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.
Flags: needinfo?(jib)
Depends on: 1129263
See Also: → 1130299
See Also: 1130299
Attachment #8556807 - Attachment is obsolete: true
This should now be fixed by bug 1129263.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8552304 - Flags: review?(roc)
Target Milestone: --- → mozilla39
attachment 8556807 [details] [diff] [review] becomes necessary for Bug 1162639. But attachment 8556807 [details] [diff] [review] has a problem. Need to fix it.
StreamSizeListener is registered to mPlaybackStream instead of mSrcStream. StreamSizeListener needs to be registered to mSrcStream.
ReOpen bug for Bug 1162639.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 1162639
Attachment #8605431 - Flags: review?(pehrsons)
blocking-b2g: --- → 2.2?
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?
Attachment #8605431 - Flags: review?(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.
Attachment #8556807 - Attachment description: Use hints to check for a DOMMediaStream's tracks → [b2g37] Use hints to check for a DOMMediaStream's tracks
Attachment #8556807 - Attachment is obsolete: false
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8556807 - Flags: review?(roc)
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 :-)
Attachment #8605431 - Attachment is obsolete: true
Attachment #8552304 - Attachment is obsolete: true
Triage: blocking, as this blocks a blocker.
blocking-b2g: 2.2? → 2.2+
(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.
Flags: needinfo?(sotaro.ikeda.g)
pehrsons, in Bug 1162639 Comment 50, :josh prefers to gather all patches to uplift to b2g v2.2. What is your preference?
Flags: needinfo?(pehrsons)
I don't really have a preference. Do what works best for you.
Flags: needinfo?(pehrsons)
Ok, thanks!
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.
Flags: needinfo?(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
Flags: needinfo?(pehrsons)
Attachment #8556807 - Flags: approval-mozilla-b2g37?
Attachment #8556807 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.