Time not correctly updating
DenialAdams opened this issue · 8 comments
I recently into strange behavior regarding the time field and streams. If I add a youtube stream to mpd, mpd seems to first have no idea of the time (mpc reports it as 0:00/0:00) and then as it parses it mpd starts correctly reporting the time. ruby-mpd always reports the time as nil, even when mpc is correctly reporting the time. I am thinking the ruby-mpd might not be getting the latest information? But I'm a bit lost as to where the issue lies. Here is an example of what I am talking about:
Youtube video I have been using for testing (happens to all videos): https://www.youtube.com/watch?v=Q1JxHz_9LBI
I have been obtaining the stream using youtube-dl --prefer-insecure -i -f140 -q --no-warnings -ge https://www.youtube.com/watch?v=Q1JxHz_9LBI via my mumble bot.
Immediately after adding the stream, nothing is playing yet
brick@treef ~/mumblecop (hg)-[default] % mpc
Stanchinsky - Piano Sonata in E-flat minor
[playing] #1/1 0:00/0:00 (0%)
volume: n/a repeat: off random: off single: off consume: on \
Stream starts playing through mpd
brick@treef ~/mumblecop (hg)-[default] % mpc
Stanchinsky - Piano Sonata in E-flat minor
[playing] #1/1 0:17/9:46 (2%)
volume: n/a repeat: off random: off single: off consume: on
brick@treef ~/mumblecop (hg)-[default] % irb
irb(main):001:0> require 'ruby-mpd'
=> true
irb(main):002:0> a = MPD.new
=> #<MPD:0x00000002593268 @hostname="localhost", @port=6600, @options={:callbacks=>false}, @password=nil, @socket=nil, @version=nil, @tags=nil, @mutex=#<Mutex:0x00000002593150>, @callbacks={}>
irb(main):003:0> a.connect
=> true
irb(main):004:0> a.current_song
=> #<MPD::Song:0x000000025767f8 @mpd=#<MPD:0x00000002593268 @hostname="localhost", @port=6600, @options={:callbacks=>false}, @password=nil, @socket=#<TCPSocket:fd 9>, @version="0.19.0", @tags=nil, @mutex=#<Mutex:0x00000002593150>, @callbacks={}>, @data={:pos=>0, :id=>35}, @time=nil, @file="http://r9---sn-ab5e6m7e.googlevideo.com/videoplayback?id=4352711f3ffd2c12&itag=140&source=youtube&pl=17&mm=31&mn=sn-ab5e6m7e&mv=m&ms=au&nh=EAI&ratebypass=yes&mime=audio/mp4&gir=yes&clen=9300109&lmt=1390341494585760&dur=585.514&mt=1438116453&sver=3&signature=81FD1414A77EFE2FDCE2CDB9F2E7B6215AE56704.19EB9387CC5F4D701538CEA4779088190B7E8797&key=dg_yt0&upn=5RvJtqAhGo0&fexp=901816,9407150,9407813,9408710,9415365,9415387,9415430,9415485,9416126,9416217,9416324,9417279,9417878&ip=96.252.105.91&ipbits=0&expire=1438138113&sparams=ip,ipbits,expire,id,itag,source,pl,mm,mn,mv,ms,nh,ratebypass,mime,gir,clen,lmt,dur", @title="Stanchinsky - Piano Sonata in E-flat minor ", @artist=nil, @album=nil, @albumartist=nil>
ruby-mpd returns time as nil.
Any thoughts? I will try digging more into the source. I'm guessing that when the song object is initialized the time is nil but when mpd updates it ruby-mpd doesn't read the updated value, but I'm not sure how it works behind the scenes. But on second thought I guess I must be wrong because I have been adding the title after adding the stream and ruby-mpd is correctly reading the song. So... I'll keep looking :)
I've looked into this and I can confirm this issue. I wanted to understand better what happens in the background, so I've tried reproducing it. I think I know what's happening here...
You see, when you check the status of MPD using mpc current it actually sends two commands to the MPD daemon via the protocol:
Jul 28 23:25 : client: [29] opened from [::1]:55668
Jul 28 23:25 : client: [29] process command list
Jul 28 23:25 : client: process command "status"
Jul 28 23:25 : client: command returned 0
Jul 28 23:25 : client: process command "currentsong"
Jul 28 23:25 : client: command returned 0
Jul 28 23:25 : client: [29] process command list returned 0
As you can see it sends a status and a currentsong (more of these in the MPD protocol specs).
When we call current_song in ruby-mpd we only send currentsong to the daemon which returns partial information:
irb(main):015:0> a.current_song
file: http://r9---sn-c0q7ln7d.googlevideo.com/videoplayback?id=4352711f3ffd2c12&itag=140&source=youtube&mn=sn-c0q7ln7d&mm=31&pl=17&ms=au&mv=m&ratebypass=yes&mime=audio/mp4&gir=yes&clen=9300109&lmt=1390341494585760&dur=585.514&signature=5E6C3ABF8FF4533300656E56F58ED0CFCF3C7BE8.0272A84119679A8CC9F8E68BB30BDC943097C8B3&sver=3&mt=1438117606&fexp=901816,9407538,9407992,9408207,9408710,9415365,9415397,9415485,9416126,9416214,9416227,9416274,9416328,9416729,9418203&key=dg_yt0&upn=71OfC1fuqEI&ip=178.48.121.158&ipbits=0&expire=1438139300&sparams=ip,ipbits,expire,id,itag,source,mn,mm,pl,ms,mv,ratebypass,mime,gir,clen,lmt,dur
Pos: 2
Id: 3
=> #<MPD::Song:0x007ff803442048 @mpd=#<MPD:0x007ff8030f8310 @hostname="localhost", @port=6600, @options={:callbacks=>false}, @password=nil, @socket=#<TCPSocket:fd 13>, @version="0.19.0", @tags=nil, @mutex=#<Mutex:0x007ff802263f98>, @callbacks={}>, @data={:pos=>2, :id=>3}, @time=nil, @file="http://r9---sn-c0q7ln7d.googlevideo.com/videoplayback?id=4352711f3ffd2c12&itag=140&source=youtube&mn=sn-c0q7ln7d&mm=31&pl=17&ms=au&mv=m&ratebypass=yes&mime=audio/mp4&gir=yes&clen=9300109&lmt=1390341494585760&dur=585.514&signature=5E6C3ABF8FF4533300656E56F58ED0CFCF3C7BE8.0272A84119679A8CC9F8E68BB30BDC943097C8B3&sver=3&mt=1438117606&fexp=901816,9407538,9407992,9408207,9408710,9415365,9415397,9415485,9416126,9416214,9416227,9416274,9416328,9416729,9418203&key=dg_yt0&upn=71OfC1fuqEI&ip=178.48.121.158&ipbits=0&expire=1438139300&sparams=ip,ipbits,expire,id,itag,source,mn,mm,pl,ms,mv,ratebypass,mime,gir,clen,lmt,dur", @title=nil, @artist=nil, @album=nil, @albumartist=nil>
As you can see not only the @time but also @title, @artist, @album and @albumartist are also nil because of the insufficient information we gather from a single currentsong command.
I wonder whether we should mimic mpc and issue a status as well as currentsong to gather full information... Ideally I'd want MPD to return the missing data in currentsong just like in status.
Ah! That makes a lot of sense! Thanks for looking into it liquid :)
So basically now the question is do we take it upstream or do we just work around it the best we can? I'll wait and see what @archseer says because I don't really feel qualified to weigh in. I can try to work on a pull request if we end up deciding to just call both status and currentsong.
Hmm, so this only happens for streams? The docs on this are fairly vague: "currentsong -- Displays the song info of the current song (same song that is identified in status)."
Yeah, I think we should query both commands and fill out the missing fields, the expected behavior definitely is returning all of the data.
Streams are where I have experienced it, not sure exactly where else it comes up if at all. I'll try to work on a pull request and come back if I need help.
Is the way to go about it altering the current_song method to also call status and then merge the hashes before creating the object? Or should I create a new method somewhere else and call both current_song and status and then merge the song object created with current_song with the information from status? I hope that questions makes sense :)
Actually, I have an idea... Code soon to follow
Created #36