[Ffmpeg-devel] [PATCH] ported SGI decoder to the new API

[Michael Niedermayer]

Hi

On Fri, Mar 30, 2007 at 08:49:37AM +0800, Xiaohui Sun wrote:
> Michael Niedermayer wrote:
> >Hi
> >
> >On Thu, Mar 29, 2007 at 06:54:19PM +0800, Xiaohui Sun wrote:
> >  
> [...]
> 
> >>+    avctx->codec_id = CODEC_ID_SGI;
> >>    
> >
> >uneeded codec_id has to be at that value already
> >
> >[...]
> >  
> >>Index: libavcodec/sgienc.c
> >>===================================================================
> >>--- libavcodec/sgienc.c	(revision 0)
> >>+++ libavcodec/sgienc.c	(revision 0)
> >>@@ -0,0 +1,258 @@
> >>    
> >[...]
> >  
> >>+static int encode_init(AVCodecContext *avctx){
> >>+    SgiContext *s = avctx->priv_data;
> >>+
> >>+    avcodec_get_frame_defaults((AVFrame*)&s->picture);
> >>+    avctx->coded_frame = (AVFrame*)&s->picture;
> >>+
> >>+    return 0;
> >>+}
> >>    
> >
> >this function is still duplicated
> >
> >  
> 
> I think we don't need this function and put these code in 
> encode_frame(), right?

would that remove the duplicated code? if not how should that help?
anyway this one is just a minor nitpick, just keep the duplicate init
if you like


> 
> [...]
> 
> >  
> >>**
> >>+ * RLE compress one line of image, with maximum size of out_size
> >>+ * @param w Image width
> >>+ * @param bpp Bytes per pixel
> >>+ * @param step Image pixel step
> >>+ * @param ptr Input buffer
> >>+ * @param outbuf Output buffer
> >>+ * @param out_size Maximum output size
> >>+ * @return size of output in bytes, or -1 if larger than out_size
> >>+ */
> >>+int encode_rle(int w, int bpp, int step, char* ptr, char* outbuf, int 
> >>out_size)
> >>+{
> >>+    int x, y, count;
> >>+    char* out = outbuf;
> >>+    int span = step * bpp;
> >>+
> >>+    for (x = 0; x < w; x += count * step) {
> >>+        /* see if we can encode the next set of pixels with RLE */
> >>+        if ((count = count_pixels(ptr, w-x, step, bpp, 1)) > 1) {
> >>+            if (out + bpp + 1 > outbuf + out_size) return -1;
> >>+            *out++ = count;
> >>+            if (bpp == 1)
> >>+                *out = *ptr;
> >>+            else
> >>+                memcpy(out, ptr, bpp);
> >>+            out += bpp;
> >>+        } else {
> >>+            /* fall back on uncompressed */
> >>+            count = count_pixels(ptr, w-x, step, bpp, 0);
> >>+            *out++ = 0x80 | count;
> >>+
> >>+            if (out + bpp * count + 1 > outbuf + out_size) return -1;
> >>+            if (step == 1)
> >>+                memcpy(out, ptr, bpp * count);
> >>+            else {
> >>+                for (y = 0; y < count; y++)
> >>+                    memcpy(out + y, ptr + span * y, bpp);
> >>+            }
> >>+            out += bpp * count;
> >>+        }
> >>+        ptr += count * span;
> >>+    }
> >>+    *out++ = 0;
> >>+    return (out - outbuf);
> >>+}
> >>    
> >
> >function is more complex then needed also it needs to have a ff_ prefix
> >
> >
> >  
> 
> May I need to judge the bpp at the beginning of the function ?

there are many possible ways to simplify it, one would be to simply remove
the unneeded checks make the function inline and call it over wraper function
so that gcc can perform the optimizations, another is to write planar rgb into
a intermediate buffer and interleave that then, ideally you should try both
and choose the one which is faster

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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/20070330/7f21f6df/attachment.pgp>