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

Jump to bottom

synchronization for libmpv #1542

Closed
bylee20 opened this issue Jan 31, 2015 · 6 comments
Closed

synchronization for libmpv #1542

bylee20 opened this issue Jan 31, 2015 · 6 comments

Comments

@bylee20
Copy link
Contributor

bylee20 commented Jan 31, 2015

libmpv provides each set of sync and async version for command/property.
async version set is very useful for not-blocking operations, but sometimes, its utilization is restricted by absence of synchronization functionality like glFinish() in OpenGL.

Suppose 'A' is an operation which can be executed asynchronously when they're executed independently and 'B' is another operation which depends on the execution state of 'A'.
Lack of synchronization enforces me to write two set of functions sync/async for 'A' or just let me give up async version.

I think something like mpv_synchronize() or mpv_flush() will make it easier to use async set of APIs.
@ghost
Copy link

ghost commented Jan 31, 2015

Currently, asynchronous commands are executed synchronously within the core (just that the API function returns immediately, instead of blocking for the result), so they're not much different from synchronous commands. mpv_command_string(ctx, "ignore"); has exactly the same behavior as a mpv_flush(); would.

@bylee20
Copy link
Contributor Author

bylee20 commented Jan 31, 2015

Ah, then, may I think any sync version function call will flush all queued commands?

@ghost
Copy link

ghost commented Jan 31, 2015

Yep, currently.

Currently, synchronous vs. asynchronous is a bit of a mess. The core always runs commands synchronous or asynchronously, depending what it is. For example, seek or loadfile are pretty much always synchronous. The result of these commands can be seen only later. Other commands, like screenshot, always block the player. If an internal mechanism is introduced to handle these things in a more consistent way, it would probably change the default behavior of such commands. Or in other words, it would require an incompatible API change (even if the C API doesn't need to be changed, fundamentally changing the commands that can be issued with the API constitutes an API change).

We still can introduce something like mpv_flush() though, if you think it's helpful.

@bylee20
Copy link
Contributor Author

bylee20 commented Jan 31, 2015

We still can introduce something like mpv_flush() though, if you think it's helpful.

If you think this behavior might be changed, i think mpv_flush() for alias of "ignore" command will be helpful in order to ensure synchronization at mpv_flush() regardless of internal behavior of mpv.
If you won't change this in close future, I think it would be enough to mention this feature in ducumentation.

Thank you!

@bylee20 bylee20 closed this as completed Jan 31, 2015
ghost pushed a commit that referenced this issue Feb 2, 2015
client API: add mpv_wait_async_requests()
ef827af 
This does what it's documented to do.

The implementation reuses the code in mpv_detach_destroy(). Due to the
way async requests currently work, just sending a synchronous dummy
request (like a "ignore" command) would be enough to ensure
synchronization, but this code will continue to work even if this
changes.

The line "ctx->event_mask = 0;" is removed, but it shouldn't be needed.
(If a client is somehow very slow to terminate, this could silence an
annoying queue overflow message, but all in all it does nothing.)

Calling mpv_wait_async_requests() and mpv_wait_event() concurrently is
in theory allowed, so change pthread_cond_signal() to
pthread_cond_broadcast() to avoid missed wakeups.

As requested in issue #1542.
@ghost
Copy link

ghost commented Feb 2, 2015

Added something... I hope it's what you expected.

@bylee20
Copy link
Contributor Author

bylee20 commented Feb 3, 2015

Thank you!

olifre pushed a commit to olifre/mpv that referenced this issue Feb 28, 2015
@olifre
client API: add mpv_wait_async_requests()
c47650b 
This does what it's documented to do.

The implementation reuses the code in mpv_detach_destroy(). Due to the
way async requests currently work, just sending a synchronous dummy
request (like a "ignore" command) would be enough to ensure
synchronization, but this code will continue to work even if this
changes.

The line "ctx->event_mask = 0;" is removed, but it shouldn't be needed.
(If a client is somehow very slow to terminate, this could silence an
annoying queue overflow message, but all in all it does nothing.)

Calling mpv_wait_async_requests() and mpv_wait_event() concurrently is
in theory allowed, so change pthread_cond_signal() to
pthread_cond_broadcast() to avoid missed wakeups.

As requested in issue mpv-player#1542.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@bylee20