CD Ripping script does not use quality subsettings set in webadmin

[PeteK]

Hey guys--

I noticed that the CD ripping script, ripDiskWrapper.sh, is not using the quality settings set in the web admin when ripping CDs.  These settings affect MP3 and OGG encoding quality.  Currently, the arguments to the encoders are hard-coded in the script.  While the encoding type is passed in as an argument to the script, the quality subsettings are not.

I just wanted to point this out so someone doesn't rip an entire collection at a quality level that they didn't want.

I've filed a mantis report here:
http://mantis.linuxmce.org/view.php?id=3637

In the meantime, the way to correct it on your system is to modify ripDiskWrapper.sh to hard-code your desired bitrate/Bitrate arguments for lame and ogg

For mp3s: modify the following line in ripDiskWrapper.sh

            OutputFile='>(lame -h --tt "$FileName" --ta "$DARTIST" --tl "$DALBUM" --tn "$TrackNumber" --ty "$CDYEAR" --tg "$CDGENRE" - "$Dir/$FileName.'"$FinalExt"'.in-progress")' # encoder


You'll want to change the line that begins with "OutputFile='>(lame "
and add your quality settings there.  For example, add "-b bitrate"  where bitrate is your desired bitrate.  You could also add "-V"  for VBR encoding.

Similarly, modify the following line for .ogg encoding

            OutputFile='>(oggenc -Q --artist "$DARTIST" --album "$DALBUM" --date "$CDYEAR" --genre "$CDGENRE" --tracknum "$TrackNumber" -o "$Dir/$FileName.'"$FinalExt"'.in-progress" -)' # encoder

You'll want to change the line that begins with "OutputFile='>(oggenc"
and add your quality settings there.  For example, add "-q quality"  where quality is your desired quality level, -1 to 10.

Note that until the fix is added to LMCE, you'll have to check this script after updates (or lock the script file).

I haven't verified my arguments to the encoders, so you may want to verify that those are correct on a disk or two before ripping your entire collection.

-Pete

[darrenmason]

Are you sure that the script isn't generated after a web admin change and reload router and/or reboot?
It may well not be, I have just seen a lot of the scripts done this way.

[PeteK]

I don't think this one is, as it is tracked in SVN, but some experimentation might be in order to verify that.

[Zaerc]

Any idea how and from where this script is called?

I was expecting it to do a bunch of sql queries to get the settings information from the database as some other scripts do.  I might be able to hack that in anyway (if nobody else is working on fixing this up).  But it might be better to do that on a higher level and pass it as extra parameters to the script.

[PeteK]

Zearc--

I had looked into this before when I was digging into updatemedia and the ripping process.  If I recall correctly, the relevant module was the one that was in the same directory as the script file.  You can also grep ripDiskWrapper.sh, as I believe the .cpp source file that calls it does so by name.  I'd find the info for you, but I'm at the office right now catching up on a few things.  If you can't find it, I'll post what I can find at home tonight.

[PeteK]

My guess is that the program does in fact query the sql database to get the correct encoding method, but that those queries were not extended to include the subcategories for quality options.  It may be as simple as adding those queries to the ripping program and having it pass those as arguments to the script, just as it does currently with encoding type.  Please let me know what you find.

[Zaerc]

Found it in: LinuxMCE-1.1-SRC/src/Disk_Drive_Functions/RipTask.cpp and looking through that now to see if I can make heads and tails out of that.  It would probably be easier for me to hack it into the wrapper script, but I'll try to figure out how to do it properly in the source and pass it as extra parameters.

Looks like it needs to be added to "strParameters", so that it can be passed to the wrapper script.

[PeteK]

That sounds correct.  One thing to keep in mind is that we need to handle the case where the encoder requires additional parameters (MP3) as well as the one where it does not (FLAC).  My first instinct would be to have the program send the same number of parameters to the script, and the script will ignore the unnecessary parameters when it doesn't require them.  That would mean sending MP3 bit rate, VBR/CBR setting, MP3 custom settings (I assume that's what the text box is for on in the MP3 settings section) and Ogg quality level to the script every time it's called, and having the script decide which (if any) sub-parameters to use based on the type of encoding chosen. But if you've got a different method you'd like to try, I'm game.

-Pete

[Zaerc]

That was my idea as well, but I can't even seem to figure out how the mp3/ogg/flac/wav parameter gets filled from the database  (parameter $8 - $ripFormatString in the script / [m_]sFormat in the .cpp code).

I'm going to have a look at the DB now to see if I can figure out where the ripping settings are stored, maybe that will give me a lead.  But I think the cpp code is still way over my head, so I might just end up doing the SELECT's from the script.  That's probably less elegant but it would avoid the need to pass extra parameters altogether.

[PeteK]

Zaerc--

That's a good way to get it working in the short term.  I'm just worried about the script getting a little convoluted in that we're pulling in related parameters in two different ways.  I've already compromised myself in adding ripped file tagging inside the script for the 0710 release (I think it was pushed in an earlier update to 0704 as well), when it really belongs in UpdateMedia.  One day I'd like to get that cleaned up.  Maybe we can just add the select calls in the script to the list of things to clean up eventually, as well.

[Zaerc]

Digging from the other end, I found out that these settings are stored in the Device_DeviceData table as follows: "mp3;320;vbr" so the settings are already appended to the encoding type.  That either means that it gets stripped from the argument, or the info is already passed to the script.

That does seem to make it a bit redundant to retreive the value again in the script itself, doesn't it?

To test it I tried ripping a CD, only to find this in the logfile:

Code: [Select]

Spawn_rip_job_1_task_0_29990.log:Parameter: flac
In spite of not just changing that to mp3;320;vbr, but even having verified that value in the DB by looking it up.   Maybe I need to reboot (on a sidenote, my test core is acting up a little lately).

EDIT:

I just looked in the wrapper script again and it does keep the extra parameters into account... by stripping them off.

Code: [Select]

ripFormat=${ripFormatString%%;*}

So I would suggest something like this (around line 141):

                        mp3)
                                FinalExt="mp3"
                                BitRate="-b `echo $ripFormatString | cut -d';' -f2`"
                                if [ "`echo $ripFormatString | cut -d';' -f3`" = "vbr" ]
                                then
                                        BitRate="$BitRate -V"
                                fi
                                OutputFile='>(lame -h --tt "$FileName" --ta "$DARTIST" --tl "$DALBUM" --tn "$TrackNumber" --ty "$CDYEAR" --tg "$CDGENRE" - "$Dir/$FileName.'"$FinalExt"'.in-progress" $BitRate)'  # encoder
                        ;;


On another sidenote, I'm a bit puzzled by the fact that cdparanoia seems to be run at a very low priority (nice -n 15):

Code: [Select]

command='nice -n 15 cdparanoia -e -d "$sourceDevice" "$Track" - 2> '"$ProgressOutput"' > '"$OutputFile"
Seems to me that could deprive cdparanoia of some precious cpu cycles and make it more error prone.

I haven't properly tested any of this as it's now way past my bedtime, so I'm hoping you can take it from here.  The ogg part should be very similar as well.


EDIT: fixed a type-o in the modification above!

[1audio]

cdparanoia seems to be run at a very low priority

Perhaps that explains why some disks can take hours to rip (I know they are not pristine, but they seem to be flawless on the CD player).

[Zaerc]

I have noticed that as well and I'm considering hardcoding cdparanoia to read at single speed, not to make it faster but to hopefully reduce the possible number of reading errors. 

My roomie has a cd collection that looks like he's been using them as coasters. 

[PeteK]

Zaerc--

That's a great find!  I'm much weaker at scripting than I am at general c++ coding so I'm hoping you or someone else can work this issue and maybe attach an updated file to the mantis report to fix this issue.  If you can't, I'll take a look and test out your script changes.  I'm not sure whey you're not getting a change in the parameter passed to the script.  It always seemed to work correctly for me after doing a quick reload router between changes.

-PeteK

[Zaerc]

Ah I think I forgot about the reload.  And I discovered an obvious type-o in my addition which I just fixed above.  From the logs it seems that the paramers are appended correctly now. 

I should be able to come up with a patch tomorrow, if nobody beats me to it. 

LOL I was scratching my head why I couldn't rip to mp3, until I realized... I did't have LAME installed, how lame is that!   Would be nice if that produced a bit more meaningful error though, maybe I can look into that as well.  Seems to be working fine now.