1112692 - JsepSessionImpl does not support more than one BUNDLE group

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Comment 1

[Byron Campen [:bwc]]

I'm going to say that supporting bundle policy is in scope for this bug, since that will be necessary for testing.

Comment 2

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Bug 1112692: (WIP) BundlePolicy support, and support for more than one BUNDLE group.

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Bug 1112692: (WIP) BundlePolicy support, and support for more than one BUNDLE group.

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Bug 1112692: (WIP) BundlePolicy support, and support for more than one BUNDLE group.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Comment 6

[Byron Campen [:bwc]]

Will try to land this in early 42.

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

https://reviewboard.mozilla.org/r/9607/#review10977

This looks pretty good.  A few minor things need attention.  You will need a DOM peer to look at the idl change.

::: dom/webidl/RTCConfiguration.webidl:25
(Diff revision 7)
> +    RTCBundlePolicy        bundlePolicy = "balanced";

Let's not screw with funny alignment spacing, a single space is enough.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.h:311
(Diff revision 7)
> -  nsresult GetNegotiatedBundleInfo(std::set<std::string>* bundleMids,
> -                                   const SdpMediaSection** bundleMsection);
> +  // Maps mid to the master bundle m-section
> +  typedef std::map<std::string, const SdpMediaSection*> BundledMids;

The comment here is a little confusing.

// Maps each mid of an m-section to the m-section that is the master of its bundle.  Unbundled m-sections map to themselves.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:163
(Diff revision 7)
> +  if (mCurrentLocalDescription) {
> +    JSEP_SET_ERROR("Changing the bundle policy is only supported before the "
> +                   "first SetLocalDescription.");
> +    return NS_ERROR_UNEXPECTED;
> +  }

Is there any sense in supporting this for new tracks?  Or is that just too hard?

I checked, but didn't find anything in the spec to suggest that this had any state restrictions.

It seems like it might be be easy enough to change calls to SetupBundle so that it only touches sections that are new.  Folding new streams into existing bundles is the only tricky part.  File a follow up perhaps?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:536
(Diff revision 7)
>    for (size_t i = 0; i < sdp->GetMediaSectionCount(); ++i) {

It sucks a little that we don't have an iterator for media sections on the Sdp class.  We're creating a few more train wrecks than I'd like here.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2792
(Diff revision 7)
>    if (mState == kJsepStateStable) {
>      // offer/answer is done. Do we actually incorporate these defaults?
> -    const Sdp* answer(GetAnswer());
> +    if (msection.GetAttributeList().HasAttribute(SdpAttribute::kMidAttribute)) {

I was going to suggest that you need to invert this statement and return early, but you can't.

A new function to set default addresses seems sensible though.  Defer to the refactor as you choose.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2909
(Diff revision 7)
> -  if (group && !group->tags.empty()) {
> -    bundleMids->insert(group->tags.begin(), group->tags.end());
> -    *bundleMsection = FindMsectionByMid(sdp, group->tags[0]);
> +      if (bundledMids->count(mid)) {
> +        JSEP_SET_ERROR("mid \'" << mid << "\' appears more than once in a "
> +                       "BUNDLE group");
> +        return NS_ERROR_INVALID_ARG;
> -  }
> +      }
>  
> -  if (!bundleMids->empty()) {
> -    if (!*bundleMsection) {
> +      const SdpMediaSection* masterBundleMsection(
> +          FindMsectionByMid(sdp, group.tags[0]));
> +
> +      if (!masterBundleMsection) {
> -      JSEP_SET_ERROR("mid specified for bundle transport in group attribute"
> +        JSEP_SET_ERROR("mid specified for bundle transport in group attribute"
> -          " does not exist in the SDP. (mid="
> +            " does not exist in the SDP. (mid=" << group.tags[0] << ")");
> -          << group->tags[0] << ")");
> -      return NS_ERROR_INVALID_ARG;
> +        return NS_ERROR_INVALID_ARG;
> -    }
> +      }
>  
> -    if (MsectionIsDisabled(**bundleMsection)) {
> +      if (MsectionIsDisabled(*masterBundleMsection)) {
> -      JSEP_SET_ERROR("mid specified for bundle transport in group attribute"
> +        JSEP_SET_ERROR("mid specified for bundle transport in group attribute"
> -          " points at a disabled m-section. (mid="
> +            " points at a disabled m-section. (mid=" << group.tags[0] << ")");
> -          << group->tags[0] << ")");
> -      return NS_ERROR_INVALID_ARG;
> +        return NS_ERROR_INVALID_ARG;
> -    }
> +      }

All this operates on group.tags[0].  It doesn't need to be inside the loop.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h:344
(Diff revision 7)
> -                      nsIThread* aThread) {
> +                      nsISupports* aThread);

Just for my understanding, why the change to nsISupports?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:503
(Diff revision 7)
>  nsresult
>  PeerConnectionImpl::ConvertRTCConfiguration(const RTCConfiguration& aSrc,
> -                                            IceConfiguration *aDst)
> +                                            PeerConnectionConfiguration *aDst)

juberti, but this could be only defined under MOZILLA_EXTERNAL_LINKAGE, and on the PeerConnectionConfiguration class instead.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:812
(Diff revision 7)
> +PeerConnectionImpl::Initialize(PeerConnectionObserver& aObserver,
> +                               nsGlobalWindow& aWindow,
> +                               const RTCConfiguration& aConfiguration,
> +                               nsISupports* aThread,
> +                               ErrorResult &rv)

I like this refactor.  Shame that it's going to rot me, and more shame because I've been waiting on Richard to finish reviewing my patch for a while now.

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:3638
(Diff revision 7)
> +    if (!firstByType.count(msection.GetMediaType())) {
> +      firstByType[msection.GetMediaType()] = &msection;
> +    } else {
> +      ASSERT_TRUE(msection.GetAttributeList().HasAttribute(
> +            SdpAttribute::kBundleOnlyAttribute));
> +    }

Might pay to check that a=bundle-only is not present on the first.  e.g.,

    bool firstByType = !byType.count(msection.GetMediaType());
    if (firstByType) {
      byType[msection.GetMediaType()] = &msection;
    }
    EXPECT_EQ(!firstByType, 
              msection.GetAttributeList().HasAttribute(
                  SdpAttribute::kBundleOnlyAttribute));

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:3686
(Diff revision 7)
> +      ++activeTransports;

It's not really active transports unless this is `activeTransports += transport->mComponents`

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:3664
(Diff revision 7)
> +  for (JsepTrackPair& pair : answerPairs) {
> +    if (types.size() == 1) {
> +      ASSERT_FALSE(pair.mBundleLevel.isSome());
> +    } else {
> +      ASSERT_TRUE(pair.mBundleLevel.isSome());
> +      ASSERT_EQ(0U, *pair.mBundleLevel);
> +    }
> +  }

Code duplication.  Here and below.  'twas a nit until I saw the test below :)

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:3683
(Diff revision 7)
> +  activeTransports = 0;
> +  for (RefPtr<JsepTransport>& transport : answerTransports) {
> +    if (transport->mComponents) {
> +      ++activeTransports;
> +    }
> +  }
> +  ASSERT_EQ(1U, activeTransports);

Duplication.  Also, use EXPECT_EQ for this sort of test.

::: media/webrtc/signaling/test/signaling_unittests.cpp:4631
(Diff revision 7)
> -                        ::testing::Values("bundle",
> +                        ::testing::Values("max-bundle",
> +                                          "balanced",
> +                                          "max-compat",
>                                            "no_bundle",
>                                            "reject_bundle"));

How well do we handle the pathological case where the answered accepts the bundle, but rejects rtcpmux?

Comment 11

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/9607/#review10977

> Just for my understanding, why the change to nsISupports?

Because the JS entrypoint (now the "real" implementation) uses nsISupports, so it was less code to make this use nsISupports too.

> juberti, but this could be only defined under MOZILLA_EXTERNAL_LINKAGE, and on the PeerConnectionConfiguration class instead.

ಠ_ಠ

> I was going to suggest that you need to invert this statement and return early, but you can't.
> 
> A new function to set default addresses seems sensible though.  Defer to the refactor as you choose.

I'll add a pointer to the refactor bug.

> Is there any sense in supporting this for new tracks?  Or is that just too hard?
> 
> I checked, but didn't find anything in the spec to suggest that this had any state restrictions.
> 
> It seems like it might be be easy enough to change calls to SetupBundle so that it only touches sections that are new.  Folding new streams into existing bundles is the only tricky part.  File a follow up perhaps?

draft-jsep-10 4.1.10 forbids changing the bundlePolicy. Sounds like something needs to be reconciled in spec-land.

> How well do we handle the pathological case where the answered accepts the bundle, but rejects rtcpmux?

We handle this fine. There's a test for this in signaling_unittests. I do want to eventually move a lot of that testing here, and have signaling unittests focus on ICE and media flows.

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Comment 13

[Byron Campen [:bwc]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Comment 14

[Byron Campen [:bwc]]

(Just need webidl review from smaug)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

r+ for the .webidl
(follows what is in the current spec)

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

https://reviewboard.mozilla.org/r/9607/#review11023

::: media/webrtc/signaling/test/jsep_session_unittest.cpp:3663
(Diff revisions 7 - 8)
>    auto offerPairs = mSessionOff.GetNegotiatedTrackPairs();
>  
>    for (JsepTrackPair& pair : offerPairs) {
>      if (types.size() == 1) {
>        ASSERT_FALSE(pair.mBundleLevel.isSome());
>      } else {
>        ASSERT_TRUE(pair.mBundleLevel.isSome());
>        ASSERT_EQ(0U, *pair.mBundleLevel);
>      }
>    }

You missed this bit.  I count four such blocks.

Comment 17

[Byron Campen [:bwc]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Comment 18

[Byron Campen [:bwc]]

Comment on attachment 8612631 [details]
MozReview Request: Bug 1112692: BundlePolicy support, and support for more than one BUNDLE group.

Carry forward r=mt, r=smaug.

Check back on try.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/134c23315fa1

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/134c23315fa1