Closed Bug 810220 Opened 12 years ago Closed 12 years ago

WebRTC setLocalDescription fails when offer has only G.711 codec

Categories

(Core :: WebRTC, defect, P1)

Product:
Core
Component:
WebRTC
Version:
19 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20

People

(Reporter: deeppai, Assigned: abr)

Details

(Whiteboard: [WebRTC][blocking-webrtc+][qa-])

Attachments

(1 file, 9 obsolete files)

Patch to fix SDP Codec Negotiation Issues
45.93 KB, patch
jesup
: checkin+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 Build ID: 20121108030652 Steps to reproduce: Firefox Nightly v19.0a1 Step 1: The following SDP offer was given to setRemoteDescription(): v=0 o=- 1 1 IN IP4 148.147.200.251 s=- b=AS:64 t=0 0 a=fingerprint:sha-256 F3:FA:20:C0:CD:48:C4:5F:02:5F:A5:D3:21:D0:2D:48:7B:31:60:5C:5A:D8:0D:CD:78:78:6C:6D:CE:CC:0C:67 m=audio 9000 RTP/AVP 0 126 c=IN IP4 148.147.200.251 b=TIAS:64000 a=rtpmap:0 PCMU/8000 a=rtpmap:126 telephone-event/8000 a=candidate:0 1 udp 2130706432 148.147.200.251 9000 typ host a=candidate:0 2 udp 2130706432 148.147.200.251 9005 typ host a=ice-ufrag:cYuakxkEKH+RApYE a=ice-pwd:bwtpzLZD+3jbu8vQHvEa6Xuq a=sendrecv Step 2: The setRemoteDescription() returned successfully. The createAnswer() success response returned the following SDP: v=0 o=Mozilla-SIPUA 16927 0 IN IP4 0.0.0.0 s=SIP Call t=0 0 a=ice-ufrag:2cb75440 a=ice-pwd:8255b062313d87f10d487f6bad194395 a=fingerprint:sha-256 6E:21:2A:05:44:E3:BE:F5:20:92:73:49:6B:CC:12:E9:65:61:2F:6F:4B:71:2D:E2:82:D6:8C:F3:F4:B1:1D:C7 m=audio 52183 RTP/AVP 109 126 c=IN IP4 148.147.194.209 a=rtpmap:109 opus/48000 a=ptime:20 a=rtpmap:126 telephone-event/8000 a=fmtp:126 0-15 a=sendrecv a=candidate:0 1 UDP 2111832319 148.147.194.209 52183 typ host a=candidate:0 2 UDP 2111832318 148.147.194.209 52184 typ host m=application 0 SCTP/DTLS 0 c=IN IP4 148.147.194.209 a=fmtp:5000 protocol=webrtc-datachannel;streams=16 a=sendrecv a=candidate:0 1 UDP 2111832319 148.147.194.209 52185 typ host a=candidate:0 2 UDP 2111832318 148.147.194.209 52186 typ host Step 3: The returned SDP was given to setLocalDescription(). Actual results: setLocalDescription failed. The error call back was called with argument as "6". Expected results: The answer generated by Firefox is incorrect. The offer has only one "m" line for audio and G.711 as the codec. The answer has two "m" lines for audio and data and the codec is opus.
Priority: -- → P2
Whiteboard: [WebRTC][blocking-webrtc+]
Priority: P2 → P1
Assignee: nobody → adam
Status: UNCONFIRMED → NEW
Ever confirmed: true
This is impossible to reproduce at the moment, until we take care of the codec negotiation failure that is causing (at least part of) bug 814038.
Okay with the patch on bug 814038, I can now partially reproduce this bug. The answer doesn't contain an application part any more; I think this was fixed by an earlier bug's resolution (I can't find it on a casual examination of WebRTC bugs). The issue of being offered PCMU and responding with OPUS remains.
For the purpose of tracking the lineage of the related code, this bug appears to have been introduced as part of the fix to bug 786327. See this changeset for details: http://hg.mozilla.org/users/rjesup_wgate.com/alder_mothball/rev/21210df44d6c
Attachment #684241 - Flags: review?(ekr)
The issue here is due to not propagating media payload list generated to the SDP update procedure after negotiation. I have a hot-patch that should fix 2 problems 1. update proper payloads and payload types thus fixing this issue 2. Update appropriate dynamic payload type value based on the ones obtained from the remote side. say, we offer OPUS 111 and remote should generate OPUS as 111 for send-recv calls atleast. This patch needs some more clean-up since it was written quickly to test the logic. I would like someone other than me to test it and confirm if it does fix the above problems. I plan to cleanup the patch once the functionality is verified.
Attachment #685082 - Flags: review?(ethanhugg)
Attachment #685082 - Flags: review?(ekr)
What is the relationship between Adam's and Suhas's patches?
(In reply to Eric Rescorla from comment #7) > What is the relationship between Adam's and Suhas's patches? On a casual examination, I believe mine solves the issue identified in this bug without changing overall behavior, while Suhas' appears to be more of a step towards addressing Bug 814227. Derf had some questions about whether we actually want multiple codecs advertised in a response; we might want to discuss the ramifications before we move forward with a change in behavior.
My patch is a fix to bug that got exposed as result of Bug786327 which was supposed to solve the point (1) below. 1. Handle multiple codec negotiation and configuration on recv side as part of SDP negotiation 2. Handle dynamic generation of codecs on enquiry from GIPS API than using hardcoded list in SIPCC 3. Support multiple supported coec generation in Answer than just the first matched codec as it is done today 4. Use codec and media related parameters negotiated in the SDP for configuring send and recv codec on the media conduit. Bug 814227 is related to point 3 above and Bug 806151 is related to point 2 above. All these 4 are required to be complaint to RFC3264. We temporarily stopped at the implementation of (1) above during SIPCC roll-up since that was solving the inter-op scenario (implying higher priority) I personally feel, reverting changes done in Bug786327 will render us void of all the 4 points above. My 2 cents
Comment on attachment 684241 [details] [diff] [review] Fixing codec negotiation to push selected codec back into the media structure for an answer Review of attachment 684241 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c @@ +2949,5 @@ > + // media->payloads[] field. Ultimately, we (probably?) > + // want to remove media->payload altogether in favor of > + // using media->payloads[] instead. See bug 814227. > + media->payload = payload; > + return payload; I think updating the media->payload alone and not updating other related entities like media->remote_dynamic_payload_value will end up reporting wrong payload types if there is a mismatch for the same codec.
Attachment #685082 - Attachment is obsolete: true
Attachment #685082 - Flags: review?(ethanhugg)
Attachment #685082 - Flags: review?(ekr)
Attachment #685393 - Attachment is obsolete: true
This new patch fixes lsm to take the codec configuration from the payloads array rather than the (now defunct?) payload field
Attachment #685394 - Attachment is obsolete: true
Attachment #684241 - Attachment is obsolete: true
Attachment #684241 - Flags: review?(ekr)
Attachment #685756 - Flags: feedback?(snandaku)
Comment on attachment 685756 [details] [diff] [review] Patch to fix SDP Codec Negotiation Issues Review of attachment 685756 [details] [diff] [review]: ----------------------------------------------------------------- Few minor nits and overall comment on how we should handle the Previous SDP negotiation. We can push it to a new bug. Since it doesn't impact much now. ::: media/webrtc/signaling/src/sipcc/core/common/vcm_util.c @@ +146,5 @@ > + break; > + } > + > + return rtp_payload; > + We need to get back to this later. I added this to make things work with the current design. Once we move away from these mapping, this function should eventually go ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c @@ +1951,2 @@ > media->previous_sdp.payload_type = local_media_types[0]; > media->previous_sdp.local_payload_type = local_media_types[0]; We probably need to update previous_sdp structure to have lists of previously negotiated payloads than single payload. It will not hurt this patch , but will definitely hurt updated-offer negotiation @@ +2088,1 @@ > media->previous_sdp.payload_type = video_media_types[0]; same comment as above for previous SDP @@ +2880,5 @@ > remote_dynamic_payload_type_value = GET_DYN_PAYLOAD_TYPE_VALUE(slave_list_p[j]); > } > } else { //if remote SDP is an answer > if (media->local_dynamic_payload_type_value == RTP_NONE || > + !media->num_payloads || media->payloads[0] != media->previous_sdp.payload_type) { the reference to media->local_dynamic_payload_type_value should go away. We need to use the list version though @@ +4809,3 @@ > break; > } > same comment for previous SDP holds here as well ::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c @@ +1000,2 @@ > } > this right hand side list should be media->local_dtp_list[] ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +580,1 @@ > // Create a media stream as if it came from GUM like the idea of defaulting !!! @@ +1337,5 @@ > a2_.SetRemote(TestObserver::OFFER, offer); > > std::cout << "Creating answer:" << std::endl; > + //removing the flags temporarily since OPUS and VP8 will have > + // remote payload types we can remove this comment ...
Attachment #685756 - Flags: feedback?(snandaku) → feedback+
(In reply to Suhas from comment #15) > @@ +2880,5 @@ > > remote_dynamic_payload_type_value = GET_DYN_PAYLOAD_TYPE_VALUE(slave_list_p[j]); > > } > > } else { //if remote SDP is an answer > > if (media->local_dynamic_payload_type_value == RTP_NONE || > > + !media->num_payloads || media->payloads[0] != media->previous_sdp.payload_type) { > > the reference to media->local_dynamic_payload_type_value should go away. We > need to use the list version though I don't think this can go away yet: all the code in lsm.c (and, AFAICT, almost all of the code in gsm_sdp.c) is reading and writing this into local_dynamic_payload_type_value. By my reading, this is okay, since the only place it really gets used (post negotiation) is to determine which payload number is put in the RTP when we send it. Since we're presently only sending the first codec in the list, we don't gain anything from the array. This needs a bigger refactor to be sure (basically, removing the local_dynamic_payload_type and remote_dynamic_payload_type fields in exactly the same way this patch removes the payload field), but it's not necessary to fix the issue at hand. However, if we change it only halfway (by changing some stuff to use the array and other stuff to use the legacy field), we'll end up in the same kind of mess that this patch was designed to fix. > ::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c > @@ +1000,2 @@ > > } > > > > this right hand side list should be media->local_dtp_list[] I don't think that's correct. The logic for the code here is "if we've selected a codec already but have not assigned a dynamic payload type number to it, copy the codec number into the dynamic payload type number." On reflection, I'm not sure how this logic avoids collisions, but it appears to be legacy code from SIPCC; here's the equivalent ikran code (note the RHS is media->payload): dcb->cur_video_avail &= ~CC_ATTRIB_CAST; if (media->local_dynamic_payload_type_value == RTP_NONE) { media->local_dynamic_payload_type_value = media->payload; } Everything else you've mentioned, I've added a TODO comment in the code. Attaching an updated patch momentarily (after I've re-run unit tests)
Attachment #685756 - Attachment is obsolete: true
Attachment #686858 - Flags: review?(rjesup)
Attachment #686858 - Flags: review?(ekr)
Comment on attachment 686858 [details] [diff] [review] Patch to fix SDP Codec Negotiation Issues Review of attachment 686858 [details] [diff] [review]: ----------------------------------------------------------------- generally lgtm with removing the unit test to a separate patch. ::: media/webrtc/signaling/src/sipcc/core/common/vcm_util.c @@ +118,5 @@ > + rtp_payload = RTP_G722; > + break; > + > + case VCM_Media_Payload_ILBC20: > + case VCM_Media_Payload_ILBC30: What are ILBC20 and ILBC30? Are we destroying information here? ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c @@ +416,5 @@ > media->previous_sdp.direction = SDP_DIRECTION_INACTIVE; > media->previous_sdp.packetization_period = media->packetization_period; > media->previous_sdp.max_packetization_period = media->max_packetization_period; > + media->previous_sdp.payload_type = > + media->num_payloads?media->payloads[0]:RTP_NONE; Can you put spaces around ?: pls? @@ +1946,5 @@ > + a codec other than the first one changing. */ > + if (dcb_p && media && media->num_payloads == 0) { > + if (!media->payloads) { > + media->payloads = (vcm_media_payload_type_t*) > + cpr_malloc(1 * sizeof(vcm_media_payload_type_t)); I generally think 1* is superfluous. Why do you have it? @@ +1950,5 @@ > + cpr_malloc(1 * sizeof(vcm_media_payload_type_t)); > + media->local_dpt_list = (int32_t*) cpr_malloc(1 * sizeof(int32_t)); > + media->remote_dpt_list = (int32_t*) cpr_malloc(1 * sizeof(int32_t)); > + } > + media->payloads[media->num_payloads++] = local_media_types[0]; Isn't media->num_payloads always 0? Maybe just media->num_payloads = 1 below. @@ +2353,4 @@ > */ > + for(i=0; i < media->num_payloads; i++) { > + //get the PT value > + if (media->remote_dpt_list[i] > 0) { Isn't 0 a valid payload type? I would prefer to have a specifically invalid value. Can you comment what the heck this test is doing. @@ +2370,5 @@ > + } > + > + switch (media->type) { > + case SDP_MEDIA_AUDIO: > + gsmsdp_set_media_attributes(mediaPayloadToVcmRtp(media->payloads[i]), sdp_p, level, 80 columns please. @@ +2843,5 @@ > > /* > * Save pay load type for use by gsmspd_compare_to_previous_sdp > */ > + media->previous_sdp.payload_type = Spaces around ?: @@ +2856,5 @@ > payload_types_count = num_master_types; > } else { > payload_types_count = num_slave_types; > } > + //Allocate memory for PT value, local PT and remote PT. This looks a lot more like freeing than allocating. @@ +2866,3 @@ > media->payloads = (vcm_media_payload_type_t*) cpr_malloc(payload_types_count * sizeof(vcm_media_payload_type_t)); > + media->local_dpt_list = (int32_t*) cpr_malloc(payload_types_count * sizeof(int32_t)); > + media->remote_dpt_list = (int32_t*) cpr_malloc(payload_types_count * sizeof(int32_t)); Should we prefer calloc? @@ +2867,5 @@ > + media->local_dpt_list = (int32_t*) cpr_malloc(payload_types_count * sizeof(int32_t)); > + media->remote_dpt_list = (int32_t*) cpr_malloc(payload_types_count * sizeof(int32_t)); > + > + if (!(media->payloads) || > + !(media->local_dpt_list) || Isn't cpr_malloc() now infallible? @@ +4814,5 @@ > media->previous_sdp.avt_payload_type = media->avt_payload_type; > media->previous_sdp.direction = media->direction; > media->previous_sdp.packetization_period = media->packetization_period; > + media->previous_sdp.payload_type = > + media->num_payloads?media->payloads[0]:RTP_NONE; spaces around ?: here and below. ::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h @@ +145,5 @@ > * NOTE: this is to support asymmetric payload type values for a given dynamic payload type. > * We answer with the same payload type value that the remote offers. > * If remote choses to answer with different value than we offer, we support asymmetric. > */ > +// int32_t payload; //payload type - one of rtp_ptype enumerations Remove this line please. @@ +246,5 @@ > */ > vcm_media_payload_type_t* payloads; > int32_t num_payloads; > + int32_t* local_dpt_list; // dynamic payload type value offered/answered by us > + int32_t* remote_dpt_list; // dynamic payload type value offered/answered by remote This code seems to use c-style comments. How long are these lists? num_payloads? Some more comments are needed. Why int32_t? Can they be signed? ::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c @@ +994,5 @@ > } > pc_track_id = 0; > dcb->cur_video_avail &= ~CC_ATTRIB_CAST; > if (media->local_dynamic_payload_type_value == RTP_NONE) { > + media->local_dynamic_payload_type_value = Spaces around ?: @@ +1015,5 @@ > } else if (!sdpmode) { > ret_val = vcmRxStart(media->cap_index, group_id, media->refid, > lsm_get_ms_ui_call_handle(dcb->line, call_id, CC_NO_CALL_ID), > + // This isn't 100% correct, but it > + // doesn't impact WebRTC code How is it not correct. Please adjust comment. @@ +1261,5 @@ > dcb->media_cap_tbl->cap[media->cap_index].pc_stream, > dcb->media_cap_tbl->cap[media->cap_index].pc_track, > lsm_get_ms_ui_call_handle(dcb->line, call_id, CC_NO_CALL_ID), > dcb->peerconnection, > + media->num_payloads?media->payloads[0]:RTP_NONE, Where is the vcmRtpToMediaPayload xlation happening ::: media/webrtc/signaling/test/offer_answer.cpp @@ +45,5 @@ > +}; > + > +namespace test { > + > +class TestObserver : public IPeerConnectionObserver Can we refactor the common code from signaling_unittest.cpp? Suggest that we just add this test in a separate CL. ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +1249,5 @@ > DONT_CHECK_AUDIO | DONT_CHECK_VIDEO); > > std::string answer = a2_.answer(); > + > + /* They didn't offer opus, so our answer shouldn't include it. */ C++-style comments. @@ +1267,4 @@ > } > > TEST_F(SignalingTest, Bug814038) > { I would prefer names that were more evocative with Bug #s in the comments,
Attachment #686858 - Flags: review?(ekr) → review+
Comment on attachment 686858 [details] [diff] [review] Patch to fix SDP Codec Negotiation Issues Review of attachment 686858 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c @@ +416,5 @@ > media->previous_sdp.direction = SDP_DIRECTION_INACTIVE; > media->previous_sdp.packetization_period = media->packetization_period; > media->previous_sdp.max_packetization_period = media->max_packetization_period; > + media->previous_sdp.payload_type = > + media->num_payloads?media->payloads[0]:RTP_NONE; Pretty please ::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h @@ +246,5 @@ > */ > vcm_media_payload_type_t* payloads; > int32_t num_payloads; > + int32_t* local_dpt_list; // dynamic payload type value offered/answered by us > + int32_t* remote_dpt_list; // dynamic payload type value offered/answered by remote Payload values are uint8_t's generally (actually 7 bits in RTP). Unless we're using negative values as a flag, or storing other info in there, I'd prefer to use uint8_t. And in C code, /* */ please unless the file already uses // and is c99 or what-have-you. Though I'll admit that just in .h files in signaling there are >2400 // comments already, so it's a bit moot.
Attachment #686858 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #19) > Comment on attachment 686858 [details] [diff] [review] > Patch to fix SDP Codec Negotiation Issues > > Review of attachment 686858 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c > @@ +416,5 @@ > > media->previous_sdp.direction = SDP_DIRECTION_INACTIVE; > > media->previous_sdp.packetization_period = media->packetization_period; > > media->previous_sdp.max_packetization_period = media->max_packetization_period; > > + media->previous_sdp.payload_type = > > + media->num_payloads?media->payloads[0]:RTP_NONE; > > Pretty please I don't know what to do with that comment. > ::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h > @@ +246,5 @@ > > */ > > vcm_media_payload_type_t* payloads; > > int32_t num_payloads; > > + int32_t* local_dpt_list; // dynamic payload type value offered/answered by us > > + int32_t* remote_dpt_list; // dynamic payload type value offered/answered by remote > > Payload values are uint8_t's generally (actually 7 bits in RTP). Unless > we're using negative values as a flag, or storing other info in there, I'd > prefer to use uint8_t. Agree -- changing to uint8_t. > And in C code, /* */ please unless the file already uses // and is c99 or > what-have-you. Though I'll admit that just in .h files in signaling there > are >2400 // comments already, so it's a bit moot. Fixed.
(In reply to Adam Roach [:abr] from comment #20) > > > > Pretty please > > I don't know what to do with that comment. Never mind -- it's clearer in the splinter. I've fixed the :? spacing everywhere (including a couple that weren't in this patch, just because it was easier to do than fixing only my own).
Comment on attachment 686858 [details] [diff] [review] Patch to fix SDP Codec Negotiation Issues Review of attachment 686858 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sipcc/core/common/vcm_util.c @@ +118,5 @@ > + rtp_payload = RTP_G722; > + break; > + > + case VCM_Media_Payload_ILBC20: > + case VCM_Media_Payload_ILBC30: What's being lost here isn't used at this point -- all the parameters are hardcoded in vcmPayloadType2AudioCodec. In any case, this translation all goes away when Bug 817065 lands. ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c @@ +416,5 @@ > media->previous_sdp.direction = SDP_DIRECTION_INACTIVE; > media->previous_sdp.packetization_period = media->packetization_period; > media->previous_sdp.max_packetization_period = media->max_packetization_period; > + media->previous_sdp.payload_type = > + media->num_payloads?media->payloads[0]:RTP_NONE; Ack. Fixed here and everywhere else. @@ +1946,5 @@ > + a codec other than the first one changing. */ > + if (dcb_p && media && media->num_payloads == 0) { > + if (!media->payloads) { > + media->payloads = (vcm_media_payload_type_t*) > + cpr_malloc(1 * sizeof(vcm_media_payload_type_t)); It's here as a bit of a reminder that we're allocating something that is used as an array of length 1, rather than just a pointer. There's no language distinction between the two, of course, which is why I coded it like this. I'm happy to take it out if that explanation doesn't justify the way it's coded to your satisfaction. @@ +1950,5 @@ > + cpr_malloc(1 * sizeof(vcm_media_payload_type_t)); > + media->local_dpt_list = (int32_t*) cpr_malloc(1 * sizeof(int32_t)); > + media->remote_dpt_list = (int32_t*) cpr_malloc(1 * sizeof(int32_t)); > + } > + media->payloads[media->num_payloads++] = local_media_types[0]; Ack. Changed per suggestion. @@ +2353,4 @@ > */ > + for(i=0; i < media->num_payloads; i++) { > + //get the PT value > + if (media->remote_dpt_list[i] > 0) { Yes, it is. And the consequence of that fact is: if the remote side is using 0 as their payload type, we'll use our own number for the same payload (which may well be 0 also) rather than trying to match theirs. I've found the additional work to try to use their dynamic payload numbers to be a bit of an odd thing: there are virtually no gains from doing so. @@ +2856,5 @@ > payload_types_count = num_master_types; > } else { > payload_types_count = num_slave_types; > } > + //Allocate memory for PT value, local PT and remote PT. I've moved the comment to where allocation actually takes place. @@ +2866,3 @@ > media->payloads = (vcm_media_payload_type_t*) cpr_malloc(payload_types_count * sizeof(vcm_media_payload_type_t)); > + media->local_dpt_list = (int32_t*) cpr_malloc(payload_types_count * sizeof(int32_t)); > + media->remote_dpt_list = (int32_t*) cpr_malloc(payload_types_count * sizeof(int32_t)); Sure, changed everywhere in this patch. @@ +2867,5 @@ > + media->local_dpt_list = (int32_t*) cpr_malloc(payload_types_count * sizeof(int32_t)); > + media->remote_dpt_list = (int32_t*) cpr_malloc(payload_types_count * sizeof(int32_t)); > + > + if (!(media->payloads) || > + !(media->local_dpt_list) || I've taken this check out; :ehugg and :jesup agree that such checks are superfluous. ::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h @@ +145,5 @@ > * NOTE: this is to support asymmetric payload type values for a given dynamic payload type. > * We answer with the same payload type value that the remote offers. > * If remote choses to answer with different value than we offer, we support asymmetric. > */ > +// int32_t payload; //payload type - one of rtp_ptype enumerations Ack @@ +246,5 @@ > */ > vcm_media_payload_type_t* payloads; > int32_t num_payloads; > + int32_t* local_dpt_list; // dynamic payload type value offered/answered by us > + int32_t* remote_dpt_list; // dynamic payload type value offered/answered by remote Fixed element size, comment style. Added comment about list sizes. ::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c @@ +1015,5 @@ > } else if (!sdpmode) { > ret_val = vcmRxStart(media->cap_index, group_id, media->refid, > lsm_get_ms_ui_call_handle(dcb->line, call_id, CC_NO_CALL_ID), > + // This isn't 100% correct, but it > + // doesn't impact WebRTC code RTP_NONE is the wrong type, but it should never fire (payload_count really should never be zero here -- it's just a double-check against null deref). In any case, this goes away when Bug 817065 lands. @@ +1261,5 @@ > dcb->media_cap_tbl->cap[media->cap_index].pc_stream, > dcb->media_cap_tbl->cap[media->cap_index].pc_track, > lsm_get_ms_ui_call_handle(dcb->line, call_id, CC_NO_CALL_ID), > dcb->peerconnection, > + media->num_payloads?media->payloads[0]:RTP_NONE, The biggest change in the part of this patch that :crypt wrote is that the payloads[] array no longer contains RTP payload types; it contains the vcm_media_payload_t type already (no translation necessary), albeit with the high 16 bits used to represent the dynamic payload type in use. I can't explain the logic behind going one direction versus the other; but, in any case, this parameter changes types completely when Bug 817065 lands. ::: media/webrtc/signaling/test/offer_answer.cpp @@ +45,5 @@ > +}; > + > +namespace test { > + > +class TestObserver : public IPeerConnectionObserver Agree that we need to clean this up -- I've removed it from the patch set. ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +1249,5 @@ > DONT_CHECK_AUDIO | DONT_CHECK_VIDEO); > > std::string answer = a2_.answer(); > + > + /* They didn't offer opus, so our answer shouldn't include it. */ Done. @@ +1267,4 @@ > } > > TEST_F(SignalingTest, Bug814038) > { Fixed.
Comment on attachment 686858 [details] [diff] [review] Patch to fix SDP Codec Negotiation Issues Review of attachment 686858 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c @@ +2353,4 @@ > */ > + for(i=0; i < media->num_payloads; i++) { > + //get the PT value > + if (media->remote_dpt_list[i] > 0) { I should note that if we care about interoperating with non-WebRTC endpoints (without requiring a lot of translation in the gateway): there are many such that fail miserably if you don't use (in the answer) the same (usually dynamic) payload value they offered you. (Static payloads are normally the same since few people do the trick of redefining them to be something else.) If we don't care (and we may not), then fine - we're spec-compliant. SIPCC/signaling may care is people use it in the future for general SIP work.
Comment on attachment 686858 [details] [diff] [review] Patch to fix SDP Codec Negotiation Issues Review of attachment 686858 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/sipcc/core/common/vcm_util.c @@ +118,5 @@ > + rtp_payload = RTP_G722; > + break; > + > + case VCM_Media_Payload_ILBC20: > + case VCM_Media_Payload_ILBC30: What's being lost here isn't used at this point -- all the parameters are hardcoded in vcmPayloadType2AudioCodec. In any case, this translation all goes away when Bug 817065 lands. ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c @@ +416,5 @@ > media->previous_sdp.direction = SDP_DIRECTION_INACTIVE; > media->previous_sdp.packetization_period = media->packetization_period; > media->previous_sdp.max_packetization_period = media->max_packetization_period; > + media->previous_sdp.payload_type = > + media->num_payloads?media->payloads[0]:RTP_NONE; Ack. Fixed here and everywhere else. @@ +1946,5 @@ > + a codec other than the first one changing. */ > + if (dcb_p && media && media->num_payloads == 0) { > + if (!media->payloads) { > + media->payloads = (vcm_media_payload_type_t*) > + cpr_malloc(1 * sizeof(vcm_media_payload_type_t)); This change was overtaken by events -- the switch to calloc eliminated it. @@ +1950,5 @@ > + cpr_malloc(1 * sizeof(vcm_media_payload_type_t)); > + media->local_dpt_list = (int32_t*) cpr_malloc(1 * sizeof(int32_t)); > + media->remote_dpt_list = (int32_t*) cpr_malloc(1 * sizeof(int32_t)); > + } > + media->payloads[media->num_payloads++] = local_media_types[0]; Ack. Changed per suggestion. @@ +2353,4 @@ > */ > + for(i=0; i < media->num_payloads; i++) { > + //get the PT value > + if (media->remote_dpt_list[i] > 0) { Yes, it is. And the consequence of that fact is: if the remote side is using 0 as their payload type, we'll use our own number for the same payload (which may well be 0 also) rather than trying to match theirs. I've found the additional work to try to use their dynamic payload numbers to be a bit of an odd thing: there are virtually no gains from doing so. @@ +2856,5 @@ > payload_types_count = num_master_types; > } else { > payload_types_count = num_slave_types; > } > + //Allocate memory for PT value, local PT and remote PT. I've moved the comment to where allocation actually takes place. @@ +2866,3 @@ > media->payloads = (vcm_media_payload_type_t*) cpr_malloc(payload_types_count * sizeof(vcm_media_payload_type_t)); > + media->local_dpt_list = (int32_t*) cpr_malloc(payload_types_count * sizeof(int32_t)); > + media->remote_dpt_list = (int32_t*) cpr_malloc(payload_types_count * sizeof(int32_t)); Sure, changed everywhere in this patch. @@ +2867,5 @@ > + media->local_dpt_list = (int32_t*) cpr_malloc(payload_types_count * sizeof(int32_t)); > + media->remote_dpt_list = (int32_t*) cpr_malloc(payload_types_count * sizeof(int32_t)); > + > + if (!(media->payloads) || > + !(media->local_dpt_list) || I've taken this check out; :ehugg and :jesup agree that such checks are superfluous. ::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h @@ +145,5 @@ > * NOTE: this is to support asymmetric payload type values for a given dynamic payload type. > * We answer with the same payload type value that the remote offers. > * If remote choses to answer with different value than we offer, we support asymmetric. > */ > +// int32_t payload; //payload type - one of rtp_ptype enumerations Ack @@ +246,5 @@ > */ > vcm_media_payload_type_t* payloads; > int32_t num_payloads; > + int32_t* local_dpt_list; // dynamic payload type value offered/answered by us > + int32_t* remote_dpt_list; // dynamic payload type value offered/answered by remote Fixed element size, comment style. Added comment about list sizes. ::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c @@ +1015,5 @@ > } else if (!sdpmode) { > ret_val = vcmRxStart(media->cap_index, group_id, media->refid, > lsm_get_ms_ui_call_handle(dcb->line, call_id, CC_NO_CALL_ID), > + // This isn't 100% correct, but it > + // doesn't impact WebRTC code RTP_NONE is the wrong type, but it should never fire (payload_count really should never be zero here -- it's just a double-check against null deref). In any case, this goes away when Bug 817065 lands. @@ +1261,5 @@ > dcb->media_cap_tbl->cap[media->cap_index].pc_stream, > dcb->media_cap_tbl->cap[media->cap_index].pc_track, > lsm_get_ms_ui_call_handle(dcb->line, call_id, CC_NO_CALL_ID), > dcb->peerconnection, > + media->num_payloads?media->payloads[0]:RTP_NONE, The biggest change in the part of this patch that :crypt wrote is that the payloads[] array no longer contains RTP payload types; it contains the vcm_media_payload_t type already (no translation necessary), albeit with the high 16 bits used to represent the dynamic payload type in use. I can't explain the logic behind going one direction versus the other; but, in any case, this parameter changes types completely when Bug 817065 lands. ::: media/webrtc/signaling/test/offer_answer.cpp @@ +45,5 @@ > +}; > + > +namespace test { > + > +class TestObserver : public IPeerConnectionObserver Agree that we need to clean this up -- I've removed it from the patch set. ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +1249,5 @@ > DONT_CHECK_AUDIO | DONT_CHECK_VIDEO); > > std::string answer = a2_.answer(); > + > + /* They didn't offer opus, so our answer shouldn't include it. */ Done. @@ +1267,4 @@ > } > > TEST_F(SignalingTest, Bug814038) > { Fixed.
(In reply to Randell Jesup [:jesup] from comment #23) > Comment on attachment 686858 [details] [diff] [review] > Patch to fix SDP Codec Negotiation Issues > > Review of attachment 686858 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c > @@ +2353,4 @@ > > */ > > + for(i=0; i < media->num_payloads; i++) { > > + //get the PT value > > + if (media->remote_dpt_list[i] > 0) { > > I should note that if we care about interoperating with non-WebRTC endpoints > (without requiring a lot of translation in the gateway): there are many such > that fail miserably if you don't use (in the answer) the same (usually > dynamic) payload value they offered you. > > (Static payloads are normally the same since few people do the trick of > redefining them to be something else.) That's part of the explanation I left out: most everyone uses 0 to represent PCMU, as do we. The only way this could cause any trouble is if the remote side used 0 to represent some codec *other* than PCMU, and then we turn around and use a different dynamic type to represent that same codec. I seriously doubt you're going to see any of the implementations that are so fragile with regards to symmetric type numbers using 0 to mean something other than PCMU.
Attachment #686858 - Attachment is obsolete: true
Attachment #687299 - Attachment is obsolete: true
I'm attaching this to ease review of the changes I made in response to review comments. It contains only the diffs between the previous patch and this patch.
(In reply to Adam Roach [:abr] from comment #25) > > (Static payloads are normally the same since few people do the trick of > > redefining them to be something else.) > > That's part of the explanation I left out: most everyone uses 0 to represent > PCMU, as do we. The only way this could cause any trouble is if the remote > side used 0 to represent some codec *other* than PCMU, and then we turn > around and use a different dynamic type to represent that same codec. I > seriously doubt you're going to see any of the implementations that are so > fragile with regards to symmetric type numbers using 0 to mean something > other than PCMU. OT, but I've seen payload 0 redefined. Silly, but true (and probably a mistake).
Comment on attachment 687300 [details] [diff] [review] Patch to fix SDP Codec Negotiation Issues Review of attachment 687300 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #687300 - Flags: review+
Attachment #687304 - Attachment is obsolete: true
Attachment #687300 - Flags: checkin?(rjesup)
https://hg.mozilla.org/integration/mozilla-inbound/rev/483a8a9702a6
OS: Windows 7 → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla20
Build fails for me (linux x86_64, gcc-4.7.2) with /sources/comm-central/mozilla/media/webrtc/signaling/src/sipcc/core/gsm/lsm.c: In function ‘lsm_update_media’: /sources/comm-central/mozilla/media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:3863:13: error: ‘for’ loop initial declarations are only allowed in C99 mode /sources/comm-central/mozilla/media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:3863:13: note: use option -std=c99 or -std=gnu99 to compile your code make[9]: *** [src/sipcc/core/gsm/lsm.o] Error 1 after this patch. Should I file new bug or this will be reverted?
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/79694053176f Please do a try run (at least debug/opt builds for all platforms) You can see the errors here: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=483a8a9702a6
Attachment #687300 - Attachment is obsolete: true
Attachment #687300 - Flags: checkin?(rjesup)
New patch is (relatively) clean on try server: https://tbpl.mozilla.org/?tree=Try&rev=7dba854bc2e3&onlyunstarred=1 The Linux and OS X failures appear to have open bugs already associated with them. The XP debug failure looks like a problem with the automation tools (there's a leak stats line followed by an error indicating that a leak stats line can't be found). The Android shutdown crash appears to be an issue with cert handling. While there's a remote chance that memory corruption inside media could cause this, the chances of such being caused by this patch seem vanishingly unlikely.
Attachment #687552 - Flags: checkin?(rjesup)
Attachment #687552 - Flags: checkin?(rjesup) → checkin+
https://hg.mozilla.org/mozilla-central/rev/93f45a98874c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: verifyme
Keywords: verifyme
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc+][qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: