[FFmpeg-devel] [RFC] RTP headers cleanup

[Ronald S. Bultje]

Hi Luca,

I just read your patches, I have no comments at all basically (just
what was already said about the rtpenc_h264/etc.h which seems kind of
overkill). There's a bunch of whitespace changes that you might want
to remove or modify. See below.

For the first patch:

On Thu, Jan 15, 2009 at 5:42 PM, Luca Abeni <lucabe72 at email.it> wrote:
> --- a/libavformat/rtp.h
> +++ b/libavformat/rtp.h
> @@ -1,6 +1,7 @@
>  /*
>   * RTP definitions
>   * Copyright (c) 2002 Fabrice Bellard.
> + * Copyright (c) 2006 Ryan Martell <rdm4 at martellventures.com>
>   *
>   * This file is part of FFmpeg.
>   *

You're removing this again in your next patch, so I'd just remove this
from this patch and add him to rtpdec.h in the end.

> @@ -18,11 +19,12 @@
>   * License along with FFmpeg; if not, write to the Free Software
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   */
> +
>  #ifndef AVFORMAT_RTP_H
>  #define AVFORMAT_RTP_H
[..]
>  #include "libavcodec/avcodec.h"
> -#include "avformat.h"
>
>  /** Structure listing useful vars to parse RTP packet payload*/
>  typedef struct rtp_payload_data
[..]
>  #endif /* AVFORMAT_RTP_H */
> +

This you should all do in a separate patch(es) (one for whitespace,
one for copyright, one for the removal of avformat.h-include), I don't
think it's related to the patch (or to each other). Other than that,
looks good.

Second patch:

> diff --git a/libavformat/rtp.h b/libavformat/rtp.h
> index dd84154..38709ee 100644
> --- a/libavformat/rtp.h
> +++ b/libavformat/rtp.h
> -
> -#define RTP_MIN_PACKET_LENGTH 12
> -#define RTP_MAX_PACKET_LENGTH 1500 /* XXX: suppress this define */
> -
[..]
> -
> -/** return < 0 if unknown payload type */
> -int rtp_get_payload_type(AVCodecContext *codec);
> -
[..]
> +/** return < 0 if unknown payload type */
> +int rtp_get_payload_type(AVCodecContext *codec);
>
> -void av_register_rtp_dynamic_payload_handlers(void);
> +#define RTP_MAX_PACKET_LENGTH 1500 /* XXX: suppress this define */
>
>  #endif /* AVFORMAT_RTP_H */
> -

This is all whitespace (also the last line removal), can this be suppressed?

> diff --git a/libavformat/rtp_aac.c b/libavformat/rtp_aac.c
> index e54ff3f..60097f1 100644
> --- a/libavformat/rtp_aac.c
> +++ b/libavformat/rtp_aac.c
> @@ -53,15 +53,15 @@ void ff_rtp_send_aac(AVFormatContext *s1, const uint8_t *buff, int size)
>
>          ff_rtp_send_data(s1, p, s->buf_ptr - p, 1);
>
> -        s->read_buf_index = 0;
> +        s->num_frames = 0;
>      }
> -    if (s->read_buf_index == 0) {
> +    if (s->num_frames == 0) {
>          s->buf_ptr = s->buf + MAX_AU_HEADERS_SIZE;
>          s->timestamp = s->cur_timestamp;
>      }
>
>      if (size < max_packet_size) {
> -        p = s->buf + s->read_buf_index++ * 2 + 2;
> +        p = s->buf + s->num_frames++ * 2 + 2;
>          *p++ = size >> 5;
>          *p = (size & 0x1F) << 3;
>          memcpy(s->buf_ptr, buff, size);

Can the rename from read_buf_index to num_frames also be done separately?

Ronald