Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(8)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 10002: Rollup of SIPCC changes: gsm

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 3 months ago by ekr-webrtc
Modified:
8 years, 3 months ago
Visibility:
Public.

Description

Rollup of SIPCC changes: gsm

Patch Set 1 #

Total comments: 75
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M media/webrtc/signaling/src/sipcc/core/gsm/ccapi.c View 6 chunks +330 lines, -8 lines 1 comment Download
M media/webrtc/signaling/src/sipcc/core/gsm/dcsm.c View 2 chunks +2 lines, -2 lines 0 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/fim.c View 3 chunks +15 lines, -4 lines 3 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/fsmcac.c View 1 chunk +1 line, -1 line 0 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c View 89 chunks +969 lines, -93 lines 19 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/gsm.c View 5 chunks +4 lines, -4 lines 0 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c View 47 chunks +909 lines, -95 lines 26 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp_crypto.c View 9 chunks +54 lines, -16 lines 2 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h View 7 chunks +61 lines, -1 line 0 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/h/gsm_sdp.h View 2 chunks +14 lines, -1 line 0 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/h/lsm.h View 3 chunks +5 lines, -0 lines 0 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/lsm.c View 26 chunks +304 lines, -132 lines 23 comments Download
M media/webrtc/signaling/src/sipcc/core/gsm/media_cap_tbl.c View 1 chunk +2 lines, -1 line 1 comment Download
M media/webrtc/signaling/src/sipcc/core/gsm/subapi.c View 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 3
ethanhugg
https://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src/sipcc/core/gsm/ccapi.c File media/webrtc/signaling/src/sipcc/core/gsm/ccapi.c (right): https://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src/sipcc/core/gsm/ccapi.c#newcode1217 media/webrtc/signaling/src/sipcc/core/gsm/ccapi.c:1217: cc_createoffer (cc_srcs_t src_id, cc_srcs_t dst_id, callid_t call_id, This section ...
8 years, 3 months ago #1
ekr-webrtc
This needs some attention. http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src/sipcc/core/gsm/fim.c File media/webrtc/signaling/src/sipcc/core/gsm/fim.c (right): http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src/sipcc/core/gsm/fim.c#newcode439 media/webrtc/signaling/src/sipcc/core/gsm/fim.c:439: Whitespace only change. Also tabs. ...
8 years, 3 months ago #2
emannion
8 years, 3 months ago #3 
http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
File media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c (right):

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:198: *                
Instead of providing events for all states
Not done in this patch. Added a bug for this. Bug 797516

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> We are going to need to do this sometime soon after landing.
> 
> Note: I have not checked that these tables are filled in correctly.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:3021: part->content_id =
NULL;
Removed this code as spec now does not have an SDP param to CreateAnswer.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Why does this stuff not appear in createoffer?

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:3067: want to set that
conditionally. */
I don't understand what this comment in the code means.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> This seems like a TODO that should be for enda.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:3121: 
Done.
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Why blank line here?

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:3245: part->content_id =
NULL;
There is some commonality but the UI methods need to know if they are called
from setLocalDesc or setRemoteDesc and this would make the common method quite
large.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Why is this here and not in setlocaldesc.
> 
> Can we refactor this function and setlocal so that the media startup code is
> common.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:3245: part->content_id =
NULL;
setLocalDesc does not do anything with the inputed SDP yet, if it does this body
of code can be shared with setRemoteDesc also.

The media startup code for both functions has differences when calling the UI
functions on error making a combined function larger than may be useful.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Why is this here and not in setlocaldesc.
> 
> Can we refactor this function and setlocal so that the media startup code is
> common.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:3329:
FSM_DEBUG_SM(DEB_F_PREFIX"Entered.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
Maybe a little big for this patch.  Added bug Bug 797534.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> This needs a TODO, since it's not done.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:3356:
FSM_DEBUG_SM(DEB_F_PREFIX"Entered.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
Need to be in a separate patch. Added Bug 797534

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> This needs a TODO, since it's not done.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:3414: /*
cpr_assert(strlen(msg->data.pc.pc_handle) < PC_HANDLE_SIZE); */
Done.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> I would like to bring this assert back, since it's a programming error that
will
> cause a system fialure.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:3452:
dcb->media_cap_tbl->cap[CC_VIDEO_1].enabled = TRUE;
The constraints patch has some changes in this code also.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Make sure that these interact properly with constraints. ISTM that if you had
> constraints in the PC ctor that this would be stomping them,

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
File media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c (right):

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:1595:
sstrncat(hash_and_fingerprint, (cc_string_t)fingerprint, FSMDEF_MAX_DIGEST_LEN);
Did not get to this yet. Added but to capture this. Bug 797512 in bugzilla.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Replace this with snprintf.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:2855: if (media->payload ==
RTP_OPUS) {
These settings were new to SIPCC from the OPUS SDP spec and were not in
existance in SIPCC before.  If they are needed by other codecs they can be
reused.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Are these settings not applicable for non-opus codecs.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:2966: char          
*protocol;
Done.
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Funny indent.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:3955: 
Done.
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Whitespace only cahnge.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:4005: 
Done.
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Whitespace only change.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:4548: 
Done.
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> whitespace only change.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:4738: has_audio = FALSE;
This is inside the !sdpmode flag so code will not get called.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> One of Enda's patches removes this, right?

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:5866: vcm_res =
vcmSetIceSessionParams(dcb_p->peerconnection, ufrag, pwd);
Done. 
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Don't call this fxn if no values.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:5893: vcm_res =
vcmSetIceMediaParams(dcb_p->peerconnection, media->level, ufrag, pwd,
I noticed that the ufrag and pwd can be null here but we still have candidates
so I only prevent calling vcmSetIceMediaParams if there are no candidates.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Don't call this fxn if no values.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:5929: char           
*session_fingerprint;
Done.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Initialize these to NULL.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:5961:
sstrncpy(line_to_split, session_fingerprint, sizeof(line_to_split));
Done.
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Indent.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:5972: sstrncpy(digest,
token, FSMDEF_MAX_DIGEST_LEN);
Done. 

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> I would like to see length checks rather than just truncation and have it
throw
> an error.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:6128:
GSMSDP_FOR_ALL_MEDIA(media, dcb_p) {
Noted and will fix later as this method is not used currently.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Please add a check here for non-numeric mids.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
File media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp_crypto.c (right):

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp_crypto.c:1112: case
SDP_TRANSPORT_RTPSAVPF:
Done in this patch.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Is there a check somewhere that fails negotiation if you did not negotiate
DTLS?
> We need one.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
File media/webrtc/signaling/src/sipcc/core/gsm/lsm.c (right):

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:792: 
Done.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Isn't this a duplicate initialization?

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:863: sdpmode = 0;
Done.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Push this test upward, I think, since it's not like it changes.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:1039:
config_get_value(CFGID_SDPMODE, &sdpmode, sizeof(sdpmode));
Done.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Indentation

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:1041: // TODO(emannion): make
negotiation fail when the other side
Done now. Quite a few changes involved.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Enda, please actually do this.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:1071: ret_val = CC_RC_SUCCESS;
Done.
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Why is this returning success. If we are in sdp mode and there is no PC this
> shoul dbe an error.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:1244:
config_get_value(CFGID_SDPMODE, &sdpmode, sizeof(sdpmode));
Done.
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Indent.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:1298: // TODO(emannion): make
negotiation fail when the other side
Done, check DTLS in negotiate_media_lines now.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Enda: please do ths.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:1303: /* TODO(emannion): his
perhaps needs some error checking for validity.
 Bug 784517  when we handle multiple media streams should handle more
validation. This will change how this portion of code behaves.

On 2012/10/01 20:57:26, ekr-webrtc wrote:
> And this.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:1931:
config_get_value(CFGID_SDPMODE, &sdpmode, sizeof(sdpmode));
Done.
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Move this config setting out of the loop.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:5473:
LSM_DEBUG(get_debug_string(DEBUG_INPUT_NULL), fname);
Done.
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Indent.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:5473:
LSM_DEBUG(get_debug_string(DEBUG_INPUT_NULL), fname);
Done.
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Indent.

http://firefox-codereview.appspot.com/10002/diff/1/media/webrtc/signaling/src...
media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:6571:
config_get_value(CFGID_SDPMODE, &sdpmode, sizeof(sdpmode));
Done.
On 2012/10/01 20:57:26, ekr-webrtc wrote:
> Indent.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 905+:3555e821a5cd+