SDP handling should support Bundle - parsing & generation work

RESOLVED DUPLICATE of bug 786234

Status

()

Core
WebRTC: Signaling
P2
normal
RESOLVED DUPLICATE of bug 786234
6 years ago
3 years ago

People

(Reporter: ehugg, Assigned: bwc)

Tracking

({feature})

Trunk
mozilla35
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [meta][leave-open] [WebRTC] [blocking-webrtc-], p=7)

Attachments

(2 attachments, 10 obsolete attachments)

(Reporter)

Description

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

a=group:BUNDLE audio video

Updated

6 years ago
Whiteboard: [WebRTC], [blocking-webrtc+]
Created attachment 662299 [details] [diff] [review]
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.
Created attachment 671063 [details] [diff] [review]
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
Attachment #662299 - Attachment is obsolete: true
Attachment #671063 - Flags: feedback?(ethanhugg)
Attachment #671063 - Flags: feedback?(ekr)
Created attachment 671089 [details] [diff] [review]
Add Bundled media to SIPCC SDP
Attachment #671063 - Attachment is obsolete: true
Attachment #671063 - Flags: feedback?(ethanhugg)
Attachment #671063 - Flags: feedback?(ekr)
Attachment #671089 - Flags: feedback?(ethanhugg)
Created attachment 671469 [details] [diff] [review]
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.
Attachment #671089 - Attachment is obsolete: true
Attachment #671089 - Flags: feedback?(ethanhugg)
(Reporter)

Comment 5

6 years ago
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.
Created attachment 672240 [details] [diff] [review]
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.
Attachment #671469 - Attachment is obsolete: true
Attachment #672240 - Flags: review?(ethanhugg)
Attachment #672240 - Flags: review?(ekr)
Assignee: nobody → emannion
Created attachment 673170 [details] [diff] [review]
Add Bundled media to SIPCC SDP

Re-based ontop of new constraints patch
Attachment #672240 - Attachment is obsolete: true
Attachment #672240 - Flags: review?(ethanhugg)
Attachment #672240 - Flags: review?(ekr)
Attachment #673170 - Flags: review?(ekr)
Attachment #673170 - Flags: feedback?(ethanhugg)
(Reporter)

Comment 8

6 years ago
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.
(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.
(Reporter)

Comment 10

6 years ago
 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.
Created attachment 674023 [details] [diff] [review]
Add Bundled media to SIPCC SDP
Attachment #673170 - Attachment is obsolete: true
Attachment #673170 - Flags: review?(ekr)
Attachment #673170 - Flags: feedback?(ethanhugg)
Attachment #674023 - Flags: review?(ekr)
Attachment #674023 - Flags: feedback?(ethanhugg)

Updated

6 years ago
Depends on: 786234
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.)
Assignee: emannion → ethanhugg
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc-]
(Reporter)

Updated

6 years ago
Attachment #674023 - Attachment is obsolete: true
Attachment #674023 - Flags: review?(ekr)
Attachment #674023 - Flags: feedback?(ethanhugg)
(Reporter)

Updated

5 years ago
Assignee: ethanhugg → adam
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.
Target Milestone: --- → mozilla28

Comment 14

5 years ago
Created attachment 810766 [details] [diff] [review]
Part 1: Add constraints to addStream

Comment 15

5 years ago
Created attachment 817449 [details] [diff] [review]
Part 1: Add constraints to addStream

Updated

5 years ago
Attachment #810766 - Attachment is obsolete: true

Comment 16

5 years ago
Created attachment 819987 [details] [diff] [review]
[WIP] Part 2: Bundle SDP handling (unrotting old patch)

Comment 17

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

Comment 18

5 years ago
Created attachment 8348811 [details] [diff] [review]
Part 1: Add constraints to addStream

Updated

5 years ago
Attachment #817449 - Attachment is obsolete: true

Updated

5 years ago
Attachment #8348811 - Flags: review?(jib)
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?
Attachment #8348811 - Flags: review?(jib) → review+

Comment 20

5 years ago
(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

5 years ago
Created attachment 8349586 [details] [diff] [review]
Part 1: Add constraints to addStream

Updated

5 years ago
Attachment #8348811 - Attachment is obsolete: true

Comment 22

5 years ago
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+
Attachment #8349586 - Flags: review+

Updated

5 years ago
Whiteboard: [WebRTC], [blocking-webrtc-] → [leave-open] [WebRTC] [blocking-webrtc-]

Updated

5 years ago
Keywords: feature
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.

Updated

5 years ago
Blocks: 970426

Updated

4 years ago
No longer blocks: 970426

Updated

4 years ago
Priority: -- → P1
Target Milestone: mozilla28 → mozilla33
It'd be very useful to break this down into smaller bugs.
Whiteboard: [leave-open] [WebRTC] [blocking-webrtc-] → [meta][leave-open] [WebRTC] [blocking-webrtc-]

Comment 27

4 years ago
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

Updated

4 years ago
Blocks: 1016476

Comment 28

4 years ago
created new bug for configuring media conduits 1016476

Updated

4 years ago
Whiteboard: [meta][leave-open] [WebRTC] [blocking-webrtc-] → [meta][leave-open] [WebRTC] [blocking-webrtc-], p=7

Updated

4 years ago
Summary: SDP handling should support Bundle → SDP handling should support Bundle - parsing & generation work
Priority: P1 → P2
Hi Jason, are you still going to handle the testing for this feature?
Flags: needinfo?(jsmith)
No.
Flags: needinfo?(jsmith)
Nils (:drno) is QA lead for testing this
(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.
QA Contact: jsmith → drno
Target Milestone: mozilla33 → mozilla35

Updated

4 years ago
Depends on: 1080765
(Assignee)

Updated

4 years ago
Assignee: adam → docfaraday
Depends on: 1091242
(Assignee)

Comment 33

4 years ago
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

4 years ago
Byron: Awesome, thanks. Hit me up for a beer in Portland. :)
(Assignee)

Comment 35

4 years ago
Patches in bug 786234 handled this.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 786234
You need to log in before you can comment on or make changes to this bug.