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

[Michael Niedermayer]

Hi

On Mon, Apr 02, 2007 at 12:05:07AM +0800, Xiaohui Sun wrote:
> Michael Niedermayer wrote:
> >Hi
> >
> >On Sun, Apr 01, 2007 at 08:38:24PM +0800, Xiaohui Sun wrote:
> >[...]
> >  
> >>Index: libavcodec/rle.c
> >>===================================================================
> >>--- libavcodec/rle.c	(revision 0)
> >>+++ libavcodec/rle.c	(revision 0)
> >>@@ -0,0 +1,253 @@
> >>    
> >
> >your rle encoder has as many lines of code as the whole targa encoder
> >this can be done simpler
> >  
> 
> I think I need to optimize for 1bpp and for step encoding situation , 
> which is needed for SGI encoding.

optimization is certainly nice yes but you dont need that much complexity
for it


> 
> [...]
> >
> >[...]
> >  
> >>+            start_offset = bytestream_get_be32(&start_table);
> >>+            len = bytestream_get_be32(&length_table);
> >>+            if(in_buf + start_offset + len > end_buf) {
> >>+                return AVERROR_INVALIDDATA;
> >>+            }
> >>    
> >
> >this check still is not catching all cases, also it seems you 
> >missunderstood
> >me, i didnt mean that you check the values in an otherwise unused table but
> >rather that you properly check the values you do use for overflow
> >
> >
> >  
> 
> you means I should check if the start_offset is overflow, and check 
> against INT32_MAX?

maybe though maybe i missunderstand you again but you definitly should
check the thing so that it wont segfault when dereferenced


> 
> >>+        for (x = s->width; x > 0; x--) {
> >>+            ptr = in_buf;
> >>+            offset = 0;
> >>+            for(z = 0; z < s->depth; z ++) {
> >>+                ptr += offset;
> >>+                bytestream_put_byte(&dest_row, *ptr);
> >>+            }
> >>+            in_buf ++;
> >>+        }
> >>    
> >
> >this is buggy
> >  
> 
> yes, it seems quite ugly, since I want to do a discrete read and a 
> sequential write, so I move the pointer each time.

it is buggy (=not working)


> 
> >
> >[...]
> >  
> >>+        case PIX_FMT_RGB32:
> >>+            dimension = SGI_MULTI_CHAN;
> >>+            depth = SGI_RGBA;
> >>+            break;
> >>    
> >
> >i think this is broken on either little or big endian
> >
> >[...]
> >  
> 
> should I use PIX_FMT_RGBA instead,  which is endian independant.

you should use the correct one,that is the one which works
they are documented in libavutil/avutil.h


> 
> >>+    return (buf - orig_buf);
> >>    
> >
> >superfluous ()
> >  
> I need to calculate the encoded size here.

yes you do but you do not need the ()

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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- 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/20070401/39729e8c/attachment.pgp>