Message ID | 20240406111014.6916-1-stefasab@gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] lavf/matroskaenc: sort options by name | expand |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
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
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.
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
> 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".
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.
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 --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 }, };