784491 - SDP handling should support Bundle - parsing & generation work

Status: RESOLVED DUPLICATE of bug 786234

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

Description

[Ethan Hugg [:ehugg]]

We currently do not support bundle in our SDP handling, e.g.

a=group:BUNDLE audio video

Comment 1

[enda mannion (:emannion)]

Attachment: Add Bundled media to SIPCC SDP

This is an incomplete patch.  Completion of this patch awaits completion of the bundle SDP spec from MMUSIC. 

This patch will add the Audio and Video lines to the bundle group but does not add the data channe; line to the same group.

Comment 2

[enda mannion (:emannion)]

Attachment: Add Bundled media to SIPCC SDP

This patch has been updated to build on m-c.
I has a few changes, it will not bundle media if only one media stream exists.

I have some other questions about this, all related to section 6.1 from the spec.
http://tools.ietf.org/html/draft-holmberg-mmusic-sdp-bundle-negotiation-00

This patch is dependent on the constraints patch.
https://bugzilla.mozilla.org/show_bug.cgi?id=784515

Comment 3

[enda mannion (:emannion)]

Attachment: Add Bundled media to SIPCC SDP

Comment 4

[enda mannion (:emannion)]

Attachment: Add Bundled media to SIPCC SDP

Another update to the patch after a merge job.

There is one outstanding bug, where I hit a crash using the new flex_string. ToDo.

Comment 5

[Ethan Hugg [:ehugg]]

Please add

if (!more)
  MOZ_CRASH();

to flex_string_append, and

if (!format)
  MOZ_CRASH();

to flex_string_sprintf to make it easier to debug on NULL input.

Comment 6

[enda mannion (:emannion)]

Attachment: Add Bundled media to SIPCC SDP

Still outstanding are.

1.  Section 6.1 in the bundle SDP RFC. Regarding same port and c= line 
2.  This bundle impl is only for audio and video, not data channels or > 2 streams.

Comment 7

[enda mannion (:emannion)]

Attachment: Add Bundled media to SIPCC SDP

Re-based ontop of new constraints patch

Comment 8

[Ethan Hugg [:ehugg]]

Comment on attachment 673170 [details] [diff] [review]
Add Bundled media to SIPCC SDP

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

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +3391,3 @@
>              GSMSDP_FOR_ALL_MEDIA(anat_media, dcb_p) {
> +                group_mid = sdp_attr_get_simple_string(sdp_p->src_sdp,
> +                                                     SDP_ATTR_MID, (uint16_t) group_id_2, 0, 1);

Here you are casting group_id_2 (a char *) to a uint16_t.  Are you running with our -Werror patch to catch these?  I assume you want to do something else here.  Same thing happens again about 10 lines later.

Comment 9

[enda mannion (:emannion)]

(In reply to Ethan Hugg [:ehugg] from comment #8)
> Comment on attachment 673170 [details] [diff] [review]
> Add Bundled media to SIPCC SDP
> 
> Review of attachment 673170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +3391,3 @@
> >              GSMSDP_FOR_ALL_MEDIA(anat_media, dcb_p) {
> > +                group_mid = sdp_attr_get_simple_string(sdp_p->src_sdp,
> > +                                                     SDP_ATTR_MID, (uint16_t) group_id_2, 0, 1);
> 
> Here you are casting group_id_2 (a char *) to a uint16_t.  Are you running
> with our -Werror patch to catch these?  I assume you want to do something
> else here.  Same thing happens again about 10 lines later.

I do have the -Werror patch in the queue so strange I am getting away with this cast.   I knew the Anat functions needed some work but but that on the long finger as we are not using ANAT.  I can update this now but have one question.  What function should I now use to convert a string to an integer, is Atoi out of the question.

Comment 10

[Ethan Hugg [:ehugg]]

 What function should I now use to convert a string to an integer, is Atoi out of the question.

Please use strtol/strtoul.  That's where we are with the atoi replacement, I'll replace all the strto[u]l calls later probably with a new function built of PR_sscanf.

Check out some of the other uses of strtoul in signaling where we check errno and see if the ptr moves, etc.

Comment 11

[enda mannion (:emannion)]

Attachment: Add Bundled media to SIPCC SDP

Comment 12

[Maire Reavy [:mreavy]]

We're no longer targeting Bundle for our first unpref'd release of WebRTC.  We still want it, but it's lower priority.  (Reassigning this from Enda to Ethan.)

Comment 13

[Maire Reavy [:mreavy]]

I'm assuming this work will start in mid-September (after Bug 906843) and continue to mid-November. I've taken 1 month estimate (160 hours) and discounted it by 40% due to Adam's work on standards.

The target milestone for this bug is Gecko 28, and I'm setting a deadline of Nov 18, assuming work starts on Sept 16.  (The deadline for Bug 906843 is currently Sept 10.)  Gecko 28 uplifts on Dec 9.

Comment 14

[Adam Roach [:abr]]

Attachment: Part 1: Add constraints to addStream

Comment 15

[Adam Roach [:abr]]

Attachment: Part 1: Add constraints to addStream

Comment 16

[Adam Roach [:abr]]

Attachment: [WIP] Part 2: Bundle SDP handling (unrotting old patch)

Comment 17

[Adam Roach [:abr]]

Part 1 will need to be updated to track the fix for Bug 929534 (which may or may not show up during rebasing).

Comment 18

[Adam Roach [:abr]]

Attachment: Part 1: Add constraints to addStream

Comment 19

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

Comment on attachment 8348811 [details] [diff] [review]
Part 1: Add constraints to addStream

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

Really r- because the constraint assignment in fsmdef.c clobbers the default value of true in gsm_sdp.c, but since there's so much plumbing, I really don't want to see it again, so I'm giving r+ assuming you'll add the .was_passed check.

::: dom/media/bridge/IPeerConnection.idl
@@ +28,5 @@
>  interface IPeerConnectionObserver : nsISupports
>  {
>  };
>  
> +[scriptable, uuid(c1cba797-6e92-450a-9338-bb12f4b9c3e4)]

IPeerConnection.idl is no longer used, except to house a bunch of constants, so this isn't necessary.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +4022,5 @@
> +        dcb->media_cap_tbl->cap[cap_index].support_direction = SDP_DIRECTION_SENDRECV;
> +        dcb->media_cap_tbl->cap[cap_index].pc_stream = msg->data.track.stream_id;
> +        dcb->media_cap_tbl->cap[cap_index].pc_track = msg->data.track.track_id;
> +
> +        if (msg->data.track.constraints) {

You should check
if (msg->data.track.constraints &&
    msg->data.track.constraints->moz_bundle_only.was_passed)

here to protect your default value of true.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +861,5 @@
>      return sdp;
>    }
>  
>    // Adds a stream to the PeerConnection.
>    void AddStream(uint32_t hint =

None of the unittests are calling AddStream() with a constraints arg yet, so this is just plumbing for now, correct?

@@ +865,5 @@
>    void AddStream(uint32_t hint =
>           DOMMediaStream::HINT_CONTENTS_AUDIO |
>           DOMMediaStream::HINT_CONTENTS_VIDEO,
> +       MediaStream *stream = nullptr,
> +       sipcc::MediaConstraints *constraints = nullptr

None of the unittests are calling AddStream() with a constraints arg yet, so this is just plumbing for now, correct?

Comment 20

[Adam Roach [:abr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #19)

> > +[scriptable, uuid(c1cba797-6e92-450a-9338-bb12f4b9c3e4)]
> 
> IPeerConnection.idl is no longer used, except to house a bunch of constants,
> so this isn't necessary.

Removed.

> ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
> @@ +4022,5 @@
> > +        dcb->media_cap_tbl->cap[cap_index].support_direction = SDP_DIRECTION_SENDRECV;
> > +        dcb->media_cap_tbl->cap[cap_index].pc_stream = msg->data.track.stream_id;
> > +        dcb->media_cap_tbl->cap[cap_index].pc_track = msg->data.track.track_id;
> > +
> > +        if (msg->data.track.constraints) {
> 
> You should check
> if (msg->data.track.constraints &&
>     msg->data.track.constraints->moz_bundle_only.was_passed)
> 
> here to protect your default value of true.

Good catch. Thanks.

> None of the unittests are calling AddStream() with a constraints arg yet, so
> this is just plumbing for now, correct?

Correct, both times.

Comment 21

[Adam Roach [:abr]]

Attachment: Part 1: Add constraints to addStream

Comment 22

[Adam Roach [:abr]]

Comment on attachment 8349586 [details] [diff] [review]
Part 1: Add constraints to addStream

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

Carrying forward jib's r+

Comment 23

[Adam Roach [:abr]]

Comment on attachment 8349586 [details] [diff] [review]
Part 1: Add constraints to addStream

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0ae017eae17

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d0ae017eae17

Comment 25

[Martin Thomson [:mt:]]

Comment on attachment 819987 [details] [diff] [review]
[WIP] Part 2: Bundle SDP handling (unrotting old patch)

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

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +3917,5 @@
> +         dst_mid = sdp_attr_get_simple_string(sdp_p->dest_sdp, SDP_ATTR_MID, level, 0, 1);
> +         sdp_get_group_id(sdp_p->dest_sdp, SDP_SESSION_LEVEL, 0, i, 1, group_id_1);
> +         sdp_get_group_id(sdp_p->dest_sdp, SDP_SESSION_LEVEL, 0, i, 2, group_id_2);
> +
> +        if (strcpy((char*)dst_mid, group_id_1) == 0) {

strcmp?  Same below.

Comment 26

[Maire Reavy [:mreavy]]

It'd be very useful to break this down into smaller bugs.

Comment 27

[:shell escalante]

could break in to SDP parsing and generation (p=7) & config of media conduits (p=3)... makes for smaller bits... not Adam for assignee.
Part 1: Byron did in 786234

Comment 28

[:shell escalante]

created new bug for configuring media conduits 1016476

Comment 29

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Hi Jason, are you still going to handle the testing for this feature?

Comment 30

[Jason Smith [:jsmith]]

No.

Comment 31

[Maire Reavy [:mreavy]]

Nils (:drno) is QA lead for testing this

Comment 32

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Maire Reavy [:mreavy] (Please needinfo me) from comment #31)
> Nils (:drno) is QA lead for testing this

Adding Nils as the QA contact on this then. Thanks Maire.

Comment 33

[Byron Campen [:bwc]]

I am going to roll the rest of the unified plan stuff (ssrc attribute support, and population of things like ssrcs, bundle stuff, and the rtp correlator) into this bug.

Comment 34

[Adam Roach [:abr]]

Byron: Awesome, thanks. Hit me up for a beer in Portland. :)

Comment 35

[Byron Campen [:bwc]]

Patches in bug 786234 handled this.