[FFmpeg-devel] Request for review

[Roman V. Shaposhnik]

Hi Michael!

Thanks a lot for reviewing #1, #3.
Btw, are you still reviewing #2, #4 and #5 or do
you want me to resubmit them with the issues you've
identifies so far fixed?

On Sat, 2009-01-17 at 02:45 +0100, Michael Niedermayer wrote:
> also i assume this has been benchmarked and no meassureable speeddifference or
> its faster after the patches?

Well, on a single architecture -- but yes. If there willing
developers, I'd be delighted to see benchmarking results on
arcs that I don't have as well.

> > -            /* 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)

Sure. Do you want me to resubmit #2 for your review without
the indentation problems, or are you fine with it as it
is (provided that I'll take care of your comment and commit
two separate transasctions)?

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

Wow! Don't know what happened. :-( Thanks for catching it!

> [...]
> > @@ -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

Got it. I'll resubmit #5 in a couple of hours.

Thanks,
Roman.