[06/11] rtpdec_h264: Clean up comments

Message ID 1336169215-59789-6-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö May 4, 2012, 10:06 p.m.
Convert C++ comments into C comments, clean up the comment
content (remove trailing periods).
---
 libavformat/rtpdec_h264.c |   87 ++++++++++++++++++++++++---------------------
 1 file changed, 47 insertions(+), 40 deletions(-)

Comments

Alex Converse May 4, 2012, 10:15 p.m. | #1
On Fri, May 4, 2012 at 3:06 PM, Martin Storsjö <martin@martin.st> wrote:
> Convert C++ comments into C comments, clean up the comment
> content (remove trailing periods).

Whats wrong with c99 comments?

> ---
>  libavformat/rtpdec_h264.c |   87 ++++++++++++++++++++++++---------------------
>  1 file changed, 47 insertions(+), 40 deletions(-)
>
> diff --git a/libavformat/rtpdec_h264.c b/libavformat/rtpdec_h264.c
> index 505bc81..a567d3d 100644
> --- a/libavformat/rtpdec_h264.c
> +++ b/libavformat/rtpdec_h264.c
> @@ -20,7 +20,7 @@
>  */
>
>  /**
> -* @file
> + * @file
>  * @brief H.264 / RTP Code (RFC3984)
>  * @author Ryan Martell <rdm4@martellventures.com>
>  *
> @@ -29,7 +29,8 @@
>  * This currently supports packetization mode:
>  * Single Nal Unit Mode (0), or
>  * Non-Interleaved Mode (1).  It currently does not support
> - * Interleaved Mode (2). (This requires implementing STAP-B, MTAP16, MTAP24, FU-B packet types)
> + * Interleaved Mode (2). (This requires implementing STAP-B, MTAP16, MTAP24,
> + *                        FU-B packet types)
>  */
>
>  #include "libavutil/base64.h"
> @@ -46,7 +47,7 @@
>  #include "rtpdec_formats.h"
>
>  struct PayloadContext {
> -    //sdp setup parameters
> +    /* sdp setup parameters */
>     uint8_t profile_idc;
>     uint8_t profile_iop;
>     uint8_t level_idc;
> @@ -68,10 +69,11 @@ static int sdp_parse_fmtp_config_h264(AVStream * stream,
>         av_log(codec, AV_LOG_DEBUG, "RTP Packetization Mode: %d\n", atoi(value));
>         h264_data->packetization_mode = atoi(value);
>         /*
> -           Packetization Mode:
> -           0 or not present: Single NAL mode (Only nals from 1-23 are allowed)
> -           1: Non-interleaved Mode: 1-23, 24 (STAP-A), 28 (FU-A) are allowed.
> -           2: Interleaved Mode: 25 (STAP-B), 26 (MTAP16), 27 (MTAP24), 28 (FU-A), and 29 (FU-B) are allowed.
> +         * Packetization Mode:
> +         * 0 or not present: Single NAL mode (Only nals from 1-23 are allowed)
> +         * 1: Non-interleaved Mode: 1-23, 24 (STAP-A), 28 (FU-A) are allowed.
> +         * 2: Interleaved Mode: 25 (STAP-B), 26 (MTAP16), 27 (MTAP24), 28 (FU-A),
> +         *                      and 29 (FU-B) are allowed.
>          */

This isn't doxy, why the extra *s?

>         if (h264_data->packetization_mode > 1)
>             av_log(codec, AV_LOG_ERROR,
> @@ -79,7 +81,7 @@ static int sdp_parse_fmtp_config_h264(AVStream * stream,
>     } else if (!strcmp(attr, "profile-level-id")) {
>         if (strlen(value) == 6) {
>             char buffer[3];
> -            // 6 characters=3 bytes, in hex.
> +            /* 6 characters=3 bytes, in hex. */
>             uint8_t profile_idc;
>             uint8_t profile_iop;
>             uint8_t level_idc;
> @@ -149,7 +151,7 @@ static int sdp_parse_fmtp_config_h264(AVStream * stream,
>     return 0;
>  }
>
> -// return 0 on packet, no more left, 1 on packet, 1 on partial packet...
> +/* return 0 on packet, no more left, 1 on packet, 1 on partial packet */
>  static int h264_handle_packet(AVFormatContext *ctx,
>                               PayloadContext *data,
>                               AVStream *st,
> @@ -173,10 +175,12 @@ static int h264_handle_packet(AVFormatContext *ctx,
>     assert(data);
>     assert(buf);
>
> +    /* Simplify the case (these are all the nal types used internally by
> +     * the h264 codec). */
>     if (type >= 1 && type <= 23)
> -        type = 1;              // simplify the case. (these are all the nal types used internally by the h264 codec)
> +        type = 1;
>     switch (type) {
> -    case 0:                    // undefined, but pass them through
> +    case 0:                    /* undefined, but pass them through */
>     case 1:
>         av_new_packet(pkt, len+sizeof(start_sequence));
>         memcpy(pkt->data, start_sequence, sizeof(start_sequence));
> @@ -186,11 +190,11 @@ static int h264_handle_packet(AVFormatContext *ctx,
>  #endif
>         break;
>
> -    case 24:                   // STAP-A (one packet, multiple nals)
> -        // consume the STAP-A NAL
> +    case 24:                   /* STAP-A (one packet, multiple nals) */
> +        /* consume the STAP-A NAL */
>         buf++;
>         len--;
> -        // first we are going to figure out the total size....
> +        /* first we are going to figure out the total size */
>         {
>             int pass= 0;
>             int total_length= 0;
> @@ -203,16 +207,16 @@ static int h264_handle_packet(AVFormatContext *ctx,
>                 while (src_len > 2) {
>                     uint16_t nal_size = AV_RB16(src);
>
> -                    // consume the length of the aggregate...
> +                    /* consume the length of the aggregate */
>                     src += 2;
>                     src_len -= 2;
>
>                     if (nal_size <= src_len) {
>                         if(pass==0) {
> -                            // counting...
> +                            /* counting */
>                             total_length+= sizeof(start_sequence)+nal_size;
>                         } else {
> -                            // copying
> +                            /* copying */
>                             assert(dst);
>                             memcpy(dst, start_sequence, sizeof(start_sequence));
>                             dst+= sizeof(start_sequence);
> @@ -227,7 +231,7 @@ static int h264_handle_packet(AVFormatContext *ctx,
>                                "nal size exceeds length: %d %d\n", nal_size, src_len);
>                     }
>
> -                    // eat what we handled...
> +                    /* eat what we handled */
>                     src += nal_size;
>                     src_len -= nal_size;
>
> @@ -237,7 +241,8 @@ static int h264_handle_packet(AVFormatContext *ctx,
>                 }
>
>                 if(pass==0) {
> -                    // now we know the total size of the packet (with the start sequences added)
> +                    /* now we know the total size of the packet (with the
> +                     * start sequences added) */
>                     av_new_packet(pkt, total_length);
>                     dst= pkt->data;
>                 } else {
> @@ -247,21 +252,21 @@ static int h264_handle_packet(AVFormatContext *ctx,
>         }
>         break;
>
> -    case 25:                   // STAP-B
> -    case 26:                   // MTAP-16
> -    case 27:                   // MTAP-24
> -    case 29:                   // FU-B
> +    case 25:                   /* STAP-B  */
> +    case 26:                   /* MTAP-16 */
> +    case 27:                   /* MTAP-24 */
> +    case 29:                   /* FU-B    */
>         av_log(ctx, AV_LOG_ERROR,
>                "Unhandled type (%d) (See RFC for implementation details\n",
>                type);
>         result = AVERROR(ENOSYS);
>         break;
>
> -    case 28:                   // FU-A (fragmented nal)
> +    case 28:                   /* FU-A (fragmented nal) */
>         buf++;
> -        len--;                  // skip the fu_indicator
> +        len--;                 /* skip the fu_indicator */
>         if (len > 1) {
> -            // these are the same as above, we just redo them here for clarity...
> +            /* these are the same as above, we just redo them here for clarity */
>             uint8_t fu_indicator = nal;
>             uint8_t fu_header = *buf;
>             uint8_t start_bit = fu_header >> 7;
> @@ -269,11 +274,13 @@ static int h264_handle_packet(AVFormatContext *ctx,
>             uint8_t nal_type = (fu_header & 0x1f);
>             uint8_t reconstructed_nal;
>
> -            // reconstruct this packet's true nal; only the data follows..
> -            reconstructed_nal = fu_indicator & (0xe0);  // the original nal forbidden bit and NRI are stored in this packet's nal;
> +            /* Reconstruct this packet's true nal; only the data follows. */
> +            /* The original nal forbidden bit and NRI are stored in this
> +             * packet's nal. */
> +            reconstructed_nal = fu_indicator & (0xe0);
>             reconstructed_nal |= nal_type;
>
> -            // skip the fu_header...
> +            /* skip the fu_header */
>             buf++;
>             len--;
>
> @@ -282,7 +289,7 @@ static int h264_handle_packet(AVFormatContext *ctx,
>                 data->packet_types_received[nal_type]++;
>  #endif
>             if(start_bit) {
> -                // copy in the start sequence, and the reconstructed nal....
> +                /* copy in the start sequence, and the reconstructed nal */
>                 av_new_packet(pkt, sizeof(start_sequence)+sizeof(nal)+len);
>                 memcpy(pkt->data, start_sequence, sizeof(start_sequence));
>                 pkt->data[sizeof(start_sequence)]= reconstructed_nal;
> @@ -297,8 +304,8 @@ static int h264_handle_packet(AVFormatContext *ctx,
>         }
>         break;
>
> -    case 30:                   // undefined
> -    case 31:                   // undefined
> +    case 30:                   /* undefined */
> +    case 31:                   /* undefined */
>     default:
>         av_log(ctx, AV_LOG_ERROR, "Undefined type (%d)", type);
>         result = AVERROR_INVALIDDATA;
> @@ -347,24 +354,24 @@ static int parse_h264_sdp_line(AVFormatContext *s, int st_index,
>         char buf1[50];
>         char *dst = buf1;
>
> -        // remove the protocol identifier..
> -        while (*p && *p == ' ') p++; // strip spaces.
> -        while (*p && *p != ' ') p++; // eat protocol identifier
> -        while (*p && *p == ' ') p++; // strip trailing spaces.
> +        /* remove the protocol identifier */
> +        while (*p && *p == ' ') p++; /* strip spaces */
> +        while (*p && *p != ' ') p++; /* eat protocol identifier */
> +        while (*p && *p == ' ') p++; /* strip trailing spaces. */
>         while (*p && *p != '-' && (dst - buf1) < sizeof(buf1) - 1) {
>             *dst++ = *p++;
>         }
>         *dst = '\0';
>
> -        // a='framesize:96 320-240'
> -        // set our parameters..
> +        /* a='framesize:96 320-240' */
> +        /* set our parameters */
>         codec->width = atoi(buf1);
> -        codec->height = atoi(p + 1); // skip the -
> +        codec->height = atoi(p + 1); /* skip the - */
>         codec->pix_fmt = PIX_FMT_YUV420P;
>     } else if (av_strstart(p, "fmtp:", &p)) {
>         return ff_parse_fmtp(stream, h264_data, p, sdp_parse_fmtp_config_h264);
>     } else if (av_strstart(p, "cliprect:", &p)) {
> -        // could use this if we wanted.
> +        /* could use this if we wanted. */
>     }
>
>     return 0;
> --
> 1.7.9.4
>
Martin Storsjö May 4, 2012, 10:21 p.m. | #2
On Fri, 4 May 2012, Alex Converse wrote:

> On Fri, May 4, 2012 at 3:06 PM, Martin Storsjö <martin@martin.st> wrote:
>> Convert C++ comments into C comments, clean up the comment
>> content (remove trailing periods).
>
> Whats wrong with c99 comments?

Hmm, I thought some of our coding style guidelines preferred C style 
comments, but after looking at other common code, there's quite a bit of 
C99/C++ style comments around too, so I guess I can keep it in that style 
here, too.

>> ---
>>  libavformat/rtpdec_h264.c |   87 ++++++++++++++++++++++++---------------------
>>  1 file changed, 47 insertions(+), 40 deletions(-)
>>
>> diff --git a/libavformat/rtpdec_h264.c b/libavformat/rtpdec_h264.c
>> index 505bc81..a567d3d 100644
>> --- a/libavformat/rtpdec_h264.c
>> +++ b/libavformat/rtpdec_h264.c
>> @@ -20,7 +20,7 @@
>>  */
>>
>>  /**
>> -* @file
>> + * @file
>>  * @brief H.264 / RTP Code (RFC3984)
>>  * @author Ryan Martell <rdm4@martellventures.com>
>>  *
>> @@ -29,7 +29,8 @@
>>  * This currently supports packetization mode:
>>  * Single Nal Unit Mode (0), or
>>  * Non-Interleaved Mode (1).  It currently does not support
>> - * Interleaved Mode (2). (This requires implementing STAP-B, MTAP16, MTAP24, FU-B packet types)
>> + * Interleaved Mode (2). (This requires implementing STAP-B, MTAP16, MTAP24,
>> + *                        FU-B packet types)
>>  */
>>
>>  #include "libavutil/base64.h"
>> @@ -46,7 +47,7 @@
>>  #include "rtpdec_formats.h"
>>
>>  struct PayloadContext {
>> -    //sdp setup parameters
>> +    /* sdp setup parameters */
>>     uint8_t profile_idc;
>>     uint8_t profile_iop;
>>     uint8_t level_idc;
>> @@ -68,10 +69,11 @@ static int sdp_parse_fmtp_config_h264(AVStream * stream,
>>         av_log(codec, AV_LOG_DEBUG, "RTP Packetization Mode: %d\n", atoi(value));
>>         h264_data->packetization_mode = atoi(value);
>>         /*
>> -           Packetization Mode:
>> -           0 or not present: Single NAL mode (Only nals from 1-23 are allowed)
>> -           1: Non-interleaved Mode: 1-23, 24 (STAP-A), 28 (FU-A) are allowed.
>> -           2: Interleaved Mode: 25 (STAP-B), 26 (MTAP16), 27 (MTAP24), 28 (FU-A), and 29 (FU-B) are allowed.
>> +         * Packetization Mode:
>> +         * 0 or not present: Single NAL mode (Only nals from 1-23 are allowed)
>> +         * 1: Non-interleaved Mode: 1-23, 24 (STAP-A), 28 (FU-A) are allowed.
>> +         * 2: Interleaved Mode: 25 (STAP-B), 26 (MTAP16), 27 (MTAP24), 28 (FU-A),
>> +         *                      and 29 (FU-B) are allowed.
>>          */
>
> This isn't doxy, why the extra *s?

I prefer it this way, and I think this is the common style.

// Martin

Patch

diff --git a/libavformat/rtpdec_h264.c b/libavformat/rtpdec_h264.c
index 505bc81..a567d3d 100644
--- a/libavformat/rtpdec_h264.c
+++ b/libavformat/rtpdec_h264.c
@@ -20,7 +20,7 @@ 
  */
 
 /**
-* @file
+ * @file
  * @brief H.264 / RTP Code (RFC3984)
  * @author Ryan Martell <rdm4@martellventures.com>
  *
@@ -29,7 +29,8 @@ 
  * This currently supports packetization mode:
  * Single Nal Unit Mode (0), or
  * Non-Interleaved Mode (1).  It currently does not support
- * Interleaved Mode (2). (This requires implementing STAP-B, MTAP16, MTAP24, FU-B packet types)
+ * Interleaved Mode (2). (This requires implementing STAP-B, MTAP16, MTAP24,
+ *                        FU-B packet types)
  */
 
 #include "libavutil/base64.h"
@@ -46,7 +47,7 @@ 
 #include "rtpdec_formats.h"
 
 struct PayloadContext {
-    //sdp setup parameters
+    /* sdp setup parameters */
     uint8_t profile_idc;
     uint8_t profile_iop;
     uint8_t level_idc;
@@ -68,10 +69,11 @@  static int sdp_parse_fmtp_config_h264(AVStream * stream,
         av_log(codec, AV_LOG_DEBUG, "RTP Packetization Mode: %d\n", atoi(value));
         h264_data->packetization_mode = atoi(value);
         /*
-           Packetization Mode:
-           0 or not present: Single NAL mode (Only nals from 1-23 are allowed)
-           1: Non-interleaved Mode: 1-23, 24 (STAP-A), 28 (FU-A) are allowed.
-           2: Interleaved Mode: 25 (STAP-B), 26 (MTAP16), 27 (MTAP24), 28 (FU-A), and 29 (FU-B) are allowed.
+         * Packetization Mode:
+         * 0 or not present: Single NAL mode (Only nals from 1-23 are allowed)
+         * 1: Non-interleaved Mode: 1-23, 24 (STAP-A), 28 (FU-A) are allowed.
+         * 2: Interleaved Mode: 25 (STAP-B), 26 (MTAP16), 27 (MTAP24), 28 (FU-A),
+         *                      and 29 (FU-B) are allowed.
          */
         if (h264_data->packetization_mode > 1)
             av_log(codec, AV_LOG_ERROR,
@@ -79,7 +81,7 @@  static int sdp_parse_fmtp_config_h264(AVStream * stream,
     } else if (!strcmp(attr, "profile-level-id")) {
         if (strlen(value) == 6) {
             char buffer[3];
-            // 6 characters=3 bytes, in hex.
+            /* 6 characters=3 bytes, in hex. */
             uint8_t profile_idc;
             uint8_t profile_iop;
             uint8_t level_idc;
@@ -149,7 +151,7 @@  static int sdp_parse_fmtp_config_h264(AVStream * stream,
     return 0;
 }
 
-// return 0 on packet, no more left, 1 on packet, 1 on partial packet...
+/* return 0 on packet, no more left, 1 on packet, 1 on partial packet */
 static int h264_handle_packet(AVFormatContext *ctx,
                               PayloadContext *data,
                               AVStream *st,
@@ -173,10 +175,12 @@  static int h264_handle_packet(AVFormatContext *ctx,
     assert(data);
     assert(buf);
 
+    /* Simplify the case (these are all the nal types used internally by
+     * the h264 codec). */
     if (type >= 1 && type <= 23)
-        type = 1;              // simplify the case. (these are all the nal types used internally by the h264 codec)
+        type = 1;
     switch (type) {
-    case 0:                    // undefined, but pass them through
+    case 0:                    /* undefined, but pass them through */
     case 1:
         av_new_packet(pkt, len+sizeof(start_sequence));
         memcpy(pkt->data, start_sequence, sizeof(start_sequence));
@@ -186,11 +190,11 @@  static int h264_handle_packet(AVFormatContext *ctx,
 #endif
         break;
 
-    case 24:                   // STAP-A (one packet, multiple nals)
-        // consume the STAP-A NAL
+    case 24:                   /* STAP-A (one packet, multiple nals) */
+        /* consume the STAP-A NAL */
         buf++;
         len--;
-        // first we are going to figure out the total size....
+        /* first we are going to figure out the total size */
         {
             int pass= 0;
             int total_length= 0;
@@ -203,16 +207,16 @@  static int h264_handle_packet(AVFormatContext *ctx,
                 while (src_len > 2) {
                     uint16_t nal_size = AV_RB16(src);
 
-                    // consume the length of the aggregate...
+                    /* consume the length of the aggregate */
                     src += 2;
                     src_len -= 2;
 
                     if (nal_size <= src_len) {
                         if(pass==0) {
-                            // counting...
+                            /* counting */
                             total_length+= sizeof(start_sequence)+nal_size;
                         } else {
-                            // copying
+                            /* copying */
                             assert(dst);
                             memcpy(dst, start_sequence, sizeof(start_sequence));
                             dst+= sizeof(start_sequence);
@@ -227,7 +231,7 @@  static int h264_handle_packet(AVFormatContext *ctx,
                                "nal size exceeds length: %d %d\n", nal_size, src_len);
                     }
 
-                    // eat what we handled...
+                    /* eat what we handled */
                     src += nal_size;
                     src_len -= nal_size;
 
@@ -237,7 +241,8 @@  static int h264_handle_packet(AVFormatContext *ctx,
                 }
 
                 if(pass==0) {
-                    // now we know the total size of the packet (with the start sequences added)
+                    /* now we know the total size of the packet (with the
+                     * start sequences added) */
                     av_new_packet(pkt, total_length);
                     dst= pkt->data;
                 } else {
@@ -247,21 +252,21 @@  static int h264_handle_packet(AVFormatContext *ctx,
         }
         break;
 
-    case 25:                   // STAP-B
-    case 26:                   // MTAP-16
-    case 27:                   // MTAP-24
-    case 29:                   // FU-B
+    case 25:                   /* STAP-B  */
+    case 26:                   /* MTAP-16 */
+    case 27:                   /* MTAP-24 */
+    case 29:                   /* FU-B    */
         av_log(ctx, AV_LOG_ERROR,
                "Unhandled type (%d) (See RFC for implementation details\n",
                type);
         result = AVERROR(ENOSYS);
         break;
 
-    case 28:                   // FU-A (fragmented nal)
+    case 28:                   /* FU-A (fragmented nal) */
         buf++;
-        len--;                  // skip the fu_indicator
+        len--;                 /* skip the fu_indicator */
         if (len > 1) {
-            // these are the same as above, we just redo them here for clarity...
+            /* these are the same as above, we just redo them here for clarity */
             uint8_t fu_indicator = nal;
             uint8_t fu_header = *buf;
             uint8_t start_bit = fu_header >> 7;
@@ -269,11 +274,13 @@  static int h264_handle_packet(AVFormatContext *ctx,
             uint8_t nal_type = (fu_header & 0x1f);
             uint8_t reconstructed_nal;
 
-            // reconstruct this packet's true nal; only the data follows..
-            reconstructed_nal = fu_indicator & (0xe0);  // the original nal forbidden bit and NRI are stored in this packet's nal;
+            /* Reconstruct this packet's true nal; only the data follows. */
+            /* The original nal forbidden bit and NRI are stored in this
+             * packet's nal. */
+            reconstructed_nal = fu_indicator & (0xe0);
             reconstructed_nal |= nal_type;
 
-            // skip the fu_header...
+            /* skip the fu_header */
             buf++;
             len--;
 
@@ -282,7 +289,7 @@  static int h264_handle_packet(AVFormatContext *ctx,
                 data->packet_types_received[nal_type]++;
 #endif
             if(start_bit) {
-                // copy in the start sequence, and the reconstructed nal....
+                /* copy in the start sequence, and the reconstructed nal */
                 av_new_packet(pkt, sizeof(start_sequence)+sizeof(nal)+len);
                 memcpy(pkt->data, start_sequence, sizeof(start_sequence));
                 pkt->data[sizeof(start_sequence)]= reconstructed_nal;
@@ -297,8 +304,8 @@  static int h264_handle_packet(AVFormatContext *ctx,
         }
         break;
 
-    case 30:                   // undefined
-    case 31:                   // undefined
+    case 30:                   /* undefined */
+    case 31:                   /* undefined */
     default:
         av_log(ctx, AV_LOG_ERROR, "Undefined type (%d)", type);
         result = AVERROR_INVALIDDATA;
@@ -347,24 +354,24 @@  static int parse_h264_sdp_line(AVFormatContext *s, int st_index,
         char buf1[50];
         char *dst = buf1;
 
-        // remove the protocol identifier..
-        while (*p && *p == ' ') p++; // strip spaces.
-        while (*p && *p != ' ') p++; // eat protocol identifier
-        while (*p && *p == ' ') p++; // strip trailing spaces.
+        /* remove the protocol identifier */
+        while (*p && *p == ' ') p++; /* strip spaces */
+        while (*p && *p != ' ') p++; /* eat protocol identifier */
+        while (*p && *p == ' ') p++; /* strip trailing spaces. */
         while (*p && *p != '-' && (dst - buf1) < sizeof(buf1) - 1) {
             *dst++ = *p++;
         }
         *dst = '\0';
 
-        // a='framesize:96 320-240'
-        // set our parameters..
+        /* a='framesize:96 320-240' */
+        /* set our parameters */
         codec->width = atoi(buf1);
-        codec->height = atoi(p + 1); // skip the -
+        codec->height = atoi(p + 1); /* skip the - */
         codec->pix_fmt = PIX_FMT_YUV420P;
     } else if (av_strstart(p, "fmtp:", &p)) {
         return ff_parse_fmtp(stream, h264_data, p, sdp_parse_fmtp_config_h264);
     } else if (av_strstart(p, "cliprect:", &p)) {
-        // could use this if we wanted.
+        /* could use this if we wanted. */
     }
 
     return 0;