Closed Bug 817065 Opened 12 years ago Closed 12 years ago

Clean up codec identification in VCM and SIPCC

Categories

(Core :: WebRTC: Signaling, defect, P3)

Product:
Core
Component:
WebRTC: Signaling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20

People

(Reporter: abr, Assigned: abr)

Details

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

Attachments

(1 file, 3 obsolete files)

Replace vcm_media_payload_t with structure leveraging rtp_ptype constants
129.63 KB, patch
ehugg
: checkin+
Details | Diff | Splinter Review 
This is a follow-up to Bug 786327 to remove the vcm_media_payload_t representation entirely from the tree, in favor of the rtp_ptype defined in ccsdp.h

The number of impacted files is relatively small:

  media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
  media/webrtc/signaling/src/sipcc/core/common/vcm_util.c
  media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
  media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
  media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
  media/webrtc/signaling/src/sipcc/core/includes/vcm_util.h
  media/webrtc/signaling/src/sipcc/include/vcm.h
  media/webrtc/signaling/src/sipcc/stub/vcm_stub.c
Priority: -- → P3
Whiteboard: [WebRTC] [blocking-webrtc-]
Assignee: nobody → adam
Attachment #688913 - Flags: review?(rjesup)
Attachment #688913 - Flags: review?(ethanhugg)
Status: NEW → ASSIGNED
Comment on attachment 688913 [details] [diff] [review]
Replace vcm_media_payload_t with structure leveraging rtp_ptype constants

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

::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
@@ -67,5 @@
>                                   mHeight(height)
>  
>    {
>    }
> -

All the changes in this file are whitespace; nuke them

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +946,5 @@
>      cc_uint16_t    salt_len;
>      char        fname[] = "vcmRxStart";
>  
>      CSFLogDebug( logTag, "%s: group_id=%d call_handle=%d payload=%d port=%d algID=%d",
> +        fname, group_id, call_handle, payload->remote_rtp_pt, port, algorithmID);

wrap

::: media/webrtc/signaling/src/sipcc/core/common/vcm_util.c
@@ +4,5 @@
>  
>  #include "debug.h"
>  #include "rtp_defs.h"
>  #include "vcm_util.h"
>  #include "ccsdp.h"

do we need all these includes if it only has one #define?

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +227,5 @@
> +    if (media->num_payloads > media->previous_sdp.num_payloads)
> +    {
> +      cpr_free(media->previous_sdp.payloads);
> +      media->previous_sdp.payloads =
> +        cpr_malloc(media->num_payloads * sizeof(vcm_payload_info_t));

cpr_realloc()

@@ +233,5 @@
> +
> +    /* Copy the payloads over */
> +    media->previous_sdp.num_payloads = media->num_payloads;
> +    cpr_memcopy(media->previous_sdp.payloads, media->payloads,
> +        media->num_payloads * sizeof(vcm_payload_info_t));

There's no actual implementation of cpr_memcopy() anywhere (though I see references *to* it in dns_utils.c).  Aha - I see you actually defined it later.  How about cpr_memcpy() instead?

@@ +234,5 @@
> +    /* Copy the payloads over */
> +    media->previous_sdp.num_payloads = media->num_payloads;
> +    cpr_memcopy(media->previous_sdp.payloads, media->payloads,
> +        media->num_payloads * sizeof(vcm_payload_info_t));
> +    media->previous_sdp.num_payloads = media->num_payloads;

I think if the number of payloads goes down, and then back up again, we might realloc when we don't need to.  However, this is probably not worth trying hard to avoid, and it may be very rare or non-existent in practice.

@@ +3001,5 @@
> +                        payload_info->audio.channels = 1;
> +                        /* See http://www.iana.org/assignments/rtp-parameters/
> +                           rtp-parameters.xml */
> +                        switch (codec) {
> +                            case 6:

Are these really integers and not symbolic defines?

@@ +3099,5 @@
> +                        case RTP_ISAC:
> +                            /* TODO: Update these from proper SDP constructs */
> +                            payload_info->audio.frequency = 16000;
> +                            payload_info->audio.packet_size = 480;
> +                            payload_info->audio.bitrate = 32000;

Can we set these in the same order for ease in reading?

::: media/webrtc/signaling/src/sipcc/cpr/include/cpr_stdlib.h
@@ +18,5 @@
>  #define cpr_malloc(a) moz_xmalloc(a)
>  #define cpr_calloc(a, b) moz_xcalloc(a, b)
>  #define cpr_realloc(a, b) moz_xrealloc(a, b)
>  #define cpr_free(a) moz_free(a)
> +#define cpr_memcopy(a,b,c) memcpy(a,b,c)

Can we make this cpr_memcpy()?
Attachment #688913 - Flags: review?(rjesup) → review+
Comment on attachment 688913 [details] [diff] [review]
Replace vcm_media_payload_t with structure leveraging rtp_ptype constants

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

Looks good overall.

::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
@@ -72,3 @@
>  };
>  }
> -#endif
\ No newline at end of file

I would be interested in knowing what your editor did with this line.  I removed all trailing whitespace from signaling, so it shouldn't have changed.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +233,5 @@
> +
> +    /* Copy the payloads over */
> +    media->previous_sdp.num_payloads = media->num_payloads;
> +    cpr_memcopy(media->previous_sdp.payloads, media->payloads,
> +        media->num_payloads * sizeof(vcm_payload_info_t));

I'm not sure I see the advantage of creating a cpr_memcpy.  The reason cpr_malloc exists is because there used to be a homegrown memory manager that I removed.  I'd rather use the standard memcpy for this, unless you had a different idea.

@@ +1996,3 @@
>      if (dcb_p && media && media->num_payloads == 0) {
> +
> +        if ((NULL != media->payloads) &&

Mozilla coding standard would be if (media->payloads &&

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

@@ +2002,3 @@
>          }
> +
> +        if (NULL == media->payloads) {

Same here, although I know signaling uses this style which would otherwise be fine with me.  More instances below.

@@ +2883,5 @@
>              pref_codec = RTP_NONE;
>          }
>      }
> +
> +    /* for CUCM, pref_codec is hardcoded to be NONE. */

We can remove this comment, CUCM is the acronym for the ippbx "Call Manager"

@@ +2961,5 @@
> +
> +                /* If the negotiated payload type in an answer is different from
> +                   what we sent in our offer, we set the local payload type to
> +                   our default (since that's what we put in our offer) */
> +                if (offer == FALSE) {

Likewise these boolean ones in Moz style would be if (!offer)

@@ +3229,5 @@
> +                 * current API's do not consider the request is for
> +                 * the same call. Need to update dynamic payload
> +                 * types for dynamic PT codecs. Would need to
> +                 * possibly add other video codecs as support is
> +                 * added here.

I think we can remove this comment it was specific to an embedded system problem.
Attachment #688913 - Flags: review?(ethanhugg) → review+
(In reply to Eric Rescorla (:ekr) from comment #6)
> https://tbpl.mozilla.org/?tree=Try&rev=75e248bb3a17

This built clean.
Attachment #688913 - Attachment is obsolete: true
Attachment #689211 - Attachment is obsolete: true
Comment on attachment 688913 [details] [diff] [review]
Replace vcm_media_payload_t with structure leveraging rtp_ptype constants

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

::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
@@ -67,5 @@
>                                   mHeight(height)
>  
>    {
>    }
> -

Removed from the patch.

@@ -72,3 @@
>  };
>  }
> -#endif
\ No newline at end of file

My editor put a 0x0a at the end of the file where it found none. If you look at CodecConfig.h in mozilla-incoming right now, it abruptly ends without any line ending:

> od -tx1 media/webrtc/signaling/src/media-conduit/CodecConfig.h
> ...
> 0003400    20  7d  0a  0a  7d  3b  0a  7d  0a  23  65  6e  64  69  66
> 0003417

Kind of a moot point now, though, as I've pulled this file entirely from my patch. (I had tried to add some new constructors to the structs, but discovered that doing so would require pushing sipcc headers deep into other parts of the core code tree.)

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +946,5 @@
>      cc_uint16_t    salt_len;
>      char        fname[] = "vcmRxStart";
>  
>      CSFLogDebug( logTag, "%s: group_id=%d call_handle=%d payload=%d port=%d algID=%d",
> +        fname, group_id, call_handle, payload->remote_rtp_pt, port, algorithmID);

done

::: media/webrtc/signaling/src/sipcc/core/common/vcm_util.c
@@ +4,5 @@
>  
>  #include "debug.h"
>  #include "rtp_defs.h"
>  #include "vcm_util.h"
>  #include "ccsdp.h"

Whoops. I had meant to completely remove this file.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +227,5 @@
> +    if (media->num_payloads > media->previous_sdp.num_payloads)
> +    {
> +      cpr_free(media->previous_sdp.payloads);
> +      media->previous_sdp.payloads =
> +        cpr_malloc(media->num_payloads * sizeof(vcm_payload_info_t));

Changed.

@@ +233,5 @@
> +
> +    /* Copy the payloads over */
> +    media->previous_sdp.num_payloads = media->num_payloads;
> +    cpr_memcopy(media->previous_sdp.payloads, media->payloads,
> +        media->num_payloads * sizeof(vcm_payload_info_t));

I'd seen that most of the standard libc constructs had been using custom variants. Without knowing the reason, I figured that the least disruptive thing was to follow the pattern. Since I found evidence in dns_utils.c that cpr_memcopy() might have existed at some point, that seemed the safest thing to use.

In any case, as the reasons for the cpr_ prefix seem to be inappliable here, I've taken Ethan's suggestion and just gone directly to the libc function.

@@ +234,5 @@
> +    /* Copy the payloads over */
> +    media->previous_sdp.num_payloads = media->num_payloads;
> +    cpr_memcopy(media->previous_sdp.payloads, media->payloads,
> +        media->num_payloads * sizeof(vcm_payload_info_t));
> +    media->previous_sdp.num_payloads = media->num_payloads;

Right. This same thought occurred to me, with the same conclusion about its likelihood. I'd prefer not to throw another length field into the mix (one for "entries used," another for "entries available") to optimize a rare to nonexistent case.

@@ +3001,5 @@
> +                        payload_info->audio.channels = 1;
> +                        /* See http://www.iana.org/assignments/rtp-parameters/
> +                           rtp-parameters.xml */
> +                        switch (codec) {
> +                            case 6:

That's an interesting philosophical question.

The rtp_ptype enumeration should (in theory) have one entry for each kind of codec. By and large, it does. We *could* go in and add new entries to that enumeration for 10 and 11 (which are currently missing from that enumeration) -- but we'd have to call them something dodgy like "RTP_L16_THE_EMPIRE_STRIKES_BACK" and "RTP_L16_RETURN_OF_THE_JEDI" to distinguish them from the current "RTP_L16" constant (currently set to 102).

Joking aside, the presence of multiple RTP_L16_... values in the enumeration is likely to cause more global confusion than the slight amount of clarity having them might add to this particular switch statement. In fact, I would argue that the situation with DVI4 and H264 in that structure is already likely to give rise to bugs in the future. I would have cleaned the duplicates out in this patch if I were 100% sure doing so wouldn't cause problems.

@@ +3099,5 @@
> +                        case RTP_ISAC:
> +                            /* TODO: Update these from proper SDP constructs */
> +                            payload_info->audio.frequency = 16000;
> +                            payload_info->audio.packet_size = 480;
> +                            payload_info->audio.bitrate = 32000;

Fixed.

@@ +3229,5 @@
> +                 * current API's do not consider the request is for
> +                 * the same call. Need to update dynamic payload
> +                 * types for dynamic PT codecs. Would need to
> +                 * possibly add other video codecs as support is
> +                 * added here.

The comment and associated code, or just the comment? Without the comment, the code is pretty inscrutable. In fact, the code is a bit inscrutable even with the comment, but at least there's some indication that the odd dynamic PT maneuvering is being done for a reason.

::: media/webrtc/signaling/src/sipcc/core/includes/vcm_util.h
@@ +5,5 @@
>  #ifndef VCM_UTIL_H_
>  #define VCM_UTIL_H_
>  #include "vcm.h"
>  #include "phone_types.h"
>  

This entire file should have gone away also. Removing from the patch.

::: media/webrtc/signaling/src/sipcc/cpr/include/cpr_stdlib.h
@@ +18,5 @@
>  #define cpr_malloc(a) moz_xmalloc(a)
>  #define cpr_calloc(a, b) moz_xcalloc(a, b)
>  #define cpr_realloc(a, b) moz_xrealloc(a, b)
>  #define cpr_free(a) moz_free(a)
> +#define cpr_memcopy(a,b,c) memcpy(a,b,c)

As the code has been updated to just use libc memcpy, I'm pulling this file from the patch.
Attachment #688913 - Attachment is obsolete: false
Attachment #688913 - Attachment is obsolete: true
Attachment #689213 - Attachment is obsolete: true
Attachment #689244 - Flags: checkin?(ethanhugg)
Comment on attachment 689244 [details] [diff] [review]
Replace vcm_media_payload_t with structure leveraging rtp_ptype constants

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d853be2f998
Attachment #689244 - Flags: checkin?(ethanhugg) → checkin+
Try run for 51d0988459d1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=51d0988459d1
Results (out of 18 total builds):
    success: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/adam@nostrum.com-51d0988459d1
https://hg.mozilla.org/mozilla-central/rev/6d853be2f998
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whiteboard: [WebRTC] [blocking-webrtc-] → [WebRTC] [blocking-webrtc-] [qa-]
Try run for 51d0988459d1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=51d0988459d1
Results (out of 19 total builds):
    success: 18
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/adam@nostrum.com-51d0988459d1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: