[FFmpeg-devel] [PATCH] encoder for adobe's flash ScreenVideo2 codec

[Diego Biurrun]

On Tue, Jul 21, 2009 at 08:57:15AM -0600, Joshua Warner wrote:
> 
> I've developed an encoder for Adobe's Flash ScreenVideo2 format, which is
> stored in flv files.  My encoder currently only supports a large subset of
> the format.  The only player that supports this codec (so far) is Adobe
> Flash Player itself,

What about writing a decoder first?

Your patch is missing a documentation update.

> --- libavcodec/Makefile	(revision 19479)
> +++ libavcodec/Makefile	(working copy)
> @@ -92,6 +92,7 @@
>  OBJS-$(CONFIG_FLASHSV_DECODER)         += flashsv.o
>  OBJS-$(CONFIG_FLASHSV_ENCODER)         += flashsvenc.o
> +OBJS-$(CONFIG_FLASHSV2_ENCODER)         += flashsv2enc.o

alignment

> --- libavcodec/flashsv2enc.c	(revision 0)
> +++ libavcodec/flashsv2enc.c	(revision 0)
> @@ -0,0 +1,962 @@
> +
> +#include <zlib.h>

If this encoder needs zlib, then you should declare the respective
dependency in configure.

> +typedef struct Block {
> +} Block;
> +
> +typedef struct Pallet {
> +} Pallet;

Ugh, capitalized names..

> +static int generate_default_pallet(Pallet * pallet);

Please reorder the functions so that this forward declaration is
unnecessary.

> +            b->width = (col < s->cols - 1)
> +                || (s->image_width % s->block_width ==
> +                    0) ? s->block_width : s->image_width % s->block_width;
> +            b->height = (row < s->rows - 1)
> +                || (s->image_height % s->block_height ==
> +                    0) ? s->block_height : s->image_height %
> +                s->block_height;

This sure is unreadable.

> +            b->row = row;
> +            b->col = col;
> +            b->enc = encbuf;
> +            b->data = databuf;

vertical alignment

> +static void reset_stats(FlashSV2Context * s)
> +{
> +    s->diff_blocks = 0.1;
> +    s->tot_blocks = 1;
> +    s->diff_lines = 0.1;
> +    s->tot_lines = 1;
> +    s->raw_size = s->comp_size = s->uncomp_size = 10;

ditto

> +    if (avcodec_check_dimensions(avctx, avctx->width, avctx->height) < 0) {
> +        return -1;
> +    }

pointless {}

> +    s->image_width = avctx->width;
> +    s->image_height = avctx->height;
> +
> +    s->block_width = (s->image_width / 12) & ~15;
> +    s->block_height = (s->image_height / 12) & ~15;

vertical alignment

> +    s->rows = (s->image_height + s->block_height - 1) / s->block_height;
> +    s->cols = (s->image_width + s->block_width - 1) / s->block_width;

vertical alignment

> +    s->frame_size = s->image_width * s->image_height * 3;
> +    s->blocks_size = s->rows * s->cols * sizeof(Block);

vertical alignment

> +    s->encbuffer = av_mallocz(s->frame_size);
> +    s->keybuffer = av_mallocz(s->frame_size);
> +    s->databuffer = av_mallocz(s->frame_size * 6);
> +    s->current_frame = av_mallocz(s->frame_size);
> +    s->key_frame = av_mallocz(s->frame_size);
> +    s->frame_blocks = av_mallocz(s->blocks_size);
> +    s->key_blocks = av_mallocz(s->blocks_size);
> +    s->pallet.index = av_mallocz(1 << 15);

vertical alignment

There are more instances below, please prettyprint your code where it
helps readability.

> +static int encode_zlib(Block * b, uint8_t * buf, int *buf_size, int comp)
> +{
> +    int res =
> +        compress2(buf, buf_size, b->sl_begin, b->sl_end - b->sl_begin,
> +                  comp);

weird linebreaks

> +static int encode_bgr(Block * b, uint8_t * src, int stride)
> +{
> +    int i;
> +    uint8_t *ptr = b->enc;
> +    for (i = 0; i < b->start; i++) {
> +        memcpy(ptr + i * b->width * 3, src + i * stride, b->width * 3);
> +    }
> +    b->sl_begin = ptr + i * b->width * 3;
> +    for (; i < b->start + b->len; i++) {
> +        memcpy(ptr + i * b->width * 3, src + i * stride, b->width * 3);
> +    }
> +    b->sl_end = ptr + i * b->width * 3;
> +    for (; i < b->height; i++) {
> +        memcpy(ptr + i * b->width * 3, src + i * stride, b->width * 3);
> +    }

many pointless {}

> +static inline unsigned int chroma_diff(unsigned int c1, unsigned int c2)
> +{
> +    unsigned int t1 =
> +        (c1 & 0x000000ff) + ((c1 & 0x0000ff00) >> 8) +
> +        ((c1 & 0x00ff0000) >> 16);
> +    unsigned int t2 =
> +        (c2 & 0x000000ff) + ((c2 & 0x0000ff00) >> 8) +
> +        ((c2 & 0x00ff0000) >> 16);

again, weird linebreaks

> +static inline int encode_15_7_sl(Pallet * pallet, uint8_t * dest,
> +                                 uint8_t * src, int width, int dist)
> +{
> +    int len = 0, x;
> +    for (x = 0; x < width; x++) {
> +        len += write_pixel_15_7(pallet, dest + len, src + 3 * x, dist);
> +    }

pointless {}

More below, please be consistent and drop them where unnecessary.

> +static int encode_15_7(Pallet * pallet, Block * b, uint8_t * src,
> +                       int stride, int dist)
> +{
> +    int i, len;
> +    uint8_t *ptr = b->enc;
> +    for (i = 0; i < b->start; i++) {
> +        len =
> +            encode_15_7_sl(pallet, ptr, src + i * stride, b->width, dist);

weird linebreak

> +    for (; i < b->start + b->len; i++) {
> +        len =
> +            encode_15_7_sl(pallet, ptr, src + i * stride, b->width, dist);
> +        ptr += len;

again

More below, please change them and earn extra good karma.

> +#define FLASHSV2_DUMB

debug leftover?

Diego