osc.lua: always observe duration#17784
Conversation
guidocella
force-pushed
the
osc-duration
branch
from
b6dd130 to
211d1b8
Compare
|
You're removing the comment which says why it's conditional. If you think that the comment no longer relevant today, or that you have other reasons which are stronger than what this comment says - which it appears that you do, then the commit message should explain why that comment is no longer relevant or otherwise can be ignored. |
|
It's just already explained in the livemarkers section of the manual. |
I didn't look for that, and I fail to see how that could be relevant. The comment you're removing says that observing duration is conditional for performance reasons, and that it's only relevant for a single use case, which is rare. If you think this comment is no longer relevant, or you have reasons stronger than that to make it unconditional anyway, then it should be explained someplace, but currently it's not explained anywhere as far as I can tell. |
|
It's obviously relevant due to the condition being |
guidocella
force-pushed
the
osc-duration
branch
2 times, most recently
from
7ac194d to
ddf3b11
Compare
Currently the duration observer is toggled in multiple places to not call request_init unecessarily. It is much simpler to always observe it and call request_init conditionally in the callback. This also allows reusing the cached value throughout the OSC. Note that dividing by 0 is not an error in Lua so there is no need to check for both 0 and nil.
guidocella
force-pushed
the
osc-duration
branch
from
ddf3b11 to
eb2f9ec
Compare
| end | ||
| end | ||
|
|
||
| return mp.format_time(state.duration or 0, state.tc_ms and "%H:%M:%S.%T" or "%H:%M:%S") |
There was a problem hiding this comment.
Why you duplicate formatting logic from mpv core?
There was a problem hiding this comment.
I don't think a time format counts as logic? It just avoids retrieving duration on every tick when it's already cached. I can revert it if you want I guess.
There was a problem hiding this comment.
Does you code support MP_NOPTS_VALUE which should show unknown?
There was a problem hiding this comment.
mp.get_property_osd('duration') returns an empty string when it's unavailable not unknown. Also this should never run if duration is unavailable because ne.visible = state.duration ~= nil, I added or 0 just in case things get updated in an unexpected order.
| end) | ||
| observe_cached("duration", function () | ||
| if user_opts.livemarkers and state.chapter_list[1] then | ||
| request_init() |
There was a problem hiding this comment.
Does it have to be whole request_init? We should update only things that depend on duration. Duration will change on every tick for live content, so it's important to keep the update clean.
There was a problem hiding this comment.
You can probably extract the code that generates the ASS' seekbar out of prepare_elements into a new function, and add a new type of request to update the seekbar. But that may not be worth it because according to 96b246d livestreams with chapters are very rare, and even on 2016 hardware request_init only increased the render time from 1.3 to 1.4ms.
3 participants
Currently the duration observer is toggled in multiple places to not call request_init unecessarily. It is much simpler to always observe it and call request_init conditionally in the callback. This also allows reusing the cached value throughout the OSC.
Note that dividing by 0 is not an error in Lua so there is no need to check for both 0 and nil.