[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