[FFmpeg-devel] [PATCH] QCELP decoder
Sat Oct 4 17:18:44 CEST 2008
On Oct 4, 2008, at 1:47 AM, Diego Biurrun wrote:
> 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
>>>> --- 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
>> 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.
done, I also rename rate in QCELPContext to framerate to be more
precise and consistent.
>> thanks for the feedback.
> Thanks for your efforts to get QCELP finally merged.
> P.S.: Your mails would be more readable if you left an empty line
> between the quoted text and your reply, thanks.
thank you for your reviewing :)
More information about the ffmpeg-devel