-
Notifications
You must be signed in to change notification settings - Fork 275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
multiple transcoding rules #371
multiple transcoding rules #371
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking ok overall.
Could you add a few words about the order in which rules would be picked? Why the last matching rule and not the first matching one?
Slim/Player/TranscodingHelper.pm
Outdated
PROFILE: foreach (@profiles) { | ||
my $instance = 0; | ||
INSTANCE: | ||
my $profile = $_ . ($instance++ ? '-' . ($instance-1) : ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here again I think incrementing $instance
on its own line wouldn't hurt the eye.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, I'm pushing the enveloppe of compactness too much here. That's my biais of trying to make patches as small as possible but that one was not a good use of Perl, it was just un-necessarily compact.
I've also verified that with
Then LMS nicely transcodes files using faad and stream using ffmpeg :-) |
Ok, I'm fine merging this if you're ok. Or is there any interaction with your other work? BTW: the mp4 -> flac online stream would require the user to install ffmpeg, right? But if the user was doing this he could play back future TIDAL mp4 streams? That would be awesome! |
Thanks - There is no immediate interaction, except than when you put the two together it gives a nice set of option to play remote streams. It's even better, mp4->flc online does NOT require ffmpeg. That's where you have 2 options (see my other PR that I just updated). 1- Use the Perl-based MP4 to ADTS extractor and in that case, mp4 streams whose audio is aac are converted into raw aac (ADTS) and then any rule "aac xxx" can be used, so "aac flc" in your case which already works for "IF" so we are all good. 2- Don't use the Perl-based MP4 to ADTS extractor and then you need to add the rule "mp4 flc" for "I" that uses ffmpeg. |
I'll look into a problem with PlayWMA and interaction with LMS standard Rules which prevents playing. I think consideration about updating WebUI Advanced/Filetype is needed otherwise it may get confusing. |
Agreed - I'll also have a look to verify that last rule is taken into account when they are equivalent |
Seems to me that the right fix (expect dropping the change of course) is to simply look at the files in reverse order.
This way, the rules coming from custom-convert files are evaluated first and so if a perfect match happens, they will be selected. I've verified that with your wma-related custom-convert. Another option (easy as well) is to "reset" the multiple-rules at for each file |
See proposition #378 |
I think I've found a simple way to allow multiple transcoding rules for the same source/target pair. It can be handy per our discussion to have a rule for "R" and one for "IF".
Currently, rules are being picked by order of appearance. As soon as a rule is a perfect match, process stops. If a rule is a partial match (not all wanted tags), it is stored and if a later rule has a better score (more tags), then it is selected. When partial matching rules have an identical score, the first one will be used.
There is not change to that, the "duplicated" rules are just seen as any other rule with the same selection logic.