summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorwm4 <wm4@nowhere>2019-10-24 17:01:53 +0200
committerwm4 <wm4@nowhere>2019-10-24 17:51:36 +0200
commit065c307e8e7dbfc15aea39b714f944a117e4916a (patch)
tree3d683c16f62cd00ec93107d4aef95bf93924d59f
parent922be711018cab55551c26fc757b7566c11602cb (diff)
downloadmpv-065c307e8e7dbfc15aea39b714f944a117e4916a.tar.bz2
mpv-065c307e8e7dbfc15aea39b714f944a117e4916a.tar.xz
client API: simplify (?) property change notification generation
Property change notification works by having the mpv core wake up all clients observing a property when the property potentially changes. The clients then read the property's value, and determine if there was an actual change. (The latter part depends what the property returned for the previous change notification, so it depends on the client, and cannot be generated by the core itself.) Until now, reading the property value was done in a pseudo-async way by queuing a callback back to the core, running it there, and then waking up the client thread again. I cannot comprehend why this was done in such a complicated, fragile way. Maybe it's a leftover from times when client.c had to do this (in short, because properties could access vo_opengl, which has thread-local state). One past idea was to make the implementation of true async properties easier (for which you would need such a state machine anyway). But they don't exist yet, and I doubt the current mess would be really helpful when actually implementing them. Simplify this, and run the update in the client's thread directly. In addition to the fundamental change, many roundabout things can be removed as a consequence. Unfortunately, I noticed that lock order issues force you to release ctx->lock before doing so, which makes things more complex due to possible concurrent mpv_unobserve_property() calls. Solve this by removing properties lazily, which means you may have to do multiple mpv_wait_event() calls before the property entry is actually destroyed. This should not matter in practice, and does not affect the semantics. It could also cause "leaks" by observing/unobserving properties in a loop, without ever calling mpv_wait_event(). Just don't do this, duh. (I considered making this dependent on whether the previous mpv_wait_event() call returned the property being removed, but a separate code path seemed too complicated. I also considered copying the name and property data when returning a MPV_EVENT_PROPERTY_CHANGE, but actually this doesn't solve the problem of update_prop() being interrupted by mpv_unobserve_property(); there are ways around it, but I just said no.) This was made using the cowboy coding software engineering methodology. If you find any bugs, keep them yourself.
-rw-r--r--player/client.c154
1 files changed, 79 insertions, 75 deletions
diff --git a/player/client.c b/player/client.c
index a1ce20fd74..cbbdb77c90 100644
--- a/player/client.c
+++ b/player/client.c
@@ -86,14 +86,11 @@ struct observe_property {
uint64_t event_mask; // ==mp_get_property_event_mask(name)
int64_t reply_id;
mpv_format format;
+ const struct m_option *type;
bool changed; // property change should be signaled to user
- bool need_new_value; // a new value should be retrieved
- bool updating; // a new value is being retrieved
- bool updated; // a new value was successfully retrieved
bool dead; // property unobserved while retrieving value
- bool new_value_valid, user_value_valid;
- union m_option_value new_value, user_value;
- struct mpv_handle *client;
+ bool value_valid;
+ union m_option_value value;
};
struct mpv_handle {
@@ -134,7 +131,6 @@ struct mpv_handle {
struct observe_property **properties;
int num_properties;
int lowest_changed; // attempt at making change processing incremental
- int properties_updating;
uint64_t property_event_masks; // or-ed together event masks of all properties
bool fuzzy_initialized; // see scripting.c wait_loaded()
@@ -359,7 +355,7 @@ static void unlock_core(mpv_handle *ctx)
void mpv_wait_async_requests(mpv_handle *ctx)
{
pthread_mutex_lock(&ctx->lock);
- while (ctx->reserved_events || ctx->properties_updating)
+ while (ctx->reserved_events)
wait_wakeup(ctx, INT64_MAX);
pthread_mutex_unlock(&ctx->lock);
}
@@ -1417,17 +1413,15 @@ int mpv_get_property_async(mpv_handle *ctx, uint64_t ud, const char *name,
static void property_free(void *p)
{
struct observe_property *prop = p;
- const struct m_option *type = get_mp_type_get(prop->format);
- if (type) {
- m_option_free(type, &prop->new_value);
- m_option_free(type, &prop->user_value);
- }
+ if (prop->type)
+ m_option_free(prop->type, &prop->value);
}
int mpv_observe_property(mpv_handle *ctx, uint64_t userdata,
const char *name, mpv_format format)
{
- if (format != MPV_FORMAT_NONE && !get_mp_type_get(format))
+ const struct m_option *type = get_mp_type_get(format);
+ if (format != MPV_FORMAT_NONE && !type)
return MPV_ERROR_PROPERTY_FORMAT;
// Explicitly disallow this, because it would require a special code path.
if (format == MPV_FORMAT_OSD_STRING)
@@ -1437,15 +1431,13 @@ int mpv_observe_property(mpv_handle *ctx, uint64_t userdata,
struct observe_property *prop = talloc_ptrtype(ctx, prop);
talloc_set_destructor(prop, property_free);
*prop = (struct observe_property){
- .client = ctx,
.name = talloc_strdup(prop, name),
.id = mp_get_property_id(ctx->mpctx, name),
.event_mask = mp_get_property_event_mask(name),
.reply_id = userdata,
.format = format,
+ .type = type,
.changed = true,
- .updating = false,
- .updated = false,
};
MP_TARRAY_APPEND(ctx, ctx->properties, ctx->num_properties, prop);
ctx->property_event_masks |= prop->event_mask;
@@ -1464,27 +1456,17 @@ static void mark_property_changed(struct mpv_handle *client, int index)
int mpv_unobserve_property(mpv_handle *ctx, uint64_t userdata)
{
pthread_mutex_lock(&ctx->lock);
- ctx->property_event_masks = 0;
int count = 0;
- for (int n = ctx->num_properties - 1; n >= 0; n--) {
+ for (int n = 0; n < ctx->num_properties; n++) {
struct observe_property *prop = ctx->properties[n];
- if (prop->reply_id == userdata) {
- if (prop->updating) {
- prop->dead = true;
- } else {
- // In case mpv_unobserve_property() is called after mpv_wait_event()
- // returned, and the mpv_event still references the name somehow,
- // make sure it's not freed while in use. The same can happen
- // with the value update mechanism.
- talloc_steal(ctx->cur_event, prop);
- }
- MP_TARRAY_REMOVE_AT(ctx->properties, ctx->num_properties, n);
+ // Perform actual removal of the property lazily to avoid creating
+ // dangling pointers and such.
+ if (prop->reply_id == userdata && !prop->dead) {
+ mark_property_changed(ctx, n);
+ prop->dead = true;
count++;
}
- if (!prop->dead)
- ctx->property_event_masks |= prop->event_mask;
}
- ctx->lowest_changed = 0;
pthread_mutex_unlock(&ctx->lock);
return count;
}
@@ -1524,40 +1506,45 @@ static void notify_property_events(struct mpv_handle *ctx, uint64_t event_mask)
wakeup_client(ctx);
}
-static void update_prop(void *p)
+static bool update_prop(struct mpv_handle *ctx, struct observe_property *prop)
{
- struct observe_property *prop = p;
- struct mpv_handle *ctx = prop->client;
+ if (!prop->type)
+ return true;
- const struct m_option *type = get_mp_type_get(prop->format);
union m_option_value val = {0};
+ pthread_mutex_unlock(&ctx->lock);
+
struct getproperty_request req = {
.mpctx = ctx->mpctx,
.name = prop->name,
.format = prop->format,
.data = &val,
};
-
- getproperty_fn(&req);
+ run_locked(ctx, getproperty_fn, &req);
pthread_mutex_lock(&ctx->lock);
- ctx->properties_updating--;
- prop->updating = false;
- m_option_free(type, &prop->new_value);
- prop->new_value_valid = req.status >= 0;
- if (prop->new_value_valid)
- memcpy(&prop->new_value, &val, type->type->size);
- if (prop->user_value_valid != prop->new_value_valid) {
- prop->updated = true;
- } else if (prop->user_value_valid && prop->new_value_valid) {
- if (!equal_mpv_value(&prop->user_value, &prop->new_value, prop->format))
- prop->updated = true;
+
+ bool val_valid = req.status >= 0;
+
+ bool changed = prop->value_valid != val_valid;
+ if (prop->value_valid && val_valid)
+ changed = !equal_mpv_value(&prop->value, &val, prop->format);
+
+ if (changed) {
+ prop->value_valid = val_valid;
+ if (val_valid) {
+ // move val to prop->value
+ m_option_free(prop->type, &prop->value);
+ memcpy(&prop->value, &val, prop->type->type->size);
+ val_valid = false;
+ }
}
- if (prop->dead)
- talloc_steal(ctx->cur_event, prop);
- wakeup_client(ctx);
- pthread_mutex_unlock(&ctx->lock);
+
+ if (val_valid)
+ m_option_free(prop->type, &val);
+
+ return changed;
}
// Set ctx->cur_event to a generated property change event, if there is any
@@ -1566,44 +1553,61 @@ static bool gen_property_change_event(struct mpv_handle *ctx)
{
if (!ctx->mpctx->initialized)
return false;
+
+ *ctx->cur_event = (struct mpv_event){
+ .event_id = MPV_EVENT_NONE,
+ };
+
+ bool need_gc = false;
int start = ctx->lowest_changed;
ctx->lowest_changed = ctx->num_properties;
for (int n = start; n < ctx->num_properties; n++) {
struct observe_property *prop = ctx->properties[n];
- if ((prop->changed || prop->updating || prop->updated) && n < ctx->lowest_changed)
+ if (prop->changed && n < ctx->lowest_changed)
ctx->lowest_changed = n;
- if (prop->changed) {
+
+ bool updated = false;
+ if (prop->changed && !prop->dead) {
prop->changed = false;
- if (prop->format != MPV_FORMAT_NONE) {
- ctx->properties_updating++;
- prop->updating = true;
- mp_dispatch_enqueue(ctx->mpctx->dispatch, update_prop, prop);
- } else {
- prop->updated = true;
- }
+ updated = update_prop(ctx, prop);
}
- bool updated = prop->updated;
- prop->updated = false;
- if (updated) {
- const struct m_option *type = get_mp_type_get(prop->format);
- prop->user_value_valid = prop->new_value_valid;
- if (prop->new_value_valid)
- m_option_copy(type, &prop->user_value, &prop->new_value);
+
+ if (prop->dead) {
+ need_gc = true;
+ } else if (updated) {
ctx->cur_property_event = (struct mpv_event_property){
.name = prop->name,
- .format = prop->user_value_valid ? prop->format : 0,
+ .format = prop->value_valid ? prop->format : 0,
+ .data = prop->value_valid ? &prop->value : NULL,
};
- if (prop->user_value_valid)
- ctx->cur_property_event.data = &prop->user_value;
*ctx->cur_event = (struct mpv_event){
.event_id = MPV_EVENT_PROPERTY_CHANGE,
.reply_userdata = prop->reply_id,
.data = &ctx->cur_property_event,
};
- return true;
+ break;
}
}
- return false;
+
+ if (need_gc) {
+ // Remove entries which have the .dead flag set. The point of doing this
+ // here is to ensure that this does not conflict with update_prop(),
+ // and that a previously returned mpv_event struct pointing to removed
+ // property entries does not result in dangling pointers.
+ ctx->property_event_masks = 0;
+ ctx->lowest_changed = 0;
+ for (int n = ctx->num_properties - 1; n >= 0; n--) {
+ struct observe_property *prop = ctx->properties[n];
+ if (prop->dead) {
+ MP_TARRAY_REMOVE_AT(ctx->properties, ctx->num_properties, n);
+ talloc_free(prop);
+ } else {
+ ctx->property_event_masks |= prop->event_mask;
+ }
+ }
+ }
+
+ return !!ctx->cur_event->event_id;
}
int mpv_hook_add(mpv_handle *ctx, uint64_t reply_userdata,