Hi
On Fri, Jan 19, 2007 at 09:32:34PM +0100, Benjamin Larsson wrote:
> $topic
[...]
> +static int copy_region_enc2(uint8_t *sptr, uint8_t *dptr,
> + int dx, int dy, int h, int w, int stride, uint8_t *pfptr)
> +{
why copy_region_enc2 and not copy_region_enc ?
> + int i,j;
> + uint8_t *nsptr;
> + uint8_t *npfptr;
> + int diff = 0;
>
> + for (i = dx+h; i > dx; i--)
> + {
> + nsptr = sptr+(i*stride)+dy*3;
> + npfptr = pfptr+(i*stride)+dy*3;
> + for (j=0 ; j<w*3 ; j++) {
> + //sum |= npfptr[j]^nsptr[j];
> + if (npfptr[j]!=nsptr[j])
> + diff = 1;
why is sum commented out? a or + xor should be faster then the if()
> + dptr[j] = nsptr[j];
> + }
> +
> + dptr += w*3;
> + }
the {} placement of the 2 for loops is inconsistant
> + if (diff)
> + return 1;
> + return 0;
> +}
i suggest:
int diff = 0;
i = dx+h;
sptr += (i*stride)+dy*3;
pfptr += (i*stride)+dy*3;
for (; i > dx; i--){
for (j=0 ; j<w*3-3; j+=4) {
diff |= *(uint32_t*)(pfptr+j) - *(uint32_t*)(sptr+j);
*(uint32_t*)(dptr+j) = *(uint32_t*)(sptr+j);
}
dptr += w*3;
sptr -= stride;
pfptr -= stride;
}
return !!diff;
and if needed a special case for width%4 != 0 but id just disallow such
width
id also maybe add a maximum difference below which the block is skiped
even if it isnt exactly equal (thats of course just an idea unrelated to
the reviewing ...)
> +
> static int flashsv_decode_init(AVCodecContext *avctx)
> {
> FlashSVContext *s = (FlashSVContext *)avctx->priv_data;
> int zret; // Zlib return code
>
> s->avctx = avctx;
> -#ifdef CONFIG_ZLIB
> s->zstream.zalloc = Z_NULL;
> s->zstream.zfree = Z_NULL;
> s->zstream.opaque = Z_NULL;
> @@ -99,10 +134,7 @@
> av_log(avctx, AV_LOG_ERROR, "Inflate init error: %d\n", zret);
> return 1;
> }
> -#else
> - av_log(avctx, AV_LOG_ERROR, "Zlib support not compiled. Needed for the decoder.\n");
> - return 1;
> -#endif
the #ifdef CONFIG_ZLIB removial should be a seperate change (and as you are
maintainer of the decoder you can of course commit that anytime)
[...]
> v_part = s->image_height % s->block_height;
>
> +
> /* the block size could change between frames, make sure the buffer
cosmetic
[...]
> @@ -166,9 +202,9 @@
> return -1;
> }
>
> - av_log(avctx, AV_LOG_DEBUG, "image: %dx%d block: %dx%d num: %dx%d part: %dx%d\n",
> - s->image_width, s->image_height, s->block_width, s->block_height,
> - h_blocks, v_blocks, h_part, v_part);
> +// av_log(avctx, AV_LOG_DEBUG, "image: %dx%d block: %dx%d num: %dx%d part: %dx%d\n",
> +// s->image_width, s->image_height, s->block_width, s->block_height,
> +// h_blocks, v_blocks, h_part, v_part);
unrelated change
[...]
> @@ -262,6 +296,259 @@
> }
>
>
> +
> +static int flashsv_encode_init(AVCodecContext *avctx)
> +{
> + FlashSVContext *s = (FlashSVContext *)avctx->priv_data;
> + int zret; // Zlib return code
> + int zlvl = 9;
> + int lvl;
> +
> + s->avctx = avctx;
> +// if(avctx->compression_level >= 0)
> + lvl = avctx->compression_level;
> + if(lvl < 0 || lvl > 10){
> + av_log(avctx, AV_LOG_ERROR, "Compression level should be 0-9, not %i\n", lvl);
> + lvl = 9;
please return -1; if the parameters are invalid instead of doing something
random which may not be what the user wants ....
> + }
> +
> + if (avcodec_check_dimensions(avctx, avctx->width, avctx->height) < 0) {
> + return -1;
> + }
here should be a check that width and height are < 4095 as this seems to be
the max which can be encoded ?
> +
> + s->first_frame = 1;
> +
> + // Needed if zlib unused or init aborted before deflateInit
> + memset(&(s->zstream), 0, sizeof(z_stream));
> +/*
> + s->zstream.zalloc = NULL; //av_malloc;
> + s->zstream.zfree = NULL; //av_free;
> + s->zstream.opaque = NULL;
> + zret = deflateInit(&(s->zstream), zlvl);
> + if (zret != Z_OK) {
> + av_log(avctx, AV_LOG_ERROR, "Inflate init error: %d\n", zret);
> + return -1;
> + }
> +*/
why is this commented out? may it be usefull in the future if not remove
it
> +
> + avctx->has_b_frames = 0;
0 should by default -> this is unneeded
[...]
> + if ((res) || (*I_frame)) {
superflous ()
[...]
> + ptr[0] = 0;
> + ptr[1] = 0;
> + buf_pos +=2;
please use bytestream.h
> + }
> + }
> + }
> +
> + if (pred_blocks)
> + *I_frame = 0;
> + else
> + *I_frame = 1;
actually you dont need to keep track of pred_blocks, a simple check for the
buf_size vs. the number of blocks*2 should do i think
[...]
> + if (!s->previous_frame)
> + s->previous_frame = av_mallocz(s->image_width*s->image_height*4);
linesize vs. image_width
[...]
> +#if 0
> + int w, h;
> + int optim_sizes[16][16];
> + int smallest_size;
> + //Try all possible combinations and store the encoded frame sizes
> + for (w=1 ; w<17 ; w++) {
> + for (h=1 ; h<17 ; h++) {
> + optim_sizes[w-1][h-1] = encode_bitstream(s, p, s->encbuffer, s->image_width*s->image_height*4, w*16, h*16, s->previous_frame);
> + //av_log(avctx, AV_LOG_ERROR, "[%d][%d]size = %d\n",w,h,optim_sizes[w-1][h-1]);
> + }
> + }
> +
> + //Search for the smallest framesize and encode the frame with those parameters
> + smallest_size=optim_sizes[0][0];
> + opt_w = 0;
> + opt_h = 0;
> + for (w=0 ; w<16 ; w++) {
> + for (h=0 ; h<16 ; h++) {
> + if (optim_sizes[w][h] < smallest_size) {
> + smallest_size = optim_sizes[w][h];
> + opt_w = w;
> + opt_h = h;
> + }
> + }
> + }
the smallest_size search and the encoding can be done in the same loop
the optim_sizes array becomes unneeded
also my feeling tells me that the gain from rectangular sizes is quite small
and not trying them would lead to some big speedup
another idea is to try the size from the last frame and then try
best+1, best-1 until both are worse or try *2 /2
also 1,2,4,8,16 as a static list of square blocks might provide a good
result?
> + res = encode_bitstream(s, p, buf, buf_size, (opt_w+1)*16, (opt_h+1)*16, s->previous_frame);
> + av_log(avctx, AV_LOG_ERROR, "[%d][%d]optimsize = %d, res = %d|\n", opt_w, opt_h, smallest_size, res);
> +
> + if (buf_size < res)
> + av_log(avctx, AV_LOG_ERROR, "buf_size %d < res %d\n", buf_size, res);
this is unacceptable, please ensure that the buffer end is not overwritten
> +
> +#else
> + opt_w=1;
> + opt_h=1;
> + res = encode_bitstream(s, p, buf, buf_size, opt_w*16, opt_h*16, s->previous_frame, &I_frame);
the opt_w*16, opt_h*16 and s->block_width/s->block_height are somehow
umm, well, the same thing but the contain different values ...
also block_size probably is then wrong though maybe unused?
> +#endif
> +
> + //save the current frame
> + memcpy(s->previous_frame, p->data[0], s->image_width*s->image_height*4);
linesize vs. image_width
[...]
> +static int flashsv_encode_end(AVCodecContext *avctx)
> +{
> + FlashSVContext *s = (FlashSVContext *)avctx->priv_data;
> +
> + deflateEnd(&(s->zstream));
> +
> + /* release the last frame */
> + if (s->prev_frame.data[0])
> + avctx->release_buffer(avctx, &s->prev_frame);
frame, previous_frame, encbuffer and tmpblock leak ...
furthermroe frame and prev_frame look unused?
[...]
> Index: libavcodec/avcodec.h
> ===================================================================
> --- libavcodec/avcodec.h (revision 7403)
> +++ libavcodec/avcodec.h (working copy)
> @@ -2308,6 +2308,7 @@
> extern AVCodec smacker_decoder;
> extern AVCodec smackaud_decoder;
> extern AVCodec kmvc_decoder;
> +extern AVCodec flashsv_encoder;
> extern AVCodec flashsv_decoder;
> extern AVCodec cavs_decoder;
> extern AVCodec vmnc_decoder;
encoder and decoders are sorted please put flashsv_encoder to the other
encoders
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- 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/20070120/2ecf8a6c/attachment.pgp>
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4