Allow Audio without Video and unidirection Audio or Video in SIPCC

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: ehugg, Assigned: emannion)

Tracking

Trunk
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 13 obsolete attachments)

83.41 KB, patch
Details | Diff | Splinter Review
Reporter

Description

7 years ago
Need to implement:

Audio without Video
Video without Audio
unidirectional streams.
Whiteboard: [WebRTC], [blocking-webrtc+]
Assignee: nobody → snandaku

Comment 1

7 years ago
Seeking more clarification ...

Does this mean ,  we need to support the following
1. SDP Offer Audio only and follow up with VCM/Media Conduit support 
2. SDP Offer Video only and follow up with VCM/Media Conduit support
3. SDP Offer or Answer with SendOnly, RecvOnly 

I am not sure how can this be configured or told via JavaScript. say , for example, SendOnly Audio Only Stream. Are we assuming something like we modify the SDP obtained on createOffer() and inserting it back in setLocalDesc()   ...
Huh?

Audio w/o video:
1. You indicate which media streams you want via a combination of addStream() and hints.
2. It's already possible to have audio w/o video just by adding only an audio stream.
3. You should be able to have video w/o audio by adding only a video stream, but there is at least 1 and possibly many checks inside SIPCC to prevent it. Those checks need to be removed.

Unidirectional anything:
1. There is a hint to indicate that you want to receive media even if you didn't offer that type of media.
2. That hint needs to be wired up.

You should do the first set of tasks (video w/o audio) first. 

None of this should have any interaction with VCM/MediaConduit, since there are separate start recv and start send calls and we use MediaConduits only as one-way objects.
Assignee

Comment 3

7 years ago
One area here I may need to revisit is then obth options can only be true or false and there is no option to make the constraint undefined.  If I have to do this then I can make each option an enum and pass that into SIPCC.
Attachment #663484 - Flags: feedback?(ethanhugg)
Attachment #663484 - Flags: feedback?(ekr)
Assignee

Comment 4

7 years ago
Added a revided patch that builds on alder tip.
Attachment #663484 - Attachment is obsolete: true
Attachment #663484 - Flags: feedback?(ethanhugg)
Attachment #663484 - Flags: feedback?(ekr)
Attachment #663535 - Flags: feedback?(ethanhugg)
Attachment #663535 - Flags: feedback?(ekr)
Comment on attachment 663484 [details] [diff] [review]
OfferWithoutAudio and OfferWithoutVideo unidirectional streams

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

Partial review. This needs fixing to properly reflect the spec and also to rework the structure and the structure passing.

::: media/webrtc/signaling/include/CC_Call.h
@@ +302,5 @@
>             @return true or false.
>           */
>          virtual bool originateP2PCall (cc_sdp_direction_t video_pref, const std::string & digits, const std::string & ip) = 0;
>          
> +        virtual int createOffer (cc_media_constraints_t constraints) = 0;

Do you really want to pass a struct and not a pointer/ref to a struct?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +149,5 @@
>                else
>                {
>                  stream = remoteStream->GetMediaStream();
>  
> +                constraint = stream->GetHintContents();

See previous comments about hints vs constraints.

@@ +476,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +PeerConnectionImpl::CreateFakeMediaStream(PRUint32 constraint, nsIDOMMediaStream** retval)

Please revert all these inappropriate hints->constraints changes.

@@ +770,5 @@
>    CSFLogDebug(logTag, "AddStream");
>  
>    // TODO(ekr@rtfm.com): Remove these asserts?
>    // Adding tracks here based on nsDOMMediaStream expectation settings
> +  PRUint32 constraints = stream->GetHintContents();

hints not constraints.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +48,5 @@
>  #endif
>  
>  namespace sipcc {
>  
> +class MediaConstraints {

Why are you inventing a class here that needs to be tweaked for every new constraint.

Either simply use std::map<std::string, std::string> (feel fry to hide this behind a typedef) or have a class which wraps a map and has methods like isTrue(), getValue(), etc.

@@ +305,5 @@
>    static PeerConnectionImpl* CreatePeerConnection();
>    static void Shutdown();
>  
>    Role GetRole() const { return mRole; }
> +  void MakeMediaStream(PRUint32 constraint, nsIDOMMediaStream** stream);

Why did this change? This is not a constraint, it's a service function to create a representation of a remote stream. hints here is an output.

@@ +310,2 @@
>    void MakeRemoteSource(nsDOMMediaStream* stream, RemoteSourceStreamInfo** info);
> +  void CreateRemoteSourceStreamInfo(PRUint32 constraint, RemoteSourceStreamInfo** info);

Same here.

@@ +379,5 @@
>  
>    // Get the DTLS identity
>    mozilla::RefPtr<DtlsIdentity> const GetIdentity() { return mIdentity; }
>  
> +  NS_IMETHODIMP CreateOffer(MediaConstraints constraints);

Please do not pass structs as arguments.

Also, this should be const.

@@ +381,5 @@
>    mozilla::RefPtr<DtlsIdentity> const GetIdentity() { return mIdentity; }
>  
> +  NS_IMETHODIMP CreateOffer(MediaConstraints constraints);
> +
> +  NS_IMETHODIMP CreateAnswer(MediaConstraints constraints, const char* offer);

Same.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +200,5 @@
>  
>      return (dcb_p->media_cap_tbl);
>  }
>  
> +void gsmsdp_set_constraints(fsmdef_dcb_t *dcb, boolean offerToReceiveAudio, boolean offerToReceiveVideo) {

This does not seem like it is going to scale well to new constraints.

@@ +203,5 @@
>  
> +void gsmsdp_set_constraints(fsmdef_dcb_t *dcb, boolean offerToReceiveAudio, boolean offerToReceiveVideo) {
> +
> +    if (FALSE == offerToReceiveAudio) {
> +        dcb->media_cap_tbl->cap[CC_AUDIO_1].support_direction = SDP_DIRECTION_SENDONLY;

I don't follow this logic. What if there is no audio stream at all and no constraint set.

::: media/webrtc/signaling/src/sipcc/include/cc_call_feature.h
@@ +185,5 @@
>   * @return SUCCESS or FAILURE
>   */
>  cc_return_t CC_CallFeature_dial(cc_call_handle_t call_handle, cc_sdp_direction_t video_pref, const cc_string_t numbers);
>  
> +cc_return_t CC_CallFeature_CreateOffer(cc_call_handle_t call_handle, cc_media_constraints_t constraints);

Please don't pass structs.

::: media/webrtc/signaling/src/sipcc/include/cc_constants.h
@@ +581,5 @@
>  } cc_media_type_t;
>  
> +typedef struct cc_media_constraints_t_ {
> +    cc_boolean    offerToReceiveVideo;
> +    cc_boolean    offerToReceiveAudio;

Maybe use sipcc naming conventions here?

This isn't going to work because there are three values: true, false, and no constraint provided. See my email to the lsit.

::: media/webrtc/signaling/src/sipcc/include/ccapi_call.h
@@ +82,5 @@
>   */
>  cc_return_t CCAPI_Call_originateCall(cc_call_handle_t handle, cc_sdp_direction_t video_pref, cc_string_t digits);
>  
>  
> +cc_return_t CCAPI_CreateOffer(cc_call_handle_t handle, cc_media_constraints_t constraints);

Pass a const * here.

::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp
@@ +570,2 @@
>   */
> +int CC_SIPCCCall::createAnswer (cc_media_constraints_t constraints, const std::string & offersdp) {

Ref or pointer, please.

::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.h
@@ +135,5 @@
>          virtual void removeStream(int streamId);
>          virtual bool setVolume(int volume);
>          virtual bool originateP2PCall (cc_sdp_direction_t video_pref, const std::string & digits, const std::string & ip);
> +        virtual int createOffer(cc_media_constraints_t constraints);
> +        virtual int createAnswer(cc_media_constraints_t constraints, const std::string & offersdp);

Pass a ref or a pointer.
Attachment #663484 - Attachment is obsolete: false
Assignee

Comment 6

7 years ago
I think is closer to what you may want but I not sure how we can avoid having some code in PCImpl that will not need updating when new constraints are added.
Attachment #663484 - Attachment is obsolete: true
Attachment #663535 - Attachment is obsolete: true
Attachment #663535 - Flags: feedback?(ethanhugg)
Attachment #663535 - Flags: feedback?(ekr)
Attachment #664066 - Flags: feedback?(ethanhugg)
Attachment #664066 - Flags: feedback?(ekr)
Assignee

Comment 7

7 years ago
A few items to note.

1.  Not sure how to use the mandatory flag I pass with each constraint into SIPCC yet. Or if I need to as of yet.

2.  In cc_invokeFeatureSDPMode to add data to the hash you need a key, I am hardcoding this in CONSTRAINT_SESSION_ID,  it works but maybe I need to generate this.

3. I think the memory I allocate in buildArray is getting released in SIPCC but not totally sure. See CreateOffer new code, cpr_free.

4. Do I need to add locking to buildArray and setBooleanConstraint?

5. I do not add the constraints to the DCB I can easily if needed.
Attachment #664066 - Attachment is obsolete: true
Attachment #664066 - Flags: feedback?(ethanhugg)
Attachment #664066 - Flags: feedback?(ekr)
Attachment #664993 - Flags: feedback?(ethanhugg)
Attachment #664993 - Flags: feedback?(ekr)
Assignee

Updated

7 years ago
Assignee: snandaku → emannion
Reporter

Comment 8

7 years ago
Comment on attachment 664993 [details] [diff] [review]
Revised: OfferWithoutAudio and OfferWithoutVideo unidirectional streams

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

I will also give it a go on Linux and see if the unittests are happy there.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +53,5 @@
> +
> +  ConstraintInfo booleanconstraint;
> +  booleanconstraint.mandatory = mandatory;
> +
> +  if (true == enabled)

This should be if (enabled) according to the moz style guide.  There are a couple others in this method.

@@ +796,5 @@
>   * CC_SDP_DIRECTION_SENDRECV will not be used when Constraints are implemented
>   */
>  NS_IMETHODIMP
> +PeerConnectionImpl::CreateOffer(const char* constraints) {
> +  MOZ_ASSERT(constraints);

I understand that this one is going away.  It should be commented as such so it doesn't look broken.  Also the comment above is no longer useful.

@@ +818,5 @@
> +}
> +
> +NS_IMETHODIMP
> +PeerConnectionImpl::CreateAnswer(const char* constraints, const char* offer) {
> +  MOZ_ASSERT(constraints);

Some here if this one is going away as well.

::: media/webrtc/signaling/src/sipcc/core/ccapp/cc_call_feature.c
@@ +145,5 @@
>      callFeature.featData.ccData.level = level;
>  
> +
> +    /* If constraints exist add to session hash */
> +    if (NULL != constraints) {

if (constraints)

::: media/webrtc/signaling/src/sipcc/include/cc_constants.h
@@ +110,5 @@
>  #define GET_CALL_ID(call_handle) (cc_callid_t)(call_handle & 0xFFFF)
>  #define CREATE_CALL_HANDLE(line, callid) (cc_call_handle_t)(((line & 0xFFF) << CC_SID_LINE_SHIFT) + (callid & 0xFFFF))
>  #define CREATE_CALL_HANDLE_FROM_SESSION_ID(session_id) (session_id & 0xFFFFFFF)
>  #define CREATE_SESSION_ID_FROM_CALL_HANDLE(call_handle) ((CC_SESSIONTYPE_CALLCONTROL << CC_SID_TYPE_SHIFT) + call_handle)
> +#define CONSTRAINT_SESSION_ID 12345

I see where we set the sessionID to this, but I didn't find anywhere that it's checked.  Is it not really used, or planned or later?  Also do we know that it won't match some other sessionID?

@@ +588,5 @@
> +} cc_media_constraint_t;
> +
> +typedef struct {
> +  cc_media_constraint_t** constraints;
> +  cc_int16_t  constraintcnt;

I would consider changing this name to constraint_count and its type to unsigned unless there's a reason it could be negative.
Attachment #664993 - Flags: feedback?(ethanhugg) → feedback+
Reporter

Comment 9

7 years ago
Comment on attachment 664993 [details] [diff] [review]
Revised: OfferWithoutAudio and OfferWithoutVideo unidirectional streams

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

::: media/webrtc/signaling/src/sipcc/core/ccapp/cc_call_feature.c
@@ +149,5 @@
> +    if (NULL != constraints) {
> +        if (constraints->constraintcnt > 0 &&
> +            (CC_FEATURE_CREATEOFFER == featureId || CC_FEATURE_CREATEANSWER == featureId )) {
> +            sessionItem.sessionID = CONSTRAINT_SESSION_ID;
> +            session_data_t * sessionData = (session_data_t *)findhash(sessionItem.sessionID);

This line produces this error on Linux
_feature.c:153:13: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]

Need to declare it at the top
Assignee

Comment 10

7 years ago
The patch has been extended to handle negotiation of single stream media. Can now negotiate with just Audio or Video.
Attachment #664993 - Attachment is obsolete: true
Attachment #664993 - Flags: feedback?(ekr)
Attachment #665394 - Flags: feedback?(ethanhugg)
Attachment #665394 - Flags: feedback?(ekr)
Assignee

Comment 11

7 years ago
Attachment #665394 - Attachment is obsolete: true
Attachment #665394 - Flags: feedback?(ethanhugg)
Attachment #665394 - Flags: feedback?(ekr)
Attachment #665406 - Flags: feedback?(ethanhugg)
Attachment #665406 - Flags: feedback?(ekr)
Assignee

Comment 12

7 years ago
Added a new re-negotiation test.
Attachment #665406 - Attachment is obsolete: true
Attachment #665406 - Flags: feedback?(ethanhugg)
Attachment #665406 - Flags: feedback?(ekr)
Attachment #665864 - Flags: feedback?(ethanhugg)
Attachment #665864 - Flags: feedback?(ekr)
Blocks: 796888
Blocks: 796892
OS: Linux → All
Hardware: x86_64 → All
Reporter

Comment 13

7 years ago
Comment on attachment 665864 [details] [diff] [review]
Revised: OfferWithoutAudio and OfferWithoutVideo unidirectional streams


These new tests don't check packets send/received.  We're going to probably need a fake video generator to properly test these.
Assignee

Comment 14

7 years ago
All tests passing now except I get this strange crash on shutdown on Mac. I must remove my patch and test next. 

[       OK ] SignalingTest.FullCallTrickle (13536 ms)
[----------] 22 tests from SignalingTest (83771 ms total)

[----------] Global test environment tear-down
[==========] 22 tests from 1 test case ran. (83974 ms total)
[  PASSED  ] 22 tests.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00007fff5f3fff68
0x000000010b92d919 in arena_malloc (arena=0x10bd00040, size=296, zero=true) at /Users/Enda/code/mozilla/mozilla-central/memory/mozjemalloc/jemalloc.c:4205
4205			return (arena_malloc_small(arena, size, zero));
(gdb) bt
#0  0x000000010b92d919 in arena_malloc (arena=0x10bd00040, size=296, zero=true) at /Users/Enda/code/mozilla/mozilla-central/memory/mozjemalloc/jemalloc.c:4205
#1  0x000000010b9197a0 in icalloc (size=296) at /Users/Enda/code/mozilla/mozilla-central/memory/mozjemalloc/jemalloc.c:4227
#2  0x000000010b9196c2 in je_calloc (num=1, size=296) at /Users/Enda/code/mozilla/mozilla-central/memory/mozjemalloc/jemalloc.c:6476
#3  0x000000010b91d381 in zone_calloc (zone=0x10bade000, num=1, size=296) at /Users/Enda/code/mozilla/mozilla-central/memory/mozjemalloc/jemalloc.c:6926
#4  0x00007fff80142d1a in malloc_zone_calloc ()
#5  0x00007fff801467bb in calloc ()
#6  0x000000010b5267b5 in PR_Calloc (nelem=1, elsize=296) at /Users/Enda/code/mozilla/mozilla-central/nsprpub/pr/src/malloc/prmem.c:443
#7  0x000000010b54a69f in pt_AttachThread () at /Users/Enda/cod
Attachment #665864 - Attachment is obsolete: true
Attachment #665864 - Flags: feedback?(ethanhugg)
Attachment #665864 - Flags: feedback?(ekr)
This is probably not your fault. I get shutdown crashes on mac regularly.
Reporter

Comment 16

7 years ago
Comment on attachment 670425 [details] [diff] [review]
Revised: OfferWithoutAudio and OfferWithoutVideo unidirectional streams


Did you upload the one that applies on top of M-C?  I get five .rej files from this on latest M-C.
Assignee

Comment 17

7 years ago
(In reply to Eric Rescorla from comment #15)
> This is probably not your fault. I get shutdown crashes on mac regularly.

Yes, I built wothout my patch and get the same error.
Assignee

Comment 18

7 years ago
I was building this patch on top of the hackhack patch and that would have causes a few conflicts.  This is definitly from mozilla-central branch.
Attachment #670425 - Attachment is obsolete: true
Reporter

Comment 19

7 years ago
Comment on attachment 670738 [details] [diff] [review]
Revised: OfferWithoutAudio and OfferWithoutVideo unidirectional streams

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

Looks good to me.  I assume we will need a follow-on patch to check packets send/recv on these unittests along with a fake video generator.
Attachment #670738 - Flags: review+
Assignee

Comment 20

7 years ago
When I merged on top of the m-c tip I yet again had a number of merge conflicts.
Attachment #670738 - Attachment is obsolete: true
Attachment #671114 - Flags: feedback?(ethanhugg)
Assignee

Comment 21

7 years ago
This time I ran hg qref
Attachment #671114 - Attachment is obsolete: true
Attachment #671114 - Flags: feedback?(ethanhugg)
Attachment #671115 - Flags: feedback?(ethanhugg)
Reporter

Updated

7 years ago
Attachment #671115 - Flags: feedback?(ethanhugg)
Attachment #671115 - Flags: feedback+
Attachment #671115 - Flags: checkin?(rjesup)
Comment on attachment 671115 [details] [diff] [review]
Revised: OfferWithoutAudio and OfferWithoutVideo unidirectional streams

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

Needs some work, and some items here aren't nits.

Also, we're now using reviews in the 'normal' fashion. Code needs r+ before checkin.  Ethan, for media/webrtc/signaling, you can give r+ (unless for some reason you don't feel comfortable doing so).

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +106,5 @@
>          PeerConnectionImpl::kIdle == mSipccState) {
>        ChangeSipccState(PeerConnectionImpl::kStarted);
>      } else {
>        CSFLogError(logTag, "%s PeerConnection in wrong state", __FUNCTION__);
> +      //Cleanup();

Is there a reason this is being kept commented out?  Is it coming back, or does it have historical significance?

@@ +111,4 @@
>        MOZ_ASSERT(PR_FALSE);
>      }
>    } else {
> +    //Cleanup();

ditto

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +82,5 @@
> +    return;
> +
> +  for (constraints_map::iterator it = mConstraints.begin();
> +          it != mConstraints.end(); ++it) {
> +    (*constraintarray)->constraints[i] = (cc_media_constraint_t*) malloc(sizeof(cc_media_constraint_t));

Either use cpr_malloc (infallible) or check the returns (repeat)

@@ +744,5 @@
>  
>  NS_IMETHODIMP
> +PeerConnectionImpl::CreateOffer(MediaConstraints& constraints) {
> +
> +  cc_media_constraints_t* cc_constraints = 0;

nullptr

@@ +745,5 @@
>  NS_IMETHODIMP
> +PeerConnectionImpl::CreateOffer(MediaConstraints& constraints) {
> +
> +  cc_media_constraints_t* cc_constraints = 0;
> +  constraints.buildArray(&cc_constraints);

At least in theory this can fail, though it doesn't have a good way to return it currently.  Give it a return value, and if it fails return an error.  Or make it infallible (cpr_malloc)

@@ +747,5 @@
> +
> +  cc_media_constraints_t* cc_constraints = 0;
> +  constraints.buildArray(&cc_constraints);
> +
> +  mCall->createOffer(cc_constraints);

Can this fail??

@@ +752,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +PeerConnectionImpl::CreateAnswer(const char* constraints, const char* aOffer) {

Is this supposed to be unused?  Is this for the future?  If it is supposed to be here but is currently unused, use (void) constraints or some such to avoid compiler warnings.

@@ +759,5 @@
>  
>    CheckIceState();
>    mRole = kRoleAnswerer;  // TODO(ekr@rtfm.com): Interrogate SIPCC here?
> +  MediaConstraints aconstraints;
> +  CreateAnswer(aconstraints, aOffer);

Can this fail??

@@ +766,5 @@
> +
> +NS_IMETHODIMP
> +PeerConnectionImpl::CreateAnswer(MediaConstraints& constraints, const char* offer) {
> +
> +  cc_media_constraints_t* cc_constraints = 0;

nullptr

@@ +767,5 @@
> +NS_IMETHODIMP
> +PeerConnectionImpl::CreateAnswer(MediaConstraints& constraints, const char* offer) {
> +
> +  cc_media_constraints_t* cc_constraints = 0;
> +  constraints.buildArray(&cc_constraints);

Repeat of previous comment

@@ +769,5 @@
> +
> +  cc_media_constraints_t* cc_constraints = 0;
> +  constraints.buildArray(&cc_constraints);
> +
> +  mCall->createAnswer(cc_constraints, offer);

Can this fail??

::: media/webrtc/signaling/src/sipcc/core/ccapp/cc_call_feature.c
@@ +120,5 @@
> +             * other usage of the hash table */
> +            session_id = abs(cpr_rand()) % 60000;
> +            sessionData = (session_data_t *)findhash(session_id);
> +            sessionData = cpr_malloc(sizeof(session_data_t));
> +            if (sessionData == NULL) {

cpr_malloc is infallible

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +2886,5 @@
>      }
>  
> +   if (msg->data.session.has_constraints) {
> +        sess_data_p = (session_data_t *)findhash(msg->data.session.sessionid);
> +        if (sess_data_p != NULL) {

if (sess_data_p) {  Unless that conflicts with file style.

@@ +2891,5 @@
> +            gsmsdp_process_cap_constraints(dcb, sess_data_p->cc_constraints);
> +
> +            if (0 > delhash(msg->data.session.sessionid)) {
> +                FSM_DEBUG_SM (DEB_F_PREFIX"failed to delete hash sessid=0x%08x\n",
> +                        DEB_F_PREFIX_ARGS(SIP_CC_PROV, __FUNCTION__), msg->data.session.sessionid);

fix indents, maybe split to 3 lines

@@ +2994,5 @@
> +            gsmsdp_process_cap_constraints(dcb, sess_data_p->cc_constraints);
> +
> +            if (0 > delhash(msg->data.session.sessionid)) {
> +                FSM_DEBUG_SM (DEB_F_PREFIX"failed to delete hash sessid=0x%08x\n",
> +                        DEB_F_PREFIX_ARGS(SIP_CC_PROV, __FUNCTION__), msg->data.session.sessionid);

indents again

@@ +3226,5 @@
>      if (JSEP_OFFER == action) {
>  
> +        cause = gsmsdp_process_offer_sdp(fcb, &msg_body, TRUE);
> +        if (cause != CC_CAUSE_OK) {
> +            ui_set_remote_description(evSetRemoteDescError, line, call_id, dcb->caller_id.call_instance_id, NULL, PC_SETREMOTEDESCERROR);

Wrap line please

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +528,4 @@
>      // Create a media stream as if it came from GUM
>      nsRefPtr<nsDOMMediaStream> domMediaStream = new nsDOMMediaStream();
>  
> +    PRUint32 aHintContents = 0;

int32_t please
Attachment #671115 - Flags: checkin?(rjesup) → review-
Assignee

Comment 23

7 years ago
Incorporated all of rjesop's comments, excellent review thanks. I added a bug for the commented cleanup() code.  

https://bugzilla.mozilla.org/show_bug.cgi?id=801620
Attachment #671115 - Attachment is obsolete: true
Attachment #671435 - Flags: feedback?(ethanhugg)
Reporter

Comment 24

7 years ago
Comment on attachment 671435 [details] [diff] [review]
Revised: OfferWithoutAudio and OfferWithoutVideo unidirectional streams

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

Please submit for review to Jesup. I'd like to get a r+ from him for this patch.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +73,5 @@
> +    return;
> +
> +  short i = 0;
> +  std::string tmpStr;
> +  *constraintarray = (cc_media_constraints_t*) malloc(sizeof(cc_media_constraints_t));

This should be cpr_malloc as well.  Then you can remove the check below.

@@ +77,5 @@
> +  *constraintarray = (cc_media_constraints_t*) malloc(sizeof(cc_media_constraints_t));
> +  if (!(*constraintarray))
> +    return;
> +
> +  (*constraintarray)->constraints = (cc_media_constraint_t**) malloc(mConstraints.size() * sizeof(cc_media_constraint_t));

Same with this one.
Attachment #671435 - Flags: feedback?(ethanhugg) → feedback-
Assignee

Comment 25

7 years ago
All comments now integrated.
Attachment #671435 - Attachment is obsolete: true
Attachment #671492 - Flags: review?(rjesup)
Attachment #671492 - Flags: review?(rjesup) → review+
Reporter

Updated

7 years ago
Attachment #671492 - Flags: checkin?(rjesup)
Attachment #671492 - Flags: checkin?(rjesup) → checkin+
Assignee

Comment 27

7 years ago
Again re-merged to build on tip, no change required.
Attachment #671492 - Attachment is obsolete: true

Comment 28

7 years ago
https://hg.mozilla.org/mozilla-central/rev/9ba7d327ce67
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
enda, I assume all the tests we can have, have been landed with the patch?
Flags: in-testsuite+
Assignee

Comment 30

7 years ago
All tests I have written have landed with the patch, it is possble that I could write eve more tests to eek out other constraint permutations if required.
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.