[FFmpeg-devel] [PATCH] QCELP decoder

[Kenan Gillet]

Hi,
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
>>>> 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.

done


>>>> + * @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.
>

done

>>>> + * 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

done


>> 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.
>
> Diego
>
> 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 :)

Kenan