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

[Michael Niedermayer]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090512/1bbe6531/attachment.pgp>