Re: [FFmpeg-devel] [PATCH] RTP: get the buffer information of the RTP extension header.

["Wujian(Chin)" <wujian2@huawei.com>]

Andreas：
>Wujian(Chin):
>> Signed-off-by: wujian_nanjing <wujian2@huawei.com>
>> ---
>>  libavcodec/avpacket.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>  libavcodec/packet.h   | 17 +++++++++++++++++
>>  libavformat/demux.c   |  7 +++++++
>>  libavformat/rtpdec.c  | 37 ++++++++++++++++++++++++++++---------
>>  4 files changed, 96 insertions(+), 9 deletions(-)
>> 
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index 
>> bcbc416..45c131a 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -46,6 +46,8 @@ void av_init_packet(AVPacket *pkt)
>>      pkt->opaque               = NULL;
>>      pkt->opaque_ref           = NULL;
>>      pkt->time_base            = av_make_q(0, 1);
>> +    pkt->ext                  = NULL;
>> +    pkt->extlen               = 0;
>>  }
>>  #endif
>>  
>> @@ -109,6 +111,46 @@ int av_new_packet(AVPacket *pkt, int size)
>>      return 0;
>>  }
>>  
>> +static void av_packet_free_ext(AVPacket *pkt) {
>> +    if (!pkt)
>> +        return;
>> +
>> +    if (pkt->ext) {
> >+        av_free(pkt->ext);
> >+        pkt->ext = NULL;
> >+    }
>> +
>> +    pkt->extlen = 0;
>> +}
> >+
> >+int av_set_packet_ext(AVPacket *pkt, uint8_t *buf, int len) {
> >+    if (!buf || (len <= 0))
> >+        return 0;
> >+
>> +    if (pkt->ext == buf) {
>> +        pkt->extlen = len;
>> +        return 0;
>> +    }
>> +
>> +    pkt->ext = av_malloc(len);
>> +    pkt->extlen = len;
>> +    (void)memcpy(pkt->ext, buf, len);
>> +
>> +    return 0;
>> +}
>> +
>> +static int av_copy_packet_ext(AVPacket *pkt, const AVPacket *src) {
>> +    pkt->ext = NULL;
>> +    pkt->extlen = 0;
>> +
>> +    av_set_packet_ext(pkt, src->ext, src->extlen);
>> +
>> +    return 0;
>> +}
>> +
>>  void av_shrink_packet(AVPacket *pkt, int size)  {
>>      if (pkt->size <= size)
>> @@ -422,6 +464,7 @@ int av_packet_copy_props(AVPacket *dst, const 
>> AVPacket *src)  void av_packet_unref(AVPacket *pkt)  {
>>      av_packet_free_side_data(pkt);
>> +    av_packet_free_ext(pkt);
>>      av_buffer_unref(&pkt->opaque_ref);
>>      av_buffer_unref(&pkt->buf);
>>      get_packet_defaults(pkt);
>> @@ -456,6 +499,7 @@ int av_packet_ref(AVPacket *dst, const AVPacket *src)
>>      }
>>  
>>      dst->size = src->size;
>> +    av_copy_packet_ext(dst, src);
>>  
>>      return 0;
>>  fail:
>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h index 
>> 404d520..137d1fa 100644
>> --- a/libavcodec/packet.h
>> +++ b/libavcodec/packet.h
>> @@ -374,6 +374,13 @@ typedef struct AVPacket {
>>      uint8_t *data;
>>      int   size;
>>      int   stream_index;
>> +
>> +    /**
>> +     * Extension header data and size
>> +     */
>> +    uint8_t *ext;
>> +    uint32_t extlen;
>> +
>>      /**
>>       * A combination of AV_PKT_FLAG values
>>       */
>> @@ -523,6 +530,16 @@ void av_init_packet(AVPacket *pkt);  int 
>> av_new_packet(AVPacket *pkt, int size);
>>  
>>  /**
>> + * Allocate the ext data from buf with len lenght.
>> + * Make sure pkt->ext is null, otherwise cause memory leak.
>> + *
>> + * @param pkt packet
>> + * @param buf buffer with ext data
>> + * @param len buffer size
>> + */
>> +int av_set_packet_ext(AVPacket *pkt, uint8_t *buf, int len);
>> +
>> +/**
>>   * Reduce packet size, correctly zeroing padding
>>   *
>>   * @param pkt packet
>> diff --git a/libavformat/demux.c b/libavformat/demux.c index 
>> 1620716..2a8b201 100644
>> --- a/libavformat/demux.c
>> +++ b/libavformat/demux.c
>> @@ -1175,6 +1175,13 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt,
>>              pkt->side_data_elems    = 0;
>>          }
>>  
>> +        if (pkt->extlen > 0) {
>> +            out_pkt->extlen = pkt->extlen;
>> +            out_pkt->ext = pkt->ext;
>> +            pkt->extlen = 0;
>> +            pkt->ext = NULL;
>> +        }
>> +
>>          /* set the duration */
>>          out_pkt->duration = (sti->parser->flags & PARSER_FLAG_COMPLETE_FRAMES) ? pkt->duration : 0;
>>          if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) { diff 
>> --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c index 
>> fa7544c..2a539e0 100644
>> --- a/libavformat/rtpdec.c
>> +++ b/libavformat/rtpdec.c
>> @@ -699,13 +699,14 @@ static int 
>> rtp_parse_packet_internal(RTPDemuxContext *s, AVPacket *pkt,  {
>>      unsigned int ssrc;
>>      int payload_type, seq, flags = 0;
>> -    int ext, csrc;
>> +    int has_ext, csrc, extlen;
>> +    uint8_t *extbuf;
>>      AVStream *st;
>>      uint32_t timestamp;
>>      int rv = 0;
>>  
>>      csrc         = buf[0] & 0x0f;
>> -    ext          = buf[0] & 0x10;
>> +    has_ext      = buf[0] & 0x10;
>>      payload_type = buf[1] & 0x7f;
>>      if (buf[1] & 0x80)
>>          flags |= RTP_FLAG_MARKER;
>> @@ -744,18 +745,25 @@ static int rtp_parse_packet_internal(RTPDemuxContext *s, AVPacket *pkt,
>>          return AVERROR_INVALIDDATA;
>>  
>>      /* RFC 3550 Section 5.3.1 RTP Header Extension handling */
>> -    if (ext) {
>> +    if (has_ext) {
>>          if (len < 4)
>>              return -1;
>>          /* calculate the header extension length (stored as number
>>           * of 32-bit words) */
>> -        ext = (AV_RB16(buf + 2) + 1) << 2;
>> +        extlen = (AV_RB16(buf + 2) + 1) << 2;
>>  
>> -        if (len < ext)
>> +        if (len < extlen)
>>              return -1;
>> -        // skip past RTP header extension
>> -        len -= ext;
>> -        buf += ext;
>> +
>> +         // Save RTP header extension data
>> +        if (extlen >> 0) {
>> +            extbuf = av_malloc(extlen);
>> +            if (extbuf)
>> +                (void)memcpy(extbuf, buf, extlen);
>> +        }
>> +
>> +        len -= extlen;
>> +        buf += extlen;
>>      }
>>  
>>      if (s->>handler && s->>handler->>parse_packet) { @@ -763,14 +771,25 
>> @@ static int rtp_parse_packet_internal(RTPDemuxContext *s, AVPacket *pkt,
>>                                        s->>st, pkt, &timestamp, buf, len, seq,
>>                                        flags);
>>      } else if (st) {
>> -        if ((rv = av_new_packet(pkt, len)) < 0)
>> +        if ((rv = av_new_packet(pkt, len)) < 0) {
>> +            if ((extlen >> 0) && extbuf)
>> +                av_free(extbuf);
>>              return rv;
>> +        }
>>          memcpy(pkt->>data, buf, len);
>>          pkt->>stream_index = st->>index;
>>      } else {
>> +        if ((extlen >> 0) && extbuf)
>> +            av_free(extbuf);
>>          return AVERROR(EINVAL);
>>      }
>>  
>> +    // attach ext data
>> +    if ((extlen >> 0) && extbuf) {
>> +        av_set_packet_ext(pkt, extbuf, extlen);
>> +        av_free(extbuf);
>> +    }
>> +
>>      // now perform timestamp things....
>>      finalize_packet(s, pkt, timestamp);
>>  

>1. Adding something in the middle of a public struct is an ABI break.
>2. Currently sizeof(AVPacket) is still part of the public ABI, so one can't even add something add the end of AVPacket.
>3. But it doesn't matter, because such stuff belongs into packet side data.
>4. The allocation in av_set_packet_ext() is unchecked.
>5. Don't cast the return value of memcpy() to void.
>6. There is a problem with parse_packet(): In case the parser adds delay, the side data and (if this patch were merged as-is) also your extension header data will end up with the wrong packet (if your parser adds a one-packet delay, then packet n will end up with the side data for packet n + 1). The reason for this is that the parsing API is ancient and directly works with buffers and not with AVPackets.

>- Andreas
1. Adding something in the middle of a public struct is an ABI break.
  --Do you have any good advice?

6. There is a problem with parse_packet(): In case the parser adds delay, the side data and (if this patch were merged as-is) also your extension header data will end up with the wrong packet (if your parser adds a one-packet delay, then packet n will end up with the side data for packet n + 1). The reason for this is that the parsing API is ancient and directly works with buffers and not with AVPackets.
  --Only parses and obtains the buffer. The caller determines whether the buffer is correct.

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".