[FFmpeg-devel] [PATCH] Add XPM decoder

[Moritz Barsnick]

On Sat, Mar 11, 2017 at 00:34:18 +0530, Paras wrote:
>  OBJS-$(CONFIG_XBM_DECODER)             += xbmdec.o
>  OBJS-$(CONFIG_XBM_ENCODER)             += xbmenc.o
> +OBJS-$(CONFIG_XPM_DECODER)             += xpmdec.o
>  OBJS-$(CONFIG_XFACE_DECODER)           += xfacedec.o xface.o

I do understand that this format is similar to XBM, but this code might
be needed to be ordered alphabetically, IIUC.

>      REGISTER_DECODER(XAN_WC4,           xan_wc4);
>      REGISTER_ENCDEC (XBM,               xbm);
> +    REGISTER_DECODER (XPM,               xpm);
>      REGISTER_ENCDEC (XFACE,             xface);

Same here (and a space too much).

> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -377,6 +377,7 @@ enum AVCodecID {
>      AV_CODEC_ID_XWD,
>      AV_CODEC_ID_CDXL,
>      AV_CODEC_ID_XBM,
> +    AV_CODEC_ID_XPM,
>      AV_CODEC_ID_ZEROCODEC,
>      AV_CODEC_ID_MSS1,
>      AV_CODEC_ID_MSA1,

Did you read the comment at the top of that section?

 * If you add a codec ID to this list, add it so that
 * 1. no value of an existing codec ID changes (that would break ABI),
 * 2. it is as close as possible to similar codecs


> +static int color_table_compare(const void *lhs, const void *rhs)
> +{
> +    return av_strcasecmp(lhs, ((const ColorEntry *)rhs)->name);
> +}
> +
> +static const ColorEntry color_table[] = {
> +    { "AliceBlue",            { 0xF0, 0xF8, 0xFF } },
> +    { "AntiqueWhite",         { 0xFA, 0xEB, 0xD7 } },
> +    { "Aqua",                 { 0x00, 0xFF, 0xFF } },
[...]

Is this duplicated from libavutil/parseutils.c?

> +static uint32_t hexstring_to_rgba(const char *p, int len){
> +    uint32_t ret = 0xFF000000;
> +    const ColorEntry *entry;
> +    char color_name[100];
> +
> +    if(*p == '#'){
> +        p++;
> +        len--;
> +        if (len == 3) {
> +            ret |= (convert(p[2]) <<  4) |
> +                   (convert(p[1]) << 12) |
> +                   (convert(p[0]) << 20);

So is this a modified or redesigned av_parse_color()? Just wondering.

> +    else{

If this is new code, and not copied verbatim from somewhere else, you
need to follow coding style.

> +        av_log(avctx, AV_LOG_ERROR, "invalid number of colors or chars per pixel\n");

Feel free to quote actual offending values...

> +        av_log(avctx, AV_LOG_ERROR, "unsupported number of chars per pixel\n");
> +        return AVERROR_PATCHWELCOME;

...especially when asking for a patch.

Sorry for nitpicking without looking at the actual code.

Moritz