[FFmpeg-devel] flashsvenc.c - sampling block size too low
Reimar Döffinger
Reimar.Doeffinger
Thu May 17 21:49:54 CEST 2007
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
More information about the ffmpeg-devel
mailing list