1274221 - HTMLMediaElement::mVideoTrackList is not synced with HTMLMediaElement::mSrcStream::mTracks.

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

Description

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

The HTMLMediaElement::mVideoTrackList is not synced with HTMLMediaElement::mSrcStream::mTracks. You can easily reproduce it by running dom/media/tests/mochitest/test_getUserMedia_mediaStreamClone.html

Comment 1

[Andreas Pehrson [:pehrsons]]

I understand this as a dynamic removal (a MediaStreamTrack ending at the source) causing removal of the corresponding MediaTrack when a HTMLMediaElement is playing a MediaStream; but in the MediaStream they're staying, though in an ended state.

There's a "removetrack" event for MediaStream that we don't yet implement, but when that will fire is not entirely clear.

Also note that, when MediaStreamTrack.stop() is called, the track should end but stay in any MediaStreams where it exists. The corresponding MediaTrack should to my understanding then be removed, since it doesn't have an ended state and will never again play any data.

So to conclude that this is an actual bug I'd like some more data on what's wrong.

Comment 2

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

Chia-hung, can you describe a JS observable symptom, and steps to reproduce? Just so I know how to prioritize this.

Comment 3

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

This will affect HTMLMediaElement.MediaTracks which behind the preference mediatracks. But this might also be something wrong in how we dealing with TRACK_EVENT_ENDED. I will figure it out and keep update.  


(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> Chia-hung, can you describe a JS observable symptom, and steps to reproduce?
> Just so I know how to prioritize this.

Comment 4

[Michael Froman [:mjf]]

Making P2/29 until we have better info.

Comment 5

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Some updates...
In test_getUserMedia_mediaStreamClone.html, it will clone itself from 2 MediaStreamTracks(1 video, 1 audio) to totally 16 tracks(2 => 4(1st clone) => 8(2nd clone) => 16 (3rd clone)). SO there is 8 VideoStreamTracks and it will create 8 VideoTracks in HTMLMediaElements. But only 4 tracks is set to ended after each track.stop() called and this mediastream called stop().

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

|FindOwnedDOMTrack| take (MediaStream* aInputStream, TrackID aInputTrackID) to find the DOMMediaStreamTrack. But those two arguments are the same when MediaStream clone itself and add into itself. Per-talk with Andreas on IRC, we might change the return type of |FindOwnedDOMTrack| to an array.

Comment 7

[Andreas Pehrson [:pehrsons]]

I understand what's happening now, and wow, nice find!

To use database terms, we assume keys for owned tracks are unique while in fact, they're not. So in some (pretty small corner of a corner case but still) cases we look up the track to end, and instead of ending the right one, we end an already ended track again, and leave the intended one untouched.

It's caused by bug 1208371, this patch: https://hg.mozilla.org/mozilla-central/rev/f738214b89b3, where we switch from ownedStream+ownedTrackID to inputStream+inputTrackID as key for owned DOM tracks.

The rationale there is
> This let's us use FindOwnedDOMTrack before the TrackID in mOwnedStream is known.
> 
> This is necessary for a stream clone with multiple tracks whose original TrackID is the same.

Since we're calling `FindOwnedDOMTrack` before the TrackID in mOwnedStream is known, we can't make the key unique (the TrackID in mOwnedStream would be required for that). On the other hand that statement could be false, a DOM Track's TrackID in the underlying stream owning it should always be known. The patch above might be from early on in bug 1208371's development and has hopefully become invalid through bug 1208371's iterations. I'll investigate.

Comment 8

[Andreas Pehrson [:pehrsons]]

I've written a fix and a test case based on bug 1208373 ("ended" event). Patches coming in a bit.

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1274221 - Test that multiple tracks of the same source all end when in the same stream. r?jib

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

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1274221 - Make FindOwnedDOMTrack look for (optionally) unique tracks. r?ctai

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

Comment 11

[Andreas Pehrson [:pehrsons]]

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

Comment 12

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

Comment on attachment 8755449 [details]
MozReview Request: Bug 1274221 - Test that multiple tracks of the same source all end when in the same stream. r?jib

https://reviewboard.mozilla.org/r/54620/#review51268

::: dom/media/tests/mochitest/test_getUserMedia_trackEnded.html:35
(Diff revision 1)
> +        stream = new MediaStream([].concat(...s.getTracks())
> +                                   .concat(...s.getTracks().map(t => t.clone()))).clone();

Nit: just arrays here, so spread operators are unnecessary.

Maybe you meant:

    new MediaStream([...s.getTracks(), ...s.getTracks().map(t => t.clone())]).clone();

though concat() is faster apparently.

Comment 13

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

https://reviewboard.mozilla.org/r/54620/#review51268

> Nit: just arrays here, so spread operators are unnecessary.
> 
> Maybe you meant:
> 
>     new MediaStream([...s.getTracks(), ...s.getTracks().map(t => t.clone())]).clone();
> 
> though concat() is faster apparently.

I meant concat() on array type values is faster.

Comment 14

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8755450 [details]
MozReview Request: Bug 1274221 - Make FindOwnedDOMTrack look for (optionally) unique tracks. r?ctai

https://reviewboard.mozilla.org/r/54622/#review51388

Looks good to me.

::: dom/media/DOMMediaStream.h:403
(Diff revision 1)
> +   * be multiple MediaStreamTracks matching the same input track, but that they
> +   * in that case all share the same MediaStreamTrackSource.
>     */
>    MediaStreamTrack* FindOwnedDOMTrack(MediaStream* aInputStream,
> -                                      TrackID aInputTrackID) const;
> +                                      TrackID aInputTrackID,
> +                                      TrackID aTrackID = TRACK_NONE) const;

Is the usage of TRACK_NONE consitency with other place?
Maybe we should define well in https://dxr.mozilla.org/mozilla-central/source/dom/media/StreamTracks.h#20
:)

::: dom/media/DOMMediaStream.cpp:627
(Diff revision 1)
>  
>    already_AddRefed<MediaStreamTrackSource>
>    GetMediaStreamTrackSource(TrackID aInputTrackID) override
>    {
>      MediaStreamTrack* sourceTrack =
>        mStream->FindOwnedDOMTrack(mStream->GetOwnedStream(), aInputTrackID);

The reason we use TRACK_NONE here because of we don't care which MST. We only care about the MSTrackSource?

::: dom/media/DOMMediaStream.cpp:982
(Diff revision 1)
>                                 MediaStreamTrackSource* aSource)
>  {
>    MOZ_RELEASE_ASSERT(mInputStream);
>    MOZ_RELEASE_ASSERT(mOwnedStream);
>  
>    MOZ_ASSERT(FindOwnedDOMTrack(GetInputStream(), aTrackID) == nullptr);

Not sure why we won't bump in this assert....

Comment 15

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/54620/#review51268

> I meant concat() on array type values is faster.

Thanks. I went a bit too fast through the concat() docs.

Comment 16

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/54622/#review51388

> Is the usage of TRACK_NONE consitency with other place?
> Maybe we should define well in https://dxr.mozilla.org/mozilla-central/source/dom/media/StreamTracks.h#20
> :)

I think "none" as in "no particular TrackID specified" is reasonable.

> The reason we use TRACK_NONE here because of we don't care which MST. We only care about the MSTrackSource?

Exactly.

> Not sure why we won't bump in this assert....

It's only for creating a DOM track coming from mInputStream. For those tracks, the mInputTrackID and mTrackID will always be the same.

When cloning a stream we take a different path (CloneDOMTrack).

Comment 17

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #16)
> https://reviewboard.mozilla.org/r/54622/#review51388
> 
> > Is the usage of TRACK_NONE consitency with other place?
> > Maybe we should define well in https://dxr.mozilla.org/mozilla-central/source/dom/media/StreamTracks.h#20
> > :)
> 
> I think "none" as in "no particular TrackID specified" is reasonable.

And TRACK_ANY makes even more sense... I'll change to that.

Comment 18

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Agreed! :)

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #17)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #16)
> > https://reviewboard.mozilla.org/r/54622/#review51388
> > 
> > > Is the usage of TRACK_NONE consitency with other place?
> > > Maybe we should define well in https://dxr.mozilla.org/mozilla-central/source/dom/media/StreamTracks.h#20
> > > :)
> > 
> > I think "none" as in "no particular TrackID specified" is reasonable.
> 
> And TRACK_ANY makes even more sense... I'll change to that.

Comment 19

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1274221 - Ensure MediaStreamListeners are always notified of created and ended tracks. r?ctai, r?jesup

This means that when a MediaStreamListener is added to a stream, we'll call
NotifyQueuedTrackChanges with TRACK_EVENT_CREATE for all tracks that already
exist.

Likewise, we'll call NotifyQueuedTrackChanges with TRACK_EVENT_ENDED for all
tracks that exist and have ended.

This fixes potential race conditions where a track was created and/or ended
before the listener was asynchronously added.

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

Comment 20

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8755449 [details]
MozReview Request: Bug 1274221 - Test that multiple tracks of the same source all end when in the same stream. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54620/diff/1-2/

Comment 21

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8755450 [details]
MozReview Request: Bug 1274221 - Make FindOwnedDOMTrack look for (optionally) unique tracks. r?ctai

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54622/diff/1-2/

Comment 22

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

Comment on attachment 8755832 [details]
MozReview Request: Bug 1274221 - Ensure MediaStreamListeners are always notified of created and ended tracks. r?ctai, r?jesup

https://reviewboard.mozilla.org/r/54838/#review51506

Comment 23

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8755832 [details]
MozReview Request: Bug 1274221 - Ensure MediaStreamListeners are always notified of created and ended tracks. r?ctai, r?jesup

https://reviewboard.mozilla.org/r/54838/#review51696

I think this patch also fix bug 1259788.

::: dom/media/MediaStreamGraph.cpp:2322
(Diff revision 1)
> +  for (StreamTracks::TrackIter it(mTracks); !it.IsEnded(); it.Next()) {
> +    MediaStream* inputStream = nullptr;
> +    TrackID inputTrackID = TRACK_INVALID;
> +    if (ProcessedMediaStream* ps = AsProcessedStream()) {
> +      inputStream = ps->GetInputStreamFor(it->GetID());
> +      MOZ_ASSERT(inputStream);

Does this mean if we call this function on non-TrackUnionStream and we will bump into this assertion?
There are 3 other classes inherited from ProcessedMediaStream, CameraPreviewMediaStream, AudioCaptureStream and AudioNodeStream.
Looks like there are no other ProcessedMediaStream except TrackUnionStream will call MediaStream::AddListenerImpl so far. CameraPreviewMediaStream uses its own AddListner function.
So should we add a comment on this somewhere?

Comment 24

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/54838/#review51696

> Does this mean if we call this function on non-TrackUnionStream and we will bump into this assertion?
> There are 3 other classes inherited from ProcessedMediaStream, CameraPreviewMediaStream, AudioCaptureStream and AudioNodeStream.
> Looks like there are no other ProcessedMediaStream except TrackUnionStream will call MediaStream::AddListenerImpl so far. CameraPreviewMediaStream uses its own AddListner function.
> So should we add a comment on this somewhere?

We will. That shouldn't happen though, because only TrackUnionStream is used for listeners in DOMMediaStream.

CameraPreviewMediaStream is a hack for the B2G camera.
AudioCaptureStream is used as input to a DOMMediaStream only and as such doesn't get listeners.
AudioNodeStream is internal to WebAudio.

We should have enough mochitest coverage to expose this if it is a problem. I'll add a comment.

Comment 25

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8755832 [details]
MozReview Request: Bug 1274221 - Ensure MediaStreamListeners are always notified of created and ended tracks. r?ctai, r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54838/diff/1-2/

Comment 26

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

https://reviewboard.mozilla.org/r/54838/#review52340

::: dom/media/MediaStreamGraph.cpp:2326
(Diff revisions 1 - 2)
> +      // The only ProcessedMediaStream where we should have listeners is
> +      // TrackUnionStream - it's what's used as owned stream in DOMMediaStream,
> +      // the only main-thread exposed stream type.
> +      // TrackUnionStream guarantees that each of its tracks has an input track.
> +      // Other types do not implement GetInputStreamFor() and will return null.
>        inputStream = ps->GetInputStreamFor(it->GetID());

Thanks!

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96d02a1982f7
Test that multiple tracks of the same source all end when in the same stream. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce89c5b1f32d
Make FindOwnedDOMTrack look for (optionally) unique tracks. r=ctai
https://hg.mozilla.org/integration/mozilla-inbound/rev/45bfebd36a99
Ensure MediaStreamListeners are always notified of created and ended tracks. r=ctai, r=jesup

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29257a0f6e04
Backed out changeset 45bfebd36a99 
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a4e814ce3cf
Backed out changeset ce89c5b1f32d 
https://hg.mozilla.org/integration/mozilla-inbound/rev/144cbe0bd3e3
Backed out changeset 96d02a1982f7

Comment 29

[Carsten Book [:Tomcat]]

backed this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=df403befe9fe76dea67ca4a0fb4386c6cab6bf93 for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29886477&repo=mozilla-inbound

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30c253e762c6
Test that multiple tracks of the same source all end when in the same stream. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/795bcfd6223d
Make FindOwnedDOMTrack look for (optionally) unique tracks. r=ctai
https://hg.mozilla.org/integration/mozilla-inbound/rev/80443106b263
Ensure MediaStreamListeners are always notified of created and ended tracks. r=ctai, r=jesup

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30c253e762c6
https://hg.mozilla.org/mozilla-central/rev/795bcfd6223d
https://hg.mozilla.org/mozilla-central/rev/80443106b263

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out this patch stack for frequent failures in test_peerConnection_addtrack_removetrack_events.html on Android 4.3 debug:

bug 1208373: https://hg.mozilla.org/integration/mozilla-inbound/rev/d433bc994ae2885a7269df4ef0969af89888c1ee
bug 1274221: https://hg.mozilla.org/integration/mozilla-inbound/rev/57453dbd063c0711348d6631cd7fc330346ef5f0
bug 1208328: https://hg.mozilla.org/integration/mozilla-inbound/rev/e8017df8752dc1bb7862dbdfe2fc0c99bfa2c146

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=30078129&repo=mozilla-inbound
09:27:59     INFO -  164 INFO Run step 80: PC_REMOTE_VERIFY_ICE_GATHERING
09:27:59     INFO -  165 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcRemote) received local trickle ICE candidates
09:27:59     INFO -  166 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcRemote) ICE gathering state is not 'new'
09:27:59     INFO -  167 INFO Run step 81: PC_LOCAL_WAIT_FOR_MEDIA_FLOW
09:27:59     INFO -  168 INFO Checking data flow to element: pcLocal_local_{9667f18d-2859-4cb6-98c6-7b28daa93262}
09:27:59     INFO -  169 INFO Checking data flow to element: pcLocal_local_{b133a0dc-5654-48b0-b332-8ce7a4021329}
09:27:59     INFO -  170 INFO Checking RTP packet flow for track {6f20246f-4b74-4d72-b1e7-0770749f4d15}
09:27:59     INFO -  171 INFO Checking RTP packet flow for track {5bc2cb99-5c1a-4222-a67b-b3fe7dd7ca96}
09:27:59     INFO -  172 INFO Element pcLocal_local_{9667f18d-2859-4cb6-98c6-7b28daa93262} saw 'timeupdate', currentTime=65.03619047619047s, readyState=4
09:27:59     INFO -  173 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Media flowing for element: pcLocal_local_{9667f18d-2859-4cb6-98c6-7b28daa93262}
09:27:59     INFO -  174 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Test timed out.


174 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Test timed out.
267 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Test timed out.
321 INFO TEST-UNEXPECTED-ERROR | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | called finish() multiple times
323 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Test timed out.
330 INFO TEST-UNEXPECTED-ERROR | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | called finish() multiple times
342 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | Number of ICE connections according to stats is not zero - Result logged after SimpleTest.finish()
343 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | stats reports exactly 1 ICE connection - Result logged after SimpleTest.finish()
344 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcLocal): local SDP contains stream {9667f18d-2859-4cb6-98c6-7b28daa93262} and track {6f20246f-4b74-4d72-b1e7-0770749f4d15} - Result logged after SimpleTest.finish()
345 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcLocal): local SDP contains stream {9667f18d-2859-4cb6-98c6-7b28daa93262} and track {5bc2cb99-5c1a-4222-a67b-b3fe7dd7ca96} - Result logged after SimpleTest.finish()
346 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcRemote): remote SDP contains stream {9667f18d-2859-4cb6-98c6-7b28daa93262} and track {6f20246f-4b74-4d72-b1e7-0770749f4d15} - Result logged after SimpleTest.finish()
347 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | PeerConnectionWrapper (pcRemote): remote SDP contains stream {9667f18d-2859-4cb6-98c6-7b28daa93262} and track {5bc2cb99-5c1a-4222-a67b-b3fe7dd7ca96} - Result logged after SimpleTest.finish()
PROCESS-CRASH | dom/media/tests/mochitest/test_peerConnection_addtrack_removetrack_events.html | application crashed [@ mozilla::gfx::Log<1, mozilla::gfx::CriticalLogger>::~Log]
06-13 09:30:13.895 F/MOZ_Assert( 2104): Assertion failure: false (An assert from the graphics logger), at /builds/slave/m-in-and-api-15-d-000000000000/build/src/gfx/2d/Logging.h:519

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbd234db0b96
Test that multiple tracks of the same source all end when in the same stream. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/7383eafd9158
Make FindOwnedDOMTrack look for (optionally) unique tracks. r=ctai
https://hg.mozilla.org/integration/mozilla-inbound/rev/b68b0a07b1dc
Ensure MediaStreamListeners are always notified of created and ended tracks. r=ctai, r=jesup

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cbd234db0b96
https://hg.mozilla.org/mozilla-central/rev/7383eafd9158
https://hg.mozilla.org/mozilla-central/rev/b68b0a07b1dc