Hi, i extended the ffmpeg vorbis decoder to decode vorbis floor0 files. It is my first try at this kind of stuff. I only had one sample tho testing wasn't too exhaustive. I tested the code on linux x86 and Solaris SPARC so far. As always comments and testers welcome. Have fun, Alex (beastd)
Alexander Strasser wrote:
Hi,
i extended the ffmpeg vorbis decoder to decode vorbis floor0 files. It is my first try at this kind of stuff. I only had one sample tho testing wasn't too exhaustive. I tested the code on linux x86 and Solaris SPARC so far.
As always comments and testers welcome.
Have fun, Alex (beastd)
------------------------------------------------------------------------
[...] + /* allocate mem for lsp coefficients */ + { + /* codebook dim is for padding if codebook dim doesn't * + * divide order+1 then we need to read more data */ + floor_setup->data.t0.lsp= + av_malloc((floor_setup->data.t0.order+1 + max_codebook_dim) * + sizeof(float)); + if( !floor_setup->data.t0.book_list ) { return 1; } + }
Indentation. It's some more also. [...]
@@ -653,6 +734,41 @@
// Process modes part
+static void create_map( vorbis_context * vc, uint_fast8_t mode_number ) +{ + vorbis_floor * floors=vc->floors; + vorbis_floor0 * vf; + int idx; + int_fast32_t * map; + int_fast32_t n; //TODO: could theoretically be smaller?
Why not int ? Anyway the code compiled fine under gcc 3.4.4 on amd64. I don't have any samples to test with so I don't know if it works. MvH Benjamin Larsson -- "incorrect information" is an oxymoron. Information is, by definition, factual, correct.
Hi, Benjamin Larsson wrote:
Alexander Strasser wrote: [...]
[...] + /* allocate mem for lsp coefficients */ + { + /* codebook dim is for padding if codebook dim doesn't * + * divide order+1 then we need to read more data */ + floor_setup->data.t0.lsp= + av_malloc((floor_setup->data.t0.order+1 + max_codebook_dim) * + sizeof(float)); + if( !floor_setup->data.t0.book_list ) { return 1; } + }
Indentation. It's some more also.
Right, there were some more remaining style errors than i thought. ( This happens when you look at your own code too many times ^^" ) Hopefully i caught all of them now; new patch attached.
[...]
@@ -653,6 +734,41 @@
// Process modes part
+static void create_map( vorbis_context * vc, uint_fast8_t mode_number ) +{ + vorbis_floor * floors=vc->floors; + vorbis_floor0 * vf; + int idx; + int_fast32_t * map; + int_fast32_t n; //TODO: could theoretically be smaller?
Why not int ?
The convention in the vorbis decoder is to use the smallest intXY_fast types. This was discussed at the inclusion back then and is inteded to help when porting to environments with lower capablities.
Anyway the code compiled fine under gcc 3.4.4 on amd64. I don't have any samples to test with so I don't know if it works.
Nice to hear. I try to upload my sample somewhere. Alex (beastd)
Alexander Strasser wrote:
Benjamin Larsson wrote: [...]
Indentation. It's some more also.
Right, there were some more remaining style errors than i thought. ( This happens when you look at your own code too many times ^^" ) Hopefully i caught all of them now; new patch attached.
Sorry, really attached now. [...] Alex (beastd)
On Fri, Jan 27, 2006 at 06:14:35PM +0100, Alexander Strasser wrote:
Alexander Strasser wrote:
Benjamin Larsson wrote: [...]
Indentation. It's some more also.
Right, there were some more remaining style errors than i thought. ( This happens when you look at your own code too many times ^^" ) Hopefully i caught all of them now; new patch attached.
Sorry, really attached now.
[...]
+ vorbis_floor_decode_func floor_decode;
You used a single a variable for pointer to decode function. There's no garuntee anywhere in the Vorbis spec that all floors in a single file are the same type. You should make this variable in the floor sturct, and per mapping/floor. Other than that, I like this patch... Only thing missing making FFmpeg Vorbis decoder complete... - ods15
Hi, Oded Shimon wrote:
On Fri, Jan 27, 2006 at 06:14:35PM +0100, Alexander Strasser wrote: [...]
+ vorbis_floor_decode_func floor_decode;
You used a single a variable for pointer to decode function. There's no garuntee anywhere in the Vorbis spec that all floors in a single file are the same type. You should make this variable in the floor sturct, and per mapping/floor.
Yes, i somehow assumed only one type was allowed. Though i yet have to find a file with mixed floor types. Anyway: fixed. [...] Alex (beastd)
On Fri, Feb 03, 2006 at 09:44:48AM +0200, Oded Shimon wrote:
On Fri, Jan 27, 2006 at 06:14:35PM +0100, Alexander Strasser wrote:
Alexander Strasser wrote:
Benjamin Larsson wrote: [...]
Indentation. It's some more also.
Right, there were some more remaining style errors than i thought. ( This happens when you look at your own code too many times ^^" ) Hopefully i caught all of them now; new patch attached.
Sorry, really attached now.
[...]
+ vorbis_floor_decode_func floor_decode;
You used a single a variable for pointer to decode function. There's no garuntee anywhere in the Vorbis spec that all floors in a single file are the same type. You should make this variable in the floor sturct, and per mapping/floor.
Other than that, I like this patch... Only thing missing making FFmpeg Vorbis decoder complete...
s/complete/complete mess/ Anyway, what kind of review was this supposed to be? Highly misleading code: + if(!floor_setup->data.t0.book_list) { return 1; } + /* read book indexes */ + { almost cosmetics which should have been in a seperate patch: - if ((floor_setup->x_list[j]>floor_setup->x_list[k]) && - (floor_setup->x_list[j]<floor_setup->x_list[floor_setup->high_neighbour[k]])) { - floor_setup->high_neighbour[k]=j; + if ((floor_setup->data.t1.x_list[j]>floor_setup->data.t1.x_list[k]) && + (floor_setup->data.t1.x_list[j]<floor_setup->data.t1.x_list[floor_setup->data.t1.high_neighbour[k]])) { + floor_setup->data.t1.high_neighbour[k]=j; strange indention: +# ifdef V_DEBUG + for(idx=0; idx<=n;++idx) { + AV_DEBUG("floor0 map: map at pos %d is %d\n", + idx, map[idx]); + } +# endif +} either ifdef should be moved left or the for right completely unneeded {} + } + + /* allocate mem for lsp coefficients */ + { + /* codebook dim is for padding if codebook dim doesn't * + * divide order+1 then we need to read more data */ + floor_setup->data.t0.lsp= too much on a single line: + do { vec[i]=q; ++i; }while(vf->map[i]==iter_cond); code like: int_fast32_t pow_of_two = 2, exponent = vf->amplitude_bits; while ( --exponent ) { pow_of_two <<= 1; } instead of pow_of_two = 1<<vf->amplitude_bits; And the name pow_of_two is definitly bad as well. and + q=exp( ( + ( (amplitude*vf->amplitude_offset)/ + ((pow_of_two-1) * sqrt(p+q)) ) + - vf->amplitude_offset ) * .11512925f + ); instead of q= pow(10, 0.05*( [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are too smart to engage in politics are punished by being governed by those who are dumber. -- Plato
On Sun, 2008-03-16 at 03:59 +0100, Michael Niedermayer wrote:
code like: int_fast32_t pow_of_two = 2, exponent = vf->amplitude_bits; while ( --exponent ) { pow_of_two <<= 1; } instead of pow_of_two = 1<<vf->amplitude_bits;
While checking why you were replying to a 2 year old post I noticed that this is suspicious in another way - it looks like amplitude_bits could have values up to 63 which would make the shift illegal (and the current code does have a shift instead of loop). The current code has: floor_setup->data.t0.amplitude_bits=get_bits(gb, 6); and later uint_fast32_t amplitude; amplitude=get_bits(&vc->gb, vf->amplitude_bits); q=exp( ( ( (amplitude*vf->amplitude_offset)/ (((1<<vf->amplitude_bits)-1) * sqrt(p+q)) ) - vf->amplitude_offset ) * .11512925f ); If amplitude_bits really can have 6-bit values this code seems wrong, and at least I didn't notice anything which would prevent that.
On Sun, 2008-03-16 at 05:44 +0200, Uoti Urpala wrote:
The current code has:
floor_setup->data.t0.amplitude_bits=get_bits(gb, 6);
and later
uint_fast32_t amplitude;
amplitude=get_bits(&vc->gb, vf->amplitude_bits);
Also this get_bits() call is illegal even for sizes that fit in int, the alt bitstream reader get_bits has a limit of 25 bits.
Alexander Strasser wrote:
Alexander Strasser wrote:
Benjamin Larsson wrote: [...]
Indentation. It's some more also.
Right, there were some more remaining style errors than i thought. ( This happens when you look at your own code too many times ^^" ) Hopefully i caught all of them now; new patch attached.
Sorry, really attached now.
Applied with the modification pointed out by Oded and other small changes. Alex (beastd)
Alexander Strasser wrote:
Benjamin Larsson wrote: [...]
Anyway the code compiled fine under gcc 3.4.4 on amd64. I don't have any samples to test with so I don't know if it works.
Nice to hear. I try to upload my sample somewhere.
http://www.fh-worms.de/~inf150/av_samples/vorbis_floor_type_0.ogg It was provided by a member of this list months ago. Then added to the mplayer sample archive, soon after that mphq got disk problems. I think one can genarate floor 0 files with old versions of Xiph's vorbis encoder. But I did not come to try it yet. Alex (beastd)
Hi,
i extended the ffmpeg vorbis decoder to decode vorbis floor0 files. It is my first try at this kind of stuff. I only had one sample tho testing wasn't too exhaustive. I tested the code on linux x86 and Solaris SPARC so far.
As always comments and testers welcome.
One comment: what about the indentation of the union for type1 in the header file? Anyway, I would say to commit. -- Alex Beregszaszi email: alex at fsn.hu Free Software Network cell: +36 70 3144424
Hi, Alex Beregszaszi wrote:
i extended the ffmpeg vorbis decoder to decode vorbis floor0 files. It is my first try at this kind of stuff. I only had one sample tho testing wasn't too exhaustive. I tested the code on linux x86 and Solaris SPARC so far.
As always comments and testers welcome.
One comment: what about the indentation of the union for type1 in the header file?
Being used to the strict anti-cosmetic rules from MPlayer project i intentionally left it that way, so reviewers could see i did not change anything in the type1 data. I think the don't indent more then 5 lines rule is in ffmpeg doc now too. But i will correct it with another cosmetic-only commit.
Anyway, I would say to commit.
Especially as it doesn't affect the type1 parts (if i didn't mess up, that is), i would like to get it in now too. I think the risk of breaking anything even before the release is low. Objections anyone? Alex (beastd)
On Sun, Jan 29, 2006 at 03:52:19AM +0100, Alexander Strasser wrote:
Alex Beregszaszi wrote:
Anyway, I would say to commit.
Objections anyone?
Nobody objected, I'd say go ahead. It would be great if you could try to produce a few more samples, though. Maybe you could ask on some Xiph/Vorbis list or something? Diego
On Thu, Feb 02, 2006 at 01:07:00PM +0100, Diego Biurrun wrote:
On Sun, Jan 29, 2006 at 03:52:19AM +0100, Alexander Strasser wrote:
Alex Beregszaszi wrote:
Anyway, I would say to commit.
Objections anyone?
Nobody objected, I'd say go ahead. It would be great if you could try to produce a few more samples, though.
http://mplayerhq.hu/~rfelker/01_Duran_Duran_Planet_Earth.ogg I haven't tested this patch yet. - ods15
On Fri, Feb 03, 2006 at 09:55:49AM +0200, Oded Shimon wrote:
On Thu, Feb 02, 2006 at 01:07:00PM +0100, Diego Biurrun wrote:
Nobody objected, I'd say go ahead. It would be great if you could try to produce a few more samples, though.
http://mplayerhq.hu/~rfelker/01_Duran_Duran_Planet_Earth.ogg
403 Forbidden Diego
On Fri, Feb 03, 2006 at 03:03:33PM +0100, Diego Biurrun wrote:
On Fri, Feb 03, 2006 at 09:55:49AM +0200, Oded Shimon wrote:
On Thu, Feb 02, 2006 at 01:07:00PM +0100, Diego Biurrun wrote:
Nobody objected, I'd say go ahead. It would be great if you could try to produce a few more samples, though.
http://mplayerhq.hu/~rfelker/01_Duran_Duran_Planet_Earth.ogg
403 Forbidden
/me kicks dalias Fixed. - ods15
Oded Shimon wrote:
On Fri, Feb 03, 2006 at 03:03:33PM +0100, Diego Biurrun wrote:
On Fri, Feb 03, 2006 at 09:55:49AM +0200, Oded Shimon wrote:
On Thu, Feb 02, 2006 at 01:07:00PM +0100, Diego Biurrun wrote:
Nobody objected, I'd say go ahead. It would be great if you could try to produce a few more samples, though.
http://mplayerhq.hu/~rfelker/01_Duran_Duran_Planet_Earth.ogg
403 Forbidden
/me kicks dalias
Fixed.
Thanks. Tested and seems to work. Alex (beastd)
participants (7)
-
alex＠fsn.hu -
banan＠student.ltu.se -
diego＠biurrun.de -
eclipse7＠gmx.net -
michaelni＠gmx.at -
ods15＠ods15.dyndns.org -
uoti.urpala＠pp1.inet.fi