[FFmpeg-devel] Request for review

[Roman V Shaposhnik]

On Fri, 2009-03-06 at 20:20 +0100, Michael Niedermayer wrote:
> > Index: libavcodec/dv.c
> > ===================================================================
> > --- libavcodec/dv.c	(revision 17733)
> > +++ libavcodec/dv.c	(working copy)
> > @@ -208,7 +208,7 @@
> >  {
> >      int j,i,c,s,p;
> >      uint32_t *factor1, *factor2;
> > -    const int *iweight1, *iweight2;
> > +    const int *weight1, *weight2;
> >  
> >      if (!d->work_chunks[dv_work_pool_size(d)-1].buf_offset) {
> >          p = i = 0;
> > @@ -232,28 +232,28 @@
> >          factor1 = &d->idct_factor[0];
> >          factor2 = &d->idct_factor[DV_PROFILE_IS_HD(d)?4096:2816];
> >          if (d->height == 720) {
> > -            iweight1 = &dv_iweight_720_y[0];
> > -            iweight2 = &dv_iweight_720_c[0];
> > +            weight1 = &dv_weight_720_y[0];
> > +            weight2 = &dv_weight_720_c[0];
> >          } else {
> > -            iweight1 = &dv_iweight_1080_y[0];
> > -            iweight2 = &dv_iweight_1080_c[0];
> > +            weight1 = &dv_weight_1080_y[0];
> > +            weight2 = &dv_weight_1080_c[0];
> >              }
> >          if (DV_PROFILE_IS_HD(d)) {
> >              for (c = 0; c < 4; c++) {
> >                  for (s = 0; s < 16; s++) {
> >                      for (i = 0; i < 64; i++) {
> > -                        *factor1++ = (dv100_qstep[s] << (c + 9)) * iweight1[i];
> > -                        *factor2++ = (dv100_qstep[s] << (c + 9)) * iweight2[i];
> > +                        *factor1++ = (dv100_qstep[s] << (c + 9)) * ((((uint64_t)2<<dv100_inverse_shift)/weight1[i]+1)>>1); 
> > +                        *factor2++ = (dv100_qstep[s] << (c + 9)) * ((((uint64_t)2<<dv100_inverse_shift)/weight2[i]+1)>>1); 
> >                      }
> >                  }
> >              }
> >          } else {
> > -            iweight1 = &dv_iweight_88[0];
> > -            for (j = 0; j < 2; j++, iweight1 = &dv_iweight_248[0]) {
> > +            weight1 = &dv_weight_88[0];
> > +            for (j = 0; j < 2; j++, weight1 = &dv_weight_248[0]) {
> >                  for (s = 0; s < 22; s++) {
> >                      for (i = c = 0; c < 4; c++) {
> >                          for (; i < dv_quant_areas[c]; i++) {
> > -                            *factor1   = iweight1[i] << (dv_quant_shifts[s][c] + 1);
> > +                            *factor1   = ((((uint64_t)2<<dv_inverse_shift)/weight1[i] + 1) >> 1) << (dv_quant_shifts[s][c] + 1);
> >                              *factor2++ = (*factor1++) << 1;
> >          }
> >      }
> 
> Maybe iam missing something but i think the code is calculating the
> decode dequantization tables, these tables are i suspect normative and
> their content cannot be changed.
> Does this mean the content of these tables stays the same?

You're 100% correct for the DVCPRO HD case. The smpte370 has integer
tables. For the DV/DVCPRO the situation is a bit more complicated.
First, the unweighting table is *not* explicitly specified but
is assumed to be an inverse of a weighting table. The trouble with
the weighting table is that it is specified as products of trigonometric
functions. The precision is not specified.

Thus, what the patch does it tries to follow the spec more closely
than what used to happen -- the unweighting tables are now true
inverse of the weighting ones to extent permitted by the precision.

Thanks,
Roman.