Closed Bug 1160280 Opened 5 years ago Closed 5 years ago

JsepSessionImpl should add ssrc attributes for recvonly m-sections

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: bwc, Assigned: bwc)

Details

Attachments

(1 file, 1 obsolete file)

Some implementations filter RTCP receiver reports based on the SSRCs in the remote SDP. Since we do not add these for m-sections with no local track (ie; recvonly), these implementations drop our receiver reports. Additionally, the bundle spec seems to require that these attributes be present.
Assignee: nobody → docfaraday
Attached file MozReview Request: bz://1160280/bwc (obsolete) —
/r/7957 - Bug 1160280: Put ssrc attributes in recvonly m-sections.

Pull down this commit:

hg pull -r b701cf47c47f9b100f6cad9a50f2b4590887e8b8 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dd4c8babc6d
Attachment #8600046 - Flags: review?(drno)
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

https://reviewboard.mozilla.org/r/7955/#review6811

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1602
(Diff revision 1)
> +  trackPairOut->mDefaultSsrc = mDefaultSsrcs[local.GetLevel()];

How do we know that mDefaultSsrcs is filled up to the required level?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1204
(Diff revision 1)
> +  while (mDefaultSsrcs.size() <= msection->GetLevel()) {
> +    uint32_t ssrc;
> +    nsresult rv = CreateSsrc(&ssrc);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    mDefaultSsrcs.push_back(ssrc);
>    }
>  
> +  std::vector<uint32_t> ssrcs;
> +  ssrcs.push_back(mDefaultSsrcs[msection->GetLevel()]);

Why do you generate multiple SSRCs in the while loop, just to pull out the one SSRC for the requested msection level?

Especially as this seems to re-generate new SSRC's if called a second time.

Also the call to CreateSsrc() guarantees a unique ssrc within the mSsrc set, but the mDefaultSsrcs vector does not guarantee anything like that. So we seem to have a guarantee the mDefaultSsrcs do not collide with anything in mSsrc, but mDefaultSsrcs can collide among each other. Is that intended?
Attachment #8600046 - Flags: review?(drno)
I've done some testing, and it seems that Chrome does not filter RRs from recvonly m-sections, with or without this patch (basing this on packet-loss statistics for the streams that Chrome sends). Can you verify whether current nightly sends RR that Chrome likes in your service?
Flags: needinfo?(gp)
Ok, let me get this ready to go, land, and can uplift. We need to make sure to call devs' attention to this since it alters our SDP somewhat.
Flags: needinfo?(gp)
https://reviewboard.mozilla.org/r/7955/#review7547

> Why do you generate multiple SSRCs in the while loop, just to pull out the one SSRC for the requested msection level?
> 
> Especially as this seems to re-generate new SSRC's if called a second time.
> 
> Also the call to CreateSsrc() guarantees a unique ssrc within the mSsrc set, but the mDefaultSsrcs vector does not guarantee anything like that. So we seem to have a guarantee the mDefaultSsrcs do not collide with anything in mSsrc, but mDefaultSsrcs can collide among each other. Is that intended?

CreateSsrc() should be inserting into mSsrcs, I'll fix. We generate extras here because mDefaultSsrcs is just a vector, and we don't want to leave the intervening values in some indeterminate state. I'm not sure I understand what you mean when you say it will re-generate SSRCs if called a second time; we never remove anything from mDefaultSsrcs, so this function will not populate any given index more than once.

> How do we know that mDefaultSsrcs is filled up to the required level?

mDefaultSsrcs is populated up to a given level when we create either an offer or answer m-section that isn't disabled. We should not be able to get to MakeNegotiatedTrackPair if we never created an enabled m-section for that level. Might be worth an assert though.
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

/r/7957 - Bug 1160280: Put ssrc attributes in recvonly m-sections.

Pull down this commit:

hg pull -r 40f340f72feb13d2385aa724a1cc2edf92a9453b https://reviewboard-hg.mozilla.org/gecko/
Attachment #8600046 - Flags: review?(drno)
Attachment #8600046 - Flags: review?(drno)
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

https://reviewboard.mozilla.org/r/7955/#review7565

Can we please get a unit test which tests this? :-)
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

/r/7957 - Bug 1160280: Put ssrc attributes in recvonly m-sections.

Pull down this commit:

hg pull -r e52d0a044e55c83730b600198ceecbc9efb87275 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

/r/7957 - Bug 1160280: Put ssrc attributes in recvonly m-sections.

Pull down this commit:

hg pull -r f47c359ae68e416bc21868f5ff7addcacd3683db https://reviewboard-hg.mozilla.org/gecko/
Attachment #8600046 - Flags: review?(drno)
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

Since I already have two large reviews on drno.
Attachment #8600046 - Flags: review?(drno) → review?(martin.thomson)
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

I can get this....
Attachment #8600046 - Flags: review?(martin.thomson) → review?(ekr)
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

https://reviewboard.mozilla.org/r/7955/#review7847

This appears to be largely correct, but I have some suggestions for improvement.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:348
(Diff revision 4)
> +  std::vector<uint32_t> mDefaultSsrcs;

The name default seems a little confusing, but I Don't have another one.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:843
(Diff revision 4)
> -  if (!ssrcs->mSsrcs.empty()) {
> -    msection->GetAttributeList().SetAttribute(ssrcs.release());
> +  if (!ssrcAttr->mSsrcs.empty()) {
> +    msection->GetAttributeList().SetAttribute(ssrcAttr.release());

Wouldn't it be cleaner to have an early return in the beginning of this fxn for ssrcs.empty()

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1201
(Diff revision 4)
> +JsepSessionImpl::SetDefaultSsrc(SdpMediaSection* msection)
> +{
> +  while (mDefaultSsrcs.size() <= msection->GetLevel()) {
> +    uint32_t ssrc;
> +    nsresult rv = CreateSsrc(&ssrc);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    mDefaultSsrcs.push_back(ssrc);
>    }
>  
> +  std::vector<uint32_t> ssrcs;
> +  ssrcs.push_back(mDefaultSsrcs[msection->GetLevel()]);
> +  SetSsrcs(ssrcs, msection);

A comment about how you get into the state where you need to generate multiple SSRCs at once would be useful.

Also, I notice that you are generating a bogus SSRC for level 0. Is that worth doing something about, e.g., GetLevel(-1)

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:116
(Diff revision 4)
> +  if (!GetLocalSSRC(&oldSsrc)) {
> +    return false;
> +  }

How can this happen? Is it just an internal error in which case we need an assert

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:125
(Diff revision 4)
> +  if (StopTransmitting() != kMediaConduitNoError) {
> +    return false;
> +  }

You only need to stop if you were already transmitting, right?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:167
(Diff revision 4)
> +  if (!GetLocalSSRC(&oldSsrc)) {
> +    return false;
> +  }
> +
> +  if (oldSsrc == ssrc) {
> +    return true;
> +  }
> +
> +  bool wasTransmitting = mEngineTransmitting;
> +  if (StopTransmitting() != kMediaConduitNoError) {
> +    return false;
> +  }
> +
> +  if (mPtrRTP->SetLocalSSRC(mChannel, ssrc)) {
> +    return false;
> +  }
> +
> +  if (wasTransmitting) {
> +    if (StartTransmitting() != kMediaConduitNoError) {
> +      return false;
> +    }
> +  }
> +  return true;

See above comments on this code block in audio

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp:368
(Diff revision 4)
> +  RefPtr<MediaSessionConduit> conduit;
> +  if (aTrack.GetMediaType() == SdpMediaSection::kAudio) {
> +    rv = GetOrCreateAudioConduit(aTrackPair, aTrack, &conduit);
> +    if (NS_FAILED(rv))
> +      return rv;
> +  } else if (aTrack.GetMediaType() == SdpMediaSection::kVideo) {
> +    rv = GetOrCreateVideoConduit(aTrackPair, aTrack, &conduit);
> +    if (NS_FAILED(rv))
> +      return rv;
> +  } else {
> +    // We've created the TransportFlow, nothing else to do here.
> +    return NS_OK;
> +  }
> +

Why did this need to move? It seems to be unchanged.
Attachment #8600046 - Flags: review?(ekr) → review+
https://reviewboard.mozilla.org/r/7955/#review7861

> The name default seems a little confusing, but I Don't have another one.

I guess mRecvonlySsrcs could work a little better here.

> Wouldn't it be cleaner to have an early return in the beginning of this fxn for ssrcs.empty()

Actually, I should probably change more than that. If we call this with an empty list of ssrcs, we should probably remove the attribute.

> A comment about how you get into the state where you need to generate multiple SSRCs at once would be useful.
> 
> Also, I notice that you are generating a bogus SSRC for level 0. Is that worth doing something about, e.g., GetLevel(-1)

I'm not sure I understand. Level is now 0-indexed.

> How can this happen? Is it just an internal error in which case we need an assert

I think it would be safe to put an assert here.

> You only need to stop if you were already transmitting, right?

StopTransmitting checks this, and no-ops if we weren't transmitting already.

> Why did this need to move? It seems to be unchanged.

Because this code updates (if necessary) the local SSRC (and codec stuff), and we were bailing out before we got here if the pipeline already existed.
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

/r/7957 - Bug 1160280: Put ssrc attributes in recvonly m-sections. r=ekr

Pull down this commit:

hg pull -r 9fa9f3db0dea04fa2a5780edcc243ab8afce021c https://reviewboard-hg.mozilla.org/gecko/
Attachment #8600046 - Flags: review+
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

Carry forward r=ekr, need to check back on try.
Flags: needinfo?(docfaraday)
Attachment #8600046 - Flags: review+
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

/r/7957 - Bug 1160280: Put ssrc attributes in recvonly m-sections. r=ekr

Pull down this commit:

hg pull -r c8563dba7f2aa896d7cbcc7b616b3271b295a056 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8600046 - Flags: review+
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

Carry forward r=ekr
Attachment #8600046 - Flags: review+
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

Approval Request Comment
[Feature/regressing bug #]:

   None.

[User impact if declined]:

   Poor incoming video quality when using unidirectional multistream with Chrome (eg; Jitsi, and Hello once we start doing multistream potentially).

[Describe test coverage new/current, TreeHerder]:

   Added some unit-test coverage of this new behavior.

[Risks and why]:

   Pretty low, although there is some potential for interop problems.

[String/UUID change made/needed]:

   None.
Flags: needinfo?(docfaraday)
Attachment #8600046 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8a1773297288
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
> Poor incoming video quality when using unidirectional multistream with
> Chrome (eg; Jitsi, and Hello once we start doing multistream potentially).
If I understand correctly, this is not a recent regression and we have this bug for a while. Why don't we let it ride the train? Thanks
(In reply to Sylvestre Ledru [:sylvestre] from comment #22)
> > Poor incoming video quality when using unidirectional multistream with
> > Chrome (eg; Jitsi, and Hello once we start doing multistream potentially).
> If I understand correctly, this is not a recent regression and we have this
> bug for a while. Why don't we let it ride the train? Thanks

I'd like to suggest the opposite. It's early in the cycle and there's no L10N implications, so why not uplift it.
Comment on attachment 8600046 [details]
MozReview Request: bz://1160280/bwc

Because we had some regressions in this part of the code lately, all changes have a chance to introduce potential regressions ("some potential for interop problems") and I wanted to understand a bit more about this feature.
Anyway, as you said, it is early in the aurora cycle, so taking it.
Attachment #8600046 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8600046 - Attachment is obsolete: true
Attachment #8620206 - Flags: review+
You need to log in before you can comment on or make changes to this bug.