Add current track's bitrate to status query.

[SamInPgh]

This information can be used by client apps to display the actual bitrate of the currently playing track rather than what is stored in the dictionary, which doesn't reflect any resampling that may have been done in the pipeline. This change will also allow the bitrate of remote tracks imported into the library via OMLI with a bitrate of 0 (due to unknown file size) to be retrieved and displayed by client apps, which wasn't possible before.

[michaelherger]

We should stop adding more details unconditionally. I even think we should not have added the gain information just above that: all of that information can be requested using the tag parameter. Just add tags:Yr for those two. What do you think?

[SamInPgh]

I understand your concerns but I think you might be missing the point. The purpose of the status query is to return information about the currently playing song, some of which is dynamic in nature, e.g. replay gain and bitrate. These two data points, in particular, are indeed available (via tags 'Y' and 'r' respectively). However, the returned values do not necessarily represent the values currently in use. For instance, replay gain ('Y') would return the track's replay gain metadata from the dictionary but the actual replay gain in use would depend on the player's "Volume Adjustment/Replay Gain" setting and other factors. Similarly, the bitrate ('r') tag would also return the value stored in the dictionary, which would not match the actual bitrate in use if the track had to, for example, be downsampled to accomodate the player's DAC limitations. This kind of data is important to many users, and it should be represented accurately. Replay gain IS represented accurately now and, with this change, the bitrate will also be accurate. No more confusing display of '5342kbps' for a song that has been downsampled to '705kbps', for instance. Ideally, the same thing should be done for samplesize and samplerate, i.e. if a track has been downsampled from 24-bit, 192kHz, 5342kbps to 16-bit, 44.1kHz, 705kbps, all of those values should be returned in the status query. I plan to look into those two next.

[michaelherger]

Well, I've learned something today. Neither did I remember there were different bitrate values for one single track, nor did I know this was important to many users 😂. Where are they all these years we've provided them with the wrong data? Wouldn't this be a bug then? Wouldn't the solution then be to return the correct data when requested, instead of returning two different values?

[SamInPgh]

Lol! Glad to be of service. To be clear, there is of course only one bit rate associated with a track when that track is played at the sample rate and sample size specified in it's dictionary metadata. When the track is played by a particular player, however, and possibly resampled at a different sample rate and/or size, it becomes a song, and that song has its own attributes, some of which, including bitrate, may differ from those of the track. What the status query reports (or SHOULD report) are the song attributes. What it reports now is a somewhat confusing mixture of track AND song attributes. This PR addresses the bitrate attribute only, which is the first step towards the goal. (Actually, it's the second step. Replaygain was the first.) The samplerate and samplesize would be the next step(s). However my initial efforts to locate values for these attributes have been fruitless. Although they both exist as part of the $song object, they never seem to be populated, whether the song has been resampled or not, whereas the bitrate attribute is.

[michaelherger]

Can we still make this change on demand only? I'd even like to make the previous one (replay gain) optional. Would Material (or some other UI you know of) break if we made that change?

[michaelherger]

I'm sorry, no. I don't want even more parameters, in particular if they have misleading names. What is "techInfo", if not signal strength, or the IP address, or digital_volume_control? We've had replay_gain at the top level, and that's ok. Let's put the rest there, too.

I know about the difference between the actual "technical" values and those stored in the DB. Yet, they have the same label. So they are what the user (developer) is asking for when using those tags. No surprise.

This difference can be documented. The new structure must be documented and understood. For Material it's a one character change. With the new data structure Craig would have to implement much more logic.

[SamInPgh]

Okay, Michael. I'll be honest. I am frustrated and even a little bit angry right now. It feels like you keep moving the target. I thought that your initial objection was based on the attributes being returned unconditionally, i.e. "Can we still make this change on demand only? I'd even like to make the previous one (replay gain) optional." I thought that made sense and so I added an argument to the status query request to make the technical info "on demand", and also made the replay gain attribute optional going forward. Now you "don't want even more parameters", so exactly how is the client app supposed to "demand" these attributes?

Additionally, you keep talking about a new data structure when I have repeatedly stated that there is no new data structure involved here. The additional technical attributes are returned at the top level, along with replay_gain. That has been the case from the beginning, as you can see by looking at the code changes. The implementation is comparable to the "alarmData" argument, whereby, if it is set to '1', additional alarm-related attributes are returned, also at the top level and not in a different data structure. Regarding the argument name, I am open to calling it whatever you think might make it's meaning more clear.

Of course, I can remove the argument and go back to returning the attributes unconditionally, as in the original commit. However, I believe that would be taking a step backwards. I wonder what the discussion was like when the "alarmData" argument was added to the status query? Was it also this controversial?

What would you think about getting some additional eyes on this issue? It feels like we need a mediator. 😊

[SamInPgh]

Just to clarify the end goal of making this change, only some of which is implemented in this PR:

Let's say we are playing a library track on a SB Radio for which "Bitrate Limiting" has been set to "128kbps, for example. What I am calling the "technical attributes" of the track, as stored in the dictionary, are as follows:

• Type: FLAC
• Bitrate: 5343kbps
• Samplerate: 192kHz
• Samplesize: 24bits

Currently, when playing this track via Material Skin with the "Display technical info" option turned on for the "Now Playing" view, the following info is displayed below the track:

5343kbps VBR, 24bit, 192kHz, FLC

This display accurately shows the attributes of the track in the dictionary but doesn't reflect what we are actually listening to after the original track has gone through the transcoding pipeline in LMS and become what I will refer to as a "song", which is the stream that is actually sent to the player. The song's technical info, i.e. what we are actually listening to, would accurately be displayed as follows:

128kbps, 48kKz, MP3

I believe that this is the technical info that users want to see when they turn on the "Display technical info" option for the Now Playing view, and it is currently not made available to the user ANYWHERE (that I know of). If they want to see the technical info for the track as stored in the dictionary, they can see it in the Browse view if they have activated the "Display technical info" option for that view.

Hopefully the above explanation makes sense, and explains why I feel strongly about implementing this change, which currently makes only the bitrate available, with type, samplerate and samplesize to follow in a future PR.

[michaelherger]

Okay, Michael. I'll be honest. I am frustrated and even a little bit angry right now.

I'm sorry to hear. Unfortunately I currently hear it from too many sides. And I myself am utterly frustrated too. As I mentioned I feel misunderstood. Not only here, but in that other big discussion (box sets) as well. I currently spend more time discussing than coding. And that's not the fun part of this project.

It feels like you keep moving the target. I thought that your initial objection was based on the attributes being returned unconditionally, i.e. "Can we still make this change on demand only? I'd even like to make the previous one (replay gain) optional."

And that's exactly where I feel misunderstood: yes, I said that. And my snippet above should have shown my approach: no new parameter (use the existing tags parameter), and everything conditional. How is this not consistent?

I thought that made sense and so I added an argument to the status query request to make the technical info "on demand", and also made the replay gain attribute optional going forward. Now you "don't want even more parameters", so exactly how is the client app supposed to "demand" these attributes?

Using the tags parameter. You want replay gain information? X or Y will get them to you. Not only for the tracks in the queue, but also the current values from the player.

Additionally, you keep talking about a new data structure when I have repeatedly stated that there is no new data structure involved here. The additional technical attributes are returned at the top level, along with replay_gain.

Blame my not being a native English speaker.

{
	attr1: val1,
   attr2: val2,
    attr3: val3
}

is not the same as

   attr1: val1:
   additionalAttr: {
      attr2: val2,
      attr3: val3
   }
}

are not the same obviously. And that additionalAttr there is what I referred to as the new data structure. I believe the flatter structure makes sense because we already have many technical details at that level. The new ones would be optional additions, without the need for a client to check where the replay_gain value was. Just keep it where it was, while making it optional like the others.

Of course, I can remove the argument and go back to returning the attributes unconditionally, as in the original commit. However, I believe that would be taking a step backwards.

Yes, I totally agree. That's why I believe there's a misunderstanding: we're much closer to each other's thinking than it looks.

I wonder what the discussion was like when the "alarmData" argument was added to the status query? Was it also this controversial?

Haha... I don't remember that detail. I do remember that we were not happy with some aspects of the alarm handling, but we were glad somebody did it (it was an outside contributor!), because we were all under a lot of pressure to get things out the door.

What would you think about getting some additional eyes on this issue? It feels like we need a mediator. 😊

I'd rather get over it than have even more discussions. I'm burning out reading and answering messages, while the fun part is put on hold. F... AI hasn't helped: there are more pull requests than ever. You'd think that was a good thing. But often it is not. Hours of reviewing code which has super convincing nonsense built-in doesn't prepare well for the good PRs.

Don't know. I hope we're getting closer. I could have coded my approach in less time than I spend on this discussion. But I consider it an investment to have other developers on board. Sometimes it's just a bit difficult to get the clear view.

[michaelherger]

Oh, forget to go back to the meat.

Let's say we are playing a library track on a SB Radio for which "Bitrate Limiting" has been set to "128kbps, for example. What I am calling the "technical attributes" of the track, as stored in the dictionary, are as follows:

And what you call "the dictionary" is what I call "the database"? The data as stored in files?

This display accurately shows the attributes of the track in the dictionary but doesn't reflect what we are actually listening to after the original track has gone through the transcoding pipeline in LMS and become what I will refer to as a "song", which is the stream that is actually sent to the player. The song's technical info, i.e. what we are actually listening to, would accurately be displayed as follows:

128kbps, 48kKz, MP3

Understood and agreed.

Hopefully the above explanation makes sense, and explains why I feel strongly about implementing this change, which currently makes only the bitrate available, with type, samplerate and samplesize to follow in a future PR.

As I said I believe we agree in most parts. The initial commit was pretty close. I then asked you to make these values optional with the help of the corresponding tags values. Your first PR + the additional condition && $tags =~ /b/ (or whatever the tag for the bitrate is). That's all, To quote my first response:

all of that information can be requested using the tag parameter. Just add tags:Yr for those two. What do you think?

Yes, I wasn't aware that technical vs. "in dictionary" wasn't the same. But from the user's POV it's just "bitrate". Therefore I think it's an ok, lightweight approach to use tags for this.

[SamInPgh]

First of all, I totally understand the amount of frustration you must be dealing with when it comes to AI. I recently had two issue raised on Git for the Denon/Marantz AVR Control plugin. The first one was where a user had a completely unique corner case that the plugin didn't handle so he asked Claude to "fix" it. He then sent me all the code changes, most of which were completely irrelevant to the issue and asked that I implement them. I explained that the issue was relevant only to his specific situation and the changes would not be merged. The second issue from another user was one that was applicable to a reasonable number of users, specifically those who use the plugin to control an AVR connected to a hardware player such as the SB Touch. In contrast to the first user, he used AI to analyze the code and the specific problem scenario and then sent me a summary containing a couple of possible ways to approach it, one of which I expanded upon to come up with a solution that I implemented and published a few days ago. It probably helped that the second user is also an active developer and understands how to use AI judiciously, which made things go smoothly.

Having said that, I am ready to go ahead and use the tag-dependent approach to implement the new technical attributes, leaving replay_gain as unconditional in order to not force Craig to request the "dictionary" version(s) of the attribute ('X' and 'Y') when they are not needed. The rest of the technical attributes are already contained in the existing tags. How does that sound?

[michaelherger]

Having said that, I am ready to go ahead and use the tag-dependent approach to implement the new technical attributes, leaving replay_gain as unconditional in order to not force Craig to request the "dictionary" version(s) of the attribute ('X' and 'Y') when they are not needed. The rest of the technical attributes are already contained in the existing tags. How does that sound?

No change to existing code, new items added in a similar way, but conditionally? Sounds good to me! Thanks!