#8315 closed patch
Flac Support
| Reported by: | SF/nolange | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Audio |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
I zipped the 3 files up. The preprocessorflag is "USE_FLAC", if set it will require either the libflac or libflac _static Library. I had duplicate symbols upon linking, might be that youll need to ignore libc as I had to.
Of course the projects/makefiles have to be adopted( adding header & source and the flac-library ), Im working with MSVS7.1, so I couldnt even submit my own Project - its incompatible to VS7.1 and earlier.
extract and simonmp3 now have a --flac switch to generate flac-Compressed containers. CD-Tracks are supported.
I tested CD-Loom and DOTT, both work fine.
Ticket imported from: #885904. Ticket imported from: patches/420.
Attachments (3)
Change History (49)
comment:1 by , 21 years ago
comment:2 by , 21 years ago
I haven't had time to look closely at this, but a few things I noticed:
* getFlacTrack etc. leak (a File object is created but not disposed) * no legal headers in flac.cpp/flac.h * odd lineends (?) in flac.cpp, in particular around FLAC__SeekableStreamDecoderTellStatus * patch is not following our source formatting conventions * enum names, in particular: enum { MP3 = 0, Vorbis = 1, Flac = 2} _sound_mode that should be something like kMP3Mode, kVorbisMode, kFlacMode * in extract & simon2mp3 help text, this line is printed twice: --help this help message * The change made to tools/util.h is not correct - you can't just override the "inline" there, it will lead to duplicate symbol errors
comment:3 by , 21 years ago
> getFlacTrack etc. leak (a File object is created but not disposed) fixed
> no legal headers in flac.cpp/flac.h fixed, unless theres something wrong with the $Header placeholder
> odd lineends (?) in flac.cpp, in particular around FLAC__SeekableStreamDecoderTellStatus converted the File to DOS-linefeeds - I hope thats what you meant, otherwise I dont have a clue
> patch is not following our source formatting conventions Sorry, dint find them. Please point me to them.
> in extract & simon2mp3 help text, this line is printed twice: --help this help message You must have eagle eyes. fixed
> The change made to tools/util.h is not correct - you can't just override the "inline" there, it will lead to duplicate symbol errors inline aint a C-Keyword( I just made sure by looking into Stroustrups Book ) and aint( shouldnt? ) defined - the line I added is only reached by C-Compilers. Which wont compile otherwise.
Ill have a closer look on my whole Code once I found the source conventions.
comment:4 by , 21 years ago
fine, I just seen that:
/* If your C compiler doesn't support 'inline', please add a check for it. */ #if defined(CHECK_FOR_OLD_C_COMPILER) #define inline #endif
must have been added after I changed it, if I dint seen it I better loose my driving license
comment:5 by , 21 years ago
No, with lineends I didn't mean that I want you to switch to DOS lineeneds. My editor can open unix/dos/mac linenends. The problem is that flac.cpp isn't using consistent lineends - parts of it use other lineends than others. Anyway, I can probably manually fix it (replace \n by \r or so)
Our source code conventions can be found on our homepage, in the documentation section. Direct link: <http://scummvm.org/ documentation.php?view=conventions>
comment:6 by , 21 years ago
| Summary: | Flac Support - now for real → Flac Support |
|---|
comment:7 by , 21 years ago
More:
* Using openStreamFile succesfully will leak "tempFile" * There is a change in sword1/control.cpp which seems completely unrelated to flac support
comment:8 by , 21 years ago
> Using openStreamFile succesfully will leak "tempFile" gonna fix it > There is a change in sword1/control.cpp which seems completely unrelated to flac support Right, my C++ Libs have a float and a double-version of sqrt() - its not related to flac, but its necessary to compile.
I dont have much time now, Ill see that I have proofreaded my stuff and changed to the source-conventions on this weekend.
comment:9 by , 21 years ago
No worries, you have time, this won't be checked in before the 0.6.0 release anyway :-)
comment:10 by , 21 years ago
> No worries, you have time, this won't be checked in before the 0.6.0 release anyway :-)
Ack, I hoped it would be, but I guess that means 0.6.0 is closer than I thought ;)
I cleaned up my stuff and the changes I done to other files. The tools-patch schould follow the next days.
sound/mixer.cpp has a confilct, but it should be easy to solve.
comment:11 by , 21 years ago
Another update... I tried to play a corrupted file, dint worked as I intended it.
* Fixed handling of bad/nonflac Files * Added Flac-support for Simon
Just aint sure how I should handle Plattforms which only allow a 3-letter-extension ?
comment:12 by , 21 years ago
Another update... I tried to play a corrupted file, dint worked as I intended it.
* Fixed handling of bad/nonflac Files * Added Flac-support for Simon
Just aint sure how I should handle Plattforms which only allow a 3-letter-extension ?
comment:13 by , 21 years ago
just made a few more tweaks to flac.cpp. I really think that should be it.
Possible Problem: A stream of a corrupt/nonflac File will report 0 as samplerate.
comment:14 by , 21 years ago
The 3 letter extension seems to be .fla (see also http://filext.com/ detaillist.php?extdetail=FLA). FLC of course is already taken by the FLIC format.
Other things: * It should be "_trackFormats" not "_kTrackFormats". But actually, since that struct is 100% loclal to audiocd.cpp, I wouldn't declare it in audiocd.h at all, but rather as a "global" type in audiocd.h (usually, try to put things in headers only if you really have to. That cuts down on recompile times when one has to make changes, and helps to keep the API fully private)
* Same for "_kStreamFormats" in audiostream.h
* a patch for the configure script would be nice. However, you do not have to provide this (esp. if you aren't on Unix ;-), we can add one ourselves later on (just mention it here for completness).
comment:15 by , 21 years ago
* updated audiocd|audiostream .h|.cpp as requested * try to load .flac, if that fails, try to load .fla (1 change in Flac.cpp)
In sorry bout the config-files, all my attempts on working with Linux/Unix failed miserably ;)
I knew about the 3-letter extension, but I wasnt how to do it, at first I thought of a makro which will choose it in dependence of the OS, but then I realized you can have more than 1 filesystem on one OS. I guess the way its now is ok.
comment:16 by , 21 years ago
So these are both the tools & scummvm patches. I dont think I changed anything than flac.cpp in scummvm, but I aint sure now.
I have updated extract, simon2mp3, queenrebuild with an --flac option. Parameters are passed directly to the encoder. For some reason playing with the blocksize has the biggest effect on resulting filesize - usually 1152 seems to be a good value.
Should I update the readme with the changes too?
comment:17 by , 21 years ago
The current version of flac.cpp doesn't compile under g++. I've tracked the problem down to the following code: // See flac.cpp PMCONVERTBUFFERS stuff
class Foo { public: Foo(); void convertA( int ); void convertB( int ); typedef void (Foo::*fptr)(int); fptr _ptr; };
Foo::Foo() : _ptr( convertA ) {}
If I try to compile this, I get the following error: cppfptr.cpp: In constructor `Foo::Foo()': cppfptr.cpp:12: error: argument of type `void (Foo::)(int)' does not match `void (Foo::*)(int)'
I hope you can help, Regards, -Gregor
comment:18 by , 21 years ago
Hum? flac.cpp compiles just fine over here (using gcc 3.3).
There are various other "issues", though: * In struct StreamFileFormat, it must be const char* decoderName; const char* fileExtension; * Likewise for struct TrackFormat * flac.o must be added to sound/module.mk * Another warning: sound/audiostream.cpp:71: warning: comparison between signed and unsigned integer expressions
comment:19 by , 21 years ago
Strange ... I've modified flac.cpp this way: _methodConvertBuffers( &FlacInputStream::convertBuffersGeneric )
At every assignment to a function pointer I've added &FlacInputStream:: .
When will you include FLAC support to CVS?
-Gregor
comment:20 by , 21 years ago
I changed the structs, and fixed the warning Issue. I also modified module.mk, though I`d rather prefer not to change things I cant test.
I also modified flac.cpp in respect to the &FlacInputStream:: issue. Though I really think this is a shortcoming of the used compiler.
comment:21 by , 21 years ago
A shortcoming in the compiler indeed - esp. odd since it works just fine with the same compiler for me (well, using GCC 3.3 - gjasny never told us the compiler version he uses)
comment:23 by , 21 years ago
Hi, I've run some tests, so using &Foo::fctn seems to be the only solution to get this code compiled with g++.
convertA &convertA Foo::convertA &Foo::convertA g++-2.95 error error error OK g++-3.2 error error error OK g++-3.3 error error error OK icc 8 OK OK OK OK
&convertA g++-3.* message: ISO C++ forbids taking the address of an unqualified non-static member function to form a pointer to member function. Say `&Foo::convertA'
Foo::convertA g++-3.3 message: assuming pointer to member `void Foo::convertA(int)' (a pointer to member can only be formed with `&Foo::convertA')
-Gregor
comment:24 by , 21 years ago
Hmm, I looked around a bit and any example Code uses the &Class::method notation. Though I dint find any reference that specifiing just the method aint allowed. For functions you can definitly leave out the &, it doesnt matters if you use it or not. The forced Class-Scope would be a exception to the way name-resolving usually works in C++, and I cant think of a Reason why it should be...
Anyways, if the new code makes trouble, let me know.
comment:25 by , 21 years ago
I've hacked the configure script. As I can't see a possibility to upload a file, I'll attach it uuencoded. Please email me to gjasny@users.sf.net if you want it per mail.
Some notes: flac.cpp must include flac.h before testing for USE_FLAC #include "sound/flac.h"
#ifdef USE_FLAC
#include "common/file.h" #include "common/util.h"
same in flac.h: #ifndef SOUND_FLAC_H #define SOUND_FLAC_H
#include "stdafx.h" #include "common/scummsys.h"
#ifdef USE_FLAC
class AudioStream;
-Gregor
begin 644 configure-flac.diff M26YD97@Z(&-O;F9I9W5R90H]/3T]/3T]/3T]/3T]/3T]/3T]/3T]/3T]/3T] M/3T]/3T]/3T]/3T]/3T]/3T]/3T]/3T]/3T]/3T]/3T]/3T]/3T]"E)#4R!F M:6QE.B`O8W9S<F]O="]S8W5M;79M+W-C=6UM=FTO8V] N9FEG=7)E+'8*<F5T M<FEE=FEN9R!R979I<VEO;B`Q+C8U"F1I9F8@+74@+3, @+7`@+7(Q+C8U(&-O M;F9I9W5R90HM+2T@8V]N9FEG=7)E"3$@1F5B(#(P,#0@,30Z,#,Z, #`@+3`P M,#`),2XV-0HK*RL@8V]N9FEG=7)E"3,@1F5B(#(P,#0@,3,Z-3@Z, 3,@+3`P M,#`*0$`@+3(V+#8@*S(V+#<@0$`@0UA81DQ!1U,](B1#6%A&3$% '4R`D0U!0 M1DQ!1U,B"B`*(",@9&5F875L="!L:6(@8F5H879I;W5R('EE<R]N;R] A=71O M"B!?=F]R8FES/6%U=&\**U]F;&%C/6%U=&\*(%]M860]875T; PH@7V%L<V$] M875T;PH@7WIL:6(]875T;PI`0"`M,3DY+#8@*S(P,"PX($!`($] P=&EO;F%L M($QI8G)A<FEE<SH*("`@+2UW:71H+6]G9RUP<F5F:7@] 4("`@(%!R969I M>"!W:&5R92!L:6)O9V<@:7,@:6YS=&%L;&5D("AO<'1I;VYA; "D*("`@+2UW M:71H+79O<F)I<RUP<F5F:7@]4(%!R969I>"!W:&5R92!L:6)V;W) B:7,@ M:7,@:6YS=&%L;&5D("AO<'1I;VYA;"D*("`@+2UD: 7-A8FQE+79O<F)I<R`@ M("`@("`@(&1I<V%B;&4@3V=G(%9O<F)I<R!S=7!P;W)T(% MA=71O9&5T96-T M70HK("`M+7=I=&@M9FQA8RUP<F5F:7@]4("`@4') E9FEX('=H97)E(&QI M8D9,04,@:7,@:6YS=&%L;&5D("AO<'1I;VYA;"D**R`@+2UD: 7-A8FQE+69L M86,@("`@("`@("`@(&1I<V%B;&4@1DQ!0R!S=7!P;W)T(% MA=71O9&5T96-T M70H@("`M+7=I=&@M;6%D+7!R969I>#U01E@@("`@4') E9FEX('=H97)E(&QI M8FUA9"!I<R!I;G-T86QL960@*&]P=&EO;F%L*0H@("`M+61I<V% B;&4M;6%D M("`@("`@("`@("`@9&ES86)L92!L:6)M860@*$U0,RD@<W5P<&] R="!;875T M;V1E=&5C=%T*("`@+2UW:71H+7IL:6(M<')E9FEX/5!&6"`@(%! R969I>"!W M:&5R92!Z;&EB(&ES(&EN<W1A;&QE9"`H;W!T:6]N86PI"D!`("TR, C8L-B`K M,C(Y+#@@0$`@9F]R(&%C7V]P=&EO;B!I; B`D0#L@9&\*("`@("`@("TM9&ES M86)L92UA;'-A*0D)7V%L<V$];F\). SL*("`@("`@("TM96YA8FQE+79O<F)I M<RD)"5]V;W)B:7,]>65S"3L["B`@("`@("`M+61I<V%B;&4M=F] R8FES*0D) M7W9O<F)I<SUN;PD[.PHK("`@("`@+2UE;F%B;&4M9FQA8RD) "5]F;&%C/7EE M<PD[.PHK("`@("`@+2UD:7-A8FQE+69L86,I"0E?9FQA8SUN;PD[. PH@("`@ M("`@+2UE;F%B;&4M;6%D*0D)7VUA9#UY97,). SL*("`@("`@("TM9&ES86)L M92UM860I"0E?;6%D/6YO"0D[.PH@("`@("`@+2UE;F%B; &4M>FQI8BD)"5]Z M;&EB/7EE<PD[.PI`0"`M,C4Q+#8@*S(U-BPQ,2!`0"!F;W(@86-?;W! T:6]N M(&EN("1`.R!D;PH@"59/4D))4U]#1DQ!1U,](BU))%]P<F5F:7@O: 6YC;'5D M92(*(`E63U)"25-?3$E"4STB+4PD7W!R969I>"]L:6(B"B`). SL**R`@("`@ M("TM=VET:"UF;&%C+7!R969I>#TJ*0HK"5]P<F5F:7@]8&5C:&\@) &%C7V]P M=&EO;B!\(&-U="`M9"`G/2<@+68@,F`**PE&3$%#7T-&3$% '4STB+4DD7W!R M969I>"]I;F-L=61E(@HK"49,04-?3$E"4STB+4PD7W!R969I>"]L: 6(B"BL) M.SL*("`@("`@("TM=VET:"UM860M<')E9FEX/2HI"B`)7W! R969I>#U@96-H M;R`D86-?;W!T:6]N('P@8W5T("UD("<])R`M9B`R8`H@"4U!1%] #1DQ!1U,] M(BU))%]P<F5F:7@O:6YC;'5D92(*0$`@+34V-BPV("LU-S8L, C4@0$`@96QS M90H@9FD*(&5C:&\@(B1?=F] R8FES(@H@"BME8VAO8VAE8VL@(D9,04,B"BMI M9B!T97-T("(D7V9L86,B(#T@875T;R`[('1H96X**R`@7V9L86,]; F\**R`@ M8V%T(#X@)%1-4$,@/#P@14]&"BLC:6YC;'5D92`\1DQ!0R] S965K86)L95]S M=')E86U?9&5C;V1E<BYH/@HK:6YT(&UA:6XH=F]I9"D@>R! &3$%#7U]S965K M86)L95]S=')E86U?9&5C;V1E<E]I;FET*"`P("D[(')E='5R;B`P.R!] "BM% M3T8**R`@8V-?8VAE8VL@)$QDQ!1U,@)$-86,04=3("1&3$% #7T-&3$%' M4R`D1DQ!65],24)3(%P**R`@+6Q&3$%#("UL;2`F)B!? 9FQA8SUY97,**V9I M"BMI9B!T97-T("(D7V9L86,B(#T@>65S(#L@=&AE;@HK("!? 9&5F7V9L86,] M)R-D969I;F4@55-%7T9,04,G"BL@($Q)0E,](B1,24)3("1&3$%#7TQ) 0E,@ M+6Q&3$%#(@HK("!)3D-,541%4STB)$E.0TQ51("1&3$% #7T-&3$%'4R(* M*V5L<V4**R`@7V1E9E]F;&%C/2<C=6YD968@55-%7T9,04, G"BMF:0HK96-H M;R`B)%]F;&%C(@HK"B!E8VAO8VAE8VL@(DU! 1"(*(&EF('1E<W0@(B1?;6%D M(B`](&%U=&\@.R!T:&5N"B`@(%]M860];F\*0$`@+3<S. "PV("LW-C<L-R!` M0"!T>7!E9&5F('-I9VYE9"`D='EP95\T7V)Y=&4@:6YT, S(["B`*("\J($QI M8G,@*B\*("1?9&5F7W9O<F)I<PHK)%]D969?9FQA8PH@)%] D969?;6%D"B`D 67V1E9E]A;'-A"B`D7V1E9E]Z;&EB"@`` ` end
comment:26 by , 21 years ago
Hmm, you did notice the "Check to Upload and Attach a File" Checkbox ?
Why do I have to include flac.h before #ifdef USE_FLAC ? Are there problems if files are completely "empty" in terms of C-Statements?
Anyways, its way to early to use my brain, so dont blame if im wrong ;)
comment:27 by , 21 years ago
I see only two buttons: Monitor and Submit. Perhaps you need to be the creator/maintainer to see this button.
To the include things: USE_FLAC doesn't get set by the Makefile or -D. It is set in the config.h file, which gets included by scummsys.h. So you need it to know if FLAC is set or not. If you include your way, you see in the flac. h file that flac.o doesn't depend on config.h. BTW: The vorbis guys include first, too.
What patches, features, documentation is missing to include it to CVS?
Cheers, -Gregor
comment:28 by , 21 years ago
Patches for README and doc/ are missing.
However, don't get me wrong. Even with those pathces, this patch won't be applied at this time.
comment:29 by , 21 years ago
gjasny, patches for README and doc/ are missing.
However, don't get me wrong. Even with that, this patch won't be applied at this time. First we have to branch for 0.6.0.
comment:30 by , 21 years ago
gjasny, patches for README and doc/ are missing.
However, don't get me wrong. Even with that, this patch won't be applied at this time. First we have to branch for 0.6.0.
comment:31 by , 21 years ago
gjasny, patches for README and doc/ are missing.
However, don't get me wrong. Even with that, this patch won't be applied at this time. First we have to branch for 0.6.0.
comment:32 by , 21 years ago
I was curious why the precompiler switches were after the includes.. now I know.
fingolfin: umm... does that mean I have to hack into tex?
comment:33 by , 21 years ago
nolange, no worries, you don't. If you add something for the README, that's fine, we can take it from there into the TeX files easily... even the README stuff isn't "pressing" at this point :-)
BTW, sorry for the quadruple comment, I was having problems with SF.net (well, at that time, not just I was having problems with SF.net :-)
comment:34 by , 21 years ago
OK, so we have branched 0.6.0.
If you update this patch to apply against latest CVS, I can check it in. Thanks!
comment:35 by , 21 years ago
| Owner: | set to |
|---|
comment:36 by , 21 years ago
Done, I also eliminated 2 conflicts in gjasny`s configure-patch. Though I dont understand everything in the configure-script it should do the same as his original Patch. Ill leave the original as well.
comment:37 by , 21 years ago
seems I cant upload the configure patch. Maybe I have to add a comment...
comment:38 by , 21 years ago
No chance, I always get: File Upload: ArtifactFile: File must be > 20 bytes and < 256000 bytes in length
Gonna add it as Text (sorry) --------------------File Start-------------- Index: configure ============================================== ===================== RCS file: /cvsroot/scummvm/scummvm/configure,v retrieving revision 1.70 diff -u -r1.70 configure --- configure 15 Feb 2004 02:48:22 -0000 1.70 +++ configure 16 Feb 2004 12:06:39 -0000 @@ -26,6 +26,7 @@
# default lib behaviour yes/no/auto _vorbis=auto +_flac=auto _mad=auto _alsa=auto _zlib=auto @@ -207,6 +208,9 @@
--with-mad-prefix=PFX Prefix where libmad is installed
(optional)
--disable-mad disable libmad (MP3) support
[autodetect]
+
+ --with-flac-prefix=PFX Prefix where libFLAC is installed
(optional)
+ --disable-flac disable FLAC support [autodetect]
--with-zlib-prefix=PFX Prefix where zlib is installed (optional) --disable-zlib disable zlib (compression) support [autodetect] @@ -243,6 +247,8 @@ --disable-alsa) _alsa=no ;; --enable-vorbis) _vorbis=yes ;; --disable-vorbis) _vorbis=no ;; + --enable-flac) _flac=yes ;; + --disable-flac) _flac=no ;; --enable-mad) _mad=yes ;; --disable-mad) _mad=no ;; --enable-zlib) _zlib=yes ;; @@ -269,6 +275,11 @@ VORBIS_CFLAGS="-I$_prefix/include" VORBIS_LIBS="-L$_prefix/lib" ;; + --with-flac-prefix=*) + _prefix=`echo $ac_option | cut -d '=' -f 2` + FLAC_CFLAGS="-I$_prefix/include" + FLAC_LIBS="-L$_prefix/lib" + ;; --with-mad-prefix=*) _prefix=`echo $ac_option | cut -d '=' -f 2` MAD_CFLAGS="-I$_prefix/include" @@ -622,6 +633,25 @@ fi echo "$_vorbis"
+echocheck "FLAC" +if test "$_flac" = auto ; then + _flac=no + cat > $TMPC << EOF +#include <FLAC/seekable_stream_decoder.h> +int main(void) { FLAC__seekable_stream_decoder_init( 0 ); return 0; } +EOF + cc_check $LDFLAGS $CXXFLAGS $FLAC_CFLAGS $FLAY_LIBS \ + -lFLAC -lm && _flac=yes +fi +if test "$_flac" = yes ; then + _def_flac='#define USE_FLAC' + LIBS="$LIBS $FLAC_LIBS -lFLAC" + INCLUDES="$INCLUDES $FLAC_CFLAGS" +else + _def_flac='#undef USE_FLAC' +fi +echo "$_flac" + # # Check for MAD (MP3 library) # @@ -809,6 +839,7 @@
/* Libs */ $_def_vorbis +$_def_flac $_def_mad $_def_alsa $_def_zlib
comment:39 by , 21 years ago
Sorry but pasting it as text is useless, because it gets line wrapped.
Maybe try putting it into a zip file and attach that?
by , 21 years ago
| Attachment: | configure-flac2.diff added |
|---|
comment:41 by , 21 years ago
Well, I must look like an idiot now after sending you two Mails without attachment. My Browser( which I also use for mails ) refuses to read files from certain directories. Dunno if thats a feature or bug.
comment:42 by , 21 years ago
I dint explicitly notify you that I finally attached the de-conflicted "configure-flac2.diff" Patch yesterday after I found the source of the trouble. Guess you might havent noticed in the mess I created ;)
comment:43 by , 21 years ago
I checked in the "tools" patch now. For the rest, I am fixing some issues with the README patch now (as well as backporting it to readme.tex).
comment:44 by , 21 years ago
Patch is in CVS now!
I changed more spots in the code where our indention rules were not followed (in particular, we always write "foo(x)" and not "foo( x )").
Note #1: that in the english language, nouns usually start with lower caps ("the number of the track" and not "the Number of the Track" besides many other examples :-)
Note #2: Doc comments are appreciated, but I recommend taking a brief look at the doxygen documentation to learn how to take full advantage of them. Alternatively, taking a peek at common/util.h may be helpful.
comment:45 by , 21 years ago
| Status: | new → closed |
|---|
comment:46 by , 6 years ago
| Component: | → Audio |
|---|
oops again, now here are the files