Skip to content

Don't show sample size for lossy formats. - #1642

Merged
michaelherger merged 2 commits into
LMS-Community:public/9.2from
SamInPgh:public/9.2
Aug 23, 2026
Merged

Don't show sample size for lossy formats.#1642
michaelherger merged 2 commits into
LMS-Community:public/9.2from
SamInPgh:public/9.2

Conversation

@SamInPgh

Copy link
Copy Markdown
Contributor

Ignore sample size from $transcoder for lossy formats. Clean up some white space and adjust Info logging.

Ignore sample size from $transcoder for lossy formats. Clean up some white space and adjust Info logging.

Signed-off-by: Sam Y <syahres@gmail.com>
Comment thread Slim/Player/Song.pm Outdated
# Init song's sample rate and sample size before any transcoding
$self->samplerate($transcoder->{'sampleRate'});
$self->samplesize($transcoder->{'sampleSize'});
if ($transcoder->{'streamformat'} !~ /mp3|aac/) { # Only set sample size for lossless

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I commented on a similar check before: can we have a isLossy($format) somewhere (eg. Slim::Music::Info)? We should not do these kinds of checks all over the place individually.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the comments on the lines in question are the real problem here. It's not really a question of whether the format is lossy so much as whether there can ever be a sample depth/size associated with the format. The only two formats in the code snippet that you refer to that never have that attribute are mp3 and aac. I personally don't think the situation warrants having a canHaveSampleSize($format) to cover only those two cases. I can change the comments (and maybe the PR description too) to be remove the reference to lossy/lossless. Would that suffice?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But Ogg doesn't really have a sample size either, does it? According to Gemini:

The Ogg container format does not have a fixed "sample size" or packet size. It uses variable-size data packets and variable bit rates.

Or Wikipedia

Bit depth is useful for describing PCM digital signals. Non-PCM formats, such as those using lossy compression, do not have associated bit depths.

So they clearly state that lossy formats do not have bit depths (sample size). What am I missing?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW: the other occurrence where you checked for these file formats was when resetting the sample size. You could change the condition here to simply check whether sample size was available?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply. You're absolutely right. I got a little distracted by the inclusion of 'wma' as a lossy format in the code you linked to, which is no longer always true. The others should be okay. I was just trying to err on the side of caution at this stage. While it is true that an 'ogg' container usually contains a lossy format like Vorbis or Opus, it may also contain 'flac'. However, the latter seems to have its own format, 'ogf', so we should be okay including it. I will look into doing as you suggest by adding an isLossy($format) function that will return TRUE for all the formats in that code snippet except for 'wma'. Does that sound reasonable?

BTW: the other occurrence where you checked for these file formats was when resetting the sample size. You could change the condition here to simply check whether sample size was available?

Good point. I will check into that possibility too.

@SamInPgh SamInPgh Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isLossy($type) function added and referenced where appropriate in Song.pm. I am also using the isLossy() call in guessBitrateFromFormat() rather then checking whether sample size is available because the function may also called from outside Song.pm in cases where the sample size may not have been reset.

isLossy() returns TRUE for mp3, aac, ogg, and ops formats.

Signed-off-by: Sam Y <syahres@gmail.com>

@michaelherger michaelherger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@michaelherger
michaelherger merged commit 9f4678e into LMS-Community:public/9.2 Aug 23, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants