Skip to content
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.

Sign up for GitHub

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

Add gstreamer-less flac encoder and tagging #121

Merged
merged 6 commits into from
Feb 2, 2017
Merged

Add gstreamer-less flac encoder and tagging #121

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cc3cbe5
Add encoding using Xiph.org 'flac' program.
MerlijnWajer Jan 29, 2017
ae9e87e
Add tagging using mutagen.
MerlijnWajer Jan 29, 2017
80b2a80
Add gstreamer-less CRC32 version
MerlijnWajer Jan 30, 2017
92aace2
Merge branch 'crc32' into flac-encoder
MerlijnWajer Jan 30, 2017
3d92187
Use proper musicbrainz tags and ALBUM tag.
MerlijnWajer Feb 2, 2017
3492d51
Add mutagen to .travis.yml
MerlijnWajer Feb 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion morituri/command/debug.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ def do(self):
logger.debug('Encoding %s to %s',
fromPath.encode('utf-8'),
toPath.encode('utf-8'))
encodetask = encode.EncodeTask(fromPath, toPath, profile)
encodetask = encode.FlacEncodeTask(fromPath, toPath)

runner.run(encodetask)

Expand Down
18 changes: 18 additions & 0 deletions morituri/common/encode.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

from morituri.extern.task import task, gstreamer
from morituri.program import sox
from morituri.program import flac

import logging
logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -182,6 +183,23 @@ def _sox_peak(self):
self.peak = sox.peak_level(self.track_path)
self.stop()

class FlacEncodeTask(task.Task):
description = 'Encoding to FLAC'

def __init__(self, track_path, track_out_path, what="track"):
self.track_path = track_path
self.track_out_path = track_out_path
self.new_path = None
self.description = 'Encoding %s to FLAC' % what

def start(self, runner):
task.Task.start(self, runner)
self.schedule(0.0, self._flac_encode)

def _flac_encode(self):
self.new_path = flac.encode(self.track_path, self.track_out_path)
self.stop()

class EncodeTask(ctask.GstPipelineTask):
"""
I am a task that encodes a .wav file.
Expand Down
4 changes: 2 additions & 2 deletions morituri/image/image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,8 @@ def add(index):
root, ext = os.path.splitext(os.path.basename(path))
outpath = os.path.join(outdir, root + '.' + profile.extension)
logger.debug('schedule encode to %r', outpath)
taskk = encode.EncodeTask(path, os.path.join(outdir,
root + '.' + profile.extension), profile)
taskk = encode.EncodeTaskFlac(path, os.path.join(outdir,
root + '.' + profile.extension))
self.addTask(taskk)

try:
Expand Down
9 changes: 6 additions & 3 deletions morituri/program/cdparanoia.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,10 +490,13 @@ def __init__(self, path, table, start, stop, overread, offset=0,
# here to avoid import gst eating our options
from morituri.common import encode

self.tasks.append(encode.EncodeTask(tmppath, tmpoutpath, profile,
taglist=taglist, what=what))
self.tasks.append(encode.FlacEncodeTask(tmppath, tmpoutpath))

# 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
# has --verify. We should just get rid of this CRC32 step.
# make sure our encoding is accurate
self.tasks.append(checksum.CRC32Task(tmpoutpath))
self.tasks.append(checksum.CRC32Task(tmppath))
self.tasks.append(encode.SoxPeakTask(tmppath))

self.checksum = None
Expand Down
19 changes: 19 additions & 0 deletions morituri/program/flac.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
from subprocess import check_call, CalledProcessError

import logging
logger = logging.getLogger(__name__)

FLAC = 'flac'


def encode(infile, outfile):
"""
Encodes infile to outfile, with flac.
Uses '-f' because morituri already creates the file.
"""
try:
check_call(['flac', '--totally-silent', '--verify', '-o', outfile,
'-f', infile])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, could we get compression option configuration? I’d like to compress with --best, and unless we make it the default, this will require a config option. More generally, I suppose people might want to change flac call parameters, so having it in config would be great. ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue for using --best and not allowing for customization.

And please, let's not add anything because "someone might want to do X". Add features when there's a documented use-case applicable to more than a few users.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--best is spending a lot of CPU cycles (and electricity) for a very minor gain in size. If there is no configuration choice, I'd argue for using the default FLAC settings.

Copy link

@IvanDSM IvanDSM Jan 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLAC -8 (--best) isn't that slow on current machines. FLAC -5 is not a smart option, it often does produce FLAC files that are bigger than -8 by 1MB or more. "a lot of CPU cycles" is a very big exaggeration in my opinion. We aren't in the Pentium III era anymore, on semi-current machines the time difference between -5 and -8 is negligible.

As for @tobbez's idea of disabling customization, that's a very bad idea. There are people who will want -5, there are other people who will want -8, there are people who want ReplayGain during encode, there are people who don't. There are people who will want "-e", there are people who won't. Those are the two most common use cases that justify enabling option customization. These use cases are why EAC, XLD, CUERipper and dBpowerAmp (the four most reputable rippers) allow customization. Are we really going to convince people to ditch EAC on Wine for whipper when our response to "How do I add the replaygain option to the FLAC encoding process on whipper?" is "Change the source code and recompile it"?

Adding a customization option for this isn't bloat, it's a fundamental feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must be sure that whatever we encode to initially is the same as what we read from the CD, since we use the FLACs for accurate rip checksums. So anything that changes the contents is a bad idea.

I don't understand why people can't run something like this:

for i in $(ls *.flac); do flac -d $i -o foo.wav && flac --best --custom-option-1 foo.wav -o $1; done

Copy link
Contributor

@RecursiveForest RecursiveForest Jan 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not believe the goal of whipper is to attract or support users who are never going to learn how to use a proper Unix operating environment. There is no reason to add more lines of code to support a non-essential "feature" when users can do the same thing with a tiny shell script. No information about the disc/CDDA is lost by changing the compression option, so in my opinion it's bloat that flies in the face of the Unix philosophy.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A script would be a good idea, but then the encoding process would be ran twice. Is a customization option (a simple entry in the configuration file with the encoding parameters, for example) that much of bloat really?

Oh, and before I forget, congratulations on the work on removing gstreamer! Great progress!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for i in $(ls *.flac); do flac -d $i -o foo.wav && flac --best --custom-option-1 foo.wav -o $1; done

^ Not safe. [1]

Rather use:
for i in *.flac; do flac -d "$i" -o foo.wav && flac --best --custom-option-1 foo.wav -o "$1"; done

Because, for example:

$ ls -1
flac file1.flac
flac file2.flac
other file.mp3

$ for i in $(ls *.flac); do echo $i ; done
flac
file1.flac
flac
file2.flac

# but:
$ for i in *.flac; do echo "$i" ; done
flac file1.flac
flac file2.flac

So maybe newcomers should not necessarily be expected to be proficient in something that even experienced users can slip sometimes...

[1] http://mywiki.wooledge.org/ParsingLs

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use --best? There is no reduction in quality, the internal check sums still work and it only takes 2 seconds to encode a track which is essentially nothing when compared to the fact that it takes 20-30 minutes to actually rip the data from CD. Conversely as pointed out above bash scripting is hackish, error prone and requires everyone to come up with their own solution to the same problem. Including the script in whipper would probably be about the same if not more cognitive load / LOC than adding an option, and certainly more than just hard-coding --best.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, wasn't morituri using gstreamer's --best equivalent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using --silent instead of --totally-silent and capturing any errors written (stderr=subprocess.PIPE). Would be much more helpful to know that flac failed with a specific error, rather than just the fact that it failed.

except CalledProcessError:
logger.exception('flac failed')
raise