[FFmpeg-devel] [PATCH] V210 decoder and encoder

[Baptiste Coudurier]

On 5/12/2009 11:59 AM, Michael Niedermayer wrote:
> On Mon, May 11, 2009 at 11:37:11PM -0700, Baptiste Coudurier wrote:
>> Hi Ramiro,
>>
>> On 5/11/2009 4:23 PM, Ramiro Polla wrote:
>>> On Mon, May 11, 2009 at 6:41 PM, Baptiste Coudurier
>>> <baptiste.coudurier at gmail.com> wrote:
>>>> On 5/11/2009 1:59 PM, Reimar D?ffinger wrote:
>>>>> On Mon, May 11, 2009 at 12:35:49PM -0700, Baptiste Coudurier wrote:
>>>>>> On 5/11/2009 12:27 PM, Reimar D?ffinger wrote:
>>>>>>>> +            *udst++ = (v & 0x3FF)      <<  6;
>>>>>>>> +            *ydst++ = (v & 0xFFC00)    >>  4;
>>>>>>>> +            *vdst++ = (v & 0x3FF00000) >> 14;
>>>>>>> Both
>>>>>>>
>>>>>>>> +            *udst++ = (v & 0x000003FF) <<  6;
>>>>>>>> +            *ydst++ = (v & 0x000FFC00) >>  4;
>>>>>>>> +            *vdst++ = (v & 0x3FF00000) >> 14;
>>>>>>> and
>>>>>>>
>>>>>>>> +            *udst++ = (v <<  6) & 0xFFC0;
>>>>>>>> +            *ydst++ = (v >>  4) & 0xFFC0;
>>>>>>>> +            *vdst++ = (v >> 14) & 0xFFC0;
>>>>>>> Seem nicer to me.
>>>>>> I don't get what you mean.
>>>>> That I consider both alternatives more readable.
>>>>>
>>>>>>> I think the later one might have a speed advantage due to needing only one
>>>>>>> constant.
>>>>>>> Also like in the encoder you don't really need one of the ands.
>>>>>> You mean ydst >> 14 ? What if the 2 bits are not zero ? Doesn't the bit
>>>>>> gets replicated ?
>>>>> No, the << 6 one, Upper bits are dropped because udst is only 16 bits, lower
>>>>> bits 0 is shifted in. Thus the & is pointless.
>>>> I see what you mean now.
>>>>
>>>> Update attached.
>>>> +            v= le2me_32(*src++);
>>>> +            *udst++ =  v <<  6;
>>>> +            *ydst++ = (v >>  4) & 0xFFC0;
>>>> +            *vdst++ = (v >> 14) & 0xFFC0;
>>>> +
>>>> +            v= le2me_32(*src++);
>>>> +            *ydst++ =  v <<  6;
>>>> +            *udst++ = (v >>  4) & 0xFFC0;
>>>> +            *ydst++ = (v >> 14) & 0xFFC0;
>>>> +
>>>> +            v= le2me_32(*src++);
>>>> +            *vdst++ =  v <<  6;
>>>> +            *ydst++ = (v >>  4) & 0xFFC0;
>>>> +            *udst++ = (v >> 14) & 0xFFC0;
>>>> +
>>>> +            v= le2me_32(*src++);
>>>> +            *ydst++ =  v <<  6;
>>>> +            *vdst++ = (v >>  4) & 0xFFC0;
>>>> +            *ydst++ = (v >> 14) & 0xFFC0;
>>> Maybe:
>>> #define FOOBAR(a, b, c) \
>>> [...]
>>>
>>> FOOBAR(u, v, y)
>>> FOOBAR(y, u, y)
>>> FOOBAR(v, y, u)
>>> FOOBAR(y, v, y)
>>>
>>> And there are some more further down the patch.
>> Why not, updated patch attached.
> [...]
> 
> 
>> +#if CONFIG_V210_DECODER
> [...]
>> +#endif
>> +
>> +#if CONFIG_V210_ENCODER
> 
> if the code is splited in 2 files these become unneeded
> 

All right, split locally, any other comment ?

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org