Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

m_option: don't overlap UPDATE and M_OPT constant values #15457

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

m_option: don't overlap UPDATE and M_OPT constant values #15457

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bdd9c2d
m_option: group UPDATE and M_OPT constants in an enum
guidocella Dec 8, 2024
e4cc7f2
various: upgrade option flags to uint64_t
guidocella Dec 8, 2024
aba2625
m_option: convert option_flags to 64-bit
guidocella Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion options/m_config_frontend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ const char *m_config_get_positional_option(const struct m_config *config, int p)
static int handle_set_opt_flags(struct m_config *config,
struct m_config_option *co, int flags)
{
int optflags = co->opt->flags;
uint64_t optflags = co->opt->flags;
bool set = !(flags & M_SETOPT_CHECK_ONLY);

if ((flags & M_SETOPT_PRE_PARSE_ONLY) && !(optflags & M_OPT_PRE_PARSE))
Expand Down
2 changes: 1 addition & 1 deletion options/m_config_frontend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ typedef struct m_config {
// m_config_notify_change_opt_ptr(). If false, it's caused either by
// m_config_set_option_*() (and similar) calls or external updates.
void (*option_change_callback)(void *ctx, struct m_config_option *co,
int flags, bool self_update);
uint64_t flags, bool self_update);
void *option_change_callback_ctx;

// For the command line parser
Expand Down
98 changes: 40 additions & 58 deletions options/m_option.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ struct m_sub_options {
const void *defaults;
// Change flags passed to mp_option_change_callback() if any option that is
// directly or indirectly part of this group is changed.
int change_flags;
uint64_t change_flags;
kasper93 marked this conversation as resolved.
Show resolved Hide resolved
// Return further sub-options, for example for optional components. If set,
// this is called with increasing index (starting from 0), as long as true
// is returned. If true is returned and *sub is set in any of these calls,
Expand Down Expand Up @@ -385,7 +385,7 @@ struct m_option {
const m_option_type_t *type;

// See \ref OptionFlags.
unsigned int flags;
uint64_t flags;

// Always force an option update even if the written value does not change.
bool force_update;
Expand Down Expand Up @@ -423,62 +423,44 @@ struct m_option {

char *format_file_size(int64_t size);

// The following are also part of the M_OPT_* flags, and are used to update
// certain groups of options.
#define UPDATE_TERM (1 << 0) // terminal options
#define UPDATE_SUB_FILT (1 << 1) // subtitle filter options
#define UPDATE_OSD (1 << 2) // related to OSD rendering
#define UPDATE_BUILTIN_SCRIPTS (1 << 3) // osc/ytdl/stats
#define UPDATE_IMGPAR (1 << 4) // video image params overrides
#define UPDATE_INPUT (1 << 5) // mostly --input-* options
#define UPDATE_AUDIO (1 << 6) // --audio-channels etc.
#define UPDATE_PRIORITY (1 << 7) // --priority (Windows-only)
#define UPDATE_SCREENSAVER (1 << 8) // --stop-screensaver
#define UPDATE_VOL (1 << 9) // softvol related options
#define UPDATE_LAVFI_COMPLEX (1 << 10) // --lavfi-complex
#define UPDATE_HWDEC (1 << 11) // --hwdec
#define UPDATE_DVB_PROG (1 << 12) // some --dvbin-...
#define UPDATE_SUB_HARD (1 << 13) // subtitle opts. that need full reinit
#define UPDATE_SUB_EXTS (1 << 14) // update internal list of sub exts
#define UPDATE_VIDEO (1 << 15) // force redraw if needed
#define UPDATE_VO (1 << 16) // reinit the VO
#define UPDATE_CLIPBOARD (1 << 17) // reinit the clipboard
#define UPDATE_OPT_LAST (1 << 17)

// All bits between of UPDATE_ flags
#define UPDATE_OPTS_MASK ((UPDATE_OPT_LAST << 1) - 1)

// The option is forbidden in config files.
#define M_OPT_NOCFG (1 << 30)

// The option should be set during command line pre-parsing
#define M_OPT_PRE_PARSE (1 << 29)

// The option expects a file name (or a list of file names)
#define M_OPT_FILE (1 << 28)

// Do not add as property.
#define M_OPT_NOPROP (1 << 27)

// Enable special semantics for some options when parsing the string "help".
#define M_OPT_HAVE_HELP (1 << 26)

// type_float/type_double: string "default" is parsed as NaN (and reverse)
#define M_OPT_DEFAULT_NAN (1 << 25)

// type time: string "no" maps to MP_NOPTS_VALUE (if unset, NOPTS is rejected)
// and
// parsing: "--no-opt" is parsed as "--opt=no"
#define M_OPT_ALLOW_NO (1 << 24)

// type channels: disallow "auto" (still accept ""), limit list to at most 1 item.
#define M_OPT_CHANNELS_LIMITED (1 << 23)

// type_float/type_double: controls if pretty print should trim trailing zeros
#define M_OPT_FIXED_LEN_PRINT (1 << 22)

// Like M_OPT_TYPE_OPTIONAL_PARAM.
#define M_OPT_OPTIONAL_PARAM (1 << 21)
enum option_flags {
Copy link
Contributor

@na-na-hi na-na-hi Dec 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this shouldn't use enum here. I consider a value of enum type can only be one of the defined exact value instead of some OR'd values.

If you want to group flags you can add a comment at the beginning of this section like the "These flags are used to describe special parser capabilities or behavior." for the next section below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was kasper's preference to use enum. I think it's more readable without all the #define and don't see a problem OR'ing constants as long as the OR'd variables are int64_t instead of the enum type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it is more easily recognizable, what flags are grouped together, but I approved previous version, as it was looking nice too. Frankly I would leave the enum conversion to separate commit, if not PR, to not mix it with real fixes that are being made in this changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as the OR'd variables are int64_t instead of the enum type.

ok but that sounds easy to get wrong, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not like it doesn't work if the variable is of the enum type, it just makes more sense to use int if the value is not one of the enum constants. Also even this example in Microsoft's docs does it and even assigns the OR'd constants to Days variables: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/enum#enumeration-types-as-bit-flags

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't take MS documentation as the benchmark of correctness. There are many code examples from them that don't even work.

In fact, C++ enum classes are strongly typed and won't allow such usage, just like enum in most non-C programming languages.

// The following are also part of the M_OPT_* flags, and are used to update
// certain groups of options.
UPDATE_TERM = (1 << 0), // terminal options
UPDATE_SUB_FILT = (1 << 1), // subtitle filter options
UPDATE_OSD = (1 << 2), // related to OSD rendering
UPDATE_BUILTIN_SCRIPTS = (1 << 3), // osc/ytdl/stats
UPDATE_IMGPAR = (1 << 4), // video image params overrides
UPDATE_INPUT = (1 << 5), // mostly --input-* options
UPDATE_AUDIO = (1 << 6), // --audio-channels etc.
UPDATE_PRIORITY = (1 << 7), // --priority (Windows-only)
UPDATE_SCREENSAVER = (1 << 8), // --stop-screensaver
UPDATE_VOL = (1 << 9), // softvol related options
UPDATE_LAVFI_COMPLEX = (1 << 10), // --lavfi-complex
UPDATE_HWDEC = (1 << 11), // --hwdec
UPDATE_DVB_PROG = (1 << 12), // some --dvbin-...
UPDATE_SUB_HARD = (1 << 13), // subtitle opts. that need full reinit
UPDATE_SUB_EXTS = (1 << 14), // update internal list of sub exts
UPDATE_VIDEO = (1 << 15), // force redraw if needed
UPDATE_VO = (1 << 16), // reinit the VO
UPDATE_CLIPBOARD = (1 << 17), // reinit the clipboard
UPDATE_OPT_LAST = (1 << 17),

M_OPT_NOCFG = (1 << 30), // The option is forbidden in config files.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the type has been changed to 64 bits, those should be moved up.

M_OPT_PRE_PARSE = (1 << 29), // The option should be set during command line pre-parsing
M_OPT_FILE = (1 << 28), // The option expects a file name (or a list of file names)
M_OPT_NOPROP = (1 << 27), // Do not add as property.
M_OPT_HAVE_HELP = (1 << 26), // Enable special semantics for some options when parsing the string "help".

M_OPT_DEFAULT_NAN = (1 << 25), // type_float/type_double: string "default" is parsed as NaN (and reverse)
M_OPT_ALLOW_NO = (1 << 24), // type time: string "no" maps to MP_NOPTS_VALUE (if unset, NOPTS is rejected)
// and parsing: "--no-opt" is parsed as "--opt=no"
M_OPT_CHANNELS_LIMITED = (1 << 23), // type channels: disallow "auto" (still accept ""), limit list to at most 1 item.
M_OPT_FIXED_LEN_PRINT = (1 << 22), // type_float/type_double: controls if pretty print should trim trailing zeros
M_OPT_OPTIONAL_PARAM = (1 << 21), // Like M_OPT_TYPE_OPTIONAL_PARAM.
};

#define UPDATE_OPTS_MASK ((UPDATE_OPT_LAST << 1) - 1)

static_assert(!(UPDATE_OPTS_MASK & M_OPT_OPTIONAL_PARAM), "");

Expand Down
5 changes: 2 additions & 3 deletions player/command.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7619,7 +7619,7 @@ static void update_track_switch(struct MPContext *mpctx, int order, int type)
mp_wakeup_core(mpctx);
}

void mp_option_change_callback(void *ctx, struct m_config_option *co, int flags,
void mp_option_change_callback(void *ctx, struct m_config_option *co, uint64_t flags,
bool self_update)
{
struct MPContext *mpctx = ctx;
Expand All @@ -7643,8 +7643,7 @@ void mp_option_change_callback(void *ctx, struct m_config_option *co, int flags,
struct track *track = mpctx->current_track[n][STREAM_SUB];
struct dec_sub *sub = track ? track->d_sub : NULL;
if (sub) {
int ret = sub_control(sub, SD_CTRL_UPDATE_OPTS,
(void *)(uintptr_t)flags);
int ret = sub_control(sub, SD_CTRL_UPDATE_OPTS, &flags);
if (ret == CONTROL_OK && flags & (UPDATE_SUB_FILT | UPDATE_SUB_HARD)) {
sub_redecode_cached_packets(sub);
sub_reset(sub);
Expand Down
2 changes: 1 addition & 1 deletion player/command.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ void property_print_help(struct MPContext *mpctx);
int mp_property_do(const char* name, int action, void* val,
struct MPContext *mpctx);

void mp_option_change_callback(void *ctx, struct m_config_option *co, int flags,
void mp_option_change_callback(void *ctx, struct m_config_option *co, uint64_t flags,
bool self_update);

void mp_notify(struct MPContext *mpctx, int event, void *arg);
Expand Down
2 changes: 1 addition & 1 deletion sub/dec_sub.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ int sub_control(struct dec_sub *sub, enum sd_ctrl cmd, void *arg)
break;
}
case SD_CTRL_UPDATE_OPTS: {
int flags = (uintptr_t)arg;
uint64_t flags = *(uint64_t *)arg;
if (m_config_cache_update(sub->opts_cache))
update_subtitle_speed(sub);
m_config_cache_update(sub->shared_opts_cache);
Expand Down
2 changes: 1 addition & 1 deletion sub/sd_ass.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1020,7 +1020,7 @@ static int control(struct sd *sd, enum sd_ctrl cmd, void *arg)
ctx->video_params = *(struct mp_image_params *)arg;
return CONTROL_OK;
case SD_CTRL_UPDATE_OPTS: {
int flags = (uintptr_t)arg;
uint64_t flags = *(uint64_t *)arg;
if (flags & UPDATE_SUB_FILT) {
filters_destroy(sd);
filters_init(sd);
Expand Down
Loading