[FFmpeg-devel] [PATCH] QT RLE header parser refactoring

[Michael Niedermayer]

On Tue, Mar 11, 2008 at 09:44:36AM -0700, Mike Melanson wrote:
> Michael Niedermayer wrote:
> > On Mon, Mar 10, 2008 at 09:01:32PM -0700, Mike Melanson wrote:
> >> Mike Melanson wrote:
> >>> Yeah. I didn't like the idea either, to be honest. :) I have better
> >>> ideas for refactoring I'll try next.
> >> Here's my next idea. This is not a final draft; it's just a prototype to 
> >> see if I'm going in the right direction. For the 8-bit decoding case, I 
> >> moved the header parsing code into the main decode function and passed 3 
> >> extra parameters into the 8-bit decoding function. There's a lot of cruft 
> >> in this one but that would go away in the final draft.
> > 
> > Yes, the header parsing should be factored as well. So yes this is the right
> > direction.
> > 
> > And after that the bpp sould be passed into the function instead of having
> > a dozen functions.
> > 
> > And then the AVPalette stuff should be cleaned up
> > Then the CHECK_STREAM_PTR / CHECK_PIXEL_PTR should be removed and
> > a minimum of checks be implemented cleanly.
> > 
> > :)
> 
> Great. Thanks for the roadmap. I'm not sure about the second step--
> passing the bpp into the function. This implies making 1 unified RLE
> decoder function rather than 7. But each RLE unpacker is slightly
> different for each. Should there be 7 different functions, or 1 function
> that has 7 case statements?

1 function with no case statements.

If you look at the diff betweem 16 and 24:
--- 16	2008-03-11 16:51:44.000000000 +0100
+++ 24	2008-03-11 16:52:31.000000000 +0100
@@ -1,4 +1,4 @@
-static void qtrle_decode_16bpp(QtrleContext *s)
+static void qtrle_decode_24bpp(QtrleContext *s)
 {
     int stream_ptr;
     int header;
@@ -7,7 +7,7 @@
     int rle_code;
     int row_ptr, pixel_ptr;
     int row_inc = s->frame.linesize[0];
-    unsigned short rgb16;
+    unsigned char r, g, b;
     unsigned char *rgb = s->frame.data[0];
     int pixel_limit = s->frame.linesize[0] * s->avctx->height;
 
@@ -38,37 +38,38 @@
     row_ptr = row_inc * start_line;
     while (lines_to_change--) {
         CHECK_STREAM_PTR(2);
-        pixel_ptr = row_ptr + (s->buf[stream_ptr++] - 1) * 2;
+        pixel_ptr = row_ptr + (s->buf[stream_ptr++] - 1) * 3;
 
         while ((rle_code = (signed char)s->buf[stream_ptr++]) != -1) {
             if (rle_code == 0) {
                 /* there's another skip code in the stream */
                 CHECK_STREAM_PTR(1);
-                pixel_ptr += (s->buf[stream_ptr++] - 1) * 2;
+                pixel_ptr += (s->buf[stream_ptr++] - 1) * 3;
                 CHECK_PIXEL_PTR(0);  /* make sure pixel_ptr is positive */
             } else if (rle_code < 0) {
                 /* decode the run length code */
                 rle_code = -rle_code;
-                CHECK_STREAM_PTR(2);
-                rgb16 = AV_RB16(&s->buf[stream_ptr]);
-                stream_ptr += 2;
+                CHECK_STREAM_PTR(3);
+                r = s->buf[stream_ptr++];
+                g = s->buf[stream_ptr++];
+                b = s->buf[stream_ptr++];
 
-                CHECK_PIXEL_PTR(rle_code * 2);
+                CHECK_PIXEL_PTR(rle_code * 3);
 
                 while (rle_code--) {
-                    *(unsigned short *)(&rgb[pixel_ptr]) = rgb16;
-                    pixel_ptr += 2;
+                    rgb[pixel_ptr++] = r;
+                    rgb[pixel_ptr++] = g;
+                    rgb[pixel_ptr++] = b;
                 }
             } else {
-                CHECK_STREAM_PTR(rle_code * 2);
-                CHECK_PIXEL_PTR(rle_code * 2);
+                CHECK_STREAM_PTR(rle_code * 3);
+                CHECK_PIXEL_PTR(rle_code * 3);
 
                 /* copy pixels directly to output */
                 while (rle_code--) {
-                    rgb16 = AV_RB16(&s->buf[stream_ptr]);
-                    stream_ptr += 2;
-                    *(unsigned short *)(&rgb[pixel_ptr]) = rgb16;
-                    pixel_ptr += 2;
+                    rgb[pixel_ptr++] = s->buf[stream_ptr++];
+                    rgb[pixel_ptr++] = s->buf[stream_ptr++];
+                    rgb[pixel_ptr++] = s->buf[stream_ptr++];
                 }
             }
         }
--------
Then you can see that all differences are 2 vs 3 and reading writing 24 vs.
16bit.
The 2/3 can be replaced by an int variable, the moving of 2 vs 3 bytes by

static inline void copy(uint8_t **dst, uint8_t **src, int bytes){
    if(bytes==2)    *(uint16_t*)(*dst)= AV_RB16(*src);
    else            memcpy(*dst, *src, bytes);
    *src+= bytes;
    *dst+= bytes;
}


[...]
-- 
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: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080311/4593c3a8/attachment.pgp>