1273136 - PeerConnection shouldn't expose a received MediaStreamTrack without guaranteeing that the underlying track goes live

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

We're now exposing MediaStreamTracks before the receiving MediaPipelines are up (they create the underlying tracks) so in case we don't finish negotiating offers and answers (getting the MediaPipelines up) we can't end those tracks.

Exposing a MediaStreamTrack before we can guarantee that the underlying track will be created is kind of a cheat. Perhaps we can create those MediaPipelines as soon as we've created the MediaStreamTracks instead?

See bug 1019579 comment 25.

Comment 1

[Maire Reavy [:mreavy]]

Raising this to a P1 until it unblocks bug 1208373 which is a P1.

Andreas -- Do you feel you have or will soon have enough info to take this on (such that we can land the other bug)?

Comment 2

[Andreas Pehrson [:pehrsons]]

I know what to do at a conceptual level and that it'll be tricky. Currently I'm waiting for Byron to see if there's something he can do (bug 1208373 comment 89). I know the MediaPipeline, MediaStreamTrack and MSG bits pretty well, but this goes deeper and needs to fit with JSEP and stuff. Ugha.

Byron - I'm needinfoing you here just so you can give us a status when you've finished thinking per bug 1208373 comment 89.

Comment 3

[Byron Campen [:bwc]]

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

Comment 4

[Andreas Pehrson [:pehrsons]]

Looking mostly good. Assertions are easy to fix - only end remote tracks instead of both. I'm a bit more worried about test_peerConnection_verifyAudioAfterRenegotiation.html timing out on Windows.

Comment 5

[Byron Campen [:bwc]]

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

Comment 6

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

Note: the Win7 opt MDA failures:
This failure appears on Try even with no patches applied, but does not appear on inbound/central/etc. Likely a perf-induced failure, perhaps latent, perhaps due to the track-cloning landing. Not a blocker to landing

Comment 7

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1273136: Start remote streams on SRD, and end them even if offer/answer never completed. r?jesup, r=pehrsons

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

Comment 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8758288 [details]
MozReview Request: Bug 1273136: Start remote streams on SRD, and end them even if offer/answer never completed. r?jesup, r=pehrsons

https://reviewboard.mozilla.org/r/56580/#review53126

Assuming you've checked that we're good without queueing tracks (adding all MediaStreamTracks to the MediaStream before exposing it to script), r+.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1753
(Diff revision 1)
> +static void StartTrack(MediaStream* source,
> +                       TrackID track_id,
> +                       MediaSegment* segment) {

This file seems to follow the aArgument pattern for naming.

Also, make it clear that we take ownership of segment.

Comment 9

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/56580/#review53126

As far as mochitest is concerned, this should be fine; onaddstream is no longer used there, everything is driven by ontrack now. It is possible we might have some problems in the field with code that still uses onaddstream.

> This file seems to follow the aArgument pattern for naming.
> 
> Also, make it clear that we take ownership of segment.

Will fix.

Comment 10

[Andreas Pehrson [:pehrsons]]

(In reply to Byron Campen [:bwc] (PTO May 26, PTO Jun 6-10) from comment #9)
> https://reviewboard.mozilla.org/r/56580/#review53126
> 
> As far as mochitest is concerned, this should be fine; onaddstream is no
> longer used there, everything is driven by ontrack now. It is possible we
> might have some problems in the field with code that still uses onaddstream.

Do we know which MediaStreamTracks to create before exposing the MediaStream? Having them exist on main thread before "addstream" should avoid problems, even if they're not live in the MSG yet.

Comment 11

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

Comment on attachment 8758288 [details]
MozReview Request: Bug 1273136: Start remote streams on SRD, and end them even if offer/answer never completed. r?jesup, r=pehrsons

https://reviewboard.mozilla.org/r/56580/#review53166

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1775
(Diff revision 1)
> -  if (segment->GetType() == MediaSegment::AUDIO) {
> +#endif
> -    source->AsSourceStream()->AddAudioTrack(track_id, track_rate, 0,
> -                                            static_cast<AudioSegment*>(segment),
> -                                            SourceMediaStream::ADDTRACK_QUEUED);

So we're killing all the queued-track additions... are we killing them from MSG as well?  (No, see below.)  Does this avoid the problems we had in bug 1116925 (bad intermittent caused by an iteration seeing one of the tracks and not the other)?  We are still using it to add tracks in MediaManager/getUserMedia, so it can't go away.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1895
(Diff revision 1)
>        MOZ_MTLOG(ML_ERROR, "NotifyPull() called from a non-SourceMediaStream");
>        return;
>      }
>  
>      // This comparison is done in total time to avoid accumulated roundoff errors.
> -    while (source_->TicksToTimeRoundDown(track_rate_, played_ticks_) <
> +    while (source_->TicksToTimeRoundDown(DEFAULT_SAMPLE_RATE, played_ticks_) <

using DEFAULT_SAMPLE_RATE here seems quite wrong - and there's no discussion in the bug (or patch) of why it's ok to use that rate always.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1750
(Diff revision 1)
> +// Should come from MediaEngineWebRTC.h, but that's a pain to include here
> +#define DEFAULT_SAMPLE_RATE 32000

Again I'm concerned about this.  Also, if it's mirrored here, please add a comment there this if change, this must change too!

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1889
(Diff revision 1)
> +    // We need to select unique ids, just use max + 1
> +    size_t maxTrackId = 0;

Shouldn't this be a uint type (uint64_t perhaps), not size_t?  Or a TrackID (which is in32_t!! so size_t (and uint32_t or above) could fail to fit in it!)

Comment 12

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/56580/#review53166

> So we're killing all the queued-track additions... are we killing them from MSG as well?  (No, see below.)  Does this avoid the problems we had in bug 1116925 (bad intermittent caused by an iteration seeing one of the tracks and not the other)?  We are still using it to add tracks in MediaManager/getUserMedia, so it can't go away.

Bug 1116925 will not happen anymore, because mochitest doesn't use onaddstream anymore. It just uses ontrack, so there's nothing to be missing. I have not touched anything related to track queueing apart from remote media tracks for webrtc.

> using DEFAULT_SAMPLE_RATE here seems quite wrong - and there's no discussion in the bug (or patch) of why it's ok to use that rate always.

track_rate_ is always set to DEFAULT_SAMPLE_RATE right now for MediaPipelineReceiveAudio::PipelineListener.

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp?from=MediaPipeline.cpp#1944

> Again I'm concerned about this.  Also, if it's mirrored here, please add a comment there this if change, this must change too!

I'll see what I can do to make this better.

> Shouldn't this be a uint type (uint64_t perhaps), not size_t?  Or a TrackID (which is in32_t!! so size_t (and uint32_t or above) could fail to fit in it!)

If TrackID is int32_t, I'll use that.

Comment 13

[Byron Campen [:bwc]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #10)
> (In reply to Byron Campen [:bwc] (PTO May 26, PTO Jun 6-10) from comment #9)
> > https://reviewboard.mozilla.org/r/56580/#review53126
> > 
> > As far as mochitest is concerned, this should be fine; onaddstream is no
> > longer used there, everything is driven by ontrack now. It is possible we
> > might have some problems in the field with code that still uses onaddstream.
> 
> Do we know which MediaStreamTracks to create before exposing the
> MediaStream? Having them exist on main thread before "addstream" should
> avoid problems, even if they're not live in the MSG yet.

Yeah, onaddstream is called last. So, this should be fine.

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8758288 [details]
MozReview Request: Bug 1273136: Start remote streams on SRD, and end them even if offer/answer never completed. r?jesup, r=pehrsons

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

Comment 15

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

(In reply to Byron Campen [:bwc] (PTO May 26, PTO Jun 6-10) from comment #12)
> https://reviewboard.mozilla.org/r/56580/#review53166
> 
> > So we're killing all the queued-track additions... are we killing them from MSG as well?  (No, see below.)  Does this avoid the problems we had in bug 1116925 (bad intermittent caused by an iteration seeing one of the tracks and not the other)?  We are still using it to add tracks in MediaManager/getUserMedia, so it can't go away.
> 
> Bug 1116925 will not happen anymore, because mochitest doesn't use
> onaddstream anymore. It just uses ontrack, so there's nothing to be missing.
> I have not touched anything related to track queueing apart from remote
> media tracks for webrtc.

Ok, good.

> 
> > using DEFAULT_SAMPLE_RATE here seems quite wrong - and there's no discussion in the bug (or patch) of why it's ok to use that rate always.
> 
> track_rate_ is always set to DEFAULT_SAMPLE_RATE right now for
> MediaPipelineReceiveAudio::PipelineListener.
> 
> https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/
> mediapipeline/MediaPipeline.cpp?from=MediaPipeline.cpp#1944

Hmmm. This is actually a problem.  This means even if we use untouched audio at the source (aec/etc off, feed directly into MSG with no resampling) that there's no way to receive audio above ~15KHz.  Admittedly at the edge of many older engineer's hearing... ;-)  But audio people and unusual usecases (music teaching/monitoring/jams/remote recording/etc) will be impacted.  And we'll resample to the Graph rate *anyways* immediately on AppendToTrack.

Using the graph rate would make a ton more sense..... Paul?

> 
> > Again I'm concerned about this.  Also, if it's mirrored here, please add a comment there this if change, this must change too!
> 
> I'll see what I can do to make this better.
> 
> > Shouldn't this be a uint type (uint64_t perhaps), not size_t?  Or a TrackID (which is in32_t!! so size_t (and uint32_t or above) could fail to fit in it!)
> 
> If TrackID is int32_t, I'll use that.

It is (and somewhat annoying, though it allows for dom/media/StreamTracks.h):
const TrackID TRACK_NONE = 0;
const TrackID TRACK_INVALID = -1;
const TrackID TRACK_ANY = -2;

See also IsTrackIDExplicit()
You should not let it wrap around, note.

Comment 16

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

Comment on attachment 8758288 [details]
MozReview Request: Bug 1273136: Start remote streams on SRD, and end them even if offer/answer never completed. r?jesup, r=pehrsons

https://reviewboard.mozilla.org/r/56580/#review53260

r+ with nits

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1889
(Diff revisions 1 - 2)
>        principal =  nsNullPrincipal::CreateWithInheritedAttributes(doc->NodePrincipal());
>      }
>  #endif
>  
>      // We need to select unique ids, just use max + 1
> -    size_t maxTrackId = 0;
> +    TrackID maxTrackId = 0;

may want to start above TRACK_NONE

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1907
(Diff revisions 1 - 2)
>          RefPtr<RemoteTrackSource> source =
>            new RemoteTrackSource(principal, nsString());
>  #else
>          RefPtr<MediaStreamTrackSource> source = new MediaStreamTrackSource();
>  #endif
> -        TrackID trackID = static_cast<TrackID>(++maxTrackId);
> +        TrackID trackID = ++maxTrackId;

Don't allow wraparound to negative -- and if it does wrap around, what happens?  Can that cause a duplicate?  Ok, not very likely outside of contrived cases.  I'd even be good with MOZ_CRASH in that case.

Comment 17

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

Note that changing from DEFAULT_SAMPLE_RATE should be a new bug (cc paul!)

Comment 18

[Paul Adenot (:padenot)]

Yes, we should allow full audio sample rate with minimal resampling, this is important when you're not sending just voice from a crappy microphone.

Using the graph rate fits in with the current effort of minimizing resampling passes.

Comment 19

[Byron Campen [:bwc]]

I can just try doing this. It results in simpler code, which I'm always a fan of. I'm currently trying to debug some LSAN failures right now, so it is going to be a bit before this is ready.

Comment 20

[Byron Campen [:bwc]]

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

Comment 21

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/56580/#review53900

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:117
(Diff revision 2)
>  void
> +SourceStreamInfo::EndTrack(MediaStream* stream, dom::MediaStreamTrack* track)
> +{
> +  if (!stream || !stream->AsSourceStream()) {
> +    return;
> +  }
> +
> +#if !defined(MOZILLA_EXTERNAL_LINKAGE)
> +  class Message : public ControlMessage {
> +   public:
> +    Message(MediaStream* stream, TrackID track)
> +      : ControlMessage(stream),
> +        track_id_(track) {}
> +
> +    virtual void Run() override {
> +      mStream->AsSourceStream()->EndTrack(track_id_);
> +    }
> +   private:
> +    TrackID track_id_;
> +  };
> +
> +  stream->GraphImpl()->AppendMessage(
> +      MakeUnique<Message>(stream, track->mTrackID));
> +#endif
> +
> +}

You don't need to be on the MediaStreamGraph to end a SourceMediaStream track.

I imagine this could cause timing changes.

Comment 22

[Andreas Pehrson [:pehrsons]]

Re comment 21: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0b75234c7c4

Comment 23

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/56580/#review53900

> You don't need to be on the MediaStreamGraph to end a SourceMediaStream track.
> 
> I imagine this could cause timing changes.

This is true, but my worry is that if the starting of the track is dispatched, and the ending of the track is not, these things could be reordered.

Comment 24

[Andreas Pehrson [:pehrsons]]

With jesups hackery (wizardry) to not leak a runnable in MediaManager: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25c6bf0a9264

Comment 25

[Byron Campen [:bwc]]

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

Comment 26

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

Attachment: Don't leak runnables needed to cleanup gUM streams

MozReview-Commit-ID: BqUMC2jpahM

Comment 27

[Nils Ohlmeier [:drno]]

Comment on attachment 8759365 [details] [diff] [review]
Don't leak runnables needed to cleanup gUM streams

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

I don't think this compiles

::: dom/media/MediaManager.h
@@ +149,5 @@
>                  AudioDevice* aAudioDevice,
>                  VideoDevice* aVideoDevice)
>    {
>      MOZ_ASSERT(NS_IsMainThread());
> +    mMainThreadCheck = PR_GetCurrentThread();

So |mMainThreadCheck| gets set here, but never read anywhere. And |mMainThreadCheck| is not even defined anywhere. I guess you meant |mMainThreadDebug| instead?

@@ +284,5 @@
> +        rv = NS_GetMainThread(getter_AddRefs(thread));
> +        if (NS_WARN_IF(NS_FAILED(rv))) {
> +          NS_ASSERTION(false, "Mainthread not available; running on current thread");
> +          // Ensure this really *was* MainThread (NS_GetCurrentThread won't work)
> +          MOZ_ASSERT_RELEASE(mMainThreadDebug == PR_GetCurrentThread());

|mMainThreadDebug| never got initialized with anything?! I'm guessing there is some variable name confusion going on here.

Comment 28

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

Attachment: Don't leak runnables needed to cleanup gUM streams

MozReview-Commit-ID: BqUMC2jpahM

Comment 29

[Nils Ohlmeier [:drno]]

Comment on attachment 8759384 [details] [diff] [review]
Don't leak runnables needed to cleanup gUM streams

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

LGTM

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b3eddbebb7a
Start remote streams on SRD, and end them even if offer/answer never completed. r=jesup, r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb38314f603
Don't leak runnables needed to cleanup gUM streams r=drno

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b3eddbebb7a
https://hg.mozilla.org/mozilla-central/rev/0cb38314f603