[FFmpeg-devel] Patch review request: 10bit DNxHD decoding support

[Joseph Artsimovich]

> >      p8idct(block, temp, NULL,         0, 1, 8, 0);
> >      p8idct(NULL , temp, dest, line_size, 8, 1, 3);  }
> > +
> > +void ff_faanidct10_put(uint16_t *dest, int line_size, DCTELEM block[64]){
> > +    FLOAT temp[64];
> > +    FLOAT* p_tmp = temp;
> > +    int i, j;
> > +
> > +    emms_c();
> 
> this should be moved out of the inner loop of the codec
Do you mean emms_c() should be moved out of this function?  You are not talking about the local variables I presume.  Well, I just copied that bit from ff_faandct().  Should that one be removed as well?

> > +
> > +    for(i=0; i<64; i++)
> > +        temp[i] = block[i] * prescale[i];
> > +
> > +    p8idct(NULL, temp, NULL,         0, 1, 8, 0);
> > +    p8idct(NULL, temp, NULL,         0, 8, 1, 0);
> > +
> > +    for (i = 0; i < 8; ++i) {
> > +        for (j = 0; j < 8; ++j) {
> > +            int val = (int)lrintf((float)p_tmp[j] * (65536.0f /
> > + 1023.0f) + 0.5f);
> 
> the scaling can be merged with prescale and +0.5 looks wrong
I have to say lrintf type of functions scare me and I normally never use them.  The "current rounding direction" sounds like undefined behaviour to me.  Is the default rounding direction always "towards zero" or does it keep changing?  And yes, +0.5 would be wrong for "towards zero".  It would be correct for "towards negative infinity".  I must have been thinking about putting floor() rather than lrintf() there.


Joseph Artsimovich
Software Developer
MirriAd Ltd