Opened 14 years ago
Closed 14 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 |
| Version: | 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)
Change History (11)
comment:1 by , 14 years ago
comment:2 by , 14 years ago
| Priority: | normal → high |
|---|
comment:3 by , 14 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.
comment:4 by , 14 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 , 14 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 , 14 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 , 14 years ago
| Owner: | set to |
|---|
comment:8 by , 14 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 , 14 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
This bug is nice to get fixed before the release. Raising priority for keeping the track.