Don't show sample size for lossy formats. - #1642
Conversation
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>
| # 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is probably the completest list we have: https://github.com/LMS-Community/slimserver/blob/public/9.2/Slim/Player/TranscodingHelper.pm#L703
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
2 participants
Ignore sample size from $transcoder for lossy formats. Clean up some white space and adjust Info logging.