From 4aa61bd6231a59ba769d4cf383dfb235c70181ed Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 27 Apr 2024 11:18:15 +0100 Subject: [PATCH 1/5] Use ivar in TranscodeCommand#transcoding_options Instead of the `format` param. This is more consistent with what we're doing in other methods on this class. --- app/models/transcode_command.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/transcode_command.rb b/app/models/transcode_command.rb index ea419f8b..8a8b9412 100644 --- a/app/models/transcode_command.rb +++ b/app/models/transcode_command.rb @@ -23,7 +23,7 @@ def generate input_options, image_options, metadata_options, - transcoding_options(@format), + transcoding_options, @output.path ].compact.join(' ') end @@ -55,8 +55,8 @@ def metadata_options "-write_id3v2 1 -id3v2_version 3 #{entries.join(' ')}" end - def transcoding_options(format) - case format + def transcoding_options + case @format when :mp3v0 '-codec:a libmp3lame -q:a 0 -f mp3' when :mp3128k From 3ac6b3df50c69a3c219319153f5d571f8429dc6f Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 27 Apr 2024 11:20:52 +0100 Subject: [PATCH 2/5] Rename TranscodeCommand#id3v23_keys -> tag_keys I'm planning on using this method for non-ID3 tags. --- app/models/transcode_command.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/transcode_command.rb b/app/models/transcode_command.rb index 8a8b9412..88c66de2 100644 --- a/app/models/transcode_command.rb +++ b/app/models/transcode_command.rb @@ -48,7 +48,7 @@ def image_options end def metadata_options - supported_keys = @metadata.slice(*id3v23_keys) + supported_keys = @metadata.slice(*tag_keys) return if supported_keys.empty? entries = supported_keys.map { |k, v| %(-metadata #{id3v23_key_for(k)}="#{v}") } @@ -68,7 +68,7 @@ def transcoding_options end end - def id3v23_keys + def tag_keys METADATA_KEYS_VS_ID3V23_TAGS.keys end From 8323ed204bcc310bee51540a60d940b93694b93c Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 27 Apr 2024 11:20:52 +0100 Subject: [PATCH 3/5] Rename TranscodeCommand#id3v23_key_for -> tag_key_for I'm planning on using this method for non-ID3 tags. --- app/models/transcode_command.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/transcode_command.rb b/app/models/transcode_command.rb index 88c66de2..42ea849e 100644 --- a/app/models/transcode_command.rb +++ b/app/models/transcode_command.rb @@ -51,7 +51,7 @@ def metadata_options supported_keys = @metadata.slice(*tag_keys) return if supported_keys.empty? - entries = supported_keys.map { |k, v| %(-metadata #{id3v23_key_for(k)}="#{v}") } + entries = supported_keys.map { |k, v| %(-metadata #{tag_key_for(k)}="#{v}") } "-write_id3v2 1 -id3v2_version 3 #{entries.join(' ')}" end @@ -72,7 +72,7 @@ def tag_keys METADATA_KEYS_VS_ID3V23_TAGS.keys end - def id3v23_key_for(key) + def tag_key_for(key) METADATA_KEYS_VS_ID3V23_TAGS[key] end end From 5e90f5b5f3619bc285035920e8d8dabb190d6029 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sat, 27 Apr 2024 11:25:53 +0100 Subject: [PATCH 4/5] Improve TranscodeCommand test name for MP3 metadata --- test/models/transcode_command_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/models/transcode_command_test.rb b/test/models/transcode_command_test.rb index 8faaf519..ace0d076 100644 --- a/test/models/transcode_command_test.rb +++ b/test/models/transcode_command_test.rb @@ -42,7 +42,7 @@ class TranscodeCommandTest < ActiveSupport::TestCase assert_contains_pair(command_string, ['-c', 'copy']) end - test 'adds metadata using ID3v2.3 format' do + test 'adds metadata tags using ID3v2.3 format for MP3 formats' do metadata = { track_title: 'track-title', track_number: 'track-number', From 1ce825703fce94b463cdbf958114149fd3886f90 Mon Sep 17 00:00:00 2001 From: James Mead Date: Fri, 5 Apr 2024 11:47:57 +0100 Subject: [PATCH 5/5] Use FLAC tag names in FLAC metadata In #183 @graue pointed out that FLAC has its own tag names [1,2]. This changes the ffmpeg transcoding for FLAC tracks to use those names. After some experiments [3], I have determined that the `write_id3v2` & `id3v2_version` muxer options are not needed (they are ignored) when writing metadata to a FLAC file and so I have removed them from the generated `ffmpeg` command in that case in order to make the command less confusing. Fixes #183. [1]: https://xiph.org/flac/faq.html#general__tagging [2]: https://xiph.org/vorbis/doc/v-comment.html [3]: https://github.com/freerange/jam-coop/pull/184#issuecomment-2080439113 --- app/models/transcode_command.rb | 32 +++++++++++++++++++++++---- test/models/transcode_command_test.rb | 14 ++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/app/models/transcode_command.rb b/app/models/transcode_command.rb index 42ea849e..5854fde5 100644 --- a/app/models/transcode_command.rb +++ b/app/models/transcode_command.rb @@ -8,6 +8,13 @@ class TranscodeCommand artist_name: 'TPE1' }.freeze + METADATA_KEYS_VS_FLAC_TAGS = { + track_title: 'TITLE', + track_number: 'TRACKNUMBER', + album_title: 'ALBUM', + artist_name: 'ARTIST' + }.freeze + def initialize(input, output, format, metadata = {}, image = nil) @input = input @output = output @@ -22,6 +29,7 @@ def generate global_options, input_options, image_options, + muxer_options, metadata_options, transcoding_options, @output.path @@ -47,12 +55,17 @@ def image_options "-i #{@image.path} -map 0:0 -map 1:0 -c copy" end + def muxer_options + return unless @format.to_s =~ /^mp3/ + + '-write_id3v2 1 -id3v2_version 3' + end + def metadata_options supported_keys = @metadata.slice(*tag_keys) return if supported_keys.empty? - entries = supported_keys.map { |k, v| %(-metadata #{tag_key_for(k)}="#{v}") } - "-write_id3v2 1 -id3v2_version 3 #{entries.join(' ')}" + supported_keys.map { |k, v| %(-metadata #{tag_key_for(k)}="#{v}") }.join(' ') end def transcoding_options @@ -69,10 +82,21 @@ def transcoding_options end def tag_keys - METADATA_KEYS_VS_ID3V23_TAGS.keys + metadata_lookup.keys end def tag_key_for(key) - METADATA_KEYS_VS_ID3V23_TAGS[key] + metadata_lookup[key] + end + + def metadata_lookup + case @format.to_s + when /^mp3/ + METADATA_KEYS_VS_ID3V23_TAGS + when /^flac/ + METADATA_KEYS_VS_FLAC_TAGS + else + raise ArgumentError, "unsupported format: #{@format}" + end end end diff --git a/test/models/transcode_command_test.rb b/test/models/transcode_command_test.rb index ace0d076..26a3593d 100644 --- a/test/models/transcode_command_test.rb +++ b/test/models/transcode_command_test.rb @@ -58,6 +58,20 @@ class TranscodeCommandTest < ActiveSupport::TestCase assert_contains_pair(command_string, ['-metadata', 'TPE1="artist-name"']) end + test 'adds metadata tags for FLAC format' do + metadata = { + track_title: 'track-title', + track_number: 'track-number', + album_title: 'album-title', + artist_name: 'artist-name' + } + command_string = TranscodeCommand.new(@input, @output, :flac, metadata).generate + assert_contains_pair(command_string, ['-metadata', 'TITLE="track-title"']) + assert_contains_pair(command_string, ['-metadata', 'TRACKNUMBER="track-number"']) + assert_contains_pair(command_string, ['-metadata', 'ALBUM="album-title"']) + assert_contains_pair(command_string, ['-metadata', 'ARTIST="artist-name"']) + end + test 'transcodes audio to mp3 using libmp3lame codec highest audio quality' do command_string = TranscodeCommand.new(@input, @output, :mp3v0).generate assert_contains_pair(command_string, ['-codec:a', 'libmp3lame'])