Metadata pipe enhancement to send volume

[aleszczynskig]

Is your feature request related to a problem? Please describe.
You have implemented sending metadata to forked-daapd using the shairport style metadata pipe. It works very well. Thank you. It is also possible to send the current volume and any volume changes to forked-daapd via the metadata pipe so that volume changes are implemented at the speaker and not in software. This works via well with Shairport-sync and forked-daapd. Can you implement this in librespot-java? I know that the changes made using the speakers will not be communicated back to librespot as the pipe is one-way however this would provide some useful functionality.

Describe the solution you'd like
Enable an option to output the audio stream to a pipe fixed at 100% and pass the current volume and volume changes in the metadata pipe to enable forked-daapd to process volume changes using hardware controls of connected devices.

Describe alternatives you've considered
None at this time

Additional context
Because shairport-sync and forked-daapd work in the airplay world, forked-daapd can control the source volume as the source details are provided to forked-daapd via the metadata pipe so that it can establish a control channel back to the source, this enables changes on the device (playback controls, volume changes to be reflected in the source). Would it also be possible to implement an airplay control interface that forked-daapd could communicate with to enable this functionality as well?

[devgianlu]

Implement the new volume "event" shouldn't be a problem since we already have the event internally. Can you ointime to the documentation where I can see the codes to send?

I guess I could also implement the control pipe, can you create a separate issue for that?

[aleszczynskig]

Here are the events for volume control from https://github.com/mikebrady/shairport-sync-metadata-reader

pvol -- play volume. The volume is sent as a string -- "airplay_volume,volume,lowest_volume,highest_volume", where "volume", "lowest_volume" and "highest_volume" are given in dB. The "airplay_volume" is what's sent by the source (e.g. iTunes) to the player, and is from 0.00 down to -30.00, with -144.00 meaning "mute". This is linear on the volume control slider of iTunes or iOS AirPlay. If the volume setting is being ignored by Shairport Sync itself, the volume, lowest_volume and highest_volume values are zero.

I believe that you ignore the volume, lowest_volume and highest_values as you will send the audio stream to forked-daapd at 100% volume. Then forked-daapd will parse the airplay_volume from 0.00 to -30.00 as it already has this capability when working with shairport-sync and the volume will be controlled at the speakers natively by forked-daapd.

I have created another feature request for the DACP control interface here. https://github.com/librespot-org/librespot-java/issues/318

Thank you for your excellent work.

[devgianlu]

Wait, we are already sending that:

librespot-java/player/src/main/java/xyz/gianlu/librespot/player/Player.java

Lines 1033 to 1039 in 5c9c9be

	private void sendVolume(int value) {
	float xmlValue;
	if (value == 0) xmlValue = 144.0f;
	else xmlValue = (value - Player.VOLUME_MAX) * 30.0f / (Player.VOLUME_MAX - 1);
	String volData = String.format("%.2f,0.00,0.00,0.00", xmlValue);
	metadataPipe.safeSend(MetadataPipe.TYPE_SSNC, MetadataPipe.CODE_PVOL, volData);
	}

[aleszczynskig]

Ah that's interesting. It doesn't seem to work for me and I know that it works with other sources to forked-daapd. I'll take a look at the logs in debug mode and see if I can find out why it is not working.

I'll report back any findings. Thanks for taking a look at this.

[devgianlu]

It was implemented back in #174

[aleszczynskig]

Are you aware of anyone who has this working?I have looked at the forked-daapd log in debug mode and it is not recording any events being received and the volume is not changing. The track metadata is coming through so the pipe seems to be working?

I am doing some investigations using shairport-sync and I will get this back to you as soon as possible.

So I have captured the log from forked-daapd when using shairport-sync with the metadata pipe and changing the volume. Forked-daapd logs the event and changes the volume in the UI. This does not happen when using spocon. Forked-daapd does not register the event. In my experience with this in the past, this can happen if the message is malformed or forked-daapd can't make sense of it.

I am doing more investigations with the shairport-sync-metadata-reader and will report my findings here aswell.
shairport-sync-forked-daapd.log

[devgianlu]

@uvjustin Was the one who requested it in the first place.

[aleszczynskig]

So I've done a comparison between shairport-sync and spocon metadata feeds using shairport-sync-metadata-reader. There is quite a difference and it seems that there is some corruption using spocon. I have not managed to record any pvol events from spocon which is why I think forked-daapd is not responding.

I have observed this when testing this before (I was constructing some very crude bash scripts to write metadata to the pipe) I'm willing to help out here wherever possible however my java skills are very limited.

shairport-sync-metadata-capture.log
spocon-metadata-capture.log

If I was guessing, I would say the issue relates to the PICT event. It's possible that the length argument sent for the base64 encoded picture may be incorrect and this causes the receiving end to over or under read the buffer which truncates or corrupts the next event.

[uvjustin]

@aleszczynskig Not sure why your metadata is corrupted. Seems to work fine for me. Note that the End data tag not seen errors are just because shairport-sync-metadata-reader is expecting a C style string with a null termination, so it is reading b64size+1 from the input. librespot-java is not sending an extra null character so the following < character is getting read too. It should really only read b64size characters here unless the spec is explicitly demanding C style strings. Anyway, this really shouldn't affect the decoding in either shairport-sync-metadata-reader or in forked-daapd because they are reading the correct size and decoding only that size (ie the inclusion/exclusion of the null doesn't matter for the decoding).
@devgianlu I think the value should be -144.0f here:

librespot-java/player/src/main/java/xyz/gianlu/librespot/player/Player.java

Line 1035 in 5c9c9be

if (value == 0) xmlValue = 144.0f;

[devgianlu]

I can add a null terminator, should it be at the end of the base64 string or after </item>? Also fixed the volume.

[aleszczynskig]

Great. Thank you for the feedback guys. That does make sense as forked-daapd was getting the correct metadata, only the volume control is broken.

Maybe the changes proposed will bring it to life for me.

In testing I noticed though that when changing the volume in the Spotify client, the volume is changed in the audio stream. Ideally when using the pipe and volume control with forked-daapd, the volume in the stream should be maxed out and only the metadata for volume changes should be sent with volume changes. It doesn't seem to do this currently. Is it possible to fix this too? Otherwise you get a a double amplification effect, first in the stream and then from the speaker itself.

Thanks for looking into this.

[uvjustin]

The null terminator is not an issue with forked-daapd. forked-daapd already adds a null terminator after </item> when parsing the metadata, and for the base64 string, sending without a null terminator should be fine, as this PR makes sure the memory for the decoded string is allocated with an extra null at the end.
The only issue with this is from shairport-sync-metadata-reader, as that is currently written expecting a null - if you want to work around that, the null would come at the end of the base64 string.

[uvjustin]

Great. Thank you for the feedback guys. That does make sense as forked-daapd was getting the correct metadata, only the volume control is broken.

Maybe the changes proposed will bring it to life for me.

In testing I noticed though that when changing the volume in the Spotify client, the volume is changed in the audio stream. Ideally when using the pipe and volume control with forked-daapd, the volume in the stream should be maxed out and only the metadata for volume changes should be sent with volume changes. It doesn't seem to do this currently. Is it possible to fix this too? Otherwise you get a a double amplification effect, first in the stream and then from the speaker itself.

Thanks for looking into this.

@aleszczynskig In your forked-daapd logs, are you seeing any of the volume debug messages from here?

[aleszczynskig]

No. Not when using librespot-java. I see these messages when using shairport-sync however and the volume changes as you would expect.

[aleszczynskig]

Just compiled 5143348 and retested. The null terminator has worked in shairport-sync-metadata-reader however the output is still garbage, is this because it is UTF-8 formatted, perhaps ssmr is expecting US_ASCII?

I still do not see any PVOL events though.

More importantly, this change seems to have broken the metadata working in forked-daapd.

[uvjustin]

I don't think the volume events that you send from a Spotify client ever enter the librespot-java event space. Librespot-java has its own volume in player that you can adjust via the API like this: curl -X POST http://localhost:24879/player/volume-down. Changing the player volume should send a pvol event.

[aleszczynskig]

@uvjustin yes you are correct, issuing the volume commands via curl does indeed send the pvol event to forked-daapd, it then works as expected. My use case is that I am using spocon as a Spotify connect destination, this is then piped to forked-daapd and out to a number of speakers around the house using airplay. I want to be able to control the volume using forked-daapd to control the speaker volumes. To do this it would require that a Spotify client (I use the Mac OS client, but also IOS and Apple Watch) can trigger the pvol events when the volume is changed. Is there a way to do this?

[uvjustin]

I think librespot has something similar after librespot-org/librespot#447

[devgianlu]

I got a bit lost in the conversation. Is there something I need to change in the metadata pipe? I do not have a setup to test this stuff so I depend entirely on your feedback.

I fixed the issue with the volume events not being dispatched correctly even if I believe it was beneficial for you (?). So, do I have to implement something like librespot-org/librespot#447?

[aleszczynskig]

@devgianlu. Thank you. I will test your new commit tomorrow and see if it works. It would be great if you could implement the fixed volume output available in librespot-org/librespot#447 as well.

[aleszczynskig]

@devgianlu - I have tested the latest commit and the volume control is working as expected. However this was after I removed the null character you included in 5143348. I noted in #317 (comment) that this entirely broke the metadata in forked-daapd. I think you may need to revert this change but once done this works correctly. Volume changes in the Spotify client are reflected in forked-daapd and the volume is changed.

I am unsure whether the output audio stream is fixed now though, I suspect that it is not, being both amplified by librespot-java and then again by forked-daapd. If you are able to implement the FIXED output as per #317 (comment) then that would entirely implement this feature as per my original enhancement request and that would be awesome.

[devgianlu]

I reverted that change. To have the fixed behavior you can just set volumeSteps to 0 in the configuration file.

[uvjustin]

My apologies, the null character won't work as it's not valid XML and the mini-xml library that forked-daapd uses to parse the XML probably won't process it. I think just taking it out makes the most sense, as the original way works and the only "issue" is ssmr writing a warning message.
Alternatively, I think replacing the /0 with = might work as that would just pad the base64 string.

[aleszczynskig]

Thanks @devgianlu and @uvjustin for your work on this. I will try now with volumeSteps = 0 and feedback. Presuming this works as expected, this enhancement will be complete and you should be able to close the ticket.

[aleszczynskig]

@devgianlu I have changed the volumeSteps to 0. Unfortunately this means that the Spotify client loses the ability to change the volume entirely which is not the desired effect.

The aim is to always the output audio data sent to the pipe at 100% volume. When the spotify client sends a signal to change volume, this does not change the audio stream but simply sends the relevant pvol event to the metadata pipe for forked-daapd to process and implement the volume change. In effect the volume handling is offloaded to forked-daapd and librespot-java simply parsing the request from the user and forwards it on. Is this possible?

Thanks

[devgianlu]

Should be good now, you can change the player.bypassSinkVolume flag to true.

[aleszczynskig]

OK - I have tested this by setting byPassSinkVolume = true and volumeSteps = 64 and it doesn't seem to work as expected. There is a double step in volume change, the first from forked-daapd as this change is almost instant and the second a few seconds later. I presume the later change is because the pipe audio stream has a different volume setting indicating that librespot-java is still changing the volume. I do not know how to test if librespot-java is changing the output volume into the pipe, so I cannot verify this, but it is very evident when listening. Is there something I should be looking for in the logs? They are very verbose so it's hard to analyse. If not, are you certain that the byPassSinkVollume setting, when set to true, sets the pipe output volume to 100% and never changes it?

With volumeSteps = 0, there is no option in the Spotify client to change the volume. It is just fixed at the initialVolume setting (which for me is ~20%).

One final observation and I don't know if this can be fixed. When you first connect, the output volume in Spotify is not synced to forked-daapd. Is it possible to send a pvol event with the initialVolume on connection? This prevents unexpected behaviour after the first volume change in spotify if there is a delta between the spotify volume and the forked-daapd volume settings.

Thank you for your continued work on this.

[devgianlu]

@aleszczynskig I made a dump mistake on the volume thing so I fixed that and now it's also sending a volume event right at the beginning.

[aleszczynskig]

Excellent. I'll test it right away.

[aleszczynskig]

Great. It all seems to be working. There is no longer a double change in volume and the initial volume seems to work as well. I think this is now working as per my initial enhancement request so I will close it.

Thank you @devgianlu and @uvjustin for all of your hard work and patience on this. I'm hoping you might be able to get #318 implemented now ;) though I don't know how simple that will be.

Thank you for a great project and sharing your time and skills for a great community. I'm always willing to help out in my own little way if there is anything I can do to contribute.

[uvjustin]

I didn't do anything here.. Thanks @devgianlu =)

[snoopbird]

Is it possible that the feature no longer works in the current dev branch? I built and tested it, but I get this result:

pi@rp4:~/shairport $ shairport-sync-metadata-reader < /srv/music/spotify.metadata 
End data tag not seen, "/data></item>" seen instead.
"ssnc" "pvol": "LM



?



 ?



  ?
   ".
End data tag not seen, "/data></item>" seen instead.
Title: "?[??H?X\?".
End data tag not seen, "/data></item>" seen instead.
Album Name: "?]Y?\?وH??[".
End data tag not seen, "/data></item>" seen instead.
Artist: "H?]\?".
End data tag not seen, "/data></item>" seen instead.
?̌ssnc" "prgr": "K?K?
 @".
End data tag not seen, "/data></item>" seen instead.
Picture received, length 188702 bytes.
End data tag not seen, "/data></item>" seen instead.
"ssnc" "prgr": "K?
?̌???
 @".

[devgianlu]

There haven't been no significant modifications since. Could you try piping to a file instead to analyze the content?

[snoopbird]

I am not sure if I got it right after all. Is that what you mean?

pi@ rp4:~/shairport/shairport-sync-metadata-reader $ ./shairport-sync-metadata-reader < /opt/spocon/test.metadata 
End data tag not seen, "/data></item>" seen instead.
Title: "?[??H?X\?".
End data tag not seen, "/data></item>" seen instead.
Album Name: "?]Y?\?وH??[".
End data tag not seen, "/data></item>" seen instead.
Artist: "H?]\?".
End data tag not seen, "/data></item>" seen instead.
?ssnc" "prgr": "K?
?̌?
 @".
End data tag not seen, "/data></item>" seen instead.
Picture received, length 188702 bytes.
End data tag not seen, "/data></item>" seen instead.
Nssnc" "pvol": "LL?


 ?



  ?



   ?
    ".

[devgianlu]

Can you upload test.metadata?

[snoopbird]

Clear. I have now "written" the metadata to a file.
test.metadata.txt

[devgianlu]

Looks fine to me, maybe someone can try parsing it and see if it works for them.