[FFmpeg-devel] [PATCH] QCELP decoder

[Diego Biurrun]

On Fri, Oct 03, 2008 at 05:56:04PM -0700, Kenan Gillet wrote:
> On Oct 3, 2008, at 4:38 PM, Diego Biurrun wrote:
> 
> > On Fri, Oct 03, 2008 at 03:48:52PM -0700, Kenan Gillet wrote:
> >>
> >> here is a round 2 of the patch based on feedback from Vitor and  
> >> Diego.
> >>
> >> --- libavcodec/qcelpdata.h	(revision 0)
> >> +++ libavcodec/qcelpdata.h	(revision 0)
> >> @@ -0,0 +1,397 @@
> >> + * Apply pitch synthetis filter and pitch pre-filter to the scaled  
> >> book vector.
> >
> > book vector?
> codebook vector

Then please write just that, "book vector" is not immediately clear.

> >> + * @param QCELPContext q the context
> >
> > stray q?
> would qctx be better ?

The problem is that the parameter is named 'q', not 'QCELPContext', so
you need to use its name, not its type, in the Doxygen comment.

> >> + * Figure out and set it in QCELPContext frame rate by its buffer  
> >> size and/or by looking at
> >> + * the first byte of its buffer if applicable.
> >
> > What does the first 'it' refer to?  I find this explanation very
> > confusing.
> what about
> 
> Determine the framerate from the frame size and/or the first byte of  
> the frame.

much better

> otherwise, we can just rename the function determine_framerate and  
> drop the comment.

Maybe this is the better solution.  Functions should always have
descriptive names of course.

> thanks for the feedback.

Thanks for your efforts to get QCELP finally merged.

Diego

P.S.: Your mails would be more readable if you left an empty line
between the quoted text and your reply, thanks.