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

percent-pos property is 'null' before quitting #66

Open
vankasteelj opened this issue Mar 11, 2020 · 8 comments
Open

percent-pos property is 'null' before quitting #66

vankasteelj opened this issue Mar 11, 2020 · 8 comments
Labels

Comments

@vankasteelj
Copy link
Contributor

Bug Description

When listening to 'statuschange' event and adding a 'percent-pos' property to observe, when quitting MPV, the percent-pos is set to 'null' on MPV > 0.29

How To Reproduce

let position = 0;

mpv.start().then(() => mpv.load(<myfile>)).then( () => mpv.observeProperty('percent-pos', 50));

mpv.on('statuschange', states => {
    position = states['percent-pos'];
    console.log(position); // Will display 0.01% up to 99.99%, no problem here
});

mpv.on('quit', () => console.log(position)) // Will display 'null' instead of the last percent-pos known, as it used to on 0.29
Software Versions
  • Node-Mpv: branch 2, commit 6fc6037 (latest at writing time)
  • MPV: 0.32.0 stable, windows build taken from the official recommended repository
Additional context

Might not be a node-mpv issue but a MPV issue? Trying to update from 0.29 has caused that problem mainly, afaik.

vankasteelj added a commit to vankasteelj/frak that referenced this issue Mar 11, 2020
@j-holub
Copy link
Owner

j-holub commented Mar 12, 2020

The question is how would the fix look like? Output 100.0 or always save the previous value when the value is null? I'm not 100% sure if that wouldn't eventually cause some error at a different place and doesn't quite seem like an elegant fix.

@vankasteelj
Copy link
Contributor Author

Currently, I save the last percent-pos elsewhere and update it if it's non-null only, so after exiting I have the last known percent-pos transmitted by mpv: vankasteelj/frak@39e1e8b

It's inelegant, but it works so far.

@vankasteelj
Copy link
Contributor Author

I don't know if mpv introduced a new state to avoid that "null" percent-pos in their latest updates, if they decided to void that value, or what.
But I know that node-mpv gives back correct "states" values after exiting, but not for the added observed-property percent-pos. It could be that observeProperty needs to be udpated in your code?

The thing is, I don't know where that "null" comes from, if it's from mpv or your code.

@vankasteelj
Copy link
Contributor Author

I believe it has to do with how events are handled since MPV 0.31, that states in the release notes something like "deprecate timed events", because as of >0.30 atm pause/resume events are fired twice.

@j-holub
Copy link
Owner

j-holub commented Apr 19, 2020

Interessting. I will have to read into that. It's always a pain to deal with different MPV versions, because you never know which one people have installed. If you use the Verison from Ubuntu or Debian Distributions it's ancient...

@j-holub
Copy link
Owner

j-holub commented Jan 14, 2021

Has this issue ever occured again? Just asking to see if it's worth investing time in.

@vankasteelj
Copy link
Contributor Author

Still using the workaround, so I'm guessing yeah, nothing changed on my side. But I havent tested without the workaround

@j-holub
Copy link
Owner

j-holub commented Mar 14, 2021

I used the following code to reproduce the issue (slightly adapted to the breaking changes I made with version 2)

mpv.start().then(() => mpv.load(<somefile>)).then( () => mpv.observeProperty('percent-pos', 50));

mpv.on('status', state => {
    if (state['property'] == 'percent-pos') {
        position = state['value'];
        console.log(position); // Will display 0.01% up to 99.99%, no problem here
    }
});

mpv.on('quit', () => console.log(position))

It does output 0, but it's undefined once before the song quits. I'm still unsure if this is an issue or not, if it causes problems or not. But I might have an idea I could publish as a PR. I'll notify you if I got one.

vankasteelj added a commit to vankasteelj/frak that referenced this issue Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants