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".
@@ -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@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
