[FFmpeg-devel] [PATCH 02/12] lavf: update auto-bsf to new BSF API

[Rodger Combs]

> On Apr 27, 2016, at 07:29, Nicolas George <george at nsup.org> wrote:
> 
> Le nonidi 9 floréal, an CCXXIV, Rodger Combs a écrit :
>> ---
>> libavformat/internal.h |  5 +++--
>> libavformat/mux.c      | 40 +++++++++++++++++++++++++---------
>> libavformat/segment.c  |  6 +++--
>> libavformat/utils.c    | 59 +++++++++++++++++++++++++++++++++++++++++---------
>> 4 files changed, 86 insertions(+), 24 deletions(-)
>> 
>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>> index 40ba089..52f9eb6 100644
>> --- a/libavformat/internal.h
>> +++ b/libavformat/internal.h
>> @@ -134,11 +134,12 @@ struct AVStreamInternal {
>>     int reorder;
>> 
>>     /**
>> -     * bitstream filter to run on stream
>> +     * bitstream filters to run on stream
>>      * - encoding: Set by muxer using ff_stream_add_bitstream_filter
>>      * - decoding: unused
>>      */
>> -    AVBitStreamFilterContext *bsfc;
>> +    AVBSFContext **bsfcs;
>> +    int nb_bsfcs;
>> 
>>     /**
>>      * Whether or not check_bitstream should still be run on each packet
>> diff --git a/libavformat/mux.c b/libavformat/mux.c
>> index 91388e3..72e284a 100644
>> --- a/libavformat/mux.c
>> +++ b/libavformat/mux.c
>> @@ -1043,7 +1043,7 @@ static int interleave_packet(AVFormatContext *s, AVPacket *out, AVPacket *in, in
>> 
>> int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt)
>> {
>> -    int ret, flush = 0;
>> +    int ret, flush = 0, i;
>> 
>>     ret = prepare_input_packet(s, pkt);
>>     if (ret < 0)
>> @@ -1061,15 +1061,35 @@ int av_interleaved_write_frame(AVFormatContext *s, AVPacket *pkt)
>>             }
>>         }
>> 
>> -        av_apply_bitstream_filters(st->internal->avctx, pkt, st->internal->bsfc);
>> -        if (pkt->size == 0 && pkt->side_data_elems == 0)
>> -            return 0;
>> -        if (!st->codecpar->extradata && st->internal->avctx->extradata) {
>> -            int eret = ff_alloc_extradata(st->codecpar, st->internal->avctx->extradata_size);
>> -            if (eret < 0)
>> -                return AVERROR(ENOMEM);
>> -            st->codecpar->extradata_size = st->internal->avctx->extradata_size;
>> -            memcpy(st->codecpar->extradata, st->internal->avctx->extradata, st->internal->avctx->extradata_size);
> 
>> +        for (i = 0; i < st->internal->nb_bsfcs; i++) {
>> +            AVBSFContext* ctx = st->internal->bsfcs[i];
> 
> Style nit: the pointer mark is part of the variable, not the type.
Fixed.

> 
> And: Unless I am mistaken, for now, we only ever add a single bitstream
> filter. The new API is tricky with several filters. I suggest you write the
> code for a single filter and leave implementing several until we actually
> need it, and therefore have a test case for it.
There's definitely going to be some trickiness around this if we end up having 1:N filters, or filters that actually requiring flushing. Otherwise, though, this should work fine, and we can address multiple-bsf or more-advanced BSF cases when they come up.

> 
>> +            if (i > 0) {
>> +                AVBSFContext* prev_ctx = st->internal->bsfcs[i - 1];
>> +                if (prev_ctx->par_out->extradata_size != ctx->par_in->extradata_size) {
>> +                    if ((ret = avcodec_parameters_copy(ctx->par_in, prev_ctx->par_out)) < 0)
>> +                        goto fail;
>> +                }
>> +            }
> 
>> +            if ((ret = av_bsf_send_packet(ctx, pkt)) < 0) {
> 
> I notice this is the only use of this function. Can pkg be NULL? Otherwise,
> flushing the filter is missing.
pkt can't be NULL here. Flushing the filters will be somewhat complex (requiring a loop over all the streams to flush them all), and AFAIK none of the current automatically-added BSFs require it, so I'm punting on that for now.

> 
>> +                av_log(ctx, AV_LOG_ERROR,
>> +                       "Failed to send packet to filter %s for stream %d",
>> +                       ctx->filter->name, pkt->stream_index);
>> +                goto fail;
>> +            }
> 
>> +            if ((ret = av_bsf_receive_packet(ctx, pkt)) < 0) {
>> +                if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF)
>> +                    return 0;
>> +                av_log(ctx, AV_LOG_ERROR,
>> +                       "Failed to send packet to filter %s for stream %d",
>> +                       ctx->filter->name, pkt->stream_index);
>> +                goto fail;
>> +            }
> 
> This is not correct, there must be a loop because a single send can cause
> several receive.
This also doesn't happen with any current BSFs, and the required looping is non-trivial, so I'm punting on this as well. Adding TODO comments for both issues.

> 
>> +            if (i == st->internal->nb_bsfcs - 1) {
>> +                if (ctx->par_out->extradata_size != st->codecpar->extradata_size) {
>> +                    if ((ret = avcodec_parameters_copy(st->codecpar, ctx->par_out)) < 0)
>> +                        goto fail;
>> +                }
>> +            }
>>         }
>> 
>>         if (s->debug & FF_FDEBUG_TS)
> 
> Regards,
> 
> --
>  Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org <mailto:ffmpeg-devel at ffmpeg.org>
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 842 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160428/2a15c966/attachment.sig>