[Ffmpeg-devel] [PATCH] QTRLE encoder

Michael Niedermayer michaelni
Thu Feb 8 19:57:23 CET 2007


Hi

On Thu, Feb 08, 2007 at 04:22:42PM +0100, Clemens Fruhwirth wrote:
> Hi folks,
> 
> Here is my QT RLE encoder. It produces a original Quicktime compatible
> stream. It's lossless, and it can output encode RGB555/RGB24, but to
> output RGB555 you need this patch from Reimar D?ffinger:
> http://article.gmane.org/gmane.comp.video.ffmpeg.devel/43934
> 
> More information on
> http://clemens.endorphin.org/weblog/archives/2007-02.shtml#e2007-02-08T13_56_24.txt
> ---
> Fruhwirth Clemens - http://clemens.endorphin.org 
> for robots: sp4mtrap at endorphin.org

[...]
> +#define min(a,b) ((a) < (b) ? (a) : (b))

FFMIN


> +
> +/* Maximum RLE codes for bulk copy and repeat */
> +#define MAX_RLE_BULK   127
> +#define MAX_RLE_REPEAT 128

not doxygen compatible comment


> +
> +typedef struct QtrleEncContext {
> +    AVCodecContext *avctx;
> +    uint8_t *previous_frame;
> +    AVFrame frame;
> +    int image_width, image_height;

why duplicate AVCodecContext.width/height?


[...]

> +    uint32_t frame_number;

AVCodecContext.frame_number should be useable


> +    int max_buf_size;

maybe this should be unsigned?


[...]
> +    avctx->bits_per_sample = s->pixel_size*8;
> +    

trailing whitespace is forbidden in svn


[...]
> +static void put_pixel(QtrleEncContext *s, uint8_t *pixpointer, uint8_t **buf)
> +{
> +    switch (s->pixel_size) {
> +    case 2:
> +        bytestream_put_be16(buf,*((uint16_t *)pixpointer));
> +        break;
> +    case 3:
> +        bytestream_put_buffer(buf, pixpointer, s->pixel_size);
> +        break;
> +    default:
> +        /* Internal Error. Die? */
> +        av_log(s->avctx, AV_LOG_ERROR, "Internal Error. Unknown pixel size in qtrleenc.c:put_pixel.\n");
> +        break;
> +    }
> +}

this looks VERY inefficient
pixel_size cannot be anything but 2 or 3 so the check is useless
also bytestream_put_be16(buf,*((uint16_t *)pixpointer)); practically does 
pixel format conversation which is not accpetable in an encoder especially
in such an ineffienct way, the encoder should require big endian rgb555 as
input (if needed the swscaler should be change to support non native endian
rgb555 that way a simply copy could always be used


> +
> +/* Scans two memory stripes in junks for equality */
> +static int scan_equal(uint8_t *a, uint8_t *b, int limit, int junksize)
> +{
> +    int orig_limit=limit;
> +    while(limit && (memcmp(a,b,junksize) == 0)) {
> +	a+=junksize;
> +	b+=junksize;
> +	limit--;
> +    }
> +    return orig_limit-limit;
> +}

tabs are forbidden in svn and again doxygen incompatible comment
also shouldnt it be chunk instead of junk?


> +
> +/* Scans two memory stripes in junks for !equlity */
> +static int scan_unequal(uint8_t *a, uint8_t *b, int limit, int junksize)
> +{
> +    int orig_limit=limit;
> +    while(limit && (memcmp(a,b,junksize) != 0)) {
> +        a+=junksize;
> +        b+=junksize;
> +        limit--;
> +    }
> +    return orig_limit-limit;
> +}

code duplication use ((!memcmp) == scan_equality) and inline for example


[...]

> +
> +static void generate_skipcodes(uint8_t **buf, int skippix)
> +{
> +    while(skippix) {
> +        int skipped=skippix<254?skippix:254;
> +        bytestream_put_byte(buf, skipped+1); // skip code
> +        bytestream_put_byte(buf, 0);         // RLE code 0: another skip code is coming.
> +        skippix-=skipped;
> +    }
> +    (*buf)--;                              // Remove last RLE code announcing a skip code

for(;skippix > 254; skippix-= 254)
    bytestream_put_be16(buf, 0xFF00);
bytestream_put_byte(buf, skippix+1)


[...]

> +    bytestream_put_byte(buf, (signed char)-1); // end RLE line

what is the signed char cast good for?


[...]

> +    bytestream_put_be32(&orig_buf,buf-orig_buf);      // patch the chunk size
> +    return (buf-orig_buf)+4;

AV_WB32() for the patching would avoid the +4


> +}
> +
> +static int qtrle_encode_frame(AVCodecContext *avctx, uint8_t *buf, int buf_size, void *data)
> +{
> +    QtrleEncContext * const s = (QtrleEncContext *)avctx->priv_data;
> +    AVFrame *pict = data;
> +    AVFrame * const p = &s->frame;
> +    int chunksize;
> +
> +    *p = *pict;
> +
> +    if (buf_size < s->max_buf_size) {
> +        /* Upper bound check for compressed data */
> +        av_log(avctx, AV_LOG_ERROR, "buf_size %d <  %d\n", buf_size, s->max_buf_size);
> +        return -1;
> +    }
> +
> +    if (avctx->gop_size == 0 || (s->frame_number % avctx->gop_size) == 0) {
> +        /* I-Frame */
> +        p->pict_type = FF_I_TYPE;
> +        p->key_frame = 1;
> +        chunksize = dump_frame(s, pict, buf, 0);
> +    } else {
> +        /* P-Frame */
> +        p->pict_type = FF_P_TYPE;
> +        p->key_frame = 0;
> +        chunksize = dump_frame(s, pict, buf, 1);
> +    }
> +
> +    /* save the current frame */
> +    memcpy(s->previous_frame, p->data[0], s->image_height*p->linesize[0]);

p->linesize[0] can be > s->image_width*pixel_size


[...]
> Index: libavcodec/qtrle.c
> ===================================================================
> --- libavcodec/qtrle.c	(revision 7881)
> +++ libavcodec/qtrle.c	(working copy)
> @@ -40,6 +40,8 @@
>  #include "common.h"
>  #include "avcodec.h"
>  #include "dsputil.h"
> +#include "bitstream.h"
> +#include "bytestream.h"
>  
>  typedef struct QtrleContext {
>  
> @@ -49,9 +51,11 @@
>  
>      unsigned char *buf;
>      int size;
> +    uint8_t *previous_frame;
>  
>  } QtrleContext;
>  
> +
>  #define CHECK_STREAM_PTR(n) \
>    if ((stream_ptr + n) > s->size) { \
>      av_log (s->avctx, AV_LOG_INFO, "Problem: stream_ptr out of bounds (%d >= %d)\n", \
> @@ -627,4 +631,3 @@
>      qtrle_decode_frame,
>      CODEC_CAP_DR1,
>  };
> -

huh? why do you change qtrle.c


[...]

> ===================================================================
> --- libavcodec/avcodec.h	(revision 7881)
> +++ libavcodec/avcodec.h	(working copy)
> @@ -2273,6 +2273,7 @@
>  extern AVCodec qdraw_decoder;
>  extern AVCodec qpeg_decoder;
>  extern AVCodec qtrle_decoder;
> +extern AVCodec qtrle_encoder;
>  extern AVCodec ra_144_decoder;

please put the encode where all other encoders are


[...]
-- 
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/20070208/0ac1e149/attachment.pgp>



More information about the ffmpeg-devel mailing list