video/out/gpu/context: convert --gpu-api to object settings list #14261
Conversation
DOCS/man/options.rst
Outdated
| @@ -6448,7 +6448,7 @@ them. | |||
| ``--gpu-sw`` | |||
| Continue even if a software renderer is detected. | |||
|
|
|||
| ``--gpu-context=<context1,context2,...[,]>`` | |||
| ``--gpu-context=<context1,context2,...>`` | |||
There was a problem hiding this comment.
What's the problem? Blank string isn't intended to be a valid value.
There was a problem hiding this comment.
Only --vo and --ao where the blank string is a valid value have , at the end in the docs.
There was a problem hiding this comment.
I don't see the value of disallowing it. It simplifies programmatic generation of context list string. If no contexts can be created it just fails like without the trailer.
There was a problem hiding this comment.
It could be misinterpreted as allowing an auto fallback, but fine.
| @@ -152,69 +152,85 @@ static bool get_desc(struct m_obj_desc *dst, int index) | |||
| return true; | |||
| } | |||
|
|
|||
| static bool check_unknown_entry(const char *name) | |||
There was a problem hiding this comment.
This shouldn't be deleted. It's used for early rejection of invalid context name, which was also done when it was a string list option with the string validate function. Deleting it causes invalid context to just fail to create and plays only audio.
There was a problem hiding this comment.
Ah that's what it was for. But it seems to work with just return false; as the function body without re-checking known entries.
There was a problem hiding this comment.
Just return false is fine.
guidocella
force-pushed
the
gpu-api-settingslist
branch
from
2a96d40
to
67d0df5
Compare
This function is used to reject invalid context names early, and without it the context fails to create and only audio is played, but it doesn't need to check for known entries again.
guidocella
force-pushed
the
gpu-api-settingslist
branch
2 times, most recently
from
11d7fc2
to
8f3ba38
Compare
|
Download the artifacts for this pull request: |
video/out/gpu/context.c
Outdated
| static struct ra_ctx *create_in_contexts(struct vo *vo, const char *name, char *api_name, | ||
| const struct ra_ctx_fns *ctxs[], size_t size, | ||
| struct ra_ctx_opts opts, | ||
| struct m_obj_settings *context_type_list) |
There was a problem hiding this comment.
A NULL context_type_list is essentially a list of one element of api_name. Lots of logic can be simplified by deleting the api_name parameter and making context_type_list not nullable.
There was a problem hiding this comment.
I'm having trouble getting this to work without ASan failing.
There was a problem hiding this comment.
It turns out I get the same memory leak on master so there is no issue with this PR. And I don't get it at all on Nvidia.
guidocella
force-pushed
the
gpu-api-settingslist
branch
2 times, most recently
from
5f84719
to
aa4b881
Compare
This follows up 96e1f1d which converted --gpu-context, and has the same advantages as listed there. Unlike with --gpu-context auto can be used anywhere in the list, e.g. --gpu-api=d3d11,auto works. I wanted to use the list of GPU contexts as the description in get_type_desc(), but there is no talloc context to allocate it to, so I set a print_help_list to print them. The printed lines don't being with spaces because otherwise they are added to the completions by etc/_mpv.zsh.
guidocella
force-pushed
the
gpu-api-settingslist
branch
from
aa4b881
to
a90c989
Compare
This follows up 96e1f1d which converted --gpu-context, and has the same advantages as listed there.
Unlike with --gpu-context auto can be used anywhere in the list, e.g. --gpu-api=d3d11,auto works.
I wanted to use the list of GPU contexts as the description in get_type_desc(), but there is no talloc context to allocate it to, so I set a print_help_list to print them. The printed lines don't being with spaces because otherwise they are added to the completions by etc/_mpv.zsh.