Opened 10 years ago

Closed 10 years ago

#4712 closed defect (fixed)

GUI Tools: cannot use lame with compress_scumm_sou

Reported by: criezy Owned by: criezy
Priority: high Component: Tools
Keywords: Cc:
Game:

Description

Using the GSoC 2009 GUI tool compiled on Saturday November 28 2009 on an
Intel mac running MacOS X 10.4.

When selecting a monster.sou file in the GUI Tools and mp3, I get the following error:

Fatal Error Occured: Error in MP3 encoder.(check parameters)
MP3 Encoder Commandline:lame -t -r --bitwidth 8 --big-endian -m m -s 22050 --vbr-new -b 24 --resample 22050 -q 2 -V 4 -B 64 "tempfile.raw" "tempfile.mp3"

I get this wether I let the GUI auto-detect the tool or if I select it manually.

If I use the CLI if work well:
$ ./tools_cli --mp3 DOTT/monster.sou
=> gives me a working monster.so3 file.

If I call directly lame on a WAV file with the same argument it also works fine:
$ lame -t -r --bitwidth 8 --big-endian -m m -s 22050 --vbr-new -b 24 --resample 22050 -q 2 -V 4 -B 64 test.wav test.mp3
=> gives me a working test.mp3 file.

For info lame is installed in /usr/local/bin which is listed in the PATH environment variable.

Ticket imported from: #2905473. Ticket imported from: bugs/4712.

Attachments (2)

tools_gui_tmp_lame_fix.patch (23.3 KB ) - added by criezy 10 years ago.
Proposed patch
tools_gui_lame_fix.patch (11.5 KB ) - added by criezy 10 years ago.
Updated and cleaned patch

Download all attachments as: .zip

Change History (11)

comment:1 by sev-, 10 years ago

This bug is nice to get fixed before the release. Raising priority for keeping the track.

comment:2 by sev-, 10 years ago

Priority: normalhigh

by criezy, 10 years ago

Proposed patch

comment:3 by criezy, 10 years ago

I just added a possible patch for the issue.

Depending where lame is installed it is not always found by the executable. And on MacOS X (and Windows I suppose), as the GUI Tools is not started from a terminal it is not easily possible to add the path to lame in the environment before starting the application.

The solution I propose is to add a 'lame path' option in the compression tools (and in the advanced MP3 option page). It defaults to 'lame' but can for example be set to '/usr/local/bin/lame'.And this solve the issue I had.

Because it is passing the path to lame as an option to the compression tool, it needs the patch #2981339 to work. For the moment the patch I have posted here includes patch #2981339 (thus the tmp in the name). If patch #2981339 gets accepted (before this one) I will regenerate a patch for this issue.

by criezy, 10 years ago

Attachment: tools_gui_lame_fix.patch added

Updated and cleaned patch

comment:4 by criezy, 10 years ago

The second patch I attached is a cleaned version that does not contain the patch #2981339 (that just got accepted) and should therefore apply cleanly on the trunk.

comment:5 by fingolfin, 10 years ago

I don't have time to review your patch right now, but adding a "lame path" option sounds very good.

Ideally, we would also have code which checks whether lame is callable (i.e. invoke "lame --version" or so, and maybe even parse the lame version to verify it's not one of the old buggy ones). This way, if a user chooses MP3 compression and the lame path is not correctly configured, we could pop up a meaningful error which asks the user to (re)choose a Lame path, resp. to install Lame.
This, however, is not something I am expecting you to add to your patch (although you are of course welcome to), but maybe you could add a TODO comment to this effect.

comment:6 by criezy, 10 years ago

I will look into this more in details next week-end.
Testing that the lame path is correct seems a good idea and I can see two places where it should be done:
1) When selecting the MP3 format for encoding and pressing Next if the Advanced Audio Option checkbox is not checked. In that case we should probably display the advanced option page anyway if lame could not be found using the current set path, and display a message to the user explaining that he needs to select a valid lame executable.
2) When the user changes the lame path in the advanced page, which wold display an error message if the new path is not correct or the version too old.

By the way, what is the lame version required by the tools?
I will probably skip the check of the lame version in a first time, but I will add it in a comment. lame --version is very verbose and I don't know if what works in my case to get the version number (e.g. lame --version | head -n 1 | cut -f 4 -d ' ') will work with all supported versions.
That would probably not be a bad idea to have that info in the ReadMe file as well (and to update that ReadMe file which seems completely outdated - that will be another patch).

comment:7 by criezy, 10 years ago

Owner: set to criezy

comment:8 by criezy, 10 years ago

Committed a modified version of the last patch in trunk and 1.1.0 branch. As suggested I have also added a check on lame.

comment:9 by criezy, 10 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.