| Message ID | 20180222232052.7496-1-jamrial@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
On Fri, Feb 23, 2018 at 12:20 AM, James Almer <jamrial@gmail.com> wrote: > av_parser_change() is effectively a noop if the avctx passed it to > doesn't have the global header or local header flags set, and > initializing a custom AVCodecContext as a copy of an > AVCodecParameters results in the flags and flags2 fields being zero. > Use instead the existing custom AVCodecContext with the required > flags set in ffmpeg_opt.c new_output_stream() among other places. > > The fate test change is the result of the hevc sps/pps/vps being > removed from frames when copying into a format that explicitly > states it uses global header. > Deleting inband headers is not necessarily a good idea, and likely also why its being skipped for h264. - Hendrik
On 2/22/2018 8:34 PM, Hendrik Leppkes wrote: > On Fri, Feb 23, 2018 at 12:20 AM, James Almer <jamrial@gmail.com> wrote: >> av_parser_change() is effectively a noop if the avctx passed it to >> doesn't have the global header or local header flags set, and >> initializing a custom AVCodecContext as a copy of an >> AVCodecParameters results in the flags and flags2 fields being zero. >> Use instead the existing custom AVCodecContext with the required >> flags set in ffmpeg_opt.c new_output_stream() among other places. >> >> The fate test change is the result of the hevc sps/pps/vps being >> removed from frames when copying into a format that explicitly >> states it uses global header. >> > > Deleting inband headers is not necessarily a good idea, and likely > also why its being skipped for h264. Well, I'd argue it is when the user and/or the output format request it. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Fri, Feb 23, 2018 at 12:37 AM, James Almer <jamrial@gmail.com> wrote: > On 2/22/2018 8:34 PM, Hendrik Leppkes wrote: >> On Fri, Feb 23, 2018 at 12:20 AM, James Almer <jamrial@gmail.com> wrote: >>> av_parser_change() is effectively a noop if the avctx passed it to >>> doesn't have the global header or local header flags set, and >>> initializing a custom AVCodecContext as a copy of an >>> AVCodecParameters results in the flags and flags2 fields being zero. >>> Use instead the existing custom AVCodecContext with the required >>> flags set in ffmpeg_opt.c new_output_stream() among other places. >>> >>> The fate test change is the result of the hevc sps/pps/vps being >>> removed from frames when copying into a format that explicitly >>> states it uses global header. >>> >> >> Deleting inband headers is not necessarily a good idea, and likely >> also why its being skipped for h264. > > Well, I'd argue it is when the user and/or the output format request it. > Unless there is an explicit option to request that, thats not exactly the case. - Hendrik
On 2/22/2018 8:52 PM, Hendrik Leppkes wrote: > On Fri, Feb 23, 2018 at 12:37 AM, James Almer <jamrial@gmail.com> wrote: >> On 2/22/2018 8:34 PM, Hendrik Leppkes wrote: >>> On Fri, Feb 23, 2018 at 12:20 AM, James Almer <jamrial@gmail.com> wrote: >>>> av_parser_change() is effectively a noop if the avctx passed it to >>>> doesn't have the global header or local header flags set, and >>>> initializing a custom AVCodecContext as a copy of an >>>> AVCodecParameters results in the flags and flags2 fields being zero. >>>> Use instead the existing custom AVCodecContext with the required >>>> flags set in ffmpeg_opt.c new_output_stream() among other places. >>>> >>>> The fate test change is the result of the hevc sps/pps/vps being >>>> removed from frames when copying into a format that explicitly >>>> states it uses global header. >>>> >>> >>> Deleting inband headers is not necessarily a good idea, and likely >>> also why its being skipped for h264. >> >> Well, I'd argue it is when the user and/or the output format request it. >> > > Unless there is an explicit option to request that, thats not exactly the case. > > - Hendrik The AVCodecContext global_header flag, which is what av_parser_change() looks at. It's either set with an AVOption (With the CLI that's using -flags +global_header) when encoding, or force enabled in https://git.videolan.org/?p=ffmpeg.git;a=blob;f=fftools/ffmpeg_opt.c;h=997d53838108c2c3deb7fa3d3f395712738dad86;hb=HEAD#l1495 as mentioned in the commit message, depending on the constant flag/capability set by the muxer.
On Fri, Feb 23, 2018 at 1:28 AM, James Almer <jamrial@gmail.com> wrote: > On 2/22/2018 8:52 PM, Hendrik Leppkes wrote: >> On Fri, Feb 23, 2018 at 12:37 AM, James Almer <jamrial@gmail.com> wrote: >>> On 2/22/2018 8:34 PM, Hendrik Leppkes wrote: >>>> On Fri, Feb 23, 2018 at 12:20 AM, James Almer <jamrial@gmail.com> wrote: >>>>> av_parser_change() is effectively a noop if the avctx passed it to >>>>> doesn't have the global header or local header flags set, and >>>>> initializing a custom AVCodecContext as a copy of an >>>>> AVCodecParameters results in the flags and flags2 fields being zero. >>>>> Use instead the existing custom AVCodecContext with the required >>>>> flags set in ffmpeg_opt.c new_output_stream() among other places. >>>>> >>>>> The fate test change is the result of the hevc sps/pps/vps being >>>>> removed from frames when copying into a format that explicitly >>>>> states it uses global header. >>>>> >>>> >>>> Deleting inband headers is not necessarily a good idea, and likely >>>> also why its being skipped for h264. >>> >>> Well, I'd argue it is when the user and/or the output format request it. >>> >> >> Unless there is an explicit option to request that, thats not exactly the case. >> >> - Hendrik > > The AVCodecContext global_header flag, which is what av_parser_change() > looks at. It's either set with an AVOption (With the CLI that's using > -flags +global_header) when encoding, or force enabled in > > https://git.videolan.org/?p=ffmpeg.git;a=blob;f=fftools/ffmpeg_opt.c;h=997d53838108c2c3deb7fa3d3f395712738dad86;hb=HEAD#l1495 > > as mentioned in the commit message, depending on the constant > flag/capability set by the muxer. Thats not what I mean, I know about that flag - and it controls AnnexB vs MP4, but it does not explicitly say "remove inband headers as well". If anything I think HEVC should follow the H264 example for consistency. There are plenty cases where removing the in-band headers results in problems. - Hendrik
On 2/23/2018 4:35 AM, Hendrik Leppkes wrote: > On Fri, Feb 23, 2018 at 1:28 AM, James Almer <jamrial@gmail.com> wrote: >> On 2/22/2018 8:52 PM, Hendrik Leppkes wrote: >>> On Fri, Feb 23, 2018 at 12:37 AM, James Almer <jamrial@gmail.com> wrote: >>>> On 2/22/2018 8:34 PM, Hendrik Leppkes wrote: >>>>> On Fri, Feb 23, 2018 at 12:20 AM, James Almer <jamrial@gmail.com> wrote: >>>>>> av_parser_change() is effectively a noop if the avctx passed it to >>>>>> doesn't have the global header or local header flags set, and >>>>>> initializing a custom AVCodecContext as a copy of an >>>>>> AVCodecParameters results in the flags and flags2 fields being zero. >>>>>> Use instead the existing custom AVCodecContext with the required >>>>>> flags set in ffmpeg_opt.c new_output_stream() among other places. >>>>>> >>>>>> The fate test change is the result of the hevc sps/pps/vps being >>>>>> removed from frames when copying into a format that explicitly >>>>>> states it uses global header. >>>>>> >>>>> >>>>> Deleting inband headers is not necessarily a good idea, and likely >>>>> also why its being skipped for h264. >>>> >>>> Well, I'd argue it is when the user and/or the output format request it. >>>> >>> >>> Unless there is an explicit option to request that, thats not exactly the case. >>> >>> - Hendrik >> >> The AVCodecContext global_header flag, which is what av_parser_change() >> looks at. It's either set with an AVOption (With the CLI that's using >> -flags +global_header) when encoding, or force enabled in >> >> https://git.videolan.org/?p=ffmpeg.git;a=blob;f=fftools/ffmpeg_opt.c;h=997d53838108c2c3deb7fa3d3f395712738dad86;hb=HEAD#l1495 >> >> as mentioned in the commit message, depending on the constant >> flag/capability set by the muxer. > > Thats not what I mean, I know about that flag - and it controls AnnexB > vs MP4, but it does not explicitly say "remove inband headers as > well". > If anything I think HEVC should follow the H264 example for consistency. > > There are plenty cases where removing the in-band headers results in problems. So, if removing in-band headers is not desired, what exactly was the point/benefit of this entire chunk of code? I'm not sure the local_header portion of av_parser_change (to dump out of band headers at every keyframe) can be triggered currently. Neither -flags +global_header or -flags2 +local_header seem to work for codec copy, seeing the former is ignored and the latter gives an error. Who wrote it, for that matter? It's been dead code for years. Maybe it should be removed altogether. Nobody missed it all this time, and skipping hevc as well would leave mpeg4 only.
On Fri, Feb 23, 2018 at 11:15:50AM -0300, James Almer wrote: > On 2/23/2018 4:35 AM, Hendrik Leppkes wrote: > > On Fri, Feb 23, 2018 at 1:28 AM, James Almer <jamrial@gmail.com> wrote: > >> On 2/22/2018 8:52 PM, Hendrik Leppkes wrote: > >>> On Fri, Feb 23, 2018 at 12:37 AM, James Almer <jamrial@gmail.com> wrote: > >>>> On 2/22/2018 8:34 PM, Hendrik Leppkes wrote: > >>>>> On Fri, Feb 23, 2018 at 12:20 AM, James Almer <jamrial@gmail.com> wrote: > >>>>>> av_parser_change() is effectively a noop if the avctx passed it to > >>>>>> doesn't have the global header or local header flags set, and > >>>>>> initializing a custom AVCodecContext as a copy of an > >>>>>> AVCodecParameters results in the flags and flags2 fields being zero. > >>>>>> Use instead the existing custom AVCodecContext with the required > >>>>>> flags set in ffmpeg_opt.c new_output_stream() among other places. > >>>>>> > >>>>>> The fate test change is the result of the hevc sps/pps/vps being > >>>>>> removed from frames when copying into a format that explicitly > >>>>>> states it uses global header. > >>>>>> > >>>>> > >>>>> Deleting inband headers is not necessarily a good idea, and likely > >>>>> also why its being skipped for h264. > >>>> > >>>> Well, I'd argue it is when the user and/or the output format request it. > >>>> > >>> > >>> Unless there is an explicit option to request that, thats not exactly the case. > >>> > >>> - Hendrik > >> > >> The AVCodecContext global_header flag, which is what av_parser_change() > >> looks at. It's either set with an AVOption (With the CLI that's using > >> -flags +global_header) when encoding, or force enabled in > >> > >> https://git.videolan.org/?p=ffmpeg.git;a=blob;f=fftools/ffmpeg_opt.c;h=997d53838108c2c3deb7fa3d3f395712738dad86;hb=HEAD#l1495 > >> > >> as mentioned in the commit message, depending on the constant > >> flag/capability set by the muxer. > > > > Thats not what I mean, I know about that flag - and it controls AnnexB > > vs MP4, but it does not explicitly say "remove inband headers as > > well". > > If anything I think HEVC should follow the H264 example for consistency. > > > > There are plenty cases where removing the in-band headers results in problems. > > So, if removing in-band headers is not desired, what exactly was the > point/benefit of this entire chunk of code? I'm not sure the > local_header portion of av_parser_change (to dump out of band headers at > every keyframe) can be triggered currently. Neither -flags > +global_header or -flags2 +local_header seem to work for codec copy, > seeing the former is ignored and the latter gives an error. > > Who wrote it, for that matter? It's been dead code for years. > Maybe it should be removed altogether. Nobody missed it all this time, > and skipping hevc as well would leave mpeg4 only. I suspect (but someone would have to check the various specs to confirm) that the in band headers are not really correct in the cases where a place for global headers exist for these headers. Even if they are correct they are a waste of space as they must be repeated at _every_ point where seeking can bring one to. The "correct" thing to do is to collect all the in band headers, put them ALL in the global header spot of the container and update the index in band to refer to them. Or use other higher level features to switch between global headers where the container supports this. This is much harder than just leaving the headers in place in band of course [...]
diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c index a37de2ff98..b0b9a0d0c2 100644 --- a/fftools/ffmpeg.c +++ b/fftools/ffmpeg.c @@ -552,7 +552,6 @@ static void ffmpeg_cleanup(int ret) av_dict_free(&ost->encoder_opts); av_parser_close(ost->parser); - avcodec_free_context(&ost->parser_avctx); av_freep(&ost->forced_keyframes); av_expr_free(ost->forced_keyframes_pexpr); @@ -2052,7 +2051,7 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p && ost->st->codecpar->codec_id != AV_CODEC_ID_MPEG2VIDEO && ost->st->codecpar->codec_id != AV_CODEC_ID_VC1 ) { - int ret = av_parser_change(ost->parser, ost->parser_avctx, + int ret = av_parser_change(ost->parser, ost->enc_ctx, &opkt.data, &opkt.size, pkt->data, pkt->size, pkt->flags & AV_PKT_FLAG_KEY); @@ -3123,9 +3122,6 @@ static int init_output_stream_streamcopy(OutputStream *ost) } ost->parser = av_parser_init(par_dst->codec_id); - ost->parser_avctx = avcodec_alloc_context3(NULL); - if (!ost->parser_avctx) - return AVERROR(ENOMEM); switch (par_dst->codec_type) { case AVMEDIA_TYPE_AUDIO: @@ -3566,14 +3562,6 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len) ret = init_output_stream_streamcopy(ost); if (ret < 0) return ret; - - /* - * FIXME: will the codec context used by the parser during streamcopy - * This should go away with the new parser API. - */ - ret = avcodec_parameters_to_context(ost->parser_avctx, ost->st->codecpar); - if (ret < 0) - return ret; } // parse user provided disposition, and update stream values diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h index 8195f73e8b..5637eae397 100644 --- a/fftools/ffmpeg.h +++ b/fftools/ffmpeg.h @@ -527,7 +527,6 @@ typedef struct OutputStream { int keep_pix_fmt; AVCodecParserContext *parser; - AVCodecContext *parser_avctx; /* stats */ // combined size of all the packets written diff --git a/tests/fate/hevc.mak b/tests/fate/hevc.mak index 184349e5dd..c404af7ed6 100644 --- a/tests/fate/hevc.mak +++ b/tests/fate/hevc.mak @@ -238,7 +238,7 @@ FATE_HEVC-$(call ALLYES, HEVC_DEMUXER MOV_DEMUXER HEVC_MP4TOANNEXB_BSF MOV_MUXER fate-hevc-bsf-mp4toannexb: tests/data/hevc-mp4.mov fate-hevc-bsf-mp4toannexb: CMD = md5 -i $(TARGET_PATH)/tests/data/hevc-mp4.mov -c:v copy -fflags +bitexact -f hevc fate-hevc-bsf-mp4toannexb: CMP = oneline -fate-hevc-bsf-mp4toannexb: REF = 1873662a3af1848c37e4eb25722c8df9 +fate-hevc-bsf-mp4toannexb: REF = 3c9d998a3aa2b9e0fb1c1f434952bf8b fate-hevc-skiploopfilter: CMD = framemd5 -skip_loop_filter nokey -i $(TARGET_SAMPLES)/hevc-conformance/SAO_D_Samsung_5.bit -sws_flags bitexact FATE_HEVC += fate-hevc-skiploopfilter
av_parser_change() is effectively a noop if the avctx passed it to doesn't have the global header or local header flags set, and initializing a custom AVCodecContext as a copy of an AVCodecParameters results in the flags and flags2 fields being zero. Use instead the existing custom AVCodecContext with the required flags set in ffmpeg_opt.c new_output_stream() among other places. The fate test change is the result of the hevc sps/pps/vps being removed from frames when copying into a format that explicitly states it uses global header. Signed-off-by: James Almer <jamrial@gmail.com> --- av_parser_change() is also for some reason only called for two or so codecs where the global_header flag will actually have an effect, annexb hevc and mpeg4/cavs, as it's explictly skipped for annexb h264, vc1 and mpeg1/2. Anyone knows the reason why? h264 at the very least sounds to me like it shouldn't be skipped, being similar to hevc in how the parameter set NALUs shouldn't make their way into packets for formats that set AVFMT_GLOBALHEADER. No test currently makes use of the local_header flag. This chunk of code for that matter should eventually be replaced by bitstream filters, as the comment above it states. extract_extradata and/or dump_extradata to be more specific. fftools/ffmpeg.c | 14 +------------- fftools/ffmpeg.h | 1 - tests/fate/hevc.mak | 2 +- 3 files changed, 2 insertions(+), 15 deletions(-)