[FFmpeg-devel] Request for review

[Michael Niedermayer]

On Wed, Jan 14, 2009 at 12:42:36PM -0800, Roman V. Shaposhnik wrote:
> Guys,
> 
> this bunch of patches is a preliminary work that needs to
> be done in order for DVCPRO HD encoding to be integrated.
> 
> Please review and comment:
>     dv1.patch -- simply replaces the # of macro blocks constants 
>                  with tables

ok


>     dv3.patch -- is pure cosmetics

ok


also i assume this has been benchmarked and no meassureable speeddifference or
its faster after the patches?


[...]


> @@ -1015,9 +1033,8 @@ static int dv_encode_video_segment(AVCodecContext *avctx, DVwork_chunk *work_chu
>                      linesize = s->picture.linesize[6 - j];
>                  } else {

>                      /* j=1 and j=3 are "dummy" blocks, used for AC data only */
> -                    data     = 0;
> +                    data     = NULL;

i really think you should just commit such trivial changes


>                      linesize = 0;
> -                    dummy    = 1;
>                  }
>              } else { /* 4:1:1 or 4:2:0 */
>                  if (j < 4) {  /* Four Y blocks */
> @@ -1032,56 +1049,28 @@ static int dv_encode_video_segment(AVCodecContext *avctx, DVwork_chunk *work_chu
>                      /* don't ask Fabrice why they inverted Cb and Cr ! */
>                      data     = s->picture.data    [6 - j] + c_offset;
>                      linesize = s->picture.linesize[6 - j];
> -                    if (s->sys->pix_fmt == PIX_FMT_YUV411P && mb_x >= (704 / 8))
> -                        do_edge_wrap = 1;
> -                }
> -            }
> -

> -            /* Everything is set up -- now just copy data -> DCT block */
> -            if (do_edge_wrap) {  /* Edge wrap copy: 4x16 -> 8x8 */
> -                uint8_t* d;
> -                DCTELEM *b = block;
> -                for (i = 0; i < 8; i++) {
> -                   d = data + 8 * linesize;
> -                   b[0] = data[0]; b[1] = data[1]; b[2] = data[2]; b[3] = data[3];
> -                   b[4] =    d[0]; b[5] =    d[1]; b[6] =    d[2]; b[7] =    d[3];
> -                   data += linesize;
> -                   b += 8;
> +                    if (s->sys->pix_fmt == PIX_FMT_YUV411P && mb_x >= (704 / 8)) {
> +                        uint8_t* b = scratch;
> +                        for (i = 0; i < 8; i++) {
> +                            uint8_t* d = data + 8 * linesize;
> +                            b[0] = data[0]; b[1] = data[1]; b[2] = data[2]; b[3] = data[3];
> +                            b[4] =    d[0]; b[5] =    d[1]; b[6] =    d[2]; b[7] =    d[3];
> +                            data += linesize;
> +                            b += 8;
> +                        }
> +                        data = scratch;
> +                        linesize = 8;
> +                    }
>

I would prefer if you did not mix indention changes with changes that move
code around, it makes such a patch hard to review.
(and indention corrections are always ok anyway so you dont need to post
 any patch for them, just commit indention fixes)


[...]
> +        /* Second pass over each MB space */
> +        pb = &pbs[start_mb];

>          for (i=0; i<s->sys->bpm; i++) {
> -            if (enc_blks[i+j].partial_bit_count)
> -                pb = dv_encode_ac(&enc_blks[i+j], pb, &pbs[j+s->sys->bpm]);
> +            if (enc_blks[start_mb+i].partial_bit_count)
> +               pb = dv_encode_ac(&enc_blks[start_mb+i], pb, &pbs[start_mb+s->sys->bpm]);

the indention is off by 1 byte


>          }
>      }
> -
>      /* Third and final pass over the whole video segment space */
>      pb = &pbs[0];
>      for (j=0; j<5*s->sys->bpm; j++) {

cosmetic


[...]
> @@ -1010,61 +1009,67 @@ static int dv_encode_video_segment(AVCodecContext *avctx, DVwork_chunk *work_chu
>      int       vs_bit_size = 0;
>      int       qnos[5];
>      int*      qnosp = &qnos[0]; 
> +    const int log2_blocksize = 3-s->avctx->lowres;

lowres is 0 during encoding

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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/20090117/38aa4046/attachment.pgp>