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 <jamr...@gmail.com> wrote:
>>
>>> Signed-off-by: James Almer <jamr...@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@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
