Message ID | 20201124144122.1994769-4-andreas.rheinhardt@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [FFmpeg-devel,1/4] avcodec/mpeg12enc: Always initialize MPEG-2 intra VLC table lengths | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
On 11/24/2020 11:41 AM, Andreas Rheinhardt wrote: > This option just creates broken output because an MPEG-1 bitstream > can't signal whether MPEG-2 intra VLC tables have been used. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Missing version bump. > > doc/APIchanges | 3 +++ > libavcodec/mpeg12enc.c | 7 +++++-- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index b70c78a483..dea54c4415 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -15,6 +15,9 @@ libavutil: 2017-10-21 > > API changes, most recent first: > > +2020-xx-xx - xxxxxxxxxx - lavc 58.xx.100 - avcodec.h > + Deprecate the "intra_vlc" option for the mpeg1video encoder. Options usually don't get APIChanges entries, either when they are added or removed. > + > 2020-xx-xx - xxxxxxxxxx - lavf 58.64.100 - avformat.h > Add AVSTREAM_EVENT_FLAG_NEW_PACKETS. > > diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c > index 2466db8a91..0b5badde3c 100644 > --- a/libavcodec/mpeg12enc.c > +++ b/libavcodec/mpeg12enc.c > @@ -1114,6 +1114,7 @@ av_cold void ff_mpeg1_encode_init(MpegEncContext *s) > if (s->codec_id == AV_CODEC_ID_MPEG1VIDEO) { > s->min_qcoeff = -255; > s->max_qcoeff = 255; > + s->intra_vlc_format = 0; > } else { > s->min_qcoeff = -2047; > s->max_qcoeff = 2047; > @@ -1136,8 +1137,6 @@ av_cold void ff_mpeg1_encode_init(MpegEncContext *s) > #define COMMON_OPTS \ > { "gop_timecode", "MPEG GOP Timecode in hh:mm:ss[:;.]ff format. Overrides timecode_frame_start.", \ > OFFSET(tc_opt_str), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0, VE },\ > - { "intra_vlc", "Use MPEG-2 intra VLC table.", \ > - OFFSET(intra_vlc_format), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, \ > { "drop_frame_timecode", "Timecode is in drop frame format.", \ > OFFSET(drop_frame_timecode), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, \ > { "scan_offset", "Reserve space for SVCD scan offset user data.", \ > @@ -1147,12 +1146,16 @@ av_cold void ff_mpeg1_encode_init(MpegEncContext *s) > > static const AVOption mpeg1_options[] = { > COMMON_OPTS > + { "intra_vlc", "deprecated, does nothing", > + OFFSET(intra_vlc_format), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE | AV_OPT_FLAG_DEPRECATED }, > FF_MPV_COMMON_OPTS > { NULL }, > }; > > static const AVOption mpeg2_options[] = { > COMMON_OPTS > + { "intra_vlc", "Use MPEG-2 intra VLC table.", > + OFFSET(intra_vlc_format), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, > { "non_linear_quant", "Use nonlinear quantizer.", OFFSET(q_scale_type), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, > { "alternate_scan", "Enable alternate scantable.", OFFSET(alternate_scan), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, > { "seq_disp_ext", "Write sequence_display_extension blocks.", OFFSET(seq_disp_ext), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 1, VE, "seq_disp_ext" }, >
On Tue, 24 Nov 2020, James Almer wrote: > On 11/24/2020 11:41 AM, Andreas Rheinhardt wrote: >> This option just creates broken output because an MPEG-1 bitstream >> can't signal whether MPEG-2 intra VLC tables have been used. If the output was really broken with this option then IMHO the deprecation is not really necessary here, and you can simply remove the option. Regards, Marton >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> Missing version bump. >> >> doc/APIchanges | 3 +++ >> libavcodec/mpeg12enc.c | 7 +++++-- >> 2 files changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/doc/APIchanges b/doc/APIchanges >> index b70c78a483..dea54c4415 100644 >> --- a/doc/APIchanges >> +++ b/doc/APIchanges >> @@ -15,6 +15,9 @@ libavutil: 2017-10-21 >> >> API changes, most recent first: >> >> +2020-xx-xx - xxxxxxxxxx - lavc 58.xx.100 - avcodec.h >> + Deprecate the "intra_vlc" option for the mpeg1video encoder. > > Options usually don't get APIChanges entries, either when they are added > or removed. > >> + >> 2020-xx-xx - xxxxxxxxxx - lavf 58.64.100 - avformat.h >> Add AVSTREAM_EVENT_FLAG_NEW_PACKETS. >> >> diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c >> index 2466db8a91..0b5badde3c 100644 >> --- a/libavcodec/mpeg12enc.c >> +++ b/libavcodec/mpeg12enc.c >> @@ -1114,6 +1114,7 @@ av_cold void ff_mpeg1_encode_init(MpegEncContext *s) >> if (s->codec_id == AV_CODEC_ID_MPEG1VIDEO) { >> s->min_qcoeff = -255; >> s->max_qcoeff = 255; >> + s->intra_vlc_format = 0; >> } else { >> s->min_qcoeff = -2047; >> s->max_qcoeff = 2047; >> @@ -1136,8 +1137,6 @@ av_cold void ff_mpeg1_encode_init(MpegEncContext *s) >> #define COMMON_OPTS > \ >> { "gop_timecode", "MPEG GOP Timecode in hh:mm:ss[:;.]ff > format. Overrides timecode_frame_start.", \ >> OFFSET(tc_opt_str), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0, VE },\ >> - { "intra_vlc", "Use MPEG-2 intra VLC table.", > \ >> - OFFSET(intra_vlc_format), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, > VE }, \ >> { "drop_frame_timecode", "Timecode is in drop frame format.", > \ >> OFFSET(drop_frame_timecode), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, > VE }, \ >> { "scan_offset", "Reserve space for SVCD scan offset user > data.", \ >> @@ -1147,12 +1146,16 @@ av_cold void ff_mpeg1_encode_init(MpegEncContext > *s) >> >> static const AVOption mpeg1_options[] = { >> COMMON_OPTS >> + { "intra_vlc", "deprecated, does nothing", >> + OFFSET(intra_vlc_format), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, > VE | AV_OPT_FLAG_DEPRECATED }, >> FF_MPV_COMMON_OPTS >> { NULL }, >> }; >> >> static const AVOption mpeg2_options[] = { >> COMMON_OPTS >> + { "intra_vlc", "Use MPEG-2 intra VLC table.", >> + OFFSET(intra_vlc_format), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, > VE }, >> { "non_linear_quant", "Use nonlinear quantizer.", > OFFSET(q_scale_type), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, >> { "alternate_scan", "Enable alternate scantable.", > OFFSET(alternate_scan), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, >> { "seq_disp_ext", "Write sequence_display_extension blocks.", > OFFSET(seq_disp_ext), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 1, VE, > "seq_disp_ext" }, >> > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer: > On 11/24/2020 11:41 AM, Andreas Rheinhardt wrote: >> This option just creates broken output because an MPEG-1 bitstream >> can't signal whether MPEG-2 intra VLC tables have been used. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> Missing version bump. >> >> doc/APIchanges | 3 +++ >> libavcodec/mpeg12enc.c | 7 +++++-- >> 2 files changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/doc/APIchanges b/doc/APIchanges >> index b70c78a483..dea54c4415 100644 >> --- a/doc/APIchanges >> +++ b/doc/APIchanges >> @@ -15,6 +15,9 @@ libavutil: 2017-10-21 >> API changes, most recent first: >> +2020-xx-xx - xxxxxxxxxx - lavc 58.xx.100 - avcodec.h >> + Deprecate the "intra_vlc" option for the mpeg1video encoder. > > Options usually don't get APIChanges entries, either when they are added > or removed. > I saw this: > 2018-02-04 - ff46124b0d - lavf 58.8.100 - avformat.h > Deprecate the current names of the RTSP "timeout", "stimeout", "user-agent" > options. Introduce "listen_timeout" as replacement for the current "timeout" > option, and "user_agent" as replacement for "user-agent". Once the deprecation > is over, the old "timeout" option will be removed, and "stimeout" will be > renamed to "stimeout" (the "timeout" option will essentially change semantics). and therefore concluded that adding an APIChanges entry is appropriate; but I can of course also omit it. Is a version bump necessary? If yes, minor or micro? - Andreas
Marton Balint: > > > On Tue, 24 Nov 2020, James Almer wrote: > >> On 11/24/2020 11:41 AM, Andreas Rheinhardt wrote: >>> This option just creates broken output because an MPEG-1 bitstream >>> can't signal whether MPEG-2 intra VLC tables have been used. > > If the output was really broken with this option then IMHO the > deprecation is not really necessary here, and you can simply remove the > option. > It is spec-incompliant (mpeg1 does not allow mpeg2 tables and therefore an mpeg1 bitstream has no field to signal which tables to use) and undecodable by a spec-compliant decoder. One could write a decoder that decodes it (namely one that uses the mpeg2-tables automatically), but I am not aware of such a decoder existing. I am not against just removing the option now. What do others think about this? - Andreas
Quoting Andreas Rheinhardt (2020-11-25 01:01:44) >One could write a decoder that decodes it That's almost a tautology ;) > I am not against just removing the option now. What do others think > about this? Kill it with fire. We should not allow creating invalid files, unless there are really good reasons for it (and even then only with big warnings).
On Wed, Nov 25, 2020 at 00:47:46 +0100, Andreas Rheinhardt wrote: > and therefore concluded that adding an APIChanges entry is appropriate; > but I can of course also omit it. > Is a version bump necessary? If yes, minor or micro? I would say so - a change in options (and/or feature set) should bump micro, in my experience. Moritz
Moritz Barsnick: > On Wed, Nov 25, 2020 at 00:47:46 +0100, Andreas Rheinhardt wrote: >> and therefore concluded that adding an APIChanges entry is appropriate; >> but I can of course also omit it. >> Is a version bump necessary? If yes, minor or micro? > > I would say so - a change in options (and/or feature set) should bump > micro, in my experience. > > Moritz You are late -- this has already been applied on Dec 02 as 4e26520039ad7b45fc8d46fa7274e96cc0bdf8f9. And nobody has complained about this missing option since then. - Andreas
On 01-02-2021 06:05 am, Andreas Rheinhardt wrote: > Moritz Barsnick: >> On Wed, Nov 25, 2020 at 00:47:46 +0100, Andreas Rheinhardt wrote: >>> and therefore concluded that adding an APIChanges entry is appropriate; >>> but I can of course also omit it. >>> Is a version bump necessary? If yes, minor or micro? >> I would say so - a change in options (and/or feature set) should bump >> micro, in my experience. >> >> Moritz > You are late -- this has already been applied on Dec 02 as > 4e26520039ad7b45fc8d46fa7274e96cc0bdf8f9. And nobody has complained > about this missing option since then. Most production users tend to use releases, so they won't encounter an issue till then. Our release frequency has also decreased in the past couple of years, so the discovery period is also affected by that. Regards, Gyan
diff --git a/doc/APIchanges b/doc/APIchanges index b70c78a483..dea54c4415 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,9 @@ libavutil: 2017-10-21 API changes, most recent first: +2020-xx-xx - xxxxxxxxxx - lavc 58.xx.100 - avcodec.h + Deprecate the "intra_vlc" option for the mpeg1video encoder. + 2020-xx-xx - xxxxxxxxxx - lavf 58.64.100 - avformat.h Add AVSTREAM_EVENT_FLAG_NEW_PACKETS. diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c index 2466db8a91..0b5badde3c 100644 --- a/libavcodec/mpeg12enc.c +++ b/libavcodec/mpeg12enc.c @@ -1114,6 +1114,7 @@ av_cold void ff_mpeg1_encode_init(MpegEncContext *s) if (s->codec_id == AV_CODEC_ID_MPEG1VIDEO) { s->min_qcoeff = -255; s->max_qcoeff = 255; + s->intra_vlc_format = 0; } else { s->min_qcoeff = -2047; s->max_qcoeff = 2047; @@ -1136,8 +1137,6 @@ av_cold void ff_mpeg1_encode_init(MpegEncContext *s) #define COMMON_OPTS \ { "gop_timecode", "MPEG GOP Timecode in hh:mm:ss[:;.]ff format. Overrides timecode_frame_start.", \ OFFSET(tc_opt_str), AV_OPT_TYPE_STRING, {.str=NULL}, 0, 0, VE },\ - { "intra_vlc", "Use MPEG-2 intra VLC table.", \ - OFFSET(intra_vlc_format), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, \ { "drop_frame_timecode", "Timecode is in drop frame format.", \ OFFSET(drop_frame_timecode), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, \ { "scan_offset", "Reserve space for SVCD scan offset user data.", \ @@ -1147,12 +1146,16 @@ av_cold void ff_mpeg1_encode_init(MpegEncContext *s) static const AVOption mpeg1_options[] = { COMMON_OPTS + { "intra_vlc", "deprecated, does nothing", + OFFSET(intra_vlc_format), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE | AV_OPT_FLAG_DEPRECATED }, FF_MPV_COMMON_OPTS { NULL }, }; static const AVOption mpeg2_options[] = { COMMON_OPTS + { "intra_vlc", "Use MPEG-2 intra VLC table.", + OFFSET(intra_vlc_format), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, { "non_linear_quant", "Use nonlinear quantizer.", OFFSET(q_scale_type), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, { "alternate_scan", "Enable alternate scantable.", OFFSET(alternate_scan), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE }, { "seq_disp_ext", "Write sequence_display_extension blocks.", OFFSET(seq_disp_ext), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 1, VE, "seq_disp_ext" },
This option just creates broken output because an MPEG-1 bitstream can't signal whether MPEG-2 intra VLC tables have been used. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- Missing version bump. doc/APIchanges | 3 +++ libavcodec/mpeg12enc.c | 7 +++++-- 2 files changed, 8 insertions(+), 2 deletions(-)