jaseg / python-mpv

Python interface to the awesome mpv media player
https://git.jaseg.de/python-mpv.git
Other
547 stars 68 forks source link

Incorrect type casting producing garbage on property access when running on i386 #50

Closed McSinyx closed 6 years ago

McSinyx commented 7 years ago

The speed option is described in mpv(1) section OPTIONS, subsection Playback Control.

I tried to used both property and option access but neither of them works:

Property access:

Traceback (most recent call last):
  File "./comp", line 370, in <module>
    comp.mp.speed *= 0.9091
  File "/home/cnx/.local/lib/python3.5/site-packages/mpv.py", line 1033, in __setattr__
    self._set_property(_py_to_mpv(name), value)
  File "/home/cnx/.local/lib/python3.5/site-packages/mpv.py", line 1025, in _set_property
    _mpv_set_property_string(self.handle, ename, _mpv_coax_proptype(value))
  File "/home/cnx/.local/lib/python3.5/site-packages/mpv.py", line 105, in raise_for_ec
    raise ex(ec, *args)
TypeError: ('Tried to get/set mpv property using wrong format, or passed invalid value', -9, (<MpvHandle object at 0xacbcfb24>, b'speed', b'0.0'))

Option access:

Traceback (most recent call last):
  File "./comp", line 370, in <module>
    comp.mp['speed'] *= 0.9091
  File "/home/cnx/.local/lib/python3.5/site-packages/mpv.py", line 1055, in __setitem__
    return self._set_property(prefix+name, value)
  File "/home/cnx/.local/lib/python3.5/site-packages/mpv.py", line 1025, in _set_property
    _mpv_set_property_string(self.handle, ename, _mpv_coax_proptype(value))
  File "/home/cnx/.local/lib/python3.5/site-packages/mpv.py", line 105, in raise_for_ec
    raise ex(ec, *args)
TypeError: ('Tried to get/set mpv property using wrong format, or passed invalid value', -9, (<MpvHandle object at 0xacc3eb24>, b'options/speed', b'0.0'))

I'm using python-mpv 0.3.4 on Debian testing.

McSinyx commented 7 years ago

Also it seems that MPV.seek(100, 'absolute-percent') doesn't seek to the exact end.

McSinyx commented 7 years ago

Just noticed ao_volume, volume and options/volume doesn't work either. The funny thing about this is that the volume property is used as an example.

jaseg commented 7 years ago

Recently there was a big change in the way properties are handled. You can downgrade to v0.2.9 for the moment. I will look into this as soon as I have time.

For the above stack trace, which libmpv version are you using?

Thank you, jaseg

McSinyx commented 7 years ago

I'm using libmpv 0.26.0-3. Currently my player comp still works fine with python-mpv 0.3.4 but I'm planning to add a few features.

Thank you a lot for keep developing this awesome library. The player using this has won a few local prizes in ICT for students (yep I'm a high school student, and I'm so sorry that sometimes my manner isn't nice when I file issues or pull requests :p I guess I can blame the hormones).

jaseg commented 7 years ago

Well, congratulations on that!

Ok, so I was unable to reproduce any of this on v0.3.4 on debian sid and the same libmpv 0.26.0-3. Regarding the speed option: The error message in your original comment says it tried to set speed to 0.0, which is out of range for it. Since I can't reproduce this right now I can only recommend you try finding the origin of that 0.0. volume seems to work fine, too.

I tried the absolute-percent-seek and that's off by a few seconds on a 20-minute video for me too. If you want to do this actual seek you could try player.time_pos = player.duration, but player.playlist_next('force') might also accomplish your goal.

Could you confirm the libmpv version you are using by running the following command? ls -l /usr/lib/x86_64-linux-gnu/$(python3 -c "import mpv; print(mpv.backend._name)") If this is in fact the 0.26.0 we both have installed we can do further debugging. The first thing you could try is to run the following to get some idea as to what your script's mpv instance is currently doing and what might be expecting.

import mpv
import time

player = mpv.MPV()
print(player.volume, type(player.volume))
print(player.ao_volume, type(player.ao_volume))
print(player.speed, type(player.ao_speed))
player.play('some_file.mp4')
time.sleep(5)
print(player.volume, type(player.volume))
print(player.ao_volume, type(player.ao_volume))
print(player.speed, type(player.ao_speed))
McSinyx commented 7 years ago

I'll use mpv playlist to play the next track eventually (yes as you might have guessed, using seek is a hack for that purpose, and it's sure ugly). Also thanks for the tip.

Here the output of the command:

$ ls -l /usr/lib/i386-linux-gnu/$(python3 -c "import mpv; print(mpv.backend._name)")
lrwxrwxrwx 1 root root 16 Jul 27 04:55 /usr/lib/i386-linux-gnu/libmpv.so.1 -> libmpv.so.1.25.0

I'm not so sure what has the version of 25.0 because apt show libmpv1 print this:

Package: libmpv1
Version: 0.26.0-3

Still I can't access ao-volume (and also ao-speed, but I think that's just a typo):

0.0 <class 'float'>
Traceback (most recent call last):
  File "foo.py", line 6, in <module>
    print(player.ao_volume, type(player.ao_volume))
  File "/home/cnx/.local/lib/python3.5/site-packages/mpv.py", line 1058, in __getattr__
    return self._get_property(_py_to_mpv(name), lazy_decoder)
  File "/home/cnx/.local/lib/python3.5/site-packages/mpv.py", line 1041, in _get_property
    cval = _mpv_get_property(self.handle, name.encode('utf-8'), fmt, outptr)
  File "/home/cnx/.local/lib/python3.5/site-packages/mpv.py", line 104, in raise_for_ec
    raise ex(ec, *args)
RuntimeError: ('Generic error getting or setting mpv property', -11, (<MpvHandle object at 0xb723365c>, b'ao-volume', 6, <cparam 'P' (0xb7180fc8)>))

This doesn't raise any exception, but I notice that the playback seems to hang after the speed property is accessed:

import mpv
import time

player = mpv.MPV()
print(player.volume, type(player.volume))
print(player.speed, type(player.speed))
player.play('gplv3.ogg')
time.sleep(5)
print(player.volume, type(player.volume))
print(player.speed, type(player.speed))

And here is the output:

0.0 <class 'float'>
0.0 <class 'float'>
Failed to open VDPAU backend libvdpau_nvidia.so: cannot open shared object file: No such file or directory
0.0 <class 'float'>
0.0 <class 'float'>
McSinyx commented 7 years ago

I did some further research and found out that the hang will be fix by adding player.wait_for_playback() so I guess it's not a bug.

I thought the 0.0 value of speed and volume is caused by false initialization, but I think it's more like type casting problem or something. I added the property observer so the code looks like this:

import mpv
import time

player = mpv.MPV(input_default_bindings=True, input_vo_keyboard=True)
@player.property_observer('speed')
@player.property_observer('volume')
def observer(name, value):
    print(name, value)
player.play('finalsong.mkv')
player.wait_for_playback()

After starting the program, I decrease speed by 10% 4 times then increase it back, decrease volume 4 times by 2 and increase it back (key sequence: [[[[]]]]99990000). And this is the output:

volume 0.0
speed 0.0
Failed to open VDPAU backend libvdpau_nvidia.so: cannot open shared object file: No such file or directory
speed 1.8741466827e-314
speed 2.862935607e-315
speed 1.2868910397e-314
speed 7.005271754e-315
speed 7.70579893e-315
speed 1.696436199e-314
speed 1.684831865e-315
speed 2.0024619646e-314
volume 0.0
volume 0.0
volume 0.0
volume 0.0
volume 0.0
volume 0.0
volume 0.0
volume 0.0

Note that the keybindings work fine and the info showed on the mpv window is still correct.

Edit: I added time-pos observer and the value is also garbage. Same with duration so I think the problem isn't limited to speed and volume.

jaseg commented 7 years ago

I tried, I can totally not reproduce this but I have a hunch. It seems you're using a 32-bit installation of libmpv, and that is probably triggering the type-casting problem. For the time being you could try a 64-bit libmpv/python installation if possible, meanwhile I'll try to procure a 32-bit libmpv/python install to reproduce this.

McSinyx commented 7 years ago

Currently I don't have an amd64 machine but I've just try to force the property format (instead of using MpvFormat.NODE) and the value seems to be correct now. I'll test on a 64-bit machine on Wednesday afternoon at school.

McSinyx commented 7 years ago

I did some further testing and it turns out there is a problem casting to POINTER(MpvNode) in MpvNode.node_cast_value called by MPV._get_property. Keeping the original pointer by changing line 211 to MpvFormat.NODE: lambda v: MpvNode.node_cast_value(v, cast(v, POINTER(MpvNode)).contents.format.value, decoder), fixes the problem but I'm not sure about the cases of NODE_MAP, NODE_ARRAY and BYTE_ARRAY because I don't know which property has that format to test. Also when you make the fix, could you please rewrite node_cast_value in the if-elif statement because I find the lambda dict is quite hard to read and it doesn't improve performance significantly.

jaseg commented 7 years ago

Thank you. I'll attempt a fix as soon as I got my hands on a 32-bit installation (the download is still running, I'm on extremely poor internet here...). The fix you posted works because the val field is the first element of the MpvNode struct. I'll try to rewrite this to do an explicit access on that field for clarity.

jaseg commented 7 years ago

BTW, NODE_MAP and NODE_ARRAY can be found in playlist and video_params. BYTE_ARRAY is used (only?) by screenshot_raw().

McSinyx commented 7 years ago

I've just tested playlist, video_params properties and MPV.screenshot_raw and they're all fine. If you still want to test on 32-bit environment then why don't you use debootstrap? It's totally doable to use a amd64 Debian to bootstrap a i386 one and that downloads a lot less, also you will still have a functional system while installing a new one.

Sorry for repeating this but would please you use the if - elif statements when you refactor MpvNode? Otherwise could you explain why using a dict of functions would be a better convention?

Edit: Somehow MPV's methods _multiply_property, _add_property and _cycle_property are still not working. I'm not sure if this is the same problem but here are the raised exceptions:

Traceback (most recent call last):
  File "./comp", line 370, in <module>
    comp.mp._multiply_property('speed', 0.9091)
  File "/home/cnx/Sources/comp/mpv.py", line 653, in _multiply_property
    self.command('multiply_property', name, factor)
  File "/home/cnx/Sources/comp/mpv.py", line 618, in command
    _mpv_command(self.handle, (c_char_p*len(args))(*args))
  File "/home/cnx/Sources/comp/mpv.py", line 104, in raise_for_ec
    raise ex(ec, *args)
ValueError: ('Invalid value for mpv parameter', -4, (<MpvHandle object at 0xacc0eecc>, <mpv.c_char_p_Array_4 object at 0xacba38e4>))

Edit of edit: This is caused by wrong mapping to the commands. I've just created a PR (#52) to fix that (and fix the doctrings too). Please accept it 🙏

McSinyx commented 7 years ago

I created a PR (#54) containing a tmp fix for this. I'd love to help on rewriting the node access If you need testing or anything else.

jaseg commented 6 years ago

Soooo... I just reworked this logic and pushed it in v0.3.8. So far, it seems to work smoothly on x86_64. Since I still don't have a working 32-bit setup that's still untested. Maybe I just re-created the same error as before. Give it a shot.

McSinyx commented 6 years ago

Thank you very much, the quick test on the previous comment runs flawlessly now. Sorry for being such a stickler, but again, could you please, replace these lambdas with if - elif clauses? If there's an advantage keeping it that way, could you explain to me in detail?

jaseg commented 6 years ago

I left it like that so far because IMHO it's more readable but I have now pushed a commit with if/elif. It looks less clean now but I could simplify the null pointer checking and add a nice error message for unknown formats. I'll put this into the next release since it's not a functional change.

Edit: and thanks for the testing!

McSinyx commented 6 years ago

Thanks a lot man, now it's a lot more readable to a person w/ only Python background like me. If I understand correctly, do you mean the decrease in readability is because of line break in the if statements? Since all we do is returning a value, I guess the code could be sth like:

if fmt == MpvFormat.NONE: return None
if fmt == MpvFormat.STRING: return decoder(v.string)
if fmt == MpvFormat.OSD_STRING: return v.string.decode('utf-8')
if fmt == MpvFormat.FLAG: return bool(v.flag)
if fmt == MpvFormat.INT64: return v.int64
if fmt == MpvFormat.DOUBLE: return v.double
if not v.node: # Check for null pointer
    return None
if fmt == MpvFormat.NODE: return v.node.contents.node_value(decoder)
if fmt == MpvFormat.NODE_ARRAY: return v.list.contents.array_value(decoder)
if fmt == MpvFormat.NODE_MAP: return v.map.contents.dict_value(decoder)
if fmt == MpvFormat.BYTE_ARRAY: return v.byte_array.contents.bytes_value()
raise TypeError('Unknown MPV node format {}. Please submit a bug report.'.format(fmt))

Also about styling, would you consider 16f9053 which matches mpv's commands in convenient functions and something similar to f22677f adding ab-loop and cycle-values (these two are binded to default mpv bindings, so I guess it won't make the module bloated to add them (?))? Thank you for putting effort maintaining this cool API.