Master Bug for Codec , Codec Configuration and Payload Handling in VCM, SIPCC and Conduit Layers

RESOLVED FIXED

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Suhas, Assigned: Suhas)

Tracking

unspecified
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 12 obsolete attachments)

5.71 KB, patch
Details | Diff | Splinter Review
32.25 KB, patch
ehugg
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Extending VCM layer to include negotiation of all the supported audio/video codecs. This bug aims to do this in semi-automatic way. There will be a followup bug to automate codec negotiation
(Assignee)

Comment 1

5 years ago
Created attachment 656944 [details]
Interim Patch to fix the hardcoded audio and video codecs configuration in VCM. more patches to follow

This is first patch in the series of patches to improve codec selection, configuration and RTP payload type mapping. 
This patch removes the dependecy from cip_Mmgr enum and updates audio-codec and video-codec configuration for most of the supported codecs. This is still done in static manner.
Attachment #656944 - Flags: review?(ethanhugg)
Attachment #656944 - Flags: review?(ekr)

Updated

5 years ago
Attachment #656944 - Attachment is patch: true

Updated

5 years ago
Attachment #656944 - Flags: review?(tterribe)

Comment 2

5 years ago
Comment on attachment 656944 [details]
Interim Patch to fix the hardcoded audio and video codecs configuration in VCM. more patches to follow

Review of attachment 656944 [details]:
-----------------------------------------------------------------

Is this a complete patch? Does it pass unit test? Will changes be required in other pieces of code?

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ -2317,5 @@
>  
>  static int vcmPayloadType2AudioCodec(vcm_media_payload_type_t payload_in,
>                                       mozilla::AudioCodecConfig **config) {
>    int wire_payload = -1;
> -  int payload = map_VCM_Media_Payload_type(payload_in);

Wait, you've removed this translation? is that OK? Do we need to change something in the caller to compensate?

@@ +2351,2 @@
>      case AudioPayloadType_OPUS:
> +      *config = new mozilla::AudioCodecConfig(wire_payload, "OPUS", 48000, 480, 1, 64000);

Why did the 4th argument change from 80 to 480?

@@ +2407,5 @@
> +    case VCM_Media_Payload_I420:
> +      *config = new mozilla::VideoCodecConfig(wire_payload, "I420", 176, 144);
> +      break;
> +    case VCM_Media_Payload_VP8:
> +      *config = new mozilla::VideoCodecConfig(wire_payload, "VP8", 640, 480);

I'm finding it hard to believe that we only support one VP8 resolution.
(Assignee)

Comment 3

5 years ago
Comment on attachment 656944 [details]
Interim Patch to fix the hardcoded audio and video codecs configuration in VCM. more patches to follow

Review of attachment 656944 [details]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ -2317,5 @@
>  
>  static int vcmPayloadType2AudioCodec(vcm_media_payload_type_t payload_in,
>                                       mozilla::AudioCodecConfig **config) {
>    int wire_payload = -1;
> -  int payload = map_VCM_Media_Payload_type(payload_in);

I did run the test-cases and as per my understanding an extra level of mapping wasn't doing anything from what we want i.e dynamic payload type and our internal corresponding payload type.

@@ +2407,5 @@
> +    case VCM_Media_Payload_I420:
> +      *config = new mozilla::VideoCodecConfig(wire_payload, "I420", 176, 144);
> +      break;
> +    case VCM_Media_Payload_VP8:
> +      *config = new mozilla::VideoCodecConfig(wire_payload, "VP8", 640, 480);

We need  a logic that would parameterize resolutions other than 640,480. I was thinking of including it as part of dynamic codec selection using MediaEngine APIs.  
I am assuming 648 * 480 could be the default resolution when there is none.
(Assignee)

Updated

5 years ago
Assignee: nobody → snandaku
(Assignee)

Comment 4

5 years ago
Created attachment 657412 [details] [diff] [review]
Moved audio-codec selection to use VCM mappings alone
(Assignee)

Updated

5 years ago
Attachment #657412 - Flags: review?(ethanhugg)
Attachment #657412 - Flags: review?(ekr)
(Assignee)

Updated

5 years ago
Attachment #656944 - Attachment is obsolete: true
Attachment #656944 - Attachment is patch: false
Attachment #656944 - Flags: review?(tterribe)
Attachment #656944 - Flags: review?(ethanhugg)
Attachment #656944 - Flags: review?(ekr)

Updated

5 years ago
Attachment #657412 - Attachment is patch: true

Comment 5

5 years ago
Comment on attachment 657412 [details] [diff] [review]
Moved audio-codec selection to use VCM mappings alone

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2332,3 @@
>    }
>    else {
> +    //static payloaf type

payload.

@@ +2337,3 @@
>    }
>  
>    if (wire_payload == -1) {

Is there any way for this to happen?

@@ +2349,4 @@
>        *config = new mozilla::AudioCodecConfig(wire_payload, "PCMU", 8000, 80, 1, 64000);
>        break;
> +    case VCM_Media_Payload_OPUS:
> +      *config = new mozilla::AudioCodecConfig(wire_payload, "OPUS", 48000, 480, 1, 64000);

Why is this 480 rather than the 80 it was before?

@@ +2380,5 @@
>  }
>  
>  
>  // TODO(ekr@rtfm.com): There's a lot of crazy mapping going on
>  // here. Is there a reason to go through the VCM mappings?

This TODO needs to go.

@@ +2399,3 @@
>    }
>  
>    if (wire_payload == -1) {

Is there any way for this to happen?
Attachment #657412 - Flags: review?(ekr) → review+

Comment 6

5 years ago
Comment on attachment 657412 [details] [diff] [review]
Moved audio-codec selection to use VCM mappings alone

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2313,4 @@
>  #define CHECK_DYNAMIC_PAYLOAD_TYPE(PTYPE)   (PTYPE & 0xFFFF0000)
>  
>  // TODO(ekr@rtfm.com): There's a lot of crazy mapping going on
>  // here. Is there a reason to go through the VCM mappings?

This todo as well.

@@ +2337,4 @@
>    }
>  
>    if (wire_payload == -1) {
>      PR_ASSERT(PR_FALSE);

I would write this as PR_ASSERT(wire_payload != -1);  It's being set for sure from the above statements so is there a better validity check you want to do here?
Attachment #657412 - Flags: review?(ethanhugg) → review+

Updated

5 years ago
Whiteboard: [WebRTC]
(Assignee)

Comment 7

5 years ago
Created attachment 658407 [details]
WIP patch to populate multiple recv codecs
(Assignee)

Comment 8

5 years ago
Comment on attachment 658407 [details]
WIP patch to populate multiple recv codecs

This is a Work in progress patch for populating multiple receive codecs as part of  Offer/Answer process and SDP negotiation process.
Summary of changes
------------------
1. Introduced a structure to store payload type and dynamic payload type list in fsmdef_media_t
2. Updated local codec configuration code in gsm_sdp.c to populate initial list of codecs we are open to receiving
3. Updated SDP negotiation codec to loop through all the matched codecs to update dynamic payload type negotiated into the list updated from step 1 and 2.

If this approach looks fine, i will clean up this patch and hook up the populated list of recv codecs into VCM layer.
Attachment #658407 - Flags: review?(ethanhugg)
Attachment #658407 - Flags: review?(emannion)
Comment on attachment 658407 [details]
WIP patch to populate multiple recv codecs

Review of attachment 658407 [details]:
-----------------------------------------------------------------

Overall I think you are making the changes in the correct places.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +1889,5 @@
> +    if (!(media->payloads))
> +    {
> +      GSM_ERR_MSG(GSM_L_C_F_PREFIX"Memory Allocation failed for payloads\n", 
> +                        DEB_L_C_F_PREFIX_ARGS(GSM, dcb_p->line, dcb_p->call_id, fname));
> +    }

If memory allocation failed maybe we should return from the function?

@@ +2042,5 @@
> +    if (!(media->payloads))
> +    {
> +      GSM_ERR_MSG(GSM_L_C_F_PREFIX"Memory Allocation failed for payloads\n", 
> +                        DEB_L_C_F_PREFIX_ARGS(GSM, dcb_p->line, dcb_p->call_id, fname));
> +    }

Again can we continue if we are out of memory?

@@ +2051,5 @@
> +        GSM_ERR_MSG(GSM_L_C_F_PREFIX"Memory allocation failed for payload_info_t \n", 
> +                          DEB_L_C_F_PREFIX_ARGS(GSM, dcb_p->line, dcb_p->call_id, fname));
> +    }
> +
> +      media->payloads[i]->internal_payload_type = -1;

why use -1 instead of an enum type?

@@ +2623,5 @@
> + */
> +static void
> +gsmsdp_update_payload_list(fsmdef_media_t *media, int32_t payload, int32_t dynamic_payload_type)
> +{
> +  static const char fname[] = "gsmsdp_update_payload_list";

We now use the __FUNCTION__ macro for log messages.

@@ +2844,5 @@
> +                      remote_dynamic_payload_type_value = GET_DYN_PAYLOAD_TYPE_VALUE(slave_list_p[j]);
> +                      local_dynamic_payload_type_value = GET_DYN_PAYLOAD_TYPE_VALUE(slave_list_p[j]);
> +                  }
> +
> +                } else { //if remote SDP is an answer

Are you breaking answer nogotiation by commenting this code?

::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
@@ +181,5 @@
> +    int32_t internal_payload_type;
> +    int32_t dynamic_payload_type_value;
> +} fsmdef_payload_info_t;
> +
> +

Should you indicate weather the payload type is local or remote?

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +1050,1 @@
>                            ret_val = vcmRxStartICE(media->cap_index, group_id, media->refid,

I'm not sure how this is supposed to work, should vcmRxStartICE be called for each num_payloads or should it take the list you built up as a parameter?
Attachment #658407 - Flags: review?(ethanhugg) → review+
Comment on attachment 658407 [details]
WIP patch to populate multiple recv codecs

Review of attachment 658407 [details]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +407,5 @@
>      media->cap_index = CC_MAX_MEDIA_CAP; /* max is invalid value */
>      media->video = NULL;
>      media->candidate_ct = 0;
>      media->rtcp_mux = FALSE;
> +    //Crypt

Please do not use "crypt" in comments. This code is going to live for a very long
time and nobody will know what it means.

@@ +1880,5 @@
>                                  (rtp_ptype *) local_media_types,
>                                  CC_MAX_MEDIA_TYPES);
> +
> +    /*
> +     *Crypt: Setup the payloads structure to appropriate memory sizes.

And here.

@@ +1889,5 @@
> +    if (!(media->payloads))
> +    {
> +      GSM_ERR_MSG(GSM_L_C_F_PREFIX"Memory Allocation failed for payloads\n", 
> +                        DEB_L_C_F_PREFIX_ARGS(GSM, dcb_p->line, dcb_p->call_id, fname));
> +    }

Or something. This is going to cause a null pointer dereference.

@@ +2042,5 @@
> +    if (!(media->payloads))
> +    {
> +      GSM_ERR_MSG(GSM_L_C_F_PREFIX"Memory Allocation failed for payloads\n", 
> +                        DEB_L_C_F_PREFIX_ARGS(GSM, dcb_p->line, dcb_p->call_id, fname));
> +    }

Same here.

@@ +2615,5 @@
> + *  Parameters:
> + *
> + *  media - Pointer to the fsmdef_media_t for a given media entry
> + *  payload - Internal payload type
> + *  dynamaic_payload_type - Dynamic payload type as negotiated.

spelling

@@ +2631,5 @@
> +    if(media->payloads[i]->internal_payload_type == payload)
> +    {
> +      media->payloads[i]->dynamic_payload_type_value = dynamic_payload_type;
> +    }
> +  }    

This should hacve a return code indicating whether it found the payload.

@@ +2840,5 @@
> +                  if (master_list_p == remote_media_types) {
> +                      remote_dynamic_payload_type_value = GET_DYN_PAYLOAD_TYPE_VALUE(master_list_p[i]);
> +                      local_dynamic_payload_type_value = GET_DYN_PAYLOAD_TYPE_VALUE(master_list_p[i]);
> +                  } else {
> +                      remote_dynamic_payload_type_value = GET_DYN_PAYLOAD_TYPE_VALUE(slave_list_p[j]);

I realize this is old code, but when can it fire?

@@ +2867,5 @@
> +                {
> +                  //the first matched codec will be returned.
> +                  media->payload = payload;
> +                  media->remote_dynamic_payload_type_value = remote_dynamic_payload_type_value;
> +                  media->local_dynamic_payload_type_value = local_dynamic_payload_type_value;

Why are you returning this if found_codec is false? Isn't that a negotiation failure.

In general, this function needs a block comment describing the algorithm.

@@ +2916,1 @@
>                         vcmFreeMediaPtr(media->video);

Does this do anything?

::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
@@ +178,5 @@
> + * more information related to a payload as seen in the SDP
> + */
> +typedef struct fsmdef_payload_info_ {
> +    int32_t internal_payload_type;
> +    int32_t dynamic_payload_type_value;

This isn't going to scale. You need to be able to handle codec parameters and you're not going to assign a constant int to every combination.

@@ +274,5 @@
>      boolean        rtcp_mux;
>  
> +    /*
> +     * crypt: list of active lists of payloads offered.
> +     * Check with Enda

Please mark comments that need to be dealt with with XXX or TODO so they get dealt with.

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +1050,1 @@
>                            ret_val = vcmRxStartICE(media->cap_index, group_id, media->refid,

I don't see how this is going to work either.

Updated

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

Comment 11

5 years ago
Created attachment 659376 [details] [diff] [review]
moving mappings to use VCM alone

Comment 12

5 years ago
Created attachment 659378 [details] [diff] [review]
moving mappings to use VCM alone

Updated

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

Updated

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

Comment 13

5 years ago
Comment on attachment 659378 [details] [diff] [review]
moving mappings to use VCM alone


This is an update of the earlier patch uploaded by Suhas that was reviewed and is now on the obsolete list.  Pacsize of 480 is 10ms at 48k so we'll stick with that value for now.

Comment 14

5 years ago
Comment on attachment 659378 [details] [diff] [review]
moving mappings to use VCM alone


Pushed to Alder - http://hg.mozilla.org/projects/alder/rev/ec4789d5678c
(Assignee)

Updated

5 years ago
Summary: Complete support for all the supported audio/video codecs in VCM layer of SIPCC → Master Bug for Codec , Codec Configuration and Payload Handling in VCM, SIPCC and Conduit Layers
(Assignee)

Comment 15

5 years ago
Created attachment 660360 [details] [diff] [review]
Support Multiple Recv Codecs - Offerer Negotiation Only
(Assignee)

Updated

5 years ago
Attachment #658407 - Attachment is obsolete: true
Attachment #658407 - Attachment is patch: false
Attachment #658407 - Flags: review?(emannion)
(Assignee)

Comment 16

5 years ago
Comment on attachment 660360 [details] [diff] [review]
Support Multiple Recv Codecs - Offerer Negotiation Only

This bug implements handling multiple codecs in the answer while negotiating at the caller.

The code leaves the rest of the cases - negotiation of offer during answer generation, non-intial offer generation logic to be same as it is currently supported.

Those will be dealt in the subsequent patches based on the priority.

New test case has been added and all the existing test cases are verified to be working as well.
Attachment #660360 - Flags: review?(ethanhugg)
Attachment #660360 - Flags: review?(emannion)
Attachment #660360 - Flags: review?(ekr)
Comment on attachment 660360 [details] [diff] [review]
Support Multiple Recv Codecs - Offerer Negotiation Only

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

I debugged this patch and it seems to be working.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +1286,4 @@
>  
> +    mozilla::AudioCodecConfig *config_raw;
> +    // If we din't go through negotiation on ANSWER, we use the existing way of
> +    // payload generation. Rejection/No Payload match is handled else-where

spelling didn't

@@ +1287,5 @@
> +    mozilla::AudioCodecConfig *config_raw;
> +    // If we din't go through negotiation on ANSWER, we use the existing way of
> +    // payload generation. Rejection/No Payload match is handled else-where
> +    if(num_payloads == 0)
> +    {

Can num_payloads == 0,  I guess this means there is only one payload type. Either needs a comment or set num_payloads to 1 when there is only 1.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +2818,5 @@
> +      * </RANT>
> +      */
> +
> +    if(offer == FALSE) {
> +      media->num_payloads = -1;

should -1 be a defined type

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +355,5 @@
> +
> +                                                               }
> +  std::string objectType;
> +  std::string content;
> +  std::string action; //replace, remove, add

should be an enum for actions.

@@ +375,5 @@
> +
> +  void ApplyRule(Rule rule)
> +  {
> +    //Tadd simple rule application. 
> +    ParseAndApply(rule.objectType, rule.content, rule.action);

What is Tadd?

@@ +381,5 @@
> +  }
> +  
> +
> +  std::string getSdp()
> +  {

mixture of lower camel case and upper camel case in function names,  getSdp v's ApplyRule, maybe change to GetSDP.

@@ +387,5 @@
> +  }
> +
> +  private:
> +
> +  void ParseAndApply(std::string obj,std::string desc, std::string action)

decs is called content above in the Rule class, maybe they should be consistant.

@@ +404,5 @@
> +      {
> +        if(action.compare("ADD") == 0)
> +        {
> +          //Add line after the line pointed to by the obj
> +          mSdp.insert(prev, desc);

Will this also insert the carriage return line feed that SDP requires?

@@ +406,5 @@
> +        {
> +          //Add line after the line pointed to by the obj
> +          mSdp.insert(prev, desc);
> +        } else if (action.compare("REPLACE") == 0) 
> +        {

move closing braces up to same lines as else if

@@ +409,5 @@
> +        } else if (action.compare("REPLACE") == 0) 
> +        {
> +          mSdp.replace(prev, (found - prev) + 1, desc);
> +        } else if(action.compare("REMOVE") == 0)
> +        {

same here like this   } else if(action.compare("REMOVE") == 0) {
Attachment #660360 - Flags: review?(ethanhugg) → review+
Comment on attachment 660360 [details] [diff] [review]
Support Multiple Recv Codecs - Offerer Negotiation Only

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

You seem to have created two ways of indicating a media type:

1. a payload integer
2. A payload_info_t

I'm not denying that version #1 is not great, but having two is even worse.

Why isn't vcmRxInitStartIce() just taking an array of payload integers, and, if necessary,
media attrs?

::: media/webrtc/signaling/src/sipcc/include/vcm.h
@@ +364,5 @@
> + * Aggregate Structure to store payload info.
> + * This structure shall be used to configure codec info
> + * as part of SDP negotiation
> + * This structure holds codec params for both audio and video codec
> + * This might not be the best way, but it seems to do solve the purpose

Please fix the grammar here.

@@ +375,5 @@
> +    int      clock_rate;
> +    int32_t  max_frame_rate;
> +    int32_t  preference;
> +    int32_t  bandwidth;
> +} payload_info_t;

This struct seems to overlap with both the payload and attrs arguments to vcmRxStartIce. We should be working out of one space, not two.
Attachment #660360 - Flags: review?(ekr) → review-
(Assignee)

Comment 19

5 years ago
Comment on attachment 660360 [details] [diff] [review]
Support Multiple Recv Codecs - Offerer Negotiation Only

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

::: media/webrtc/signaling/src/sipcc/include/vcm.h
@@ +364,5 @@
> + * Aggregate Structure to store payload info.
> + * This structure shall be used to configure codec info
> + * as part of SDP negotiation
> + * This structure holds codec params for both audio and video codec
> + * This might not be the best way, but it seems to do solve the purpose

Will Do

@@ +375,5 @@
> +    int      clock_rate;
> +    int32_t  max_frame_rate;
> +    int32_t  preference;
> +    int32_t  bandwidth;
> +} payload_info_t;

The reason why we still need an integer representation as well as a structure based representation is we don't supporting payload list generation for all the scenarios as i have mentioned in <RANT> section of my comments as well as overall comment on uploading the patch.  
As discussed in yesterday's meeting,  we are handling only the scenario of being able to support multiple matched payloads on the caller/offerer side of the picture. This case corresponds to SDP negotiation that happens when the Offerer gets an Answer.

Once we also support the case of multiple matched payload generation for the Answerer/callee while performing SDP negotiation on the received offer , we will get rid of integer payload type altogether in the VcmRXStartICE() and use only payload_info_t structure ...  

As a result, we will have only one payload generated by the Answerer to be a match - which is the first payload matched  & Offerer being able to setup receivers for more than one matched payload.

In summary, this is just half the patch and  am sticking to our discussion yesterday to keep the scope of this patch small. We should get one more patch to implement the other part of SDP negotiation.
(Assignee)

Comment 20

5 years ago
Comment on attachment 660360 [details] [diff] [review]
Support Multiple Recv Codecs - Offerer Negotiation Only

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +1286,4 @@
>  
> +    mozilla::AudioCodecConfig *config_raw;
> +    // If we din't go through negotiation on ANSWER, we use the existing way of
> +    // payload generation. Rejection/No Payload match is handled else-where

will fix

@@ +1287,5 @@
> +    mozilla::AudioCodecConfig *config_raw;
> +    // If we din't go through negotiation on ANSWER, we use the existing way of
> +    // payload generation. Rejection/No Payload match is handled else-where
> +    if(num_payloads == 0)
> +    {

yes .. this is possible in the case of SDP Negotiation that happens when OFFER == true where we are not using this structure at all.  This patch will not handle multiple matched payload generation in OFFER == true scenario rather it just returns the first matched payload. 
More details in gsmsdp_negotiate_codec()

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +355,5 @@
> +
> +                                                               }
> +  std::string objectType;
> +  std::string content;
> +  std::string action; //replace, remove, add

I have no opinions either ways . Happy to make it enum if needed

@@ +375,5 @@
> +
> +  void ApplyRule(Rule rule)
> +  {
> +    //Tadd simple rule application. 
> +    ParseAndApply(rule.objectType, rule.content, rule.action);

Was supposed to mean "Dead Simple" ...:) .... Since this is a dumbest SDP modification rule :-D

@@ +387,5 @@
> +  }
> +
> +  private:
> +
> +  void ParseAndApply(std::string obj,std::string desc, std::string action)

will fix

@@ +404,5 @@
> +      {
> +        if(action.compare("ADD") == 0)
> +        {
> +          //Add line after the line pointed to by the obj
> +          mSdp.insert(prev, desc);

I played around inserting \r\n for the ADD scenario. But it ended up inserting extra new line. 
With the current code, it seems to do just fine ..

@@ +406,5 @@
> +        {
> +          //Add line after the line pointed to by the obj
> +          mSdp.insert(prev, desc);
> +        } else if (action.compare("REPLACE") == 0) 
> +        {

Will do

Comment 21

5 years ago
Comment on attachment 660360 [details] [diff] [review]
Support Multiple Recv Codecs - Offerer Negotiation Only

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

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +2860,5 @@
> +                
> +                /*
> +                 * Read the <RANT> above for more information
> +                 */
> +                if(offer == FALSE)

Mozilla standard here would be if (!offer)

@@ +2933,5 @@
>  
>                      GSM_DEBUG(DEB_L_C_F_PREFIX"codec= %d\n",
>                            DEB_L_C_F_PREFIX_ARGS(GSM, dcb_p->line, dcb_p->call_id, fname),
> +                          payload);
> +                    if(offer == TRUE) {

Similarly "if (offer)" - (only marking first instance).
(Assignee)

Comment 22

5 years ago
Created attachment 660550 [details] [diff] [review]
Multiple Recv Codecs Version 2
(Assignee)

Updated

5 years ago
Attachment #660360 - Attachment is obsolete: true
Attachment #660360 - Flags: review?(emannion)
(Assignee)

Comment 23

5 years ago
Created attachment 660551 [details] [diff] [review]
Multiple Recv Codecs Version 2
(Assignee)

Updated

5 years ago
Attachment #660550 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
Created attachment 660563 [details] [diff] [review]
Multiple Recv Codecs Version 2
(Assignee)

Updated

5 years ago
Attachment #660551 - Attachment is obsolete: true
(Assignee)

Comment 25

5 years ago
Created attachment 660564 [details] [diff] [review]
Multiple Recv Codecs Version 2
(Assignee)

Updated

5 years ago
Attachment #660563 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #660564 - Flags: review?(ekr)
(Assignee)

Updated

5 years ago
Attachment #660564 - Flags: review?(ethanhugg)
Attachment #660564 - Flags: review?(emannion)
Comment on attachment 660564 [details] [diff] [review]
Multiple Recv Codecs Version 2

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

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +419,5 @@
> +      prev = found + 1;
> +    }
> +  }
> +  std::string mSdp;  
> +};

My earlier comments still apply to here.
Attachment #660564 - Flags: review?(emannion) → review+

Comment 27

5 years ago
Comment on attachment 660564 [details] [diff] [review]
Multiple Recv Codecs Version 2

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

Builds and tests on 64bit Linux debug.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +2881,5 @@
>                          GSM_DEBUG(DEB_L_C_F_PREFIX"codec= %d ignored - attribs not accepted\n", 
>                               DEB_L_C_F_PREFIX_ARGS(GSM, dcb_p->line, 
>                               dcb_p->call_id, fname), media->payload);
> +			            explicit_reject = TRUE;
> +                        continue; // keep looking

These lines appear to be a whitespace-only change that make the indentation worse.  Could you check to make sure you are not inserting tabs?

@@ +2915,5 @@
>      }
>  
> +    // we should be ok to return a valid payload here. The calling function doesn't care 
> +     //unless it is RTP_NONE   
> +    if(found_codec == TRUE)

Moz standard is if(found_codec)
Attachment #660564 - Flags: review?(ethanhugg) → review+
Comment on attachment 660564 [details] [diff] [review]
Multiple Recv Codecs Version 2

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

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +2644,5 @@
> +    int32_t         num_match_payloads = 0; 
> +    int32_t         payload; 
> +    int32_t         remote_dynamic_payload_type_value;
> +    int32_t         local_dynamic_payload_type_value;
> +    int32_t         payload_index = -1; // index into payload list negotiated

I would assign this to 0 and then put the increment after the array index.

@@ +2645,5 @@
> +    int32_t         payload; 
> +    int32_t         remote_dynamic_payload_type_value;
> +    int32_t         local_dynamic_payload_type_value;
> +    int32_t         payload_index = -1; // index into payload list negotiated
> +    int             clockrate = 0; //TODO: Crypt: Probably this should be int64_t type

Why was this added to this patch?

@@ +2901,5 @@
> +                    
> +                }
> +                found_codec = TRUE;
> +                payload_index++;
> +                media->payloads[payload_index] = vcmRtpToMediaPayload(payload,

What happens if this fails?

@@ +2904,5 @@
> +                payload_index++;
> +                media->payloads[payload_index] = vcmRtpToMediaPayload(payload,
> +                                                   remote_dynamic_payload_type_value,
> +                                                   media->mode);
> +                media->num_payloads++;

I am worried that there can be > than num_local_types matches here. If I am reading this code correctly, this is in the inner loop so there can actually be count(master) * count(slave) entries in this array. You need to allocate the list based on the maximum possible if nothing is wrong and then have a sanity check to make sure you don't overrun.

More generally, how do you handle multiple matches.

::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
@@ +274,5 @@
>  
> +    /*
> +     * List of active lists of payloads negotiated
> +     */
> +    vcm_media_payload_type_t* payloads;

Where does this memory get freed?

::: media/webrtc/signaling/src/sipcc/include/vcm.h
@@ +359,5 @@
>    vcm_audioAttrs_t audio; /**< audio line attribs */
>    vcm_videoAttrs_t video; /**< Video Atrribs */
>  } vcm_mediaAttrs_t;
>  
> +

Spurious change.

@@ +609,5 @@
>          int pc_track_id,
>          cc_call_handle_t  call_handle,
>          const char *peerconnection,
> +        int num_payloads,
> +        vcm_media_payload_type_t* payloads,        

const

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +339,5 @@
>    int levels_;
>  };
>  
> +/**
> + * Simple Class to specify RULE to modify a SDP

This appears to be largely duplicative of the existing ParsedSDP module in this unit test. Please extend that rather than re-parsing the SDP.

@@ +355,5 @@
> +
> +                                                               }
> +  std::string objectType;
> +  std::string content;
> +  std::string action; //replace, remove, add

This should be an enum. I think ehugg mentioned this already.
(Assignee)

Comment 29

5 years ago
Created attachment 660661 [details] [diff] [review]
Multiple Recv Codecs V3
(Assignee)

Updated

5 years ago
Attachment #660564 - Attachment is obsolete: true
Attachment #660564 - Flags: review?(ekr)
(Assignee)

Comment 30

5 years ago
Created attachment 660667 [details] [diff] [review]
Multiple Recv Codecs V3
(Assignee)

Updated

5 years ago
Attachment #660661 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #660667 - Flags: review?(ethanhugg)
Attachment #660667 - Flags: review?(emannion)
Attachment #660667 - Flags: review?(ekr)
(Assignee)

Comment 31

5 years ago
Created attachment 660678 [details] [diff] [review]
Multiple Recv Codecs V3
(Assignee)

Updated

5 years ago
Attachment #660667 - Attachment is obsolete: true
Attachment #660667 - Flags: review?(ethanhugg)
Attachment #660667 - Flags: review?(emannion)
Attachment #660667 - Flags: review?(ekr)
(Assignee)

Updated

5 years ago
Attachment #660678 - Flags: review?(ethanhugg)
Attachment #660678 - Flags: review?(emannion)
Attachment #660678 - Flags: review?(ekr)
Comment on attachment 660678 [details] [diff] [review]
Multiple Recv Codecs V3

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +1232,5 @@
>    }
>  
> +  if(!payloads) {
> +      return VCM_ERROR;
> +  }

May also want to print to CSFLogDebug when !payloads

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +2910,5 @@
> +                    
> +                }
> +                found_codec = TRUE;
> +                if(media->num_payloads >= payload_types_count)
> +                {

We are using UNIX style braces elsewhere where the {  is at the end of the previous line.  This comment applies to a few other places including up one page in this file.

@@ +2929,5 @@
>  
> +    // we should be ok to return a valid payload here. The calling function doesn't care 
> +     //unless it is RTP_NONE   
> +    if(found_codec)
> +    {

And here, use UNIX style.
Attachment #660678 - Flags: review?(emannion) → review+
Comment on attachment 660678 [details] [diff] [review]
Multiple Recv Codecs V3

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +1215,5 @@
>          int pc_track_id,
>          cc_call_handle_t  call_handle,
>          const char *peerconnection,
> +        int num_payloads,
> +        vcm_media_payload_type_t* payloads,    

this should be const, right?

@@ +1263,5 @@
> +    
> +    for(int i=0; i <num_payloads ; i++)
> +    {
> +      int ret = vcmPayloadType2AudioCodec(payloads[i], &config_raw);
> +      if (ret) {

This is a can't happen, right? Maybe add an assert?

@@ +1294,5 @@
> +
> +    for(int i=0; i <num_payloads; i++)
> +    {
> +      int ret = vcmPayloadType2VideoCodec(payloads[i], &config_raw);
> +      if (ret) {

Same as above.

@@ +2443,5 @@
>    return 0;
>  
>  }
>  
> +

Spurious whitespace change.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +2912,5 @@
> +                found_codec = TRUE;
> +                if(media->num_payloads >= payload_types_count)
> +                {
> +                  //we maxed our allocated memory. ignore and return;
> +                  return payload;

Doesn't this code reverse the logic so that we are now returning the last match rather than the firsT?

@@ +2930,5 @@
> +    // we should be ok to return a valid payload here. The calling function doesn't care 
> +     //unless it is RTP_NONE   
> +    if(found_codec)
> +    {
> +      return (payload);

And here.

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ -916,5 @@
>      vcm_mediaAttrs_t attrs;
>      int              sdpmode = 0;
>      int pc_stream_id = 0;
>      int pc_track_id = 0;
> -

Spurious whitespace change.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +310,5 @@
> + */
> +class Rule
> +{
> +  public:
> +  Rule(std::string objType, std::string cont, RuleAction act): objectType(objType),

This rule thing seems clumsy. Why not simply have explicit modifiers? I.e., AddAfter(), Remove(), whatever?

@@ +345,2 @@
>      for(;;) {
> +      found = sdp_.find('\n', found + 1);

Why are you re-parsing every time?

@@ +672,5 @@
> +    a2_.CreateAnswer(bhints, a1_.offer());
> +    a2_.SetLocal(TestObserver::ANSWER, a2_.answer());
> +    ParsedSDP sdpWrapper(a2_.answer());
> +    //add new payload to the list - PCMA
> +    Rule mRule("m=audio", "m=audio 65375 RTP/SAVPF 109 8\r\n",kReplace);

Don't name things which aren't members m*
(Assignee)

Comment 34

5 years ago
Comment on attachment 660678 [details] [diff] [review]
Multiple Recv Codecs V3

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +1215,5 @@
>          int pc_track_id,
>          cc_call_handle_t  call_handle,
>          const char *peerconnection,
> +        int num_payloads,
> +        vcm_media_payload_type_t* payloads,    

will do

@@ +1232,5 @@
>    }
>  
> +  if(!payloads) {
> +      return VCM_ERROR;
> +  }

done

@@ +1263,5 @@
> +    
> +    for(int i=0; i <num_payloads ; i++)
> +    {
> +      int ret = vcmPayloadType2AudioCodec(payloads[i], &config_raw);
> +      if (ret) {

done

@@ +1294,5 @@
> +
> +    for(int i=0; i <num_payloads; i++)
> +    {
> +      int ret = vcmPayloadType2VideoCodec(payloads[i], &config_raw);
> +      if (ret) {

done

@@ +2443,5 @@
>    return 0;
>  
>  }
>  
> +

done

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +2910,5 @@
> +                    
> +                }
> +                found_codec = TRUE;
> +                if(media->num_payloads >= payload_types_count)
> +                {

done

@@ +2912,5 @@
> +                found_codec = TRUE;
> +                if(media->num_payloads >= payload_types_count)
> +                {
> +                  //we maxed our allocated memory. ignore and return;
> +                  return payload;

As mentioned in the comments, the calling function doesn't do anything with the return value unless and until it is  RTP_NONE. So any valid payload can be returned or the RTP_NONE.
In that case you should change the signature to a standard success/fail signature
(Assignee)

Comment 36

5 years ago
Created attachment 661610 [details] [diff] [review]
Multiple Recv Codecs V3
(Assignee)

Updated

5 years ago
Attachment #660678 - Attachment is obsolete: true
Attachment #660678 - Flags: review?(ethanhugg)
Attachment #660678 - Flags: review?(ekr)
(Assignee)

Comment 37

5 years ago
Comment on attachment 661610 [details] [diff] [review]
Multiple Recv Codecs V3

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

Incorporated comments.
Should be good to push.
Attachment #661610 - Flags: review?(ethanhugg)

Comment 38

5 years ago
Comment on attachment 661610 [details] [diff] [review]
Multiple Recv Codecs V3


Suhas states that the signature to gsmsdp_negotiate_codec will be changed to boolean type in another bug.
Attachment #661610 - Flags: review?(ethanhugg) → review+
Could someone please give me a brief outline (in simple terms) as to what has to be done for this bug? (especially this part -"This bug aims to do this in semi-automatic way. There will be a followup bug to automate codec negotiation")
(Assignee)

Comment 40

5 years ago
This bug was created to achieve the following
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.


To keep the scope the bug simple, we decided to implement the first item from the above list as part of the following patches
   Moving mappings to use VCM alone
   Multiple Recv Codecs V3 

This has been pushed to Alder as part of SIPCC Rollup.

For the remaining items, we could create individual bugs to handle them based on the priority.

Comment 41

5 years ago
Marking this as resolved.  The multiple recv codec patch landed in the alder rollup Bug 792188.  I think new issues around this should be handled in new bugs.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: verifyme

Updated

5 years ago
Flags: in-testsuite+
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.