diff mbox series

[FFmpeg-devel,4/4] avcodec/mpeg12enc: Deprecate using MPEG-2 intra VLC table for mpeg1video

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

Checks

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

Commit Message

Andreas Rheinhardt Nov. 24, 2020, 2:41 p.m. UTC 
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(-)

Comments

James Almer Nov. 24, 2020, 9:17 p.m. UTC | #1 
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" },
>
Marton Balint Nov. 24, 2020, 9:56 p.m. UTC | #2 
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".
Andreas Rheinhardt Nov. 24, 2020, 11:47 p.m. UTC | #3 
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
Andreas Rheinhardt Nov. 25, 2020, 12:01 a.m. UTC | #4 
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
Anton Khirnov Dec. 2, 2020, 11:49 a.m. UTC | #5 
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).
Moritz Barsnick Feb. 1, 2021, 12:10 a.m. UTC | #6 
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
Andreas Rheinhardt Feb. 1, 2021, 12:35 a.m. UTC | #7 
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
Gyan Doshi Feb. 1, 2021, 4:51 a.m. UTC | #8 
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 mbox series

Patch

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" },