Only display 'VBR' / 'CBR' qualifier for lossy formats. - #1646
Conversation
When displaying bitrates, no distinction is made as to whether the 'CBR' or 'VBR' qualifier is appropriate for the format type and it is often confusingly displayed for lossless formats such as FLAC. Signed-off-by: Sam Y <syahres@gmail.com>
|
Hi,
What about this usecase?
WAV lossless CBR
FLAC lossless VBR.
Phill.
…On Thu, 27 Aug 2026 at 17:44, Sam Y ***@***.***> wrote:
When displaying bitrates, no distinction is made as to whether the 'CBR'
or 'VBR' qualifier is appropriate for the format type and it is often
confusingly shown for lossless formats such as FLAC.
------------------------------
You can view, comment on, or merge this pull request online at:
#1646
Commit Summary
- 943d2bb
<943d2bb>
Only display 'VBR'/'CBR' qualifier for lossy formats.
File Changes
(3 files <https://github.com/LMS-Community/slimserver/pull/1646/files>)
- *M* Slim/Control/Queries.pm
<https://github.com/LMS-Community/slimserver/pull/1646/files#diff-f8c8468e86fb82e648220537d8b3b120c511f94fd48239594a6570723a002197>
(2)
- *M* Slim/Schema/RemoteTrack.pm
<https://github.com/LMS-Community/slimserver/pull/1646/files#diff-8a4b5f53f9eb8cf83bbc17d5503c04d033b57b8309cbd4b5cfcea48b9a625d75>
(9)
- *M* Slim/Schema/Track.pm
<https://github.com/LMS-Community/slimserver/pull/1646/files#diff-bc45680f0d00bb713d9216b794abe47497fd3222a189ec9ca89b7cac5c1ea5ab>
(13)
Patch Links:
- https://github.com/LMS-Community/slimserver/pull/1646.patch
- https://github.com/LMS-Community/slimserver/pull/1646.diff
—
Reply to this email directly, view it on GitHub
<#1646?email_source=notifications&email_token=AANORYJQQKQWH57QTJXFHG35MBQNDA5CNFSNUABEM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UF42DGNZXHAYTMMZVGGTHEZLBONXW5KTTOVRHGY3SNFRGKZFFMV3GK3TUVRTG633UMVZF6Y3MNFRWW>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANORYKZY3QT4ZMO523VIKL5MBQNDAVCNFSNUABDKJSXA33TNF2G64TZHM2DENJZGQ4DAO2JONZXKZJ3GUZDMOJZGU3DMNZVUF3AE>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
--
I Can Resist Everything Except Temptation -- Oscar Wilde
|
| my $mode = ''; | ||
| if (Slim::Music::Info::isLossy($format) ) { # only relevant for lossy formats | ||
| $mode = defined $vbrScale ? ' VBR' : ' CBR'; | ||
| } | ||
| return int ($bitrate/1000) . Slim::Utils::Strings::string('KBPS') . $mode; |
There was a problem hiding this comment.
Why not use Slim::Schema::Track->buildPrettyBitRate()?
There was a problem hiding this comment.
There's nothing preventing it. As in the change to Track.pm however, I was trying to minimize changes to the existing logic which, in this case, could certainly be made less redundant by doing what you suggest. I'll look into it.
There was a problem hiding this comment.
Done. Thanks for the suggestion.
| my ( $self, $bitrate, $vbrScale ) = @_; | ||
|
|
||
| my $mode = defined $vbrScale ? 'VBR' : 'CBR'; | ||
| my ( $self, $bitrate, $vbrScale, $format ) = @_; |
There was a problem hiding this comment.
We're using the convention of using $self when dealing with an object instance, or $class when this is just a class method. As we're not accessing that variable itself, it should be $class for a static method.
Yes, sometimes we have $selfOrClass. But we don't need this here.
There was a problem hiding this comment.
I'm just adding an additional argument to the function while trying to minimize any changes to existing code, as per the old American saying "If it ain't broke, don't fix it." So you're suggesting that I should change the pre-existing $self argument to $class in the function definition?
|
That will cause regressions for anything that was parsing the bitrate value
into a numeric, to say reconfigure an output path or engiuage low bitrate
enhavment features , this might anyway depending on how the values were
parsed with removing the CBR/VBR...
…On Thu, 27 Aug 2026 at 21:06, Michael Herger ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Slim/Schema/RemoteTrack.pm
<#1646 (comment)>
:
> + my $mode = '';
+ if (Slim::Music::Info::isLossy($format) ) { # only relevant for lossy formats
+ $mode = defined $vbrScale ? ' VBR' : ' CBR';
+ }
+ return int ($bitrate/1000) . Slim::Utils::Strings::string('KBPS') . $mode;
Why not use Slim::Schema::Track->buildPrettyBitRate()?
—
Reply to this email directly, view it on GitHub
<#1646?email_source=notifications&email_token=AANORYIMCDMPZPH775UVPYT5MCIDJA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMBUGUYTOMRWHAY2M4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-5045172681>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANORYLVHUMMNUDUWE5PRRT5MCIDJAVCNFSNUABDKJSXA33TNF2G64TZHM2DENJZGQ4DAO2JONZXKZJ3GUZDMOJZGU3DMNZVUF3AE>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
I Can Resist Everything Except Temptation -- Oscar Wilde
|
WAV is always CBR and FLAC is always VBR, so the qualifier is redundant and meaningless. |
If you know of any such potential regression problems, please point them out so I can try to address them. My research and testing thus far has not revealed any. Keep in mind that this PR is for LMS 9.2.0, which is, by definition, a beta release. |
Use buildPrettyBitRate() method in both Track.pm and RemoteTrack.pm to avoid logic duplication. Also change $self to $class in the signature as it's a static method. Signed-off-by: Sam Y <syahres@gmail.com>
|
I'm sorry for asking the most basic question again: shouldn't it be in the UIs responsibility to show things or not? The server shall return whatever it knows about the stream, but let the front end decide whether it's relevant to their users or not? |
Up to a point, "yes". The goal being pursued is to have a higher degree of consistency among the UI's in how individual attributes are displayed --- not which of them are displayed, which is admittedly up to the UI. In this case, we are trying to standardize how the "bitrate" attribute is displayed, and this PR is only the first step in that direction. For starters, all "WAV" files are inherently always CBR, with a constant bitrate value equal to "samplerate * samplesize * channels". On the other hand, "FLAC" files are inherently always VBR. So it is redundant and confusing to display the "CBR/VBR" qualifier for these formats, not to mention the screen real estate being wasted by doing so. Even worse, before these changes, LMS (buildPrettyBitRate) would often show a FLAC file as "CBR" because it wasn't given access to the file format when making its decision. I don't know if you have stopped following the Material Skin forum thread but the discussion on how to display both the source and post-transcoding technical info brought this "CBR/VBR" issue to the forefront recently when this last anomaly was pointed out. You might want to start reading the the thread at this point if you're interested. See the quoted screenshot on the first post for an example of a FLAC file incorrectly showing a CBR bitrate. I should also point out that this PR is just the beginning of getting the bitrate attribute to be displayed consistently and accurately. There is a little more to be done in standardizing the format stored in the $song object that I will be pursuing after this. After that, the bitrate format returned in the metadata by the Radio Paradise plugin will be addressed. Currently, for interactive FLAC files, it looks something like "850k VBR FLAC" To be conistent with LMS going forward, it should be changed to just "850kbps", as both "VBR" and "FLAC" are redundant here, albeit for different reasons. I know that RP is "your baby" so I will tread lightly and consult with you on those changes. In closing, I ask that you again trust me on this. I do not use AI at all ("not that there's anything wrong with that", to quote Jerry Seinfeld - do they broadcast "Seinfeld" in Switzerland?), and I will take full responsibility for fixing any problems caused by my changes (unlike AI). ;-) Now that Craig has made all the changes necessary to support displaying both the pre- and post-transcoding technical info (as has the developer of the "Echo Classic" skin), I am eager to also make this change available to the LMS 9.2.0 users, who are serving as de-facto beta testers for all of this and whose feedback has been very productive and helpful thus far. Thanks. |
|
Ok, got it. I've started kind of a poll (https://forums.lyrion.org/forum/user-forums/3rd-party-software/106269-announce-material-skin?p=1833088#post1833088). Let's see. Technically this PR is fine. And I can certainly live with whatever that string is... (now I have to go and fix/remove that string in RP - awaiting the result for consistency) |
|
Radio Paradise preparation: michaelherger/RadioParadise@595afcb - does make sense? |
Using buildPrettyBitRate() there is not going to work, as it will be dependent on changes made in LMS 9.2.0, specifically the addition of the isLossy() function. I would just hardcode the string there as '####kbps' for FLAC, although I know that's not ideal. I apologize if my middle-of-the-night response to your forum post was out of line. I was half asleep when I wrote it and didn't fully understand that you were conducting an informal poll there. That's what happens when you're old and make the mistake of looking at your phone during a nighttime pit stop. 🥴 |
|
And as the people has spoken I'm going to merge this PR - which should improve the display "for free". Oh, but only if I tell it the format... Thanks! |
3 participants
When displaying bitrates, no distinction is made as to whether the 'CBR' or 'VBR' qualifier is appropriate for the format type and it is often confusingly shown for lossless formats such as FLAC.