[FFmpeg-devel] MPEG-2 Acceleration Refactor

[John Dalgliesh]

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.

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;

as you can see one of the original lines is left unmodified.

Another way to think about it is to try to minimize the size of (i.e. the 
number of lines changed by) your patch.

>> Other than that, I don't see anything cosmetically wrong, so if
>> anybody
>> submits the patch to closer scrutiny, hopefully few problems will
>> remain.
>
> So should I start a new thread for the patch once there are no more
> problems cosmetically?

I think this thread is fine.

> Cheers,
> Greg

{P^/