[Ffmpeg-devel] [PATCH] flash screen video encoder

[Benjamin Larsson]

Hi, thanks for the review.

The plan is to get a working encoder commited and then later add 2 pass
and optimizations.


Michael Niedermayer wrote:

>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 ?
>  
>

Old cruft, fixed.

>
>  
>
>>+    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()
>  
>

Old code, I switched to or + xor for now.

>
>  
>
>>+            dptr[j] = nsptr[j];
>>+        }
>>+
>>+        dptr += w*3;
>>+    }
>>    
>>
>
>the {} placement of the 2 for loops is inconsistant
>  
>

Fixed.

>
>  
>
>>+    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
>  
>

Well that would be a feature loss(width%4), but the function should be
optimized more. But I think it's good enough for the a first commit.

>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 ...)
>  
>

Yeah, I've been thinking about that but for now my aim is a lossless
encoder.

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

Fixed.

>
>[...]
>  
>
>>     v_part = s->image_height % s->block_height;
>> 
>>+
>>     /* the block size could change between frames, make sure the buffer
>>    
>>
>
>cosmetic
>  
>

Gone.

>
>[...]
>  
>
>>@@ -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
>  
>

Gone.

>
>[...]
>  
>
>>@@ -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 ....
>  
>

Gone. I don't see any reason to use a lower zlevel then 9 on todays
computers.

>
>  
>
>>+    }
>>+
>>+    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 ?
>
>  
>

Added.

>  
>
>>+
>>+    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
>
>  
>

I don't understand the zlib compresison format exactly. I guess that it
starts with a header and then the payload and at the end some kind of
termination sequence. It might be possible to not write that termination
stuff in every compressed block thus the encoder might be able to gain a
few more bytes. So I would prefer to keep that code in there until I get
a multipass encoder working.

>  
>
>>+
>>+    avctx->has_b_frames = 0;
>>    
>>
>
>0 should by default -> this is unneeded
>  
>

Gone.

>
>[...]
>  
>
>>+            if ((res) || (*I_frame)) {
>>    
>>
>
>superflous ()
>  
>

Fixed.

>
>[...]
>  
>
>>+                ptr[0] = 0;
>>+                ptr[1] = 0;
>>+                buf_pos +=2;
>>    
>>
>
>please use bytestream.h
>  
>

Do you mean bytestream_put_be16(ptr,0) ?

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

I prefer it this way as I could extend the I frame decision then. Think
of a frame that all but one block changes. Then it might be of advantage
to encode it as an I frame. The format only allow block changes at I
frames.

>
>[...]
>  
>
>>+    if (!s->previous_frame)
>>+        s->previous_frame = av_mallocz(s->image_width*s->image_height*4);
>>    
>>
>
>linesize vs. image_width
>  
>

Fixed.

>
>[...]
>  
>
>>+#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?
>  
>

I'm quite sure it can be optimized, this code is just template code for
the 2 pass encoder. I think I would like 3 modes for this encoder (1
beeing the default):

1: 1-pass fixed 16x16 block encoder
2: 2-pass 16-96x16-96 block search encoder
3: 2-pass 16-256x16-256 block search encoder

Something like that should be a good compromize between speed and
compression.

>
>  
>
>>+    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
>
>
>  
>

That code isn't used. Another check added.

>  
>
>>+
>>+#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?
>  
>

Removed them.


>
>  
>
>>+#endif
>>+
>>+    //save the current frame
>>+    memcpy(s->previous_frame, p->data[0], s->image_width*s->image_height*4);
>>    
>>
>
>linesize vs. image_width
>  
>

Fixed.

>
>[...]
>  
>
>>+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?
>  
>

Fixed/removed.


>
>[...]
>
>  
>
>>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
>
>  
>

Fixed.

>[...]
>
>  
>
>------------------------------------------------------------------------
>
New patch attached.


MvH
Benjamin Larsson

-- 
new tiny signature

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: flashsv2.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070121/974b5d87/attachment.txt>