Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash due to uninitialized state #933

Open
christoph-heinrich opened this issue Jul 17, 2024 · 2 comments
Open

Crash due to uninitialized state #933

christoph-heinrich opened this issue Jul 17, 2024 · 2 comments

Comments

@christoph-heinrich
Copy link
Contributor

christoph-heinrich commented Jul 17, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

christoph-heinrich commented Jul 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants