[FFmpeg-devel] [PATCH] avformat/concatdec: port to the new bitstream filter API
Aaron Levinson
alevinsn at aracnet.com
Wed May 3 09:24:46 EEST 2017
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".
>> @@ -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.
>> - 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.
>> 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.
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.
>> - 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
>
More information about the ffmpeg-devel
mailing list