patternpythonMinor
Identify audio using Gracenote
Viewed 0 times
usingidentifyaudiogracenote
Problem
This script records audio from the computer's output and passes it to a C program that fingerprints it and queries the Gracenote database. If it identifies the audio then it offers options to look it up on Discogs. I made this because I wanted a way to identify tracks in DJ mixes that didn't involve using Shazam on my phone.
Running the script requires setting up audio devices using Soundflower and installing SwitchAudioSource to programmatically switch device (details here).
I am particularly interested in comments on the design. I think the most significant problem is that the
Would an OO-style have been preferable here?
The functions could easily be split into modules as they have quite distinct tasks. Does the length of this script justify breaking it into separate modules?
This is also the first time I have written something using .rc files and command-line flags (most of my experience is JS in the browser) so I am interested in views on whether I have used them sensibly.
```
#!/usr/bin/python
import wave
import time
import subprocess
import os.path
import json
import argparse
import webbrowser
import sys
import requests
import keyring
import pyaudio
from rauth import OAuth1Service
# ---------------- Config -----------------
CHUNK = 1024
FORMAT = pyaudio.paInt16
CHANNELS = 2
RATE = 44100
RECORD_SECONDS = 6
SAVE_PATH = os.path.expanduser("~") + "/Music/recordings/"
WAVE_OUTPUT_FILENAME = "temp_{}.wav".format(int(time.time()))
COMPLETE_NAME = os.path.join(SAVE_PATH, WAVE_OUTPUT_FILENAME)
CONFIG_PATH = os.path.expanduser("~") + "/.identifyaudiorc"
def load_user_config(path):
user_config = {}
try:
Running the script requires setting up audio devices using Soundflower and installing SwitchAudioSource to programmatically switch device (details here).
I am particularly interested in comments on the design. I think the most significant problem is that the
main() function is overly dense. I decided not to use a more object-oriented style because I wasn't sure of the benefit over using a few functions. If I were to do more with either Discogs or Gracenote I would probably use the Python API clients they both provide, so rather than recreate a partial implementation of them I just used several functions.Would an OO-style have been preferable here?
The functions could easily be split into modules as they have quite distinct tasks. Does the length of this script justify breaking it into separate modules?
This is also the first time I have written something using .rc files and command-line flags (most of my experience is JS in the browser) so I am interested in views on whether I have used them sensibly.
```
#!/usr/bin/python
import wave
import time
import subprocess
import os.path
import json
import argparse
import webbrowser
import sys
import requests
import keyring
import pyaudio
from rauth import OAuth1Service
# ---------------- Config -----------------
CHUNK = 1024
FORMAT = pyaudio.paInt16
CHANNELS = 2
RATE = 44100
RECORD_SECONDS = 6
SAVE_PATH = os.path.expanduser("~") + "/Music/recordings/"
WAVE_OUTPUT_FILENAME = "temp_{}.wav".format(int(time.time()))
COMPLETE_NAME = os.path.join(SAVE_PATH, WAVE_OUTPUT_FILENAME)
CONFIG_PATH = os.path.expanduser("~") + "/.identifyaudiorc"
def load_user_config(path):
user_config = {}
try:
Solution
To OO or to not to OO
Your program is fairly small and well organized and easy to read.
I don't see any reason at this point to make it "more OO".
If in the future you write more programs like this you'll want to
organize the code into libraries to re-use common code. But at
this point what you have is perfectly fine.
Now on to some stylistic comments...
Global variables
You have two kinds of global variables:
run to run, e.g.
and
First of all as globals the
should have ALL CAPS names (and
more descriptive name -- it is even used?) This will
make it obvious to the reader that they are globals.
Secondly, I would move the initialization of
the path name globals to
In fact, none of these values need to be global since
a good aspect of your code!
As an enhancement, consider adding command line options to set
these paths - I think being able to set the location of the recordings
directory would be convenient.
load_user_config()
What you have is fine, but be aware that there are tons of python libraries
to load (and save) configuration files. Just google, e.g., "python load config file".
long if-blocks
logic more apparent I would organize it like this:
Now the reader can easily see what happens when the call to SwitchAudioSource fails
without having to search for the matching
Another long
readable if it was written:
In fact, now it is obvious that you aren't doing anything if
there isn't a match. Perhaps you want something like:
Your program is fairly small and well organized and easy to read.
I don't see any reason at this point to make it "more OO".
If in the future you write more programs like this you'll want to
organize the code into libraries to re-use common code. But at
this point what you have is perfectly fine.
Now on to some stylistic comments...
Global variables
You have two kinds of global variables:
- constants like
RECORD_LENGTH,CHUNK
- values which invoke code and could vary from
run to run, e.g.
WAVE_OUTPUT_FILENAME, CONFIG_PATH- values which are initialized to objects, e.g.
p
and
discogsFirst of all as globals the
p and discogs variablesshould have ALL CAPS names (and
p should be given amore descriptive name -- it is even used?) This will
make it obvious to the reader that they are globals.
Secondly, I would move the initialization of
the path name globals to
main:def main():
user_home = os.path.expanduser("~")
config_path = os.path.join(user_home, ".identifyaudiorc")
save_path = os.path.join(user_home, "/Music/recordings/")
complete_name = os.path.join(save_path, "temp_{}.wav".format(int(time.time())))
...In fact, none of these values need to be global since
main passes them to whatever function needs them - and that'sa good aspect of your code!
As an enhancement, consider adding command line options to set
these paths - I think being able to set the location of the recordings
directory would be convenient.
load_user_config()
What you have is fine, but be aware that there are tons of python libraries
to load (and save) configuration files. Just google, e.g., "python load config file".
long if-blocks
main has a very long spanning if-block, and to make thelogic more apparent I would organize it like this:
def main():
...
if not subprocess.call(["SwitchAudioSource", ...):
raise RuntimeError("Couldn't switch to multi-output device.")
...Now the reader can easily see what happens when the call to SwitchAudioSource fails
without having to search for the matching
else clause.Another long
if-block is the if match: statement. It would be morereadable if it was written:
if not match:
return
print json.dumps(resp["re...In fact, now it is obvious that you aren't doing anything if
there isn't a match. Perhaps you want something like:
if not match:
raise RuntimeError("Unable to read audio.")
...Code Snippets
def main():
user_home = os.path.expanduser("~")
config_path = os.path.join(user_home, ".identifyaudiorc")
save_path = os.path.join(user_home, "/Music/recordings/")
complete_name = os.path.join(save_path, "temp_{}.wav".format(int(time.time())))
...def main():
...
if not subprocess.call(["SwitchAudioSource", ...):
raise RuntimeError("Couldn't switch to multi-output device.")
...if not match:
return
print json.dumps(resp["re...if not match:
raise RuntimeError("Unable to read audio.")
...Context
StackExchange Code Review Q#128528, answer score: 2
Revisions (0)
No revisions yet.