[FFmpeg-devel] [RFC] RTP headers cleanup

[Luca Abeni]

Hi Ronald,

Ronald S. Bultje wrote:
> 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).

Yes... I fixed that in my local copy.


> There's a bunch of whitespace changes that you might want
> to remove or modify. See below.

Thanks for noticing; I am removing these unneeded whitespace changes in
my local copy.


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

I wanted to ensure that all the intermediate versions are correct (with
the correct copyright attribution). So, if the changes are committed in
two steps I think Ryan's copyright should be here...


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

Well, whitespaces are there by error, and I am removing them;
avformat.h's removal is also an error (fixed locally), but I think
the copyright should go together with the code...

>> 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?

Yes, fixed here; thanks.


>> 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?

I did that originally, but then I realized that this patch is introducing
a new structure (RTPMuxContext) and it does not make too much sense to
introduce it with a misnamed field.



			Thanks,
				Luca