History

client API: avoid returning stale value on property notifications

This could happen if a property was flagged as changed, then updated, then flagged again, but gen_property_change_event() was called before the value was updated a second time. Then the function simply returned the old value, and would later trigger a new change event again. This was considered acceptable before, since property notifications are asynchronous anyway (so they may always be "outdated", it just mattered whether the most recent value was eventually delivered). But consider ordering with events. It seems desirable that specific important events (e.g. MPV_EVENT_START_FILE) should not be followed by property updates that happened before it, because that would make application logic really a mess, and property notification near-useless in certain cases. Avoid this by never returning a value if it was marked changed, but not updated yet. Unfortunately, this could lead to clients never receiving a value (or receiving it with a high random delay), if they're too slow to read it (or the property simply updates too often). Note that this is done for _all_ property notifications, not just returned events. Hopefully not a problem in practice. If it turns out to be one, this mechanism could be restricted to actually returned events, for which this really matters.

wm4 committed 6 years ago

Commit 8a4f8121 parent 575197f1 file · +8 −4Browse files

Unified · Split

player/client.c+8−4
@@ -1638,10 +1638,12 @@ static void send_client_property_changes(struct mpv_handle *ctx) · 1637 unmodified lines
16381638 changed = true;
16391639 }
16401640
1641− if (changed) {
1642− ctx->new_property_events = true;
1643− } else if (prop->value_ret_ts == prop->value_ts) {
1641+ // Avoid retriggering the change event if the property didn't change,
1642+ // and the previous value was actually returned to the client.
1643+ if (!changed && prop->value_ret_ts == prop->value_ts) {
16441644 prop->value_ret_ts = prop->change_ts; // no change => no event
1645+ } else {
1646+ ctx->new_property_events = true;
16451647 }
16461648
16471649 prop->value_ts = prop->change_ts;
@@ -1698,7 +1700,9 @@ static bool gen_property_change_event(struct mpv_handle *ctx) · 50 unmodified lines
16981700
16991701 struct observe_property *prop = ctx->properties[ctx->cur_property_index++];
17001702
1701− if (prop->value_ret_ts != prop->value_ts) {
1703+ if (prop->value_ts == prop->change_ts && // not a stale value?
1704+ prop->value_ret_ts != prop->value_ts) // other value than last time?
1705+ {
17021706 prop->value_ret_ts = prop->value_ts;
17031707 prop_unref(ctx->cur_property);
17041708 ctx->cur_property = prop;

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t