Skip to content

Conversation

@Akemi
Copy link
Member

@Akemi Akemi commented Jul 13, 2024

@kasper93 just wonder if this is what you had in mind. was the test supposed to test a vo on every platform or is it more like a test that libmpv init works properly and if no vo can be initialised it should error out gracefully?

this does the latter now on macOS and errors out properly if the context can't be initialised because no NSApplication environment is found.

if we want the former, we would need to init an NSApplication in the test.

@github-actions
Copy link

github-actions bot commented Jul 13, 2024

Download the artifacts for this pull request:

Windows
macOS

@kasper93
Copy link
Member

@kasper93 just wonder if this is what you had in mind. was the test supposed to test a vo on every platform or is it more like a test that libmpv init works properly and if no vo can be initialised it should error out gracefully?

Both. The idea is to test if things init/deinit multiple times without crashing / leaking memory. I don't impose strict rules on this test, because I'm aware that most things will not work anyway on CI without display/gpu. Locally though I tested with multiple different vo and so on, just to see what happens.

In summary, this PR is good change. But at the same time extending this to make the initialization process go further would be beneficial too, to bring more coverage on this one, not required though.

Akemi added 2 commits July 13, 2024 17:54
if no NSApplication has been initialized, applications using Appkit
functionality are not supposed to work properly or just deadlock
indefinitely. properly error out on macvk context creation in that case.
@sfan5 sfan5 merged commit 05b0b7c into mpv-player:master Jul 16, 2024
@Akemi Akemi deleted the mac_nsapp branch July 16, 2024 10:49
@Akemi Akemi mentioned this pull request Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants