[FFmpeg-devel] Request for review

[Michael Niedermayer]

On Mon, Mar 02, 2009 at 03:03:14PM -0800, Roman V. Shaposhnik wrote:
> Well, since nobody's eager to educate me on the finer
> points of rate distortion, here's a practical patch
> that needs reviewing. ;-)
> 
> The objective of this patch is to reduce the number
> of static tables. weigting table is left in place,
> but the unweighting table gets calculated from it
> instead of being statically defined.
> 
> For the DVCPRO HD the formula gives an identical result.
> For everything else I ran an attached script and got
> identical PSNR and STDDEV. Here's the output
> for a single generation:
> 
> stddev:    0.44 PSNR: 55.15 bytes:186624000/186624000
> stddev:    0.44 PSNR: 55.15 bytes:186624000/186624000
> stddev:    0.40 PSNR: 55.99 bytes:186624000/186624000
> stddev:    0.40 PSNR: 55.99 bytes:186624000/186624000
> stddev:    0.46 PSNR: 54.74 bytes:186624000/186624000
> stddev:    0.46 PSNR: 54.74 bytes:186624000/186624000
> stddev:    0.49 PSNR: 54.27 bytes:186624000/186624000
> stddev:    0.49 PSNR: 54.27 bytes:186624000/186624000
> stddev:    0.51 PSNR: 53.96 bytes:186624000/186624000
> stddev:    0.51 PSNR: 53.96 bytes:186624000/186624000
> stddev:    0.50 PSNR: 54.12 bytes: 95178240/ 95178240
> stddev:    0.50 PSNR: 54.12 bytes: 95178240/ 95178240
> stddev:    0.80 PSNR: 50.04 bytes:155520000/155520000
> stddev:    0.80 PSNR: 50.04 bytes:155520000/155520000
> 
> Please let me know if there's anything else I should
> do, or whether it would be ok to commit the changes.
> 
> Thanks,
> Roman.
> 
> P.S. And the video files themselves are from here:
>    http://media.xiph.org/video/derf/


> 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?

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- 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/20090306/28f567c0/attachment.pgp>