[Ffmpeg-devel] [PATCH] THP PCM decoder (GSoC Qualification)

[Marco Gerards]

Michael Niedermayer <michaelni at gmx.at> writes:

> Hi
>
> On Tue, Apr 03, 2007 at 03:21:56PM +0200, Marco Gerards wrote:
> [...]
>> In this new patch there are still two issues.  For some reason the
>> quality of the sound was not that good.  It wasn't as bad as someone
>> reported, for example he said that there were issues with mono sound.
>> I can not reproduce this.
>> 
>> The problem is in stereo sound.  I have disabled stereo in this patch
>> and the sound is just fine now.  To be honest, I am not sure what the
>> problem is.  I have tried all kinds of things without much success.
>> Perhaps it is better to commit this first.  After that I can fix this
>> with another patch.
>
> hmm did you properly interleave right and left channel samples? that is
> does it work if you decode just the other channel?
> could it be that the encoding is R+L,R-L ? instead of R,L

I tried about anything.  This is the first time I am working with
audio, so I perhaps made a silly mistake.

What I tried was interleaving the data send to the output buffer.  The
PCM data in the THP files is stored in packets.  First x samples for
the first channel are stored, after that x samples for the second
channel.

When I decode the other channel it seems to work as well.  So I assume
the problem is in how I fill the output buffer and how I tell ffmpeg
the format (stereo, interleaved samples) of this data.  How does one
in general deal with stereo from a decoder?

[...]

>> Index: libavcodec/allcodecs.c
>> ===================================================================
>> --- libavcodec/allcodecs.c	(revision 8605)
>> +++ libavcodec/allcodecs.c	(working copy)
>> @@ -242,6 +242,7 @@
>>      REGISTER_ENCDEC (ADPCM_SBPRO_3, adpcm_sbpro_3);
>>      REGISTER_ENCDEC (ADPCM_SBPRO_4, adpcm_sbpro_4);
>>      REGISTER_ENCDEC (ADPCM_SWF, adpcm_swf);
>> +    REGISTER_ENCDEC (ADPCM_THP, adpcm_thp);
>
> ENCDEC ?


Baptiste said "use CODEC and dummy return -1 to stay in conformance
with others (see IMA_QT)".  I assumed it also applied to allcodecs.c.
Should I change this to REGISTER_DECODER?

>>      REGISTER_ENCDEC (ADPCM_XA, adpcm_xa);
>>      REGISTER_ENCDEC (ADPCM_YAMAHA, adpcm_yamaha);
>>  
>> Index: libavcodec/Makefile
>> ===================================================================
>> --- libavcodec/Makefile	(revision 8605)
>> +++ libavcodec/Makefile	(working copy)
>> @@ -246,6 +246,8 @@
>>  OBJS-$(CONFIG_ADPCM_SBPRO_4_ENCODER)   += adpcm.o
>>  OBJS-$(CONFIG_ADPCM_SWF_DECODER)       += adpcm.o
>>  OBJS-$(CONFIG_ADPCM_SWF_ENCODER)       += adpcm.o
>> +OBJS-$(CONFIG_ADPCM_THP_DECODER)       += adpcm.o
>> +OBJS-$(CONFIG_ADPCM_THP_ENCODER)       += adpcm.o
>>  OBJS-$(CONFIG_ADPCM_XA_DECODER)        += adpcm.o
>
> unless you write an ADPCM_THP encoder there shouldnt be
> a CONFIG_ADPCM_THP_ENCODER ...

Same as above.  Should I remove this?

> [...]
>> +        init_get_bits(&gb, src, buf_size * 8);
>> +        src += buf_size;
>> +
>> +                    get_bits(&gb, 32); /* Channel size */
>> +        samplecnt = get_bits(&gb, 32);
>
> get_bits is not guranteed to be able to read 32 bit, use get_bits_long()
> or better yet, use bytestream_get_be32() and similar they are faster and
> simpler for bytealigned stuff

Ok.

--
Marco