Closed Bug 1273136 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Version:
48 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: pehrsons, Assigned: bwc)

References

Details

Attachments

(2 files, 1 obsolete file)

MozReview Request: Bug 1273136: Start remote streams on SRD, and end them even if offer/answer never completed. r?jesup, r=pehrsons
58 bytes, text/x-review-board-request
pehrsons
: review+
jesup
: review+
Details
Don't leak runnables needed to cleanup gUM streams
4.04 KB, patch
drno
: review+
Details | Diff | Splinter Review 
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.
Rank: 25
Priority: -- → P2
Blocks: 1208373
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)?
Rank: 25 → 15
Flags: needinfo?(pehrsons)
Priority: P2 → P1
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.
Flags: needinfo?(pehrsons) → needinfo?(docfaraday)
Priority: P1 → --
Priority: -- → P1
Assignee: nobody → docfaraday
Flags: needinfo?(docfaraday)
Blocks: 1275119
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.
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 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.
Attachment #8758288 - Flags: review?(pehrsons) → review+
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.
(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 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!)
Attachment #8758288 - Flags: review?(rjesup)
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.
(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 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/
Attachment #8758288 - Attachment description: MozReview Request: Bug 1273136: Start remote streams on SRD, and end them even if offer/answer never completed. r?jesup, r?pehrsons → MozReview Request: Bug 1273136: Start remote streams on SRD, and end them even if offer/answer never completed. r?jesup, r=pehrsons
Attachment #8758288 - Flags: review?(rjesup)
(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.
Flags: needinfo?(padenot)
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.
Attachment #8758288 - Flags: review?(rjesup) → review+
Note that changing from DEFAULT_SAMPLE_RATE should be a new bug (cc paul!)
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.
Flags: needinfo?(padenot)
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.
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.
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.
With jesups hackery (wizardry) to not leak a runnable in MediaManager: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25c6bf0a9264
MozReview-Commit-ID: BqUMC2jpahM
Attachment #8759365 - Flags: review?(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.
MozReview-Commit-ID: BqUMC2jpahM
Attachment #8759384 - Flags: review?(drno)
Attachment #8759365 - Attachment is obsolete: true
Attachment #8759365 - Flags: review?(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
Attachment #8759384 - Flags: review?(drno) → review+
Pushed by rjesup@wgate.com:
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
https://hg.mozilla.org/mozilla-central/rev/1b3eddbebb7a
https://hg.mozilla.org/mozilla-central/rev/0cb38314f603
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.