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

Baptiste Coudurier baptiste.coudurier
Tue May 12 21:57:14 CEST 2009


On 5/12/2009 12:22 PM, Michael Niedermayer wrote:
> On Tue, May 12, 2009 at 12:22:12PM -0700, Baptiste Coudurier wrote:
>> 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 ?
> 
> no, i think not
> 

Great applied.

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



More information about the ffmpeg-devel mailing list