[FFmpeg-devel] [PATCH] QCELP decoder

[Michael Niedermayer]

On Sun, Oct 05, 2008 at 09:18:32AM -0700, Kenan Gillet wrote:
> Hi,
> On Oct 4, 2008, at 5:09 PM, Michael Niedermayer wrote:
> 
> > On Fri, Oct 03, 2008 at 03:48:52PM -0700, Kenan Gillet wrote:
> >> Hi,
> >>
> >> here is a round 2 of the patch based on feedback from Vitor and  
> >> Diego.
> >> It includes:
> >> - some spelling/grammar fixes,
> >> - some cosmetics,
> >> - changes to output float instead of int,
> >> - bug fixes uncovered from the change of output,
> >> - improvements to the pitch pre/synthesis filters.
> >>
> >> Kenan
> > [...]
> >> +typedef enum
> >> +{
> >> +    BLANK = 0,
> >
> > is this supposed to mean silence? if so it should be named accordingly
> 
> I suppose since if encountered the scaled codebook vector must be fill  
> with 0.
> Should i change it to SILENCE ?

yes


> 
> 
> > [...]
> >> +static int qcelp_find_frame_end(const uint8_t *buf, const int  
> >> buf_size) {
> >> +    // Let's try and see if this packet holds exactly one frame
> >> +
> >> +    switch (buf_size) {
> >> +        case 35: // RATE_FULL
> >> +        case 17: // RATE_HALF
> >> +        case  8: // RATE_QUARTER
> >> +        case  4: // RATE_OCTAVE
> >> +
> >> +                 // the first byte describing the type of frame is  
> >> missing.
> >> +                 // TODO: not tested, it needs samples.
> >> +        case 34: // RATE_FULL
> >> +        case 16: // RATE_HALF
> >> +        case  7: // RATE_QUARTER
> >> +        case  3: // RATE_OCTAVE
> >> +            return buf_size;
> >> +
> >> +        case  2:
> >> +        case  1:
> >> +        case  0:
> >> +            return END_NOT_FOUND;
> >> +    }
> >> +
> >> +    /*
> >> +     * If we reach this point it means the packet holds a multiset  
> >> of
> >> +     * frames, each one of them in codec frame format, all with  
> >> the same
> >> +     * framerate, as described in:
> >> +     *
> >> +     * http://tools.ietf.org/html/draft-mckay-qcelp-02
> >> +     *
> >> +     * TODO: not tested, it needs samples.
> >> +     */
> >> +
> >> +    switch (buf[0]) {
> >> +        case RATE_FULL:
> >> +            return 35;
> >> +        case RATE_HALF:
> >> +            return 17;
> >> +        case RATE_QUARTER:
> >> +            return  8;
> >> +        case RATE_OCTAVE:
> >> +            return  4;
> >> +    }
> >> +
> >> +    return END_NOT_FOUND;
> >> +}
> >
> > The code above looks wrong and messy, either
> > A. a parser is needed in which case it must function with any amount  
> > of data,
> >   that is its input may always be a single byte or random pieces of  
> > the
> >   bitstream. (above clearly can not even remotely handle these)
> > B. no parser is needed (in which case this file clearly is unneeded)
> > C. something else is going on, but this requires to be documented and
> >   explained
> 
> I don't really know for sure.
> My guess is that the parser is needed when the container is RTP because
> a packet can contain multiple frame
> http://tools.ietf.org/html/draft-mckay-qcelp-02
> but i have seen/tested such example.
> 
> I suggest we go with B. since 'no parser' is needed for the samples I  
> have.

ok


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

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081005/02e46ade/attachment.pgp>