| Message ID | 20170429002756.1184-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On 4/28/2017 9:27 PM, James Almer wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/concatdec.c | 94 ++++++++++++++++--------------------------------- > 1 file changed, 30 insertions(+), 64 deletions(-) Ping
On Fri, 28 Apr 2017 21:27:56 -0300 James Almer <jamrial@gmail.com> wrote: > Signed-off-by: James Almer <jamrial@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; > } > @@ -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); > - 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"); > 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"); > av_packet_unref(pkt); > - 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.
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@gmail.com> wrote: > >> Signed-off-by: James Almer <jamrial@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 >
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@gmail.com> wrote: >> >>> Signed-off-by: James Almer <jamrial@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
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; } @@ -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); - 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"); 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"); av_packet_unref(pkt); - 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; }
Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/concatdec.c | 94 ++++++++++++++++--------------------------------- 1 file changed, 30 insertions(+), 64 deletions(-)