#8707 closed patch
TOOLS: Addition of libvorbis and libflac
| Reported by: | SF/lightcast | Owned by: | SF/knakos |
|---|---|---|---|
| Priority: | normal | Component: | Tools |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
The first part of my SoC work.
The vast majority of the changes occur in compress.c The new function encodeRaw() takes in an array of raw audio data and encodes it using either libvorbis or libflac instead of using the external tools. encodeAudio() was rewritten to use this function instead of calling the external tools, except in the case of MP3 compression where it remains unchanged.
Other changes: FLAC arguments are now stored in an actual struct instead of simply passing argv[]. All compression levels, block size, verify, and silent are supported as FLAC arguments.
Hard bitrate restrictions now work when using Vorbis compression. They were broken in the original tools.
Modified compress_queen to use compress.c
Modified compress_scumm_bun and compress_scumm_san to use --vorbis instead of --ogg to be consistent with the other tools.
Modified the command line arguments of compress_kyra, compress_scumm_san, and compress_scumm_bun to be more consistent, which also removed a couple of HACK's.
Modified all of the tools with updated help messages to reflect the changes to FLAC arguments.
Several cosmetic changes (i.e. invalid arguments listed in the help messages of some tools, unnecessary whitespace removed, etc.)
Compiled on MSVC, MinGW, and gcc. Tested on Simon 1 & 2, Full Throttle, Flight of the Amazon Queen, and Broken Sword 2. This covers all the modified functions in compress.c, so all the other tools should also work.
Slight Quirk: Vorbis compressed files produced from a MSVC compiled executable are slightly larger than both the original tools and the MinGW/gcc compiled tools. They still work but for some reason the bitrate calculated for a specified quality level is slightly higher. I tried changing optimizations and re-compiling the libraries but nothing changed the results. I'm not sure what's causing this to happen.
Ticket imported from: #1746270. Ticket imported from: patches/812.
Attachments (3)
Change History (27)
by , 17 years ago
| Attachment: | SoC Tools Update.patch added |
|---|
comment:1 by , 17 years ago
comment:2 by , 17 years ago
Looks good.
One little question. Recently there was a major update to compress_saga tool. Did you sync your tree with trunk?
comment:3 by , 17 years ago
Yes I synced before submitting. The only change to compress_saga was to change the help message and to remove the isSigned flag from rawAudioType. Unless the saga files are different 8-bit audio samples are always unsigned and 16-bit audio samples are always signed so it was unnecessary to have a flag for it. I didn't see anywhere else the flag was used so I got rid of it.
comment:4 by , 17 years ago
Just fine. Sean also added flac to dxa & the swords, which was recently "requested" :-)
comment:5 by , 17 years ago
| Owner: | set to |
|---|
comment:6 by , 17 years ago
Great! I haven't checked everything in this patch, but I am excited nevertheless :)
So how do we proceed with this? Make a tools branch for it and let Sean develop in there? Personally I prefer a continuos development history (i.e. commits etc.) over big code bombs, because that way you do not just get the code, you also can better understand how it grew and how parts of it worked.
However, I am just suggesting this, it's up to you guys to decide (esp. the mentor) :).
comment:7 by , 17 years ago
Sadly, compress.c doesn't compile for me -- probably because I have version of FLAC which is "too old", i.e. 1.1.1 and no 1.1.3/1.1.4. Of course it'll probably break again in 1.1.5 *g*.
The error is:
compress.c: In function 'encodeRaw': compress.c:420: error: 'FLAC__StreamEncoderInitStatus' undeclared (first use in this function) compress.c:420: error: (Each undeclared identifier is reported only once compress.c:420: error: for each function it appears in.) compress.c:420: error: parse error before 'initStatus' compress.c:448: warning: implicit declaration of function 'FLAC__stream_encoder_set_compression_level' compress.c:454: error: 'initStatus' undeclared (first use in this function) compress.c:454: warning: implicit declaration of function 'FLAC__stream_encoder_init_file' compress.c:456: error: 'FLAC__STREAM_ENCODER_INIT_STATUS_OK' undeclared (first use in this function) compress.c:458: error: 'FLAC__StreamEncoderInitStatusString' undeclared (first use in this function) make: *** [compress.o] Error 1
Maybe we can support both API versions (like we do in sounds/flac.cpp) ?
In the meantime it would already help if one could turn off Ogg and/or FLAC encoding (e.g. add a DISABLE_FLAC preprocessor macro?). Note: I am just thinking aloud... As it is, we are approaching a complexity where we may have to consider adding a configure script to tools/ !
comment:8 by , 17 years ago
Yes it is the version, since FLAC encoding completely changed between 1.1.2 and 1.1.3. I haven't looked at how it would be done using 1.1.2 or earlier but the porting guide says that the current version is vastly simplified. I'll see what I can do about it. As for disabling Ogg and FLAC I agree that it would be a good idea. It should only take a couple ifdef's around the new code in compress.c.
comment:9 by , 17 years ago
I agree, we could use a configure script for the tools, with several libraries been used by tools now.
FLAC 1.1.1 is almost 3 years ago, why not require FLAC 1.1.3 or later? requiring a more recent version would be more sensible in these situations, instead of supporting multiple API versions. There is no reason people can't upgrade to FLAC 1.1.3 or later either.
I disagree, about making FLAC and Vorbis support optional. That would cause more confusion for users, if we have tools packages for platforms, which don't offer all encoding options.
comment:10 by , 17 years ago
The changes to compress_saga were mostly related to rewriting the detection part (which was chaotic), and to remove unneeded dependencies. The format of the compressed files itself is unchanged. I've removed isSigned but decided to put it back, because it's needed for the Macbinary files used in a specific ITE version. But yes, your assumption is correct. The MacBinary files need to be decoded separately anyway, so removing the isSigned flag is no biggie
comment:11 by , 17 years ago
And I forgot to say: nice work, you managed to sort out a lot of the code used in our tools :) Well done!
comment:12 by , 17 years ago
In my opinion, it would be viable to make libogg and libflac optional, with appropriate ifdefs. In the case where the libraries are not compiled in the executable, the tool could revert to using oggenc and flac externally, like what's currently happening. This way, everyone can happily compile the tools, even if they don't want to include libogg and libflac
comment:13 by , 17 years ago
I just added in preprocessor macros to disable linking libvorbis and/or libflac, in which case it falls back to the old way of calling the external binaries. As for supporting old version of the libraries, I believe that libvorbis should be compatible across all versions and libflac only differs between 1.1.2 and 1.1.3. I looked at the documentation for libflac 1.1.2 and it looks like it would be easy to support the old version. It's mostly just changing all the FLAC__stream_encoder_* functions to FLAC__file_encoder_* functions. The only big difference is that FLAC 1.1.2 and below did not have a set_compression_level function so I would need to set all the compression settings for each level, but they are listed in the current documentation. If anyone wants me to add support for previous versions of libflac let me know.
comment:14 by , 17 years ago
I'm with Kirben on this. The tools are meant to be run on desktop platforms (i.e. not underpowered). Thus:
1) Supporting older FLACs is like the kiss of death, judging by Fingolfin's recent stories and bitterness caused by the interface inconsistencies. So supporting the latest FLAC is enough for me. If though, newer FLAC versions are not available for all our supported desktop platforms, then we should consider adding support for older ones. I take it that Fingolfin hints to such a case.
2) Conditional builds also seem unecessary to me on the same basis, but since lightcast has coded that in, well it's alright :-)
About the organizational part: Sean's work really complements and augments the mainline scummvm tools package. Our recent experiences with gsoc students with a dedicated branch show a lot of svn activity is spent on syncing between the trunk and the branch. So, in my view, we either allow this (and the following work) be committed to the trunk or, as a middle case we can make a branch of the tools subfolder only. Also, do we add lightcast to the commit list of scummvm or do I commit the patches myself?
by , 17 years ago
| Attachment: | SoC Tools Update #2.patch added |
|---|
comment:15 by , 17 years ago
I added the updated patch with the preprocessor macros to disable linking libvorbis and/or libflac. I won't add support for old versions of FLAC unless it turns out it is necessary. File Added: SoC Tools Update #2.patch
comment:16 by , 17 years ago
Supporting old FLAC versions is not strictly necessary from my point of view, but at the very least it should be documented which version of FLAC is required to build.
And I still think that it should be possible to build the tools without libflac/libogg etc.. And *of course* the official binaries should be built with those enabled. But I don't see why we should force everybody to install libflac even if they don't need it.
In fact I would prefer if the encoding code was modular. That is, the compression tools can call any of several "encoding modules". We'd have the three classical modules "MP3 via lame", "Ogg via vorbis tools", "FLAC via flac", plus the new "MP3 via liblame", "Ogg via libogg/libvorbis" and "FLAC via libflac" tools. It would be possible to build the tools with all six of these modules enabled, or only a subsection. If we want to add e.g. speex support in the future, this would then be relatively easy (just to give a not completely unlikely example, I am still not suggesting we do so). Even if we don't add additional "encoders", a modular and orthogonal structure of the code helps debugging and development.
The official binary would have all options enabled, except for liblame encoding (we can't ship binaries with that enabled, due to legal reasons); private builds could have all enabled. The GUI version of the encoding tools could allow the user to select his preferred encoding method. The user could only selected "encoding via external tool" if the resp. tool is present. In the GUI, the user could also specify the path to the lame/oggenc/flac tools.
Some random remarks: The modules above essentially mean that there should be a clear cut well defined API for the "encoder stuff". Consider designing and using a C++ base class for this, which the encoders implement (a factory pattern might be a good idea here). The default encoder settings could be tweaked, too -- the last time I tried, Ogg Vorbis produced bigger files than MP3 by default, which lead many people to believing that this is a property of Ogg, but in reality it's just that apparently the default Ogg settings are for a much higher quality than the MP3 settings. I believe it would be better to either use settings for both which produce comparable sized output, or produce "comparable" quality levels (and thus likely smaller ogg files). This is a minor point, of course, you could put it on your "potential TODO" list, if you like and if you have one (the wiki is a great place for such a thing, BTW).
These are some of my thoughts on the matter. Feel free to argue with me about them, nothing is set in stone :-).
As for branching: Well, this was meant to be a tools-only branch anyway. Given that the tools code is not changing that much, I don't think there'll be much merging work to crop up. Still, I am not forcing anybody to work in the repository, I just think that it helps to meet the end goals better, but that's just me (but don't forget you'll soon have to write a mid-term evaluation report, and that in the end all the code must be available to Google in some way).
Finally, great work, keep it going!
comment:18 by , 17 years ago
So what do you say Sean? Feel up to working off a branch containing only the tools subfolder directly at our repository? Me and Fingolfin would prefer it this way. Let's get this issue over and done quickly (either me committing or you working off a branch) so we can move ahead.
I also note Fingolfin's thoughts on the modularity of implementation. In the scope of gsoc, and since your proposal was accepted this way, I do not think it is mandated move to an OO architecture. It would be nice though. But anyway, the second major part of lightcast's work is coming up and that is the gui. Maybe Sean can factor in some of those comments in there?
comment:19 by , 17 years ago
I can work off of a branch if you want. I agree that it would be hard to both create the GUI and rewrite the tools to be more OO during SoC. I'll see what I can do if I finish the GUI earlier than I planned though.
comment:20 by , 17 years ago
Of course OO architecture is not all mandatory, I was just stating some thoughts and ideas of mine, feel free to use them or not as you deem fit :). Although I think modularity (with or without OO) would be very important.
comment:21 by , 17 years ago
OK. Please, checkout https://scummvm.svn.sourceforge.net/svnroot/scummvm/tools/branches/gsoc2007-toolsgui and commit this patch to that branch. I gave you SVN write permissions.
If you are going to use something like SVK, please, turn off any additional autogenerated commit log messages.
Also please, make sure that you read our Commit guidelines.
comment:22 by , 17 years ago
| Status: | new → closed |
|---|
comment:24 by , 6 years ago
| Component: | → Tools |
|---|
Minor issue with changes to the readme, using FLAC compression on Broken Sword 1 music and speech files will not produce larger files.