[FFmpeg-devel] MPEG-2 Acceleration Refactor

[Corey Hickey]

John Dalgliesh wrote:
> On Sun, 17 Jun 2007 at 22:47 -0700, Greg Hulands wrote:
>> Hi Corey,
>> On 17/06/2007, at 10:20 PM, Corey Hickey wrote:
>> [...]
>>
>>>> @@ -1638,7 +1642,10 @@
>>>>          /* special case for the first coef. no need to add a
>>>> second vlc table */
>>>>          UPDATE_CACHE(re, &s->gb);
>>>>          if (((int32_t)GET_CACHE(re, &s->gb)) < 0) {
>>>> -            level= (3*qscale*quant_matrix[0])>>5;
>>>> +            if (speed == DECODE_FAST)
>>>> +               level= (3*qscale)>>1;
>>>> +            else
>>>> +               level= (3*qscale*quant_matrix[0])>>5;
>>> I missed them last time, but I think the quoted line immediately above
>>> should remain at the same indentation so it doesn't look changed.
>>> There
>>> are some other instances like this further down.
>> I think there is a problem with where you are viewing the patch. If
>> you notice that the - at the start of the line is not the same width
>> as the + and it is causing  the alignment problem you are seeing. I
>> just looked at that part of the patch in nano and it looks fine.
>>
>> There is another one in there which is amongst a heap of removals. I
>> think that what has happened is that when I removed the _fast
>> functions that where basically duplicates of the existing ones with
>> minor modifications, diff has been confused. All changes in the code
>> are at the correct indentation levels for the line above.
>>
>> If I have misunderstood, can you please clarify.
> 
> What he is saying is that the indentation of the existing line should not 
> change ... even if it then looks wrong in your patch. This theoretically 
> preserves the history of that line better, although since this is a 
> merging of functions and one of the two lines will always be flagged as 
> changed, IMHO it's not such a big deal.

It's not for the history preservation, as I understand it, but to make
the patch easier to review. I'm sure someone who has reviewed more than
the two (three?) patches I've reviewed can elaborate...

> So again,
> old patch:
> -            level= (3*qscale*quant_matrix[0])>>5;
> +            if (speed == DECODE_FAST)
> +               level= (3*qscale)>>1;
> +            else
> +               level= (3*qscale*quant_matrix[0])>>5;
> new possible patch:
> +            if (speed == DECODE_FAST)
> +            level= (3*qscale)>>1;
> +            else
>                level= (3*qscale*quant_matrix[0])>>5;

Right, but I think you have erred slightly in your example. You mean:
--------------------------------------------------
+            if (speed == DECODE_FAST)
+               level= (3*qscale)>>1;
+            else
             level= (3*qscale*quant_matrix[0])>>5;
--------------------------------------------------

If I'm messing up here, forgive me and I'll go drink some cola. Maybe
I'll do that anyway.

-Corey