diff mbox series

[FFmpeg-devel] lavf/matroskaenc: sort options by name

Message ID 20240406111014.6916-1-stefasab@gmail.com
State New
Headers show
Series [FFmpeg-devel] lavf/matroskaenc: sort options by name | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Stefano Sabatini April 6, 2024, 11:10 a.m. UTC 
---
 libavformat/matroskaenc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Andreas Rheinhardt April 6, 2024, 11:25 a.m. UTC | #1 
Stefano Sabatini:
> ---
>  libavformat/matroskaenc.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 566e9f4981..8ebe6e4334 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -3500,20 +3500,20 @@ static const AVCodecTag additional_subtitle_tags[] = {
>  #define OFFSET(x) offsetof(MatroskaMuxContext, x)
>  #define FLAGS AV_OPT_FLAG_ENCODING_PARAM
>  static const AVOption options[] = {
> -    { "reserve_index_space", "Reserve a given amount of space (in bytes) at the beginning of the file for the index (cues).", OFFSET(reserve_cues_space), AV_OPT_TYPE_INT,   { .i64 = 0 },   0, INT_MAX,   FLAGS },
> -    { "cues_to_front", "Move Cues (the index) to the front by shifting data if necessary", OFFSET(move_cues_to_front), AV_OPT_TYPE_BOOL, { .i64 = 0}, 0, 1, FLAGS },
> +    { "allow_raw_vfw", "allow RAW VFW mode", OFFSET(allow_raw_vfw), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
>      { "cluster_size_limit",  "Store at most the provided amount of bytes in a cluster. ",                                     OFFSET(cluster_size_limit), AV_OPT_TYPE_INT  , { .i64 = -1 }, -1, INT_MAX,   FLAGS },
>      { "cluster_time_limit",  "Store at most the provided number of milliseconds in a cluster.",                               OFFSET(cluster_time_limit), AV_OPT_TYPE_INT64, { .i64 = -1 }, -1, INT64_MAX, FLAGS },
> +    { "cues_to_front", "Move Cues (the index) to the front by shifting data if necessary", OFFSET(move_cues_to_front), AV_OPT_TYPE_BOOL, { .i64 = 0}, 0, 1, FLAGS },
>      { "dash", "Create a WebM file conforming to WebM DASH specification", OFFSET(is_dash), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
>      { "dash_track_number", "Track number for the DASH stream", OFFSET(dash_track_number), AV_OPT_TYPE_INT, { .i64 = 1 }, 1, INT_MAX, FLAGS },
> -    { "live", "Write files assuming it is a live stream.", OFFSET(is_live), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
> -    { "allow_raw_vfw", "allow RAW VFW mode", OFFSET(allow_raw_vfw), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
> -    { "flipped_raw_rgb", "Raw RGB bitmaps in VFW mode are stored bottom-up", OFFSET(flipped_raw_rgb), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
> -    { "write_crc32", "write a CRC32 element inside every Level 1 element", OFFSET(write_crc), AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, FLAGS },
>      { "default_mode", "Controls how a track's FlagDefault is inferred", OFFSET(default_mode), AV_OPT_TYPE_INT, { .i64 = DEFAULT_MODE_PASSTHROUGH }, DEFAULT_MODE_INFER, DEFAULT_MODE_PASSTHROUGH, FLAGS, .unit = "default_mode" },
> +    { "flipped_raw_rgb", "Raw RGB bitmaps in VFW mode are stored bottom-up", OFFSET(flipped_raw_rgb), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
>      { "infer", "For each track type, mark each track of disposition default as default; if none exists, mark the first track as default.", 0, AV_OPT_TYPE_CONST, { .i64 = DEFAULT_MODE_INFER }, 0, 0, FLAGS, .unit = "default_mode" },
>      { "infer_no_subs", "For each track type, mark each track of disposition default as default; for audio and video: if none exists, mark the first track as default.", 0, AV_OPT_TYPE_CONST, { .i64 = DEFAULT_MODE_INFER_NO_SUBS }, 0, 0, FLAGS, .unit = "default_mode" },
> +    { "live", "Write files assuming it is a live stream.", OFFSET(is_live), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
>      { "passthrough", "Use the disposition flag as-is", 0, AV_OPT_TYPE_CONST, { .i64 = DEFAULT_MODE_PASSTHROUGH }, 0, 0, FLAGS, .unit = "default_mode" },
> +    { "reserve_index_space", "Reserve a given amount of space (in bytes) at the beginning of the file for the index (cues).", OFFSET(reserve_cues_space), AV_OPT_TYPE_INT,   { .i64 = 0 },   0, INT_MAX,   FLAGS },
> +    { "write_crc32", "write a CRC32 element inside every Level 1 element", OFFSET(write_crc), AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, FLAGS },
>      { NULL },
>  };
>  

See https://ffmpeg.org/pipermail/ffmpeg-devel/2024-February/320849.html
Additionally I do not agree that sorting options by name is the best
way; it should be sorted by what are (believed to be) the most commonly
used options.

- Andreas
Stefano Sabatini April 6, 2024, 11:33 a.m. UTC | #2 
On date Saturday 2024-04-06 13:25:49 +0200, Andreas Rheinhardt wrote:
> Stefano Sabatini:
> > ---
> >  libavformat/matroskaenc.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
[...]
> See https://ffmpeg.org/pipermail/ffmpeg-devel/2024-February/320849.html

> Additionally I do not agree that sorting options by name is the best
> way;

> it should be sorted by what are (believed to be) the most commonly
> used options.

This is highly arbitrary and subjective, the best way is sort-by-name
as already done in other places (and I still find the git blame
argument rather weak for the reasons I already explained).

The alternative - which I observe all the time - is that when a new
option is added an arbitrary place is assigned and more confusion is
added.
Anton Khirnov April 7, 2024, 6:16 a.m. UTC | #3 
Quoting Andreas Rheinhardt (2024-04-06 13:25:49)
> See https://ffmpeg.org/pipermail/ffmpeg-devel/2024-February/320849.html
> Additionally I do not agree that sorting options by name is the best
> way; it should be sorted by what are (believed to be) the most commonly
> used options.

+1
Zhao Zhili April 7, 2024, 8:01 a.m. UTC | #4 
> On Apr 7, 2024, at 14:16, Anton Khirnov <anton@khirnov.net> wrote:
> 
> Quoting Andreas Rheinhardt (2024-04-06 13:25:49)
>> See https://ffmpeg.org/pipermail/ffmpeg-devel/2024-February/320849.html
>> Additionally I do not agree that sorting options by name is the best
>> way; it should be sorted by what are (believed to be) the most commonly
>> used options.
> 
> +1

https://patchwork.ffmpeg.org/project/ffmpeg/patch/20240106165246.274472-1-stefasab@gmail.com/

I have the same consideration in another patch. Maybe group related options together than sort whole
options.

> 
> -- 
> Anton Khirnov
> _______________________________________________
> 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".
Stefano Sabatini April 10, 2024, 9:07 a.m. UTC | #5 
On date Sunday 2024-04-07 16:01:27 +0800, Zhao Zhili wrote:
> 
> > On Apr 7, 2024, at 14:16, Anton Khirnov <anton@khirnov.net> wrote:
> > 
> > Quoting Andreas Rheinhardt (2024-04-06 13:25:49)
> >> See https://ffmpeg.org/pipermail/ffmpeg-devel/2024-February/320849.html
> >> Additionally I do not agree that sorting options by name is the best
> >> way; it should be sorted by what are (believed to be) the most commonly
> >> used options.
> > 
> > +1
> 
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20240106165246.274472-1-stefasab@gmail.com/
> 
> I have the same consideration in another patch. Maybe group related
> options together than sort whole options.

This hardly works for the aforementioned reasons, no objective
criteria means that there will be never a consistent way to sort the
options. What happens in practice is that options are added more or
less randomly, which doesn't help readability and discoverability
especially when there are many options.
Paul B Mahol April 11, 2024, 9 a.m. UTC | #6 
On Wed, Apr 10, 2024 at 11:08 AM Stefano Sabatini <stefasab@gmail.com>
wrote:

> On date Sunday 2024-04-07 16:01:27 +0800, Zhao Zhili wrote:
> >
> > > On Apr 7, 2024, at 14:16, Anton Khirnov <anton@khirnov.net> wrote:
> > >
> > > Quoting Andreas Rheinhardt (2024-04-06 13:25:49)
> > >> See
> https://ffmpeg.org/pipermail/ffmpeg-devel/2024-February/320849.html
> > >> Additionally I do not agree that sorting options by name is the best
> > >> way; it should be sorted by what are (believed to be) the most
> commonly
> > >> used options.
> > >
> > > +1
> >
> >
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20240106165246.274472-1-stefasab@gmail.com/
> >
> > I have the same consideration in another patch. Maybe group related
> > options together than sort whole options.
>
> This hardly works for the aforementioned reasons, no objective
> criteria means that there will be never a consistent way to sort the
> options. What happens in practice is that options are added more or
> less randomly, which doesn't help readability and discoverability
> especially when there are many options.
>

We have TC now. Oh yes, I completely forgot what is TC.


> _______________________________________________
> 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".
>
diff mbox series

Patch

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 566e9f4981..8ebe6e4334 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -3500,20 +3500,20 @@  static const AVCodecTag additional_subtitle_tags[] = {
 #define OFFSET(x) offsetof(MatroskaMuxContext, x)
 #define FLAGS AV_OPT_FLAG_ENCODING_PARAM
 static const AVOption options[] = {
-    { "reserve_index_space", "Reserve a given amount of space (in bytes) at the beginning of the file for the index (cues).", OFFSET(reserve_cues_space), AV_OPT_TYPE_INT,   { .i64 = 0 },   0, INT_MAX,   FLAGS },
-    { "cues_to_front", "Move Cues (the index) to the front by shifting data if necessary", OFFSET(move_cues_to_front), AV_OPT_TYPE_BOOL, { .i64 = 0}, 0, 1, FLAGS },
+    { "allow_raw_vfw", "allow RAW VFW mode", OFFSET(allow_raw_vfw), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
     { "cluster_size_limit",  "Store at most the provided amount of bytes in a cluster. ",                                     OFFSET(cluster_size_limit), AV_OPT_TYPE_INT  , { .i64 = -1 }, -1, INT_MAX,   FLAGS },
     { "cluster_time_limit",  "Store at most the provided number of milliseconds in a cluster.",                               OFFSET(cluster_time_limit), AV_OPT_TYPE_INT64, { .i64 = -1 }, -1, INT64_MAX, FLAGS },
+    { "cues_to_front", "Move Cues (the index) to the front by shifting data if necessary", OFFSET(move_cues_to_front), AV_OPT_TYPE_BOOL, { .i64 = 0}, 0, 1, FLAGS },
     { "dash", "Create a WebM file conforming to WebM DASH specification", OFFSET(is_dash), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
     { "dash_track_number", "Track number for the DASH stream", OFFSET(dash_track_number), AV_OPT_TYPE_INT, { .i64 = 1 }, 1, INT_MAX, FLAGS },
-    { "live", "Write files assuming it is a live stream.", OFFSET(is_live), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
-    { "allow_raw_vfw", "allow RAW VFW mode", OFFSET(allow_raw_vfw), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
-    { "flipped_raw_rgb", "Raw RGB bitmaps in VFW mode are stored bottom-up", OFFSET(flipped_raw_rgb), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
-    { "write_crc32", "write a CRC32 element inside every Level 1 element", OFFSET(write_crc), AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, FLAGS },
     { "default_mode", "Controls how a track's FlagDefault is inferred", OFFSET(default_mode), AV_OPT_TYPE_INT, { .i64 = DEFAULT_MODE_PASSTHROUGH }, DEFAULT_MODE_INFER, DEFAULT_MODE_PASSTHROUGH, FLAGS, .unit = "default_mode" },
+    { "flipped_raw_rgb", "Raw RGB bitmaps in VFW mode are stored bottom-up", OFFSET(flipped_raw_rgb), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
     { "infer", "For each track type, mark each track of disposition default as default; if none exists, mark the first track as default.", 0, AV_OPT_TYPE_CONST, { .i64 = DEFAULT_MODE_INFER }, 0, 0, FLAGS, .unit = "default_mode" },
     { "infer_no_subs", "For each track type, mark each track of disposition default as default; for audio and video: if none exists, mark the first track as default.", 0, AV_OPT_TYPE_CONST, { .i64 = DEFAULT_MODE_INFER_NO_SUBS }, 0, 0, FLAGS, .unit = "default_mode" },
+    { "live", "Write files assuming it is a live stream.", OFFSET(is_live), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },
     { "passthrough", "Use the disposition flag as-is", 0, AV_OPT_TYPE_CONST, { .i64 = DEFAULT_MODE_PASSTHROUGH }, 0, 0, FLAGS, .unit = "default_mode" },
+    { "reserve_index_space", "Reserve a given amount of space (in bytes) at the beginning of the file for the index (cues).", OFFSET(reserve_cues_space), AV_OPT_TYPE_INT,   { .i64 = 0 },   0, INT_MAX,   FLAGS },
+    { "write_crc32", "write a CRC32 element inside every Level 1 element", OFFSET(write_crc), AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, FLAGS },
     { NULL },
 };