osc.lua: cache observed properties#17766
Conversation
For properties that were already being observed but still retrieved over and over, cache their values using the new observe_cached(). osd-dimensions observer said that if cached we may have to worry about property ordering but it seems to work fine. It is nice to cache it because it was the property retrieved the most frequently, in 5 different places.
|
I'm still not sure what is the benefit of this caching, but whatever, if it works. |
| mp.observe_property("playback-time", "number", request_tick) | ||
| mp.observe_property("osd-dimensions", "native", function() | ||
| -- (we could use the value instead of re-querying it all the time, but then | ||
| -- we might have to worry about property update ordering) |
There was a problem hiding this comment.
Have you verified that this is fine?
There was a problem hiding this comment.
Well I wrote so in the opening post. But more testing is welcome if you can think of interactions that would cause breakage.
There was a problem hiding this comment.
My bad, I didn't read the OP and was just looking at the diff.
|
You have to note that this all observe effort might be severe deoptimization. If you call render function on every and all property update callback. IIRC the updates are coalesced and limited by max tick rate. But in general driving the render by hundreds of properties instead just using get_native is noise in practice. |
mpvOS is currently in severe need of a profiler for the osc so that we can microoptimize things more accurately :^) (no, but seriously, being able to profile the lua scripts would be nice) |
What do you mean? No update callback is added. I'm caching properties that were already being observed anyway. |
I'm saying in general, you really like to observe anything and it shows by recent activity. There are many changes that are not fixing and real issues, it's increasing risk factor for thing to break in strange ways. |
It would be good indeed. So far we only have stats page 0. |
How does this observe "anything" if 0 new observers are added?? It was avih's preference to override Also aren't you the one who started caching things in scripts in console.lua and commands.lua?
#17563 removed code repetition in current and future caching. And I've been explicitly asked to cache
I guess you were referring to the observers that only call |
There was critical bottleneck due to property calling VOCTRL and delaying whole lua thread by whole render cycle.
I'm not saying the change is bad. But caching things introduce possibly synchronization issues. It's not good for highly interactive elements. By doing observe_callback -> cache -> request_tick -> delay -> use_cached, you may be using old values. For example caching mouse-pos is quite problematic, because by the time the value is used, mouse may already by in different position. Making UI look laggy and unresponsive. Another example to lesser degree may be, osd-dimensions, where during window resize we would render OSC not at current size. This is arguably small, but all those desync/latency issues are like that, it just may degrade the snappines of ui. Conversely caching hwdec value is generally fine, because it's not interactive or based on user input. If it's delayed by even few ticks it doesn't really matter. Another issue may be that tick happens in-between some properties are update. Which again may or may not cause glitches. |
It depends on the exact sequence of events, but largely, new values from observation would be updated while waiting for the event which triggers the rendering, and then, throughout the rendering, only cached values are used.
You will need to back this up with at least observation that it can happen in practice. Also, a counter argument to this is that the mouse position could change at different parts of the rendering if each time it's required it's read via the property. Largely, normal observation should have recent enough updated values before rendering starts. And if you're not sure of that, or have some evidence that it might introduce UI lag, then the alternative would be to read all the values once when rendering starts. But in my experience, lag is not introduced by observation. The biggest introducer of lag in my experience is output frames buffering, which is beyond the control of the OSC. |
|
I really don't see caching osd-dimensions causing the OSC to not be rendered at the current size. Try it yourself if you believe it does. Either way the current way on master where some observered properties are cached and others aren't it just arbitrary. |
It's easy to test, if you want to see yourself. If your observe cached value is in the past, it will render behind the current position. Depending how much other work is in-between the observe callback time and the use time. local assdraw = require "mp.assdraw"
local tick_delay = 1 / 60
local work_delay = 30 / 1000
local tick_timer
local tick_last_time = 0
local observed = {}
local function work(s)
local deadline = mp.get_time() + s
while mp.get_time() < deadline do end
end
local tick
local function request_tick()
if tick_timer == nil then
tick_timer = mp.add_timeout(0, tick)
end
if not tick_timer:is_enabled() then
local timeout = tick_delay - (mp.get_time() - tick_last_time)
tick_timer.timeout = math.max(timeout, 0)
tick_timer:resume()
end
end
local function fmt(p)
if not p then return "nil" end
return string.format("x=%d y=%d hover=%s",
p.x or 0, p.y or 0, tostring(p.hover))
end
local function render()
local osd_w, osd_h = mp.get_osd_size()
if not osd_w or osd_w == 0 then return end
work(work_delay) -- simulate some work that happesn in real render()
local live = mp.get_property_native("mouse-pos") or {}
local match = live.x == observed.x and live.y == observed.y
and live.hover == observed.hover
local ass = assdraw.ass_new()
ass:new_event()
ass:pos(20, 20)
ass:append("{\\fs20\\bord2\\1c&HFFFFFF&\\3c&H000000&}")
ass:append("observed: " .. fmt(observed) .. "\\N")
ass:append("live: " .. fmt(live) .. "\\N")
ass:append(string.format("{\\1c&H%s&}match: %s",
match and "00FF00" or "0000FF", tostring(match)))
if observed.x then
ass:new_event()
ass:pos(0, 0)
ass:append("{\\bord1\\1a&HFF&\\3c&H0000FF&}")
ass:draw_start()
ass:round_rect_cw(observed.x - 12, observed.y - 12,
observed.x + 12, observed.y + 12, 12)
ass:draw_stop()
end
if live.x then
ass:new_event()
ass:pos(0, 0)
ass:append("{\\bord0\\1c&H00FF00&}")
ass:draw_start()
ass:round_rect_cw(live.x - 4, live.y - 4,
live.x + 4, live.y + 4, 4)
ass:draw_stop()
end
mp.set_osd_ass(osd_w, osd_h, ass.text)
end
tick = function()
tick_last_time = mp.get_time()
render()
end
mp.observe_property("mouse-pos", "native", function(_, val)
observed = val or {}
request_tick()
end)
mp.observe_property("osd-dimensions", "native", request_tick)
mp.add_periodic_timer(tick_delay, request_tick) |
It was only example of possibility, it's not something you will notice if you are few dozen pixels behind. It would have to be big desync. |
Yes, which I stated a bit differently as
However also, largely that's not an issue, and there's a reason why timer events don't fire before the event queue is clear of other events (like property observers) - exacly so that timers fire only after all obervers and other callbacks are up to date. That was intentional design decision (first implemenbted in js, and then adopted in lua). Obviously, if you spend a lot of time from when the process starts until you start using the values, then there might be some cases of outdated values. But however you look at it, if there's one thing you don't want - is to read different values from the same property throughout the rendering, because that can lead to inconsistency at best, and crashes at worst (one part of the code makes some assumption based on the value, and then the supposedly same value is different), which is arguably worse than just lag. Which is where my other suggestion comes in. If you don't trust that the observed values are up to date, then just read all of them once as late as possible before rendering starts. Either way, doing repeated get_property for the same property throughout the rendering is just bad news. It either introduces inconsistency, or it adds unnecessary work and potential delays (accessing a lua value is mostly faster than reading it from an mpv property). |
|
Also, there are other ways to address this issue. My approach several years ago was to just change get_property itself to use observed values instead of reading the value on each call (it reads the value on first property access, and while at it sets up the observer, but not for blacklisted frequent props like time-pos), which avoids adding all those explicit cached values and changing every call site which uses get_property. I've heard claims, but no one provided numbers, that this can be wasteful because it can read more than necessary (i.e. while the OSC is not in use). But there are other approaches too, for instance, something similar to above but simpler and without auto-observe, by calling something like Basically, I think any solution which automates this in one place and avoids touching dozens of places at the code should be preferable, as long as it does the job well. |
That's interesting.
Agreed.
I noted that it would duplicate the observers of properties that were already being observed. (But I did not measure the impact.) That would not be an issue along with this PR because But actually, all EDIT: apparently even the volume and mute observers were added recently for this reason, in 89427a4. EDIT2: wm4 himself noted in 48f9062 that not observing properties causes such bugs. |
If a property is observed, mpv needs to obtain the value of the property on every tick or certain events and compare it with the the current value to see if it changed. This is done multiple times per tick, one for each mpv client. Some properties like those requiring VOCTRLs can be slow to get, and properties with a large amount of data in it can be slow to compare. If a property is automatically observed because a client requests it once (which cannot be done for properties that cannot be observed), all above will be done for every property even requested for every client. Once they add up they can produce performance impact. |
Additionally to VOCTRL properities which effectively needs to wait for VO thread. The example of property with large amount of data is |
On every tick is only for properties associated to
I suspected that converting large playlists to a Lua table was what is slow more than the comparison. osc observes playlist-pos and playlist-count to work around it. |
|
FYI, I've been playing around a bit with the luajit profiler, from some cursory test of just opening a video and scrubbing through the seekbar, I get the following without this PR (it's a bit noisy so I'm only pasting the top couple entries): Notice that Now with this PR:
|
Sure, thing. Moving cost around :) |
Getting osd size obviously shouldn't be the most costly thing in a ui script. The current state of |
|
Percentage values alone are not enough to analyze the performance, because it only says where most time is spent, but doesn't actually say how much this time is. If previously function X was 50% of render and overall render took 10 ms, and now function Y is 50% but the overall time is now 5 ms, then overall it got x2 faster, but you can't say such things if only looking at percentages of overall. So if X was 50% and 5 ms previously, and Y was 25% and 2.5 ms previously, and now X is 1% and Y is 50% but still 2.5ms, then nothing moved around. Instead, it's just pure win. |
5 participants
For properties that were already being observed but still retrieved over and over, cache their values using the new observe_cached().
osd-dimensions observer said that if cached we may have to worry about property ordering but it seems to work fine. It is nice to cache it because it was the property retrieved the most frequently, in 5 different places.