[FFmpeg-devel] [PATCH] reformat rtsp.c

Benoit Fouet benoit.fouet
Tue Jan 5 09:28:20 CET 2010


Hi,

On Mon, 4 Jan 2010 19:12:17 -0500 Ronald S. Bultje wrote:
> Hi,
> 
> $subj, requested by Diego.
> 
> How do I format this? The following looks logical, but is ugly:
> 
> switch (x) {
> case x: {
>     int bla;
> }
> }
> 
> Ronald
> 
> Index: libavformat/rtsp.c
> ===================================================================
> --- libavformat/rtsp.c	(revision 21020)
> +++ libavformat/rtsp.c	(working copy)
> @@ -101,23 +103,24 @@
> 
> [...]
> 
>      } else {
> -        /* We are in a standard case ( from http://www.iana.org/assignments/rtp-parameters) */
> +        /* We are in a standard case
> +         * ( from http://www.iana.org/assignments/rtp-parameters). */

I'd remove the space between the parenthesis and from

> @@ -160,18 +167,18 @@
>      return -1;
>  }
>  
> -/* return the length and optionnaly the data */
> +/** return the length and optionally the data */
>  static int hex_to_data(uint8_t *data, const char *p)
>  {
>      int c, len, v;
>  
>      len = 0;
>      v = 1;
> -    for(;;) {
> +    for (;;) {
>          skip_spaces(&p);
>          if (*p == '\0')
>              break;
> -        c = toupper((unsigned char)*p++);
> +        c = toupper((unsigned char) *p++);

I'd leave the space out

> @@ -193,33 +200,33 @@
> 
> [...]
> 
> +    case CODEC_ID_MPEG4:
> +    case CODEC_ID_AAC:
> +        if (!strcmp(attr, "config")) {
> +            /* decode the hexa encoded parameter */
> +            int len = hex_to_data(NULL, value);
> +            if (codec->extradata)
> +                av_free(codec->extradata);

not related to the prettyfication, but the if is useless

> @@ -227,22 +234,29 @@
>  #define ATTR_NAME_TYPE_STR 1
>  static const AttrNameMap attr_names[]=
>  {
> -    {"SizeLength",       ATTR_NAME_TYPE_INT, offsetof(RTPPayloadData, sizelength)},
> -    {"IndexLength",      ATTR_NAME_TYPE_INT, offsetof(RTPPayloadData, indexlength)},
> -    {"IndexDeltaLength", ATTR_NAME_TYPE_INT, offsetof(RTPPayloadData, indexdeltalength)},
> -    {"profile-level-id", ATTR_NAME_TYPE_INT, offsetof(RTPPayloadData, profile_level_id)},
> -    {"StreamType",       ATTR_NAME_TYPE_INT, offsetof(RTPPayloadData, streamtype)},
> -    {"mode",             ATTR_NAME_TYPE_STR, offsetof(RTPPayloadData, mode)},
> -    {NULL, -1, -1},
> +    { "SizeLength",       ATTR_NAME_TYPE_INT,
> +      offsetof(RTPPayloadData, sizelength) },
> +    { "IndexLength",      ATTR_NAME_TYPE_INT,
> +      offsetof(RTPPayloadData, indexlength) },
> +    { "IndexDeltaLength", ATTR_NAME_TYPE_INT,
> +      offsetof(RTPPayloadData, indexdeltalength) },
> +    { "profile-level-id", ATTR_NAME_TYPE_INT,
> +      offsetof(RTPPayloadData, profile_level_id) },
> +    { "StreamType",       ATTR_NAME_TYPE_INT,
> +      offsetof(RTPPayloadData, streamtype) },
> +    { "mode",             ATTR_NAME_TYPE_STR,
> +      offsetof(RTPPayloadData, mode) },
> +    { NULL, -1, -1 },

I find the former version much more readable than the latter

> @@ -262,24 +276,26 @@
>       * encoded, giving a 12KB * (4/3) = 16KB FMTP line. */
>      char value[16384];
>      int i;
> -
>      RTSPStream *rtsp_st = st->priv_data;
>      AVCodecContext *codec = st->codec;
>      RTPPayloadData *rtp_payload_data = &rtsp_st->rtp_payload_data;
>  
>      /* loop on each attribute */
> -    while(rtsp_next_attr_and_value(&p, attr, sizeof(attr), value, sizeof(value)))
> -    {
> -        /* grab the codec extra_data from the config parameter of the fmtp line */
> +    while (rtsp_next_attr_and_value(&p, attr, sizeof(attr),
> +           value, sizeof(value))) {

align with the arguments ?

> @@ -1160,7 +1182,7 @@
>      return 0;
>  
>  fail:
> -    for (i=0; i<rt->nb_rtsp_streams; i++) {
> +    for (i = 0; i<rt->nb_rtsp_streams; i++) {

I don't understand why the '=' and not the '<'

> @@ -1298,9 +1321,8 @@
>              continue;
>          } else if (!strncasecmp(reply->server, "WMServer/", 9)) {
>              rt->server_type = RTSP_SERVER_WMS;
> -        } else if (rt->server_type == RTSP_SERVER_REAL) {
> +        } else if (rt->server_type == RTSP_SERVER_REAL)
>              strcpy(real_challenge, reply->real_challenge);
> -        }

why ?

> @@ -1536,22 +1557,20 @@
>          return len;
>      if (len == 0)
>          return AVERROR_EOF;
> -    if (rt->transport == RTSP_TRANSPORT_RDT)
> +    if (rt->transport == RTSP_TRANSPORT_RDT) {
>          ret = ff_rdt_parse_packet(rtsp_st->transport_priv, pkt, buf, len);
> -    else
> +    } else
>          ret = rtp_parse_packet(rtsp_st->transport_priv, pkt, buf, len);

this syntax looks weird to me (but IIRC it was already discussed)

Ben



More information about the ffmpeg-devel mailing list