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.

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

mac/event: use mpv_event struct directly for event handling #15620

Closed
wants to merge 1 commit into from

Conversation

Akemi
Copy link
Member

@Akemi Akemi commented Jan 2, 2025

another preparation to make swift 6 concurrency changes a bit easier. also makes it a bit less verbose.

Copy link

github-actions bot commented Jan 2, 2025

Download the artifacts for this pull request:

Windows
macOS

@kasper93
Copy link
Contributor

kasper93 commented Jan 3, 2025

swift 6 concurrency changes

Why those changes are concurrency related? I don't mind not using pointer to event, but why it makes any difference here?

@Akemi
Copy link
Member Author

Akemi commented Jan 3, 2025

because an UnsafeMutablePointer is mutable and not 'Sendable' (https://developer.apple.com/documentation/swift/sendable). a struct on the other hand can be (made) 'Sendable'. the event loop runs/will run on separate context and swift 6 strict concurrency will complain (error out) because of this.

there are more changes needed, though don't want to lump all the changes together. otherwise it will be quite a big messy change.

@kasper93
Copy link
Contributor

kasper93 commented Jan 3, 2025

the event loop runs/will run on separate context and swift 6 strict concurrency will complain (error out) because of this.

But we need to handle events synchronously, because the possible memory allocations inside of them are only valid until next event is received. So we shouldn't cross any context border, unless we do deep copy of events, but this would be tricky.

@Akemi
Copy link
Member Author

Akemi commented Jan 3, 2025

the mpv_events are handled synchronously, the data inside of those mpv_events are copied over to the Event struct and passed to the different objects that subscribed to a given event. those other objects may be accessed by different contexts. though since we do call into those possible other contexts, the whole 'event loop' has to conform to the strict concurrency.

i am still trying to wrap my head around the pile of warnings/errors i get atm, so it is still quite possible that i am doing something wrong.

my plan is to first do/fix/change the things that don't need a bump of the swift version and are somewhat unquestionable.

@kasper93
Copy link
Contributor

kasper93 commented Jan 5, 2025

the mpv_events are handled synchronously, the data inside of those mpv_events are copied over to the Event struct and passed to the different objects that subscribed to a given event. those other objects may be accessed by different contexts.

This step needs to be done carefully. mpv_event and any data in it is valid only until next mpv_wait_event call. Shallow copy of mpv_event is not enough for it to be safely processed later. If needed you may use mpv_event_to_node, but even then mpv_node has to be deep copied as some of its content may become invalidated on next mpv_wait_event.

@Akemi
Copy link
Member Author

Akemi commented Jan 5, 2025

yeah definitely, i made sure the data that is currently used and copied over to the Event struct is properly (deep) copied over.

though on another note, i am probably going to change the swift event handling and this change is not needed anymore (probably), for the sake of the strict concurrency that is. though still testing.

@Akemi
Copy link
Member Author

Akemi commented Jan 9, 2025

closing this. i have to fix the whole thing a bit differently than i anticipated.

@Akemi Akemi closed this Jan 9, 2025
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.

2 participants