From 1ce825703fce94b463cdbf958114149fd3886f90 Mon Sep 17 00:00:00 2001
From: James Mead <james@floehopper.org>
Date: Fri, 5 Apr 2024 11:47:57 +0100
Subject: [PATCH] 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'])