[FFmpeg-soc] [soc]: r2552 - mlp/mlpdec.c

[Ramiro Polla]

Diego Biurrun wrote:
> On Sun, Jun 22, 2008 at 07:15:10PM +0100, Ramiro Polla wrote:
>> ramiro wrote:
>>> Log:
>>> Copy IIR precision to FIR if only IIR is used, since the filtering code
>>> always uses the FIR precision.
>>>
>>> --- mlp/mlpdec.c	(original)
>>> +++ mlp/mlpdec.c	Sun Jun 22 20:02:56 2008
>>> @@ -685,6 +685,11 @@ static int read_decoding_params(MLPDecod
>>>              }
>>> +            /* Both filters must have the same precision, so the filtering
>>> +             * code always use the FIR precision. If only IIR is used, we copy
>>                                ^^^
>>
>> Oops, typo. Since I'll have to fix this anyways, does anyone have a 
>> better idea for the comment? I don't like the text the way it is now. 
>> It's supposed to mean something like "To simplify the filtering code, 
>> since both precisions are the same, only the FIR precision is used. If 
>> only IIR filtering is used, copy its values to the FIR filter so it can 
>> be used by the filtering code."
> 
> First off: If you want to have a comment on the text, don't cut it off.
> I had to dig out the source file for this review.  That's cumbersome and
> I cannot promise you that I will bother the next time around.

Sorry... But isn't the original svn log+commit good enough? It's just 
one e-mail up. No need to go to the source.

> I think the main problem in the comment is the lack of proper subjects.
> You need too much context to figure out what you are talking about:
> 
> Compare
> 
>   /* Both filters must have the same precision, so the filtering
>    * code always uses the FIR precision. If only IIR is used,
>    * we copy its precision to FIR. */
> 
> with
> 
>   /* The FIR and IIR filters must have the same precision, so the
>    * filtering code always uses the precision of the FIR filter.
>    * If only the IIR filter is used, we copy its precision value
>    * to the FIR filter. */
> 
> which should already be much better.  The following is more verbose but
> hopefully easier to understand.
> 
>   /* The FIR and IIR filters must have the same precision.
>    * To simplify the filtering code, only the precision of the
>    * FIR filter is considered. If only the IIR filter is employed,
>    * the FIR filter precision is set to that of the IIR filter, so
>    * that the filtering code can use it. */

Much better...

> BTW, all of this begs the question why they have two different precision
> values to begin with ...

I'm not sure what you mean by this. Do you mean why the context has 
place for reading the 2 precisions, why they may differ in the 
bitstream, or why the bitstream has both values coded in, or... ?

Ramiro Polla