szclsya / mpdris2-rs

Exposing MPRIS V2.2 D-Bus interface for mpd
GNU General Public License v3.0
31 stars 1 forks source link

[BUG] dooesn't react to meta changes from streams. #10

Closed barolo closed 2 months ago

barolo commented 2 months ago

If the playing track is a stream, for example one of the radio streams from SomaFM or RadioParadise and the the playing track metadata changes mpdris should react to the change and show new meta. Currently it does nothing.

szclsya commented 2 months ago

Should be fixed in 77e9cd2.

barolo commented 2 months ago

Now it fails to connect for some reason [same remote host as before, nothing was changed].

szclsya commented 2 months ago

Because socket support is added and now the port is part of the --host argument. (or use MPD_HOST).

Edit: Added back --port for compatibility.

barolo commented 2 months ago

Because socket support is added and now the port is part of the --host argument. (Or use MPD_HOST).

Ah!, Now it works, meta from streams too. Would it be very annoying to implement splitting it on hyphen into artist and title fields when it is a stream? From what I've seen this format is common. Would be nice to have a station name too.

szclsya commented 2 months ago

Doing splitting on this side might be a bad idea as since Internet radio is just a normal song in MPD's protocol, doing the splitting might interfere with local songs.

As of station name, I'm not quite sure now Internet radios implement this kind of thing. Do they just set the title tag (which contains both the artist and title) and don't set the artist? In that case I can make it show the station name (denoted "Name" in MPD's protocol, not sure if it's specific about the station I tested) if artist tag is missing.

barolo commented 2 months ago

Doing splitting on this side might be a bad idea as since Internet radio is just a normal song in MPD's protocol, doing the splitting might interfere with local songs.

I agree, it should be done on the player's side, I know at least two that do this, with MPD it will probably never happen though [they are very icky about icy streams and hope that everyone will move to vorbis]

Doesn't MPD report what kind of data it is playing though?

As of station name, I'm not quite sure now Internet radios implement this kind of thing.

I've checked some popular ones and they all use icy-name, no idea if MPD does something with it. You'd have to limit the characters as some like to be descriptive [but always start with the station's name] ....

szclsya commented 2 months ago

Looks like MPD indeed translates icy-name into name, and after checking my library it seems that local music don't use this tag, so I've added station name in the notification. (see a175efe)

they are very icky about icy streams and hope that everyone will move to vorbis

Or even better, Opus! But I digress.

barolo commented 2 months ago

Looks like MPD indeed translates icy-name into name, and after checking my library it seems that local music don't use this tag, so I've added station name in the notification. (see a175efe)

no change, probably MPD isn't passing it or something.

Edit. I can see it through mpc though.

Or even better, Opus! But I digress.

Vorbis is a container [for opus too] and metadata standard. Insofar I know only one station which uses it, RadioParadise, they pack flac into vorbis, and since vorbis chains tracks [instead of continuous stream] they also have native per track album art.

barolo commented 2 months ago

Update. It works actually, but again only in notification, That's why I haven't noticed it at first. Nothing in Plasma applet. Also, as mentioned, you should limit the length, because it can be something like Fluid: Drown in the electronic sound of instrumental hiphop, future soul and chilled trap. or even longer. 20 chars should be enough.

szclsya commented 2 months ago

That's because we don't actually format what KDE shows here. According to the MPRIS2 metadata guideline we only supply the metadata. It's up to the MPRIS2 frontend to decide what to display. And based on KDE's kmpris implementation, they are only using title, album and artist.

One way I can make it work is since icey don't send album information (to my knowledge) we can fill the station name in that, but that's technically violating the specification.

barolo commented 2 months ago

That's because we don't actually format what KDE shows here. According to the MPRIS2 metadata guideline we only supply the metadata. It's up to the MPRIS2 frontend to decide what to display. And based on KDE's kmpris implementation, they are only using title, album and artist.

One way I can make it work is since icey don't send album information (to my knowledge) we can fill the station name in that, but that's technically violating the specification.

Yeah, I've never seen album passed in icy. Players do what they want with the icy stuff so I wouldn't worry too much.

szclsya commented 2 months ago

Should be fixed in f3ac379.

Vorbis is a container [for opus too] and metadata standard. Insofar I know only one station which uses it, RadioParadise, they pack flac into vorbis, and since vorbis chains tracks [instead of continuous stream] they also have native per track album art.

I think ogg is the container and vorbis/opus are the codecs?

barolo commented 2 months ago

Should be fixed in f3ac379.

Vorbis is a container [for opus too] and metadata standard. Insofar I know only one station which uses it, RadioParadise, they pack flac into vorbis, and since vorbis chains tracks [instead of continuous stream] they also have native per track album art.

I think ogg is the container and vorbis/opus are the codecs?

You're right! I've got hem backwards somehow.

barolo commented 2 months ago

Should be fixed in f3ac379.

Just tested it and it works beautifully, the name isn't shortened in Plasma's applet but it's actually better this way because the applet elides the name itself. Thanks!

I have a small nitpick, everything seems to be bunched together in the notification, would it be possible to format it?

Closing it since the main issue is fixed.

szclsya commented 2 months ago

You mean customizable notification template? We will need to add a template engine as dependency for that.

barolo commented 2 months ago

You mean customizable notification template? We will need to add a template engine as dependency for that.

I don't care about customizing it if that's what you've meant. What I meant to simply just have station name and the rest of the meta on separate lines.

szclsya commented 2 months ago

Wish granted! (see cdc64b7 and 2e38a0c).

barolo commented 2 months ago

Perfect, throws some errors but seems to be working just fine.

ERROR [mpdris2_rs::plugins::fdo_notification] <b>Myth Syzer - They Found $ In Range</b>
Fluid: Drown in the electro...
szclsya commented 2 months ago

Ah that's just some debug code that I forgot to remove. Should be good now.