search

tomasklaen / uosc

Feature-rich minimalist proximity-based UI for MPV player.
GNU Lesser General Public License v2.1
1.87k stars 68 forks source link

Crash due to uninitialized state #933

Open christoph-heinrich opened 4 months ago

christoph-heinrich commented 4 months ago

I've just encountered the following crash image

It happened in VolumeSlider:render() because state.volume was still nil by the time the first render happened.

Afaict that's not just a problem with volume, but can in theory happen with a lot of state variables in elements. We have no way to make sure everything necessary is non nil by the time we render.

Considering how rarely this happens there is no reason to rush a solution, but we should discuss our options.

The obvious solution is putting nil checks all over the place, but maybe there is a better solution? We could also set some default value whenever it would be nil, but then we don't have the option of somehow representing nil in the UI, if that even makes sense for some things.

tomasklaen commented 4 months ago

Probably adding a default value parameter to create_state_setter() and set_state() that will set the value to that if the mpv reported one is nil, and use it where appropriate.

Regarding volume specifically, isn't it kind of a bug that mpv even reports nil here? Or is it a valid state somehow? If it's valid, we should switch volume element into disabled state (as when audio is not available). If it's not, than I guess just default it to 1 for that tiny moment while it's uninitialized.

christoph-heinrich commented 4 months ago

I think what's happening here is that it's not guaranteed that all observe-property-callbacks run before the render-timer-callback gets run for the first time.

When that crash happened the cpu was already busy doing something else when mpv was started, so it could be that that's why initialization took unusually long and volume wasn't set yet. In that case we'd have to keep track which state variables have already been initialized and only render once all those callbacks have happened at least once. Then all the nils left are ones where mpv can return nil for that property during normal operation.

However I can't think of a good way to implement that. If the state table contained all fields from the start, we could create a second table containing all fields with false, on each observer callback that gets set to true and once all are true the table gets set to nil and then all relevant code gets bypassed because the table is nil. I guess we could create a function that gets called instead of mp.observe_property that takes care of registering the variable in that table.