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

[Michael Niedermayer]

Hi

On Wed, Apr 04, 2007 at 10:30:29PM +0200, Marco Gerards wrote:
> Michael Niedermayer <michaelni at gmx.at> writes:
> 
> >> >> 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?
> >> >
> >> > stereo in ffmpeg is always
> >> > channel0-sample0, channel1-sample0, channel0-sample1, channel1-sample1, channel0-sample2, channel1-sample2, ...
> >> 
> >> Well, I have this now and it works... kinda.  When I am playing either
> >> one of the two channels as mono sound, it works.  When I am using both
> >> channels, there is some kind of noise/cracks.  The noise is not there
> >> all the time and it is not very loud, but I am mentioning this because
> >> there must still be something that I do wrong...
> >
> > hmm, is AVCodecContext.channels set correctly
> > does it also sound bad if you dump it to a file or let ffmpeg transcode it
> > to pcm wav and then play that ... ?
> 
> The problem was indeed a silly bug in my code.  I increased samples
> once too often, so the output buffer was reported to be one sample
> bigger than it actually was.  I fixed this and all other mistakes you
> found.
> 
> If you do not see any problems with the way I fixed this bug, I think
> this patch is ready to be committed. :-)

almost, i just found a few more minor things :)


[...]

> +        long table[16][2];

why not int? the values stored in it seem to be limited to 16bit ...


[...]
> +        for (ch = 0; ch <= st; ch++) {
> +            samples = (unsigned short *) data + ch;
> +
> +            /* Read in every sample for this channel.  */
> +            for (i = 0; i < samplecnt / 14; i++) {
> +                uint8_t index = get_bits (&gb, 4) & 7;
> +                unsigned int exp = 1 << get_bits (&gb, 4);

storing "log2"(exp) instead of exp would avoid a multiply


> +                signed long factor1 = table[index * 2][ch];

isnt int enough?


> +                signed long factor2 = table[index * 2 + 1][ch];
> +
> +                /* Decode 14 samples.  */
> +                for (n = 0; n < 14; n++) {
> +                    int sampledat = get_sbits (&gb, 4);
> +
> +                    *samples = ((prev1[ch]*factor1 

trailing whitespace


[...]
> +    else {
> +       ret = av_get_packet(pb, pkt, thp->audiosize);
> +       if (ret != thp->audiosize) {
> +          av_free_packet(pkt);
> +          return AVERROR_IO;
> +       }
>  
> +      pkt->stream_index = thp->audio_stream_index;
> +      thp->audiosize = 0;
> +      thp->frame++;
> +    }

indention is not 4 spaces and it seems the existing code in thp.c is also
not consistently indented, somehow i must have missed that ...
this should be fixed 
and of course the existing code reindention should be seperate from functional
changes

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070405/39615956/attachment.pgp>