[FFmpeg-devel] [PATCH 16/50] avformat/dv: use av_packet_alloc() to allocate packets

Andreas Rheinhardt andreas.rheinhardt at gmail.com
Sun Feb 7 09:47:56 EET 2021


James Almer:
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
> This one must be committed after the upcoming major bump, as it changes
> the return value of an avpriv function.
> 
>  libavdevice/iec61883.c |  2 +-
>  libavformat/dv.c       | 56 ++++++++++++++++++++++++++++--------------
>  2 files changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/libavdevice/iec61883.c b/libavdevice/iec61883.c
> index cafafb2672..4582ada151 100644
> --- a/libavdevice/iec61883.c
> +++ b/libavdevice/iec61883.c
> @@ -191,7 +191,7 @@ static int iec61883_parse_queue_dv(struct iec61883_data *dv, AVPacket *pkt)
>      int size;
>  
>      size = avpriv_dv_get_packet(dv->dv_demux, pkt);
> -    if (size > 0)
> +    if (size)
>          return size;
>  
>      packet = dv->queue_first;
> diff --git a/libavformat/dv.c b/libavformat/dv.c
> index 26a78139f5..f5c0c44afc 100644
> --- a/libavformat/dv.c
> +++ b/libavformat/dv.c
> @@ -45,7 +45,7 @@ struct DVDemuxContext {
>      AVFormatContext*  fctx;
>      AVStream*         vst;
>      AVStream*         ast[4];
> -    AVPacket          audio_pkt[4];
> +    AVPacket          *audio_pkt[4];
>      uint8_t           audio_buf[4][8192];
>      int               ach;
>      int               frames;
> @@ -261,11 +261,10 @@ static int dv_extract_audio_info(DVDemuxContext *c, const uint8_t *frame)
>              c->ast[i]->codecpar->codec_type = AVMEDIA_TYPE_AUDIO;
>              c->ast[i]->codecpar->codec_id   = AV_CODEC_ID_PCM_S16LE;
>  
> -            av_init_packet(&c->audio_pkt[i]);
> -            c->audio_pkt[i].size         = 0;
> -            c->audio_pkt[i].data         = c->audio_buf[i];
> -            c->audio_pkt[i].stream_index = c->ast[i]->index;
> -            c->audio_pkt[i].flags       |= AV_PKT_FLAG_KEY;
> +            c->audio_pkt[i]->size         = 0;
> +            c->audio_pkt[i]->data         = c->audio_buf[i];
> +            c->audio_pkt[i]->stream_index = c->ast[i]->index;
> +            c->audio_pkt[i]->flags       |= AV_PKT_FLAG_KEY;
>          }
>          c->ast[i]->codecpar->sample_rate    = dv_audio_frequency[freq];
>          c->ast[i]->codecpar->channels       = 2;
> @@ -332,6 +331,12 @@ static int dv_init_demux(AVFormatContext *s, DVDemuxContext *c)
>      c->vst->codecpar->bit_rate   = 25000000;
>      c->vst->start_time        = 0;
>  
> +    for (int i = 0; i < 4; i++) {
> +        c->audio_pkt[i] = av_packet_alloc();
> +        if (!c->audio_pkt[i])
> +            return AVERROR(ENOMEM);

This leads to lots of leaks on error, for both avpriv_dv_init_demux (for
which a free function will be necessary) as well as the dv demuxer
(whose read_close function is not called automatically on error).

> +    }
> +
>      return 0;
>  }
>  
> @@ -353,13 +358,15 @@ DVDemuxContext *avpriv_dv_init_demux(AVFormatContext *s)
>  
>  int avpriv_dv_get_packet(DVDemuxContext *c, AVPacket *pkt)
>  {
> -    int size = -1;
> +    int size = 0;
>      int i;
>  
>      for (i = 0; i < c->ach; i++) {
> -        if (c->ast[i] && c->audio_pkt[i].size) {
> -            *pkt                 = c->audio_pkt[i];
> -            c->audio_pkt[i].size = 0;
> +        if (c->ast[i] && c->audio_pkt[i]->size) {
> +            int ret = av_packet_ref(pkt, c->audio_pkt[i]);
> +            if (ret < 0)
> +                return ret;
> +            c->audio_pkt[i]->size = 0;
>              size                 = pkt->size;
>              break;
>          }
> @@ -384,9 +391,9 @@ int avpriv_dv_produce_packet(DVDemuxContext *c, AVPacket *pkt,
>      /* FIXME: in case of no audio/bad audio we have to do something */
>      size = dv_extract_audio_info(c, buf);
>      for (i = 0; i < c->ach; i++) {
> -        c->audio_pkt[i].pos  = pos;
> -        c->audio_pkt[i].size = size;
> -        c->audio_pkt[i].pts  = (c->sys->height == 720) ? (c->frames & ~1) : c->frames;
> +        c->audio_pkt[i]->pos  = pos;
> +        c->audio_pkt[i]->size = size;
> +        c->audio_pkt[i]->pts  = (c->sys->height == 720) ? (c->frames & ~1) : c->frames;
>          ppcm[i] = c->audio_buf[i];
>      }
>      if (c->ach)
> @@ -396,15 +403,14 @@ int avpriv_dv_produce_packet(DVDemuxContext *c, AVPacket *pkt,
>       * channels 0,1 and odd 2,3. */
>      if (c->sys->height == 720) {
>          if (buf[1] & 0x0C) {
> -            c->audio_pkt[2].size = c->audio_pkt[3].size = 0;
> +            c->audio_pkt[2]->size = c->audio_pkt[3]->size = 0;
>          } else {
> -            c->audio_pkt[0].size = c->audio_pkt[1].size = 0;
> +            c->audio_pkt[0]->size = c->audio_pkt[1]->size = 0;
>          }
>      }
>  
>      /* Now it's time to return video packet */
>      size = dv_extract_video_info(c, buf);
> -    av_init_packet(pkt);

This function here is a bit nuts: It predates reference-counted buffers
and so it simply resets pkt->buf (it in fact treats pkt purely as
something for output as if it were already blank). The mov and avi
demuxers work around this by copying pkt->buf and putting it in the
returned packet afterwards (which is probably unnecessary when the above
call is ; whereas the iec61883 device/demuxer really puts a blank packet
in there and makes it refcounted afterwards. It would be nice if this
whole mess were to be cleaned up.

>      pkt->data         = buf;
>      pkt->pos          = pos;
>      pkt->size         = size;
> @@ -439,8 +445,8 @@ static int64_t dv_frame_offset(AVFormatContext *s, DVDemuxContext *c,
>  void ff_dv_offset_reset(DVDemuxContext *c, int64_t frame_offset)
>  {
>      c->frames = frame_offset;
> -    c->audio_pkt[0].size = c->audio_pkt[1].size = 0;
> -    c->audio_pkt[2].size = c->audio_pkt[3].size = 0;
> +    c->audio_pkt[0]->size = c->audio_pkt[1]->size = 0;
> +    c->audio_pkt[2]->size = c->audio_pkt[3]->size = 0;
>  }
>  
>  /************************************************************
> @@ -539,7 +545,7 @@ static int dv_read_packet(AVFormatContext *s, AVPacket *pkt)
>  
>      size = avpriv_dv_get_packet(&c->dv_demux, pkt);
>  
> -    if (size < 0) {
> +    if (!size) {
>          int ret;
>          int64_t pos = avio_tell(s->pb);
>          if (!c->dv_demux.sys)
> @@ -614,6 +620,17 @@ static int dv_probe(const AVProbeData *p)
>      return 0;
>  }
>  
> +static int dv_read_close(AVFormatContext *s)
> +{
> +    RawDVContext *r   = s->priv_data;
> +    DVDemuxContext *c = &r->dv_demux;
> +
> +    for (int i = 0; i < 4; i++)
> +        av_packet_free(&c->audio_pkt[i]);
> +
> +    return 0;
> +}
> +
>  AVInputFormat ff_dv_demuxer = {
>      .name           = "dv",
>      .long_name      = NULL_IF_CONFIG_SMALL("DV (Digital Video)"),
> @@ -622,5 +639,6 @@ AVInputFormat ff_dv_demuxer = {
>      .read_header    = dv_read_header,
>      .read_packet    = dv_read_packet,
>      .read_seek      = dv_read_seek,
> +    .read_close     = dv_read_close,
>      .extensions     = "dv,dif",
>  };
> 



More information about the ffmpeg-devel mailing list