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

dbus/mpris issues - artUrl, available and active not updating correctly? #890

Closed
exoqrtx opened this issue Aug 9, 2019 · 26 comments
Closed

Comments

@exoqrtx
Copy link

exoqrtx commented Aug 9, 2019

I've been using the dBus/mpris interface to get album art and metadata for a python project I'm working on. I've noticed some possible errors I wanted to run by you though.

First, if a song is played that doesn't have album artwork, the mpris:artUrl tag in the Metadata passed through dbus seems to default to the last sent artwork. I would think this should default to a null string or something similar, or am I missing something.

Second, the tags for Active and Available seem to be somewhat fickle. For the most part they work ok, but if my connection is dodgy and starts dropping out it'll send False for both and not update until I disconnect and reconnect. Also, looking at them it seems that Available mostly indicates that a connection is active to some device/source, while Active indicates something is actually playing. Is this correct?

I'm using this on a Raspberry Pi 3B+, running Arch linux with a manual build of shairport-sync, currently on 3.3.1 (3.3.1-OpenSSL-Avahi-ALSA-soxr-convolution-metadata-mqtt-dbus-mpris-sysconfdir:/etc)

Thanks for all the hard work!

@mikebrady
Copy link
Owner

mikebrady commented Aug 9, 2019

Thanks for the interesting post. I am away from machines at present – it will be late next week before I can check the issues you mention.

Meanwhile, available means that particular interface is available for use. That is, that Shairport Sync is connected to a source, e.g. iTunes, that can implement the interface's properties and commands. For example, if the source is iOS, then the org.gnome.ShairportSync.RemoteControl interface is available. If the source is iTunes, then the org.gnome.ShairportSync.AdvancedRemoteControl interface is also available.

The Active property in the org.gnome.ShairportSync interface reflects whether Shairport Sync is in the active mode or not.

@exoqrtx
Copy link
Author

exoqrtx commented Aug 9, 2019

Sounds good, in the meantime I'll keep playing around with available/active and update with anything else I find and hopefully give you some more concrete test cases to look at.

@mikebrady
Copy link
Owner

I think you are right about the mpris:artURL not updating correctly, but I don't know yet whether that is due to Shairport Sync or the metadata source.

I am still away from development machines until late next week...

(BTW, there is are simple mpris and dbus test clients available with the --with-dbus-test-client and --with-dbus-test-client configuration options.)

@mikebrady
Copy link
Owner

There is indeed a bug in the way Shairport Sync deals with blank artwork information which is causing the problem you have identified. Additionally if the network fails temporarily, the Available properties will be negated but they will not be reasserted when the network comes back. As I mentioned, it will be the end of next week before I can fix these problems. Thanks for finding them!

@exoqrtx
Copy link
Author

exoqrtx commented Aug 12, 2019

Happy to help, I use shairport-sync almost daily, so many thanks to you for your awesome work. Once you have time to fix them I'd be happy to help test out the changes in the environments I'm running.

mikebrady added a commit that referenced this issue Aug 15, 2019
…ris interfaces.

specifically Issue #890 -- #890

Begin to recognise empty pictures.

Need to make the messages respond only to changes.

Need to make an interface newly available if it became unavailable due to a network transient
@mikebrady
Copy link
Owner

mikebrady commented Aug 18, 2019

Hi there. So, I've done some work on this, in the unstable branch.

First, if a song doesn't have artwork, the mpris:artUrl tag is now correctly updated to give an empty pathname.

Second, it should (!) be somewhat less sensitive to loss of network. Previously, if it failed to make a connection in five successive tries, it would consider the server had disappeared. Now it waits for a more definite signal. (I may have to revisit this...)

Third, the mpris and dbus property changed messages should now only be issued for true changes – so, for example, if the artwork or the genre remains the same, you won't get repeated artwork or genre messages in the metadata.

I'd be obliged if you would give it a try — as I mentioned, the updates are in the unstable branch.

@exoqrtx
Copy link
Author

exoqrtx commented Aug 19, 2019

Awesome, I'll try and give it a shot tonight, thanks!

@exoqrtx
Copy link
Author

exoqrtx commented Aug 19, 2019

I compiled it and it seems to be going good so far. I had to re-configure some stuff in my code to handle the stuff only being passed on updates, but after I did that it seems to work great and handle the missing artwork as expected. I haven't had any network issues yet, but I'll keep an eye out for that and keep you informed.

@exoqrtx
Copy link
Author

exoqrtx commented Aug 20, 2019

One possible downside of only sending updates across for Metadata is that if you're not using a loop like GLib.MainLoop to monitor any updates, but are instead directly polling dbus at '/org/gnome/ShairportSync' then Metadata now only contains the last thing passed, rather than the full metadata.

@mikebrady
Copy link
Owner

Thanks. That is indeed true. It would be possible to modify the code so that, whenever a change occurred, the entire new metadata was generated. What to you think?

@exoqrtx
Copy link
Author

exoqrtx commented Aug 20, 2019

I think that route would give the most flexibility for users, since they could manually poll or have a listener loop and get the full data using either method. Otherwise the metadata field is state-dependent which is still workable, but may be confusing to some if they're not expecting it.

@mikebrady
Copy link
Owner

mikebrady commented Aug 20, 2019

Thanks — I am inclined to agree. Unfortunately, I will be away from my machines for another week. I’ll make the change then. Meantime, if you find any other problems, I’d be glad to hear of them!

@mikebrady
Copy link
Owner

Hi there. I've updated the unstable branch to output the complete metadata whenever a change occurs (and tidied up a few other things).

@exoqrtx
Copy link
Author

exoqrtx commented Aug 29, 2019

I've installed it and so far so good, I'll give you some followup data in a bit.

mikebrady added a commit that referenced this issue Aug 29, 2019
    Fix Issue #890 -- #890
    Recognise situation where a track has no artwork properly.
    Less likely to drop the 'availabile' property.
    Always output the full metadata whenever any part of it has changed.
    Metadata coming from dacp is now incorporated,
mikebrady added a commit that referenced this issue Aug 29, 2019
Fix metadata issues with the dbus and the MPRIS interfaces -- handle missing artwork properly, make the 'available' property more resilient.
@mikebrady
Copy link
Owner

I've made a few more small "housekeeping" changes and rolled it all into the development branch, so I'll be deleting the unstable branch...

@mikebrady
Copy link
Owner

Looking at the code that monitors the remote device, I can't help thinking it's too complicated, so I may make some more changes to it.

@exoqrtx
Copy link
Author

exoqrtx commented Aug 30, 2019

So far the changes you've made when I last compiled the unstable branch have been working great. If you make any changes though I'd be happy to help test them out.

@mikebrady
Copy link
Owner

Thanks for the offer! I've just pushed some more changes which are minimal, but should make the status of those available flags a bit more reliable.

@exoqrtx
Copy link
Author

exoqrtx commented Sep 10, 2019

Sorry for the delay, I've been a bit under the weather, but I'll try this out and let you know.

@exoqrtx
Copy link
Author

exoqrtx commented Sep 10, 2019

The development branch isn't compiling for me right now. I'm getting the following:

/usr/bin/ld: shairport.o: in function exit_function': /home/user/shairport-sync/shairport.c:1222: undefined reference to stop_mpris_service'
/usr/bin/ld: /home/user/shairport-sync/shairport.c:1216: undefined reference to `terminate_mqtt'

It's possible I'm doing something wrong or maybe I'm catching this while you're in the middle of something though.

@mikebrady
Copy link
Owner

Thanks — Let me check!

@mikebrady
Copy link
Owner

You were right – I've fixed it and just pushed an update in the development branch. Apologies!

@mikebrady
Copy link
Owner

Also there was a similar problem when you include the MPRIS interface. It is fixed now. Apologies again – I was trying to clean up some code and this happens!

@exoqrtx
Copy link
Author

exoqrtx commented Sep 12, 2019

Compiled and installed. I'll let you know if I come across anything.

@mikebrady
Copy link
Owner

Many thanks.

@mikebrady
Copy link
Owner

Thanks for this contribution. Closing the issue. Please open a new one if necessary.

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

No branches or pull requests

2 participants