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

Michael Niedermayer michaelni
Fri May 18 11:16:43 CEST 2007


Hi

On Thu, May 17, 2007 at 03:52:11PM -0500, Jason Askew wrote:
[...]

> +    FlashSVSectionStat *stats;

this is unneeded, you can just parse the current entry of the 2pass stats
in each frame, this would avoid O(frames) memory


[...]
> +    //if this is pass2, parse log file
> +    if(avctx->flags&CODEC_FLAG_PASS2){
> +        int e;
> +
> +        if(avctx->stats_in == NULL) {
> +            av_log(avctx, AV_LOG_ERROR, " Second pass flag and stats_in is NULL.\n");
> +            return -1;
> +        }

this check is not codec specific so it does either belong nowhere
or in a generic place where it would protect all codecs from this
condition


> +
> +        //find the count at the end of stats_in
> +        char* es = strrchr(avctx->stats_in, ':');
> +        if(es==NULL) {
> +            av_log(avctx, AV_LOG_ERROR, " Did not find count at end of log file.\n");
> +            return -1;
> +        }
> +
> +        int count;

mixing of declarations and statements breaks gcc 2.95


[...]
> +        //create array to hold stats data
> +        s->stats = av_mallocz(count * sizeof(FlashSVSectionStat));

missing check for integer overflow


[...]

> +//performs the same actions as copy_region_enc but does not commit zlib data to buffer
> +//and returns the size
> +static int sample_block_size(FlashSVContext *s, AVFrame *p,
> +        int block_width, int block_height, uint8_t *previous_frame, int* I_frame) {
> +
> +    int h_blocks, v_blocks, h_part, v_part, i, j;
> +    //block header amount + block size over head
> +    int blockSize = 6;
> +    int res;
> +
> +    int comBnd = compressBound(block_height * block_width * 3);
> +    uint8_t *buf = av_mallocz(comBnd);
> +
> +    if (!buf) {
> +        av_log(s->avctx, AV_LOG_ERROR, "Memory allocation failed - blocksize compression buffer.\n");
> +        return -1;
> +    }
> +
> +
> +    h_blocks = s->image_width / block_width;
> +    h_part = s->image_width % block_width;
> +    v_blocks = s->image_height / block_height;
> +    v_part = s->image_height % block_height;
> +
> +    /* loop over all block columns */
> +    for (j = 0; j < v_blocks + (v_part?1:0); j++)
> +    {
> +
> +        int hp = j*block_height; // horiz position in frame
> +        int hs = (j<v_blocks)?block_height:v_part; // size of block
> +
> +        /* loop over all block rows */
> +        for (i = 0; i < h_blocks + (h_part?1:0); i++)
> +        {
> +            int wp = i*block_width; // vert position in frame
> +            int ws = (i<h_blocks)?block_width:h_part; // size of block
> +            int ret=Z_OK;
> +
> +            //copy the block to the temp buffer before compression (if it differs from the previous frame's block)
> +            res = copy_region_enc(p->data[0], s->tmpblock, s->image_height-(hp+hs+1), wp, hs, ws, p->linesize[0], previous_frame);
> +
> +            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;
> +            }
> +        }
> +    }
> +    av_free(buf);
> +
> +    //av_log(s->avctx, AV_LOG_ERROR, "block size:  %d                             \n", blockSize);
> +
> +    return blockSize;
> +}

this code is duplicated


[...]
> +            int smallW, smallH;
> +            smallW = 0;
> +            smallH = 0;

the declaration and initalization can be merged



> +            int sizeIndex = 0;
> +            unsigned int smallest = s->tpSizes[0];
> +            for (h=0 ; h<TP_BLCK_SIZE ; h++) {
> +                for (w=0 ; w<TP_BLCK_SIZE ; w++) {
> +                    if (s->tpSizes[sizeIndex] < smallest) {
> +                        smallest = s->tpSizes[sizeIndex];
> +                        smallW = w;
> +                        smallH = h;
> +                    }
> +                    sizeIndex++;
> +                    //av_log(avctx, AV_LOG_ERROR, "[%d][%d]size = %d                       \n",w,h,s->tpSizes[h*TP_BLCK_SIZE+w]);
> +                    s->tpSizes[h*TP_BLCK_SIZE+w] = 0;
> +                    //av_log(avctx, AV_LOG_ERROR, "[%d][%d]size = %d                       \n",w,h,s->tpSizes[h*TP_BLCK_SIZE+w]);
> +                }
> +            }
> +            //av_log(avctx, AV_LOG_INFO, "  [%d][%d] smallest = %d   --------\n",smallW+1,smallH+1,smallest);
> +
> +            s->key_frame_cnt++;
> +            snprintf(avctx->stats_out, 256, "%d:%d:%d\n",smallW+1, smallH+1, s->key_frame_cnt);

smallest= 0;
for(i=0; i<TP_BLCK_SIZE*TP_BLCK_SIZE; i++){
    if(s->tpSizes[i] < s->tpSizes[smallest])
        smallest= i;
    s->tpSizes[i]=0;
}

snprintf(avctx->stats_out, 256, "%d:%d:%d\n", 
        smallest%TP_BLCK_SIZE+1,
        smallest/TP_BLCK_SIZE+1,
        ++s->key_frame_cnt);



[...]
> +    //pull best block size from first past data
> +    if(!(avctx->flags&CODEC_FLAG_PASS2)) {
> +        opt_w=4;
> +        opt_h=4;
> +    } else {

if(avctx->flags&CODEC_FLAG_PASS2) {
}else{

is clearer



> +        if(avctx->frame_number != 0 && I_frame == 1){
> +            s->key_frame_cnt++;
> +        }

there are 2 s->key_frame_cnt++ in the code one under PASS1 one under PASS2
if(), cant they be merged?



> +        //av_log(avctx, AV_LOG_INFO, "  attempting to load frame stats info [%d]\n",s->key_frame_cnt);
> +        if(s->key_frame_cnt < s->stat_count-1) {
> +            opt_w=s->stats[s->key_frame_cnt].blk_size_w;
> +            opt_h=s->stats[s->key_frame_cnt].blk_size_h;
> +        } else {
> +            opt_w=s->stats[s->stat_count-1].blk_size_w;
> +            opt_h=s->stats[s->stat_count-1].blk_size_h;
> +        }

how can s->key_frame_cnt >= s->stat_count-1 ?
if its just for corrupted stats this would be an error not a
"use something random"


> +        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);
> +        }

forgotten debug code?

[...]

> +
> +    if(avctx->flags&CODEC_FLAG_PASS1){
> +        av_free(s->tpSizes);
> +        av_free(avctx->stats_out);
> +    }
> +
> +    if((avctx->flags&CODEC_FLAG_PASS2)){
> +        av_free(s->stats);
> +    }

the if() are unneeded

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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070518/243b939a/attachment.pgp>



More information about the ffmpeg-devel mailing list