[FFmpeg-devel] [PATCH] avformat/concatdec: port to the new bitstream filter API

James Almer jamrial at gmail.com
Thu May 4 02:15:29 EEST 2017


On 5/3/2017 3:24 AM, Aaron Levinson wrote:
> I don't seem to have the original e-mail in my inbox anymore, so I'll
> respond to wm4's response.  Overall, the new code is a lot cleaner and
> easier to understand than the existing code.
> 
> See below for more.
> 
> Aaron Levinson
> 
> On 5/2/2017 5:04 PM, wm4 wrote:
>> On Fri, 28 Apr 2017 21:27:56 -0300
>> James Almer <jamrial at gmail.com> wrote:
>>
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>  libavformat/concatdec.c | 94
>>> ++++++++++++++++---------------------------------
>>>  1 file changed, 30 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
>>> index dd52e4d366..fa9443ff78 100644
>>> --- a/libavformat/concatdec.c
>>> +++ b/libavformat/concatdec.c
>>> @@ -34,8 +34,7 @@ typedef enum ConcatMatchMode {
>>>  } ConcatMatchMode;
>>>
>>>  typedef struct ConcatStream {
>>> -    AVBitStreamFilterContext *bsf;
>>> -    AVCodecContext *avctx;
>>> +    AVBSFContext *bsf;
>>>      int out_stream_index;
>>>  } ConcatStream;
>>>
>>> @@ -196,7 +195,8 @@ static int detect_stream_specific(AVFormatContext
>>> *avf, int idx)
>>>      ConcatContext *cat = avf->priv_data;
>>>      AVStream *st = cat->avf->streams[idx];
>>>      ConcatStream *cs = &cat->cur_file->streams[idx];
>>> -    AVBitStreamFilterContext *bsf;
>>> +    const AVBitStreamFilter *filter;
>>> +    AVBSFContext *bsf;
>>>      int ret;
>>>
>>>      if (cat->auto_convert && st->codecpar->codec_id ==
>>> AV_CODEC_ID_H264) {
>>> @@ -206,29 +206,28 @@ static int
>>> detect_stream_specific(AVFormatContext *avf, int idx)
>>>              return 0;
>>>          av_log(cat->avf, AV_LOG_INFO,
>>>                 "Auto-inserting h264_mp4toannexb bitstream filter\n");
>>> -        if (!(bsf = av_bitstream_filter_init("h264_mp4toannexb"))) {
>>> +        filter = av_bsf_get_by_name("h264_mp4toannexb");
>>> +        if (!filter) {
>>>              av_log(avf, AV_LOG_ERROR, "h264_mp4toannexb bitstream
>>> filter "
>>>                     "required for H.264 streams\n");
>>>              return AVERROR_BSF_NOT_FOUND;
>>>          }
>>> +        ret = av_bsf_alloc(filter, &bsf);
>>> +        if (ret < 0)
>>> +            return ret;
>>>          cs->bsf = bsf;
>>>
>>> -        cs->avctx = avcodec_alloc_context3(NULL);
>>> -        if (!cs->avctx)
>>> -            return AVERROR(ENOMEM);
>>> -
>>> -        /* This really should be part of the bsf work.
>>> -           Note: input bitstream filtering will not work with bsf that
>>> -           create extradata from the first packet. */
>>> -        av_freep(&st->codecpar->extradata);
>>> -        st->codecpar->extradata_size = 0;
>>> +        ret = avcodec_parameters_copy(bsf->par_in, st->codecpar);
>>> +        if (ret < 0)
>>> +           return ret;
>>>
>>> -        ret = avcodec_parameters_to_context(cs->avctx, st->codecpar);
>>> -        if (ret < 0) {
>>> -            avcodec_free_context(&cs->avctx);
>>> +        ret = av_bsf_init(bsf);
>>> +        if (ret < 0)
>>>              return ret;
>>> -        }
>>>
>>> +        ret = avcodec_parameters_copy(st->codecpar, bsf->par_out);
>>> +        if (ret < 0)
>>> +            return ret;
>>>      }
>>>      return 0;
>>>  }
> 
>>> @@ -291,8 +290,11 @@ static int match_streams(AVFormatContext *avf)
>>>      memset(map + cat->cur_file->nb_streams, 0,
>>>             (cat->avf->nb_streams - cat->cur_file->nb_streams) *
>>> sizeof(*map));
>>>
>>> -    for (i = cat->cur_file->nb_streams; i < cat->avf->nb_streams; i++)
>>> +    for (i = cat->cur_file->nb_streams; i < cat->avf->nb_streams;
>>> i++) {
>>>          map[i].out_stream_index = -1;
>>> +        if ((ret = detect_stream_specific(avf, i)) < 0)
>>> +            return ret;
>>> +    }
>>>      switch (cat->stream_match_mode) {
>>>      case MATCH_ONE_TO_ONE:
>>>          ret = match_streams_one_to_one(avf);
>>> @@ -305,9 +307,6 @@ static int match_streams(AVFormatContext *avf)
>>>      }
>>>      if (ret < 0)
>>>          return ret;
>>> -    for (i = cat->cur_file->nb_streams; i < cat->avf->nb_streams; i++)
>>> -        if ((ret = detect_stream_specific(avf, i)) < 0)
>>> -            return ret;
>>>      cat->cur_file->nb_streams = cat->avf->nb_streams;
>>>      return 0;
>>>  }
> 
> This just moves already existing code around.  While it is unclear to me
> why this is being done (a comment and/or log message would help), I
> would suspect that this particular change is unrelated to the purpose of
> this patch:  "port to the new bitstream filter API".

This is needed to actually propagate the filtered extradata to the
output file. As i said in a previous email, the bsf implementation
before this patch is completely broken.

> 
>>> @@ -370,10 +369,8 @@ static int concat_read_close(AVFormatContext *avf)
>>>      for (i = 0; i < cat->nb_files; i++) {
>>>          av_freep(&cat->files[i].url);
>>>          for (j = 0; j < cat->files[i].nb_streams; j++) {
>>> -            if (cat->files[i].streams[j].avctx)
>>> -                avcodec_free_context(&cat->files[i].streams[j].avctx);
>>>              if (cat->files[i].streams[j].bsf)
>>> -               
>>> av_bitstream_filter_close(cat->files[i].streams[j].bsf);
>>> +                av_bsf_free(&cat->files[i].streams[j].bsf);
>>>          }
>>>          av_freep(&cat->files[i].streams);
>>>          av_dict_free(&cat->files[i].metadata);
>>> @@ -524,56 +521,25 @@ static int open_next_file(AVFormatContext *avf)
>>>
>>>  static int filter_packet(AVFormatContext *avf, ConcatStream *cs,
>>> AVPacket *pkt)
>>>  {
>>> -    AVStream *st = avf->streams[cs->out_stream_index];
>>> -    AVBitStreamFilterContext *bsf;
>>> -    AVPacket pkt2;
>>>      int ret;
>>>
>>>      av_assert0(cs->out_stream_index >= 0);
> 
> I wonder if it is even relevant anymore to leave this av_assert0() call
> in, because even if somehow the condition is false, it probably doesn't
> matter since it isn't used anymore.

Sure, I'll remove it.

> 
>>> -    for (bsf = cs->bsf; bsf; bsf = bsf->next) {
>>> -        pkt2 = *pkt;
>>> -
>>> -        ret = av_bitstream_filter_filter(bsf, cs->avctx, NULL,
>>> -                                         &pkt2.data, &pkt2.size,
>>> -                                         pkt->data, pkt->size,
>>> -                                         !!(pkt->flags &
>>> AV_PKT_FLAG_KEY));
>>> +    if (cs->bsf) {
>>> +        ret = av_bsf_send_packet(cs->bsf, pkt);
>>>          if (ret < 0) {
>>> +            av_log(avf, AV_LOG_ERROR, "h264_mp4toannexb filter "
>>> +                   "failed to send input packet\n");
> 
> Would be nice to include the error code in the log message--same for the
> next one.

The error code is propagated and printed if needed (by the cli, for
example).

> 
>>>              av_packet_unref(pkt);
>>>              return ret;
>>>          }
>>>
>>> -        if (cs->avctx->extradata_size > st->codecpar->extradata_size) {
>>> -            int eret;
>>> -            if (st->codecpar->extradata)
>>> -                av_freep(&st->codecpar->extradata);
>>> -
>>> -            eret = ff_alloc_extradata(st->codecpar,
>>> cs->avctx->extradata_size);
>>> -            if (eret < 0) {
>>> -                av_packet_unref(pkt);
>>> -                return AVERROR(ENOMEM);
>>> -            }
>>> -            st->codecpar->extradata_size = cs->avctx->extradata_size;
>>> -            memcpy(st->codecpar->extradata, cs->avctx->extradata,
>>> cs->avctx->extradata_size);
>>> -        }
>>> -
>>> -        av_assert0(pkt2.buf);
>>> -        if (ret == 0 && pkt2.data != pkt->data) {
>>> -            if ((ret = av_copy_packet(&pkt2, pkt)) < 0) {
>>> -                av_free(pkt2.data);
>>> -                return ret;
>>> -            }
>>> -            ret = 1;
>>> -        }
>>> -        if (ret > 0) {
>>> +        ret = av_bsf_receive_packet(cs->bsf, pkt);
>>> +        if (ret < 0) {
>>> +            av_log(avf, AV_LOG_ERROR, "h264_mp4toannexb filter "
>>> +                   "failed to receive output packet\n");
> 
> According to the documentation for av_bsf_send_packet(), "After sending
> each packet, the filter must be completely drained by calling
> av_bsf_receive_packet() repeatedly until it returns AVERROR(EAGAIN) or
> AVERROR_EOF."  I would guess that might not apply for this special case
> decoder, but if so, I think it would be appropriate to document it in a
> comment.  Also, based on the documentation, it would appear that certain
> error returns are valid, but it treats all errors as failures here.

Unnecessary to call receive_packet repeatedly for h264_mp4toannexb it
seems, but i guess other bsfs may be added in the future where it could
be useful, so changed.

> 
> Also, I think that the intention would be a little clearer if you passed
> a local variable, say pkt2, into av_bsf_receive_packet() instead of pkt,
> the parameter passed to this function.  In the case of success, could
> then do *pkt = pkt2.
> 
>>>              av_packet_unref(pkt);
> 
> av_packet_unref() is now called in the case that av_bsf_receive_packet()
> returns an error.  I wonder if this is needed, since
> av_bsf_send_packet() takes ownership of the original packet if
> successful, and if av_bsf_receive_packet() returns an error, I would
> think that there would be no point to calling av_packet_unref(), as it
> shouldn't be returning an output packet in this case.

Looking at the code, you're right. It returns the packet only on success.

Removed then, and pushed.

> 
>>> -            pkt2.buf = av_buffer_create(pkt2.data, pkt2.size,
>>> -                                        av_buffer_default_free,
>>> NULL, 0);
>>> -            if (!pkt2.buf) {
>>> -                av_free(pkt2.data);
>>> -                return AVERROR(ENOMEM);
>>> -            }
>>> +            return ret;
>>>          }
>>> -        *pkt = pkt2;
>>>      }
>>>      return 0;
>>>  }
>>
>> I don't know the code, but I don't see anything obviously wrong. It
>> expects 1:1 input/output from the h264 BSF, but that's probably ok.
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



More information about the ffmpeg-devel mailing list