[FFmpeg-devel] [PATCH]Basic XSUB encoder

[Diego Biurrun]

On Sun, Feb 01, 2009 at 11:39:46PM +0100, Bj?rn Axelsson wrote:
> 
> The patch passes make test but make checkheaders fails on alsa-audio.h
> which I doubt is my fault...

No, not your fault; I'll fix it in a moment.

> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ libavcodec/xsubenc.c	2009-02-01 23:22:13.000000000 +0100
> @@ -0,0 +1,223 @@
> +/*
> + * DivX subtitle encoding for ffmpeg

DivX subtitle encoder

> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.

This is not a standard license header.

> +static void xsub_encode_rle(uint8_t **pq,
> +                            const uint8_t *bitmap, int linesize,
> +                            int w, int h)

nit: This should fit on two lines.

> +            // Run can't be longer than 255, unless it is the rest of a row
> +            if(x1 < w && len > 255) {
> +                int diff = len - 255;

Doesn't this generate a warning about mixed declarations and statements?

> +    for(i=0; i<3; i++) {

if/for are inconsistently formatted throughout, I recommend K&R style,
i.e. a space after the keyword and before the opening {.

> +static int encode_xsub_subtitles(AVCodecContext *avctx, uint8_t *outbuf, AVSubtitle *h)

long line

> +    uint8_t *hdr = outbuf;
> +    uint8_t *rlelenptr;
> +    uint8_t *q;
> +    uint8_t *resized_bitmap;
> +
> +    uint16_t width;
> +    uint16_t height;

Maybe put these on one line, not important..

> +    if (h->rects[0]->nb_colors > 4)
> +        av_log(avctx, AV_LOG_WARNING, "Max 4 subtitle colors supported (%d found)\n", h->rects[0]->nb_colors);

Let's talk to users in something more closely resembling English:

"No more than 4 subtitle colors supported (%d found).\n"

> +    // Width and height must be powers of 2
> +    // 2 pixels required on either side of subtitle

Periods will help readability here.

> +static int xsub_init_encoder(AVCodecContext *avctx)
> +{
> +}
> +
> +static int xsub_close_encoder(AVCodecContext *avctx)
> +{
> +}

av_cold

> +AVCodec xsub_encoder = {
> +    "xsub",
> +    CODEC_TYPE_SUBTITLE,
> +    CODEC_ID_XSUB,
> +    0,
> +    xsub_init_encoder,
> +    xsub_encode,
> +    xsub_close_encoder,
> +};

long_name is missing.

> Index: libavcodec/allcodecs.c
> ===================================================================
> --- libavcodec/allcodecs.c.orig	2009-02-01 22:22:51.000000000 +0100
> +++ libavcodec/allcodecs.c	2009-02-01 22:31:44.000000000 +0100
> @@ -174,7 +174,6 @@
>      REGISTER_DECODER (WNV1, wnv1);
>      REGISTER_DECODER (XAN_WC3, xan_wc3);
>      REGISTER_DECODER (XL, xl);
> -    REGISTER_DECODER (XSUB, xsub);
>      REGISTER_ENCDEC  (ZLIB, zlib);
>      REGISTER_ENCDEC  (ZMBV, zmbv);

This entry is misplaced.  That's unrelated to your patch of course..

Diego