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

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Suhas]

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

Comment 1

[Suhas]

Attachment: 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.

Comment 2

[Eric Rescorla (:ekr)]

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.

Comment 3

[Suhas]

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.

Comment 4

[Suhas]

Attachment: Moved audio-codec selection to use VCM mappings alone

Comment 5

[Eric Rescorla (:ekr)]

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([e-mail]): 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?

Comment 6

[Ethan Hugg [:ehugg]]

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([e-mail]): 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?

Comment 7

[Suhas]

Attachment: WIP patch to populate multiple recv codecs

Comment 8

[Suhas]

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.

Comment 9

[enda mannion (: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?

Comment 10

[Eric Rescorla (:ekr)]

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.

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: moving mappings to use VCM alone

Comment 12

[Ethan Hugg [:ehugg]]

Attachment: moving mappings to use VCM alone

Comment 13

[Ethan Hugg [:ehugg]]

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

[Ethan Hugg [:ehugg]]

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


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

Comment 15

[Suhas]

Attachment: Support Multiple Recv Codecs - Offerer Negotiation Only

Comment 16

[Suhas]

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.

Comment 17

[enda mannion (:emannion)]

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) {

Comment 18

[Eric Rescorla (:ekr)]

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.

Comment 19

[Suhas]

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.

Comment 20

[Suhas]

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

[Ethan Hugg [:ehugg]]

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).

Comment 22

[Suhas]

Attachment: Multiple Recv Codecs Version 2

Comment 23

[Suhas]

Attachment: Multiple Recv Codecs Version 2

Comment 24

[Suhas]

Attachment: Multiple Recv Codecs Version 2

Comment 25

[Suhas]

Attachment: Multiple Recv Codecs Version 2

Comment 26

[enda mannion (: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.

Comment 27

[Ethan Hugg [:ehugg]]

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)

Comment 28

[Eric Rescorla (:ekr)]

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.

Comment 29

[Suhas]

Attachment: Multiple Recv Codecs V3

Comment 30

[Suhas]

Attachment: Multiple Recv Codecs V3

Comment 31

[Suhas]

Attachment: Multiple Recv Codecs V3

Comment 32

[enda mannion (:emannion)]

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.

Comment 33

[Eric Rescorla (: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
@@ +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*

Comment 34

[Suhas]

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.

Comment 35

[Eric Rescorla (:ekr)]

In that case you should change the signature to a standard success/fail signature

Comment 36

[Suhas]

Attachment: Multiple Recv Codecs V3

Comment 37

[Suhas]

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.

Comment 38

[Ethan Hugg [:ehugg]]

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.

Comment 39

[Nagarjuna Varma [:Infinity]]

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")

Comment 40

[Suhas]

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

[Ethan Hugg [:ehugg]]

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.