[FFmpeg-devel] flashsvenc.c - sampling block size too low

[Reimar Döffinger]

Hello,
On Thu, May 17, 2007 at 01:04:58PM -0500, Jason Askew wrote:
[...]
> +    //if this is pass2, parse log file
> +    if((avctx->flags&CODEC_FLAG_PASS2)){

superflous pair of ()

> +
> +        if(avctx->stats_in == NULL) {
> +            av_log(avctx, AV_LOG_ERROR, " Second pass flag and stats_in is NULL.\n");
> +            return -1;
> +        }
> +
> +        int e;
> +
> +        //find the count at the end of stats_in
> +        char* es = strrchr(avctx->stats_in, ':');

Variable declarations must be at the beginning of a block.

> +        if(es==NULL) {
> +            av_log(avctx, AV_LOG_ERROR, " Did not find count at end of log file.\n");
> +            return -1;
> +        }
> +
> +        int count;
> +        e = sscanf(es, ":%d", &count);
> +        if(e != 1) {
> +            av_log(avctx, AV_LOG_ERROR, " Did not find count at end of log file.\n");
> +            return -1;
> +        }

I'd propose using strtol so you can check that there's actually a number
and not something like 123asaf, which this construction would accept...

> +        int frame, x, y;
> +        char* pNext = avctx->stats_in;

pNext? That looks really ugly to me, why not just next or so?

> +        int statIndex = 0;

In general, in ffmpeg using _ is preferred over (mis)using uppercase to
mark word boundaries.

[...]
> +    //setup of buffer to hold block size stat data
> +    if((avctx->flags&CODEC_FLAG_PASS1)){

another () too much.

> +//performs the same actions as copy_region_enc but does not commit zlib data to buffer
> +//and returns the size

And comments should be doxygen-compatible

> +static int sample_block_size(FlashSVContext *s, AVFrame *p,
> +        int block_width, int block_height, uint8_t *previous_frame, int* I_frame) {
[...]
> +    /* loop over all block columns */
> +    for (j = 0; j < v_blocks + (v_part?1:0); j++)

!!v_part is used in other places instead of (v_part?1:0).

[...]
> +            if (res || *I_frame) {
> +                unsigned long zsize;
> +                zsize = comBnd;
> +
> +                ret = compress2(buf, &zsize, s->tmpblock, 3*ws*hs, 9);
> +
> +                if (ret != Z_OK)
> +                    av_log(s->avctx, AV_LOG_ERROR, "error while compressing block %dx%d\n", i, j);
> +
> +                blockSize += zsize + 2;
> +            } else {
> +                blockSize += 2;
> +            }

That += 2 could factored out, thus avoiding the else. No idea if that
has a real advantage though.

> @@ -238,8 +388,79 @@ static int flashsv_encode_frame(AVCodecC
>          }
>      }
>  
> -    opt_w=4;
> -    opt_h=4;
> +    pfptr = s->previous_frame;
> +    //if linesize is negative, prep pointer to match upside down ptr movement of data[0]
> +    if(p->linesize[0] < 0) {
> +        pfptr = pfptr - ((s->image_height-1) * p->linesize[0]);

I would suggest
pfptr += (s->image_height-1) * -p->linesize[0];
Because 1) + seems more clear to me 2) if image_height happens to be
unsigned, this will fail on 64 bit systems, as I recently had to find
out in MPlayer...

[...]
> +        if(I_frame == 1){
> +            //av_log(avctx, AV_LOG_INFO, "  got %d    %d x %d \n", s->stats[s->key_frame_cnt].frame_num, opt_w, opt_h);
> +        }
> +    }
> +
> +    //calc frame size for different block sizes
> +    if((avctx->flags&CODEC_FLAG_PASS1) && I_frame == 0){

Hmm... can have I_frame have other values than 0 and 1? If yes, use
defines or an enum. If not, use just I_frame and !I_frame.

Greetings,
Reimar D?ffinger