add configuration option to pass extra args to flac encode

Add an argument to the whipper cd rip comand that can be provided on the
command line or in the whipper.cd.rip section of the config file.

It is retrieved inside the _ripIfNotRipped() private function, split
into a list and passed down through to program/flac.py, which actually
calls the flac binary.

Add debug logging to that function so the actual command shows up in the
debug log.

Add a unittest to ensure that arguments passed to flac.encode() work as
expected.

Tested ripping a disc with --extra-flac-encode-args="--best -e
--no-padding" from both the command line and config file as well as with
the default empty value.

Signed-off-by: Kevin Mitchell <[email protected]>

whipper/command/cd.py

@@ -326,6 +326,11 @@ def add_arguments(self):
                                  help="continue ripping further tracks "
                                  "instead of giving up if a track "
                                  "can't be ripped")
+        self.parser.add_argument('--extra-flac-encode-args',
+                                 action="store", dest="extra_flac_encode_args",
+                                 help="extra arguments to pass to the flac "
+                                 "encode command",
+                                 default="")
 
     def handle_arguments(self):
         self.options.output_directory = os.path.expanduser(
@@ -471,10 +476,12 @@ def _ripIfNotRipped(number):
                             self.itable.tracks[number - 1].isrc is not None):
                         tag_list['ISRC'] = self.itable.tracks[number - 1].isrc
 
+                    extraFlacArgs = self.options.extra_flac_encode_args.split()
                     try:
                         self.program.ripTrack(self.runner, trackResult,
                                               offset=int(self.options.offset),
                                               device=self.device,
+                                              extraFlacArgs=extraFlacArgs,
                                               taglist=tag_list,
                                               overread=self.options.overread,
                                               what='track %d of %d%s' % (

whipper/common/encode.py

@@ -50,9 +50,11 @@ def _sox_peak(self):
 class FlacEncodeTask(task.Task):
     description = 'Encoding to FLAC'
 
-    def __init__(self, track_path, track_out_path, what="track"):
+    def __init__(self, track_path, track_out_path, extra_flac_args,
+                 what="track"):
         self.track_path = track_path
         self.track_out_path = track_out_path
+        self.extra_flac_args = extra_flac_args
         self.new_path = None
         self.description = 'Encoding %s to FLAC' % what
 
@@ -61,7 +63,7 @@ def start(self, runner):
         self.schedule(0.0, self._flac_encode)
 
     def _flac_encode(self):
-        flac.encode(self.track_path, self.track_out_path)
+        flac.encode(self.track_path, self.track_out_path, self.extra_flac_args)
         self.stop()
 
 

whipper/common/program.py

@@ -557,8 +557,8 @@ def verifyTrack(runner, trackResult):
                      'result %r', trackResult.testcrc, t.checksum, ret)
         return ret
 
-    def ripTrack(self, runner, trackResult, offset, device, taglist,
-                 overread, what=None, coverArtPath=None):
+    def ripTrack(self, runner, trackResult, offset, device, extraFlacArgs,
+                 taglist, overread, what=None, coverArtPath=None):
         """
         Rip and store a track of the disc.
 
@@ -573,6 +573,8 @@ def ripTrack(self, runner, trackResult, offset, device, taglist,
         :type offset: int
         :param device: path to the hardware disc drive
         :type device: str
+        :param extraFlacArgs: additional encoding args to pass to flac binary
+        :type extraFlacArgs: list
         :param taglist: dictionary of tags for the given track
         :type taglist: dict
         :param overread: whether to force overreading into the
@@ -600,6 +602,7 @@ def ripTrack(self, runner, trackResult, offset, device, taglist,
                                            stop, overread,
                                            offset=offset,
                                            device=device,
+                                           extraFlacArgs=extraFlacArgs,
                                            taglist=taglist,
                                            what=what,
                                            coverArtPath=coverArtPath)

whipper/program/cdparanoia.py

@@ -440,7 +440,8 @@ class ReadVerifyTrackTask(task.MultiSeparateTask):
     _tmppath = None
 
     def __init__(self, path, table, start, stop, overread, offset=0,
-                 device=None, taglist=None, what="track", coverArtPath=None):
+                 device=None, extraFlacArgs=None, taglist=None, what="track",
+                 coverArtPath=None):
         """
         Init ReadVerifyTrackTask.
 
@@ -456,6 +457,8 @@ def __init__(self, path, table, start, stop, overread, offset=0,
         :type offset: int
         :param device: the device to rip from
         :type device: str
+        :param extraFlacArgs: additional encoding args to pass to flac binary
+        :type extraFlacArgs: list
         :param taglist: a dict of tags
         :type taglist: dict
         """
@@ -499,7 +502,8 @@ def __init__(self, path, table, start, stop, overread, offset=0,
 
         from whipper.common import encode
 
-        self.tasks.append(encode.FlacEncodeTask(tmppath, tmpoutpath))
+        self.tasks.append(encode.FlacEncodeTask(tmppath, tmpoutpath,
+                                                extraFlacArgs))
 
         # MerlijnWajer: XXX: We run the CRC32Task on the wav file, because it's
         # in general stupid to run the CRC32 on the flac file since it already

whipper/program/flac.py

@@ -4,17 +4,22 @@
 logger = logging.getLogger(__name__)
 
 
-def encode(infile, outfile):
+def encode(infile, outfile, extraArgs=None):
     """
     Encode infile to outfile, with flac.
 
+    extraArgs is a list of additional arguments for the flac binary
+
     Uses ``-f`` because whipper already creates the file.
     """
+    cmd = ['flac', '--silent', '--verify' ]
+    cmd.extend(extraArgs or [])
+    cmd.extend(['-o', outfile, '-f', infile])
+    logger.debug("executing %r", cmd)
     try:
         # TODO: Replace with Popen so that we can catch stderr and write it to
         # logging
-        check_call(['flac', '--silent', '--verify', '-o', outfile,
-                    '-f', infile])
+        check_call(cmd)
     except CalledProcessError:
         logger.exception('flac failed')
         raise

whipper/test/test_program_flac.py

@@ -0,0 +1,45 @@
+# -*- Mode: Python; test-case-name: whipper.test.test_program_flac -*-
+
+import os
+import subprocess
+from tempfile import NamedTemporaryFile
+import wave
+
+from whipper.program import flac
+from whipper.test import common
+
+def read_flac(path):
+    with NamedTemporaryFile(suffix='.wav') as fp:
+        subprocess.check_call(['flac', '--silent', '-d', path, '-fo', fp.name])
+        wf = wave.open(fp.name)
+        return wf._data_chunk.read()
+
+class FlacTestCase(common.TestCase):
+    def setUp(self):
+        self.original_path = os.path.join(os.path.dirname(__file__),
+                                          'track.flac')
+
+    def testEncode(self):
+        with (NamedTemporaryFile(suffix='.wav') as decoded,
+              NamedTemporaryFile(suffix='.flac') as encoded_default,
+              NamedTemporaryFile(suffix='.flac') as encoded_optimum):
+            # Create a wav file
+            subprocess.check_call(['flac', '--silent', '-d', self.original_path,
+                                   '-fo', decoded.name])
+
+            # Encode it with different extraArgs
+            flac.encode(decoded.name, encoded_default.name)
+            flac.encode(decoded.name, encoded_optimum.name,
+                        ['--best', '-e'])
+
+            # Ensure the file with higher compression is smaller
+            size_default = os.path.getsize(encoded_default.name)
+            size_optimum = os.path.getsize(encoded_optimum.name)
+            self.assertLess(size_optimum, size_default)
+
+            # Make sure the the contents are identical
+            data_original = read_flac(self.original_path)
+            data_default = read_flac(encoded_default.name)
+            data_optimum = read_flac(encoded_default.name)
+            self.assertEqual(data_original, data_default)
+            self.assertEqual(data_original, data_optimum)