j-holub / Node-MPV

A NodeJs Module for MPV Player
https://www.npmjs.com/package/node-mpv
MIT License
116 stars 73 forks source link

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

Open vankasteelj opened 4 years ago

vankasteelj commented 4 years ago

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
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.

j-holub commented 4 years ago

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 commented 4 years ago

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: https://github.com/vankasteelj/frak/commit/39e1e8bcdcff14fa27657615ef12db84a6185bc8

It's inelegant, but it works so far.

vankasteelj commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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

vankasteelj commented 3 years ago

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

j-holub commented 3 years ago

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.