[FFmpeg-devel] [PATCH] Add XPM decoder

Nicolas George george at nsup.org
Sun Mar 12 21:04:31 EET 2017


Le duodi 22 ventôse, an CCXXV, Paras Chadha a écrit :
> Xpm decoder was added
> Also added xpm_pipe demuxer with its probe function
> 
> Signed-off-by: Paras Chadha <paraschadha18 at gmail.com>
> ---
>  Changelog                |   1 +
>  doc/general.texi         |   2 +
>  libavcodec/Makefile      |   1 +
>  libavcodec/allcodecs.c   |   1 +
>  libavcodec/avcodec.h     |   1 +
>  libavcodec/codec_desc.c  |   7 +
>  libavcodec/version.h     |   4 +-
>  libavcodec/xpmdec.c      | 426 +++++++++++++++++++++++++++++++++++++++++++++++
>  libavformat/Makefile     |   1 +
>  libavformat/allformats.c |   1 +
>  libavformat/img2.c       |   1 +
>  libavformat/img2dec.c    |  10 ++
>  12 files changed, 454 insertions(+), 2 deletions(-)
>  create mode 100644 libavcodec/xpmdec.c
> 
> diff --git a/Changelog b/Changelog
> index 13628ca..716b6ff 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -26,6 +26,7 @@ version <next>:
>  - native Opus encoder
>  - ScreenPressor decoder
>  - incomplete ClearVideo decoder
> +- XPM decoder
> 
>  version 3.2:
>  - libopenmpt demuxer
> diff --git a/doc/general.texi b/doc/general.texi
> index 30450c0..83f54b3 100644
> --- a/doc/general.texi
> +++ b/doc/general.texi
> @@ -607,6 +607,8 @@ following image formats are supported:
>      @tab WebP image format, encoding supported through external library libwebp
>  @item XBM  @tab X @tab X
>      @tab X BitMap image format
> + at item XPM  @tab X @tab X
> +    @tab X PixMap image format
>  @item XFace @tab X @tab X
>      @tab X-Face image format
>  @item XWD  @tab X @tab X
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 65ccbad..b8d7a00 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -650,6 +650,7 @@ OBJS-$(CONFIG_XFACE_ENCODER)           += xfaceenc.o xface.o
>  OBJS-$(CONFIG_XL_DECODER)              += xl.o
>  OBJS-$(CONFIG_XMA1_DECODER)            += wmaprodec.o wma.o wma_common.o
>  OBJS-$(CONFIG_XMA2_DECODER)            += wmaprodec.o wma.o wma_common.o
> +OBJS-$(CONFIG_XPM_DECODER)             += xpmdec.o
>  OBJS-$(CONFIG_XSUB_DECODER)            += xsubdec.o
>  OBJS-$(CONFIG_XSUB_ENCODER)            += xsubenc.o
>  OBJS-$(CONFIG_XWD_DECODER)             += xwddec.o
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index 074efd4..b7d03ad 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -378,6 +378,7 @@ static void register_all(void)
>      REGISTER_ENCDEC (XBM,               xbm);
>      REGISTER_ENCDEC (XFACE,             xface);
>      REGISTER_DECODER(XL,                xl);
> +    REGISTER_DECODER(XPM,               xpm);
>      REGISTER_ENCDEC (XWD,               xwd);
>      REGISTER_ENCDEC (Y41P,              y41p);
>      REGISTER_DECODER(YLC,               ylc);
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 30ac236..e32f579 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -439,6 +439,7 @@ enum AVCodecID {
>      AV_CODEC_ID_FMVC,
>      AV_CODEC_ID_SCPR,
>      AV_CODEC_ID_CLEARVIDEO,
> +    AV_CODEC_ID_XPM,
> 
>      /* various PCM "codecs" */
>      AV_CODEC_ID_FIRST_AUDIO = 0x10000,     ///< A dummy id pointing at the start of audio codecs
> diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
> index 06bcfc3..88cfddb 100644
> --- a/libavcodec/codec_desc.c
> +++ b/libavcodec/codec_desc.c
> @@ -1591,6 +1591,13 @@ static const AVCodecDescriptor codec_descriptors[] = {
>          .props     = AV_CODEC_PROP_INTRA_ONLY | AV_CODEC_PROP_LOSSLESS,
>      },
>      {
> +        .id        = AV_CODEC_ID_XPM,
> +        .type      = AVMEDIA_TYPE_VIDEO,
> +        .name      = "xpm",
> +        .long_name = NULL_IF_CONFIG_SMALL("XPM (X PixMap) image"),
> +        .props     = AV_CODEC_PROP_INTRA_ONLY | AV_CODEC_PROP_LOSSLESS,
> +    },
> +    {
>          .id        = AV_CODEC_ID_XWD,
>          .type      = AVMEDIA_TYPE_VIDEO,
>          .name      = "xwd",
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index b00e011..3ed5a71 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -28,8 +28,8 @@
>  #include "libavutil/version.h"
> 
>  #define LIBAVCODEC_VERSION_MAJOR  57
> -#define LIBAVCODEC_VERSION_MINOR  82
> -#define LIBAVCODEC_VERSION_MICRO 102
> +#define LIBAVCODEC_VERSION_MINOR  83
> +#define LIBAVCODEC_VERSION_MICRO 100
> 
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>                                                 LIBAVCODEC_VERSION_MINOR, \
> diff --git a/libavcodec/xpmdec.c b/libavcodec/xpmdec.c
> new file mode 100644
> index 0000000..7a1f4e1
> --- /dev/null
> +++ b/libavcodec/xpmdec.c
> @@ -0,0 +1,426 @@
> +/*
> + * XPM image format
> + *
> + * Copyright (c) 2012 Paul B Mahol
> + * Copyright (c) 2017 Paras Chadha
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg 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.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "libavutil/parseutils.h"
> +#include "libavutil/avstring.h"
> +#include "avcodec.h"
> +#include "internal.h"
> +
> +typedef struct XPMContext {

> +    uint32_t  *pixels;
> +    int      pixels_size;

The spacing is strange.

> +} XPMDecContext;
> +
> +typedef struct ColorEntry {
> +    const char *name;            ///< a string representing the name of the color
> +    uint32_t    rgb_color;    ///< RGB values for the color
> +} ColorEntry;
> +
> +static int color_table_compare(const void *lhs, const void *rhs)
> +{
> +    return av_strcasecmp(lhs, ((const ColorEntry *)rhs)->name);
> +}
> +
> +static const ColorEntry color_table[] = {

<snip>

The code duplication with parseutils is unacceptable, and I find the
reasons given by Paul weak. In particular, I do not see any conflict
with the database on my X.org version.

> +};
> +

> +static int convert(uint8_t x)

convert is not a very good name.

> +{

> +    if (x >= 'a') {
> +        x -= 87;
> +    } else if (x >= 'A') {
> +        x -= 55;
> +    } else {

Avoid magic numbers in the code; x - 87 = x - 'a' + 10,
x - 55 = x - 'A' + 10, and "& ~32" can avoid making two cases anyway.

> +        x -= '0';
> +    }
> +    return x;
> +}
> +

> +/*
> +**  functions same as strcspn but ignores characters in reject if they are inside a C style comment...
> +**  @param string, reject - same as that of strcspn
> +**  @return length till any character in reject does not occur in string
> +*/

This is not a valid Doxygen comment.

> +static size_t mod_strcspn(const char *string, const char *reject)
> +{
> +    int i, j;
> +

> +    for (i = 0; string && string[i]; i++) {

The first clause of the condition is silly.

> +        if (string[i] == '/' && string[i+1] == '*') {
> +            i += 2;

> +            while ( string && string[i] && (string[i] != '*' || string[i+1] != '/') )

Nit: no spaces within parentheses. And ditto for the first clause.

> +                i++;

> +            i++;

If the loop exits due to the "string[i]" part, this leaves I beyond the
end of the string, causing an illegal access on the next rounds.

> +        } else if (string[i] == '/' && string[i+1] == '/') {
> +            i += 2;

> +            while ( string && string[i] && string[i] != '\n' )

Ditto for the first clause.

> +                i++;
> +        } else {

> +            for (j = 0; reject && reject[j]; j++) {
> +                if (string[i] == reject[j])
> +                    break;

Use strchr().

> +            }
> +            if (reject && reject[j])
> +                break;
> +        }
> +    }
> +    return i;
> +}
> +

> +static uint32_t hexstring_to_rgba(const char *p, int len)

This is a misnomer.

> +{
> +    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);
> +        } else if (len == 4) {
> +            ret  = (convert(p[3]) <<  4) |
> +                   (convert(p[2]) << 12) |
> +                   (convert(p[1]) << 20) |
> +                   (convert(p[0]) << 28);
> +        } else if (len == 6) {
> +            ret |=  convert(p[5])        |
> +                   (convert(p[4]) <<  4) |
> +                   (convert(p[3]) <<  8) |
> +                   (convert(p[2]) << 12) |
> +                   (convert(p[1]) << 16) |
> +                   (convert(p[0]) << 20);
> +        } else if (len == 8) {
> +            ret  =  convert(p[7])        |
> +                   (convert(p[6]) <<  4) |
> +                   (convert(p[5]) <<  8) |
> +                   (convert(p[4]) << 12) |
> +                   (convert(p[3]) << 16) |
> +                   (convert(p[2]) << 20) |
> +                   (convert(p[1]) << 24) |
> +                   (convert(p[0]) << 28);
> +        }
> +    } else {

> +        strncpy(color_name, p, len);
> +        color_name[len] = '\0';

This is completely wrong.

> +
> +        entry = bsearch(color_name,
> +                        color_table,
> +                        (sizeof(color_table)/sizeof(color_table[0])),
> +                        sizeof(ColorEntry),
> +                        color_table_compare);
> +
> +        if (!entry)
> +            return ret;
> +
> +        ret = entry->rgb_color;
> +    }

> +    return ret;

Is it specified somewhere that unknown colors should yield black?
Otherwise, an error code should be returned.

Also, you forgot to parse colors in standard X11 scheme
"rgb:RRRR/GGGG/BBBB".

> +}
> +
> +static int ascii2index(const uint8_t *cpixel, int cpp)
> +{
> +    const uint8_t *p = cpixel;
> +    int n = 0, m = 1, i;
> +
> +    for (i = 0; i < cpp; i++) {
> +        if (*p < ' ' || *p > '~')
> +            return AVERROR_INVALIDDATA;
> +        n += (*p++ - ' ') * m;
> +        m *= 95;
> +    }
> +    return n;
> +}
> +
> +static int xpm_decode_frame(AVCodecContext *avctx, void *data,
> +                            int *got_frame, AVPacket *avpkt)
> +{
> +    XPMDecContext *x = avctx->priv_data;
> +    AVFrame *p=data;
> +    const uint8_t *end, *ptr = avpkt->data;
> +    int ncolors, cpp, ret, i, j;
> +    int64_t size;
> +    uint32_t *dst;
> +
> +    avctx->pix_fmt = AV_PIX_FMT_BGRA;
> +
> +    end = avpkt->data + avpkt->size;
> +    if (memcmp(ptr, "/* XPM */", 9)) {
> +        av_log(avctx, AV_LOG_ERROR, "missing signature\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +

> +    ptr += mod_strcspn(ptr, "\"");

If I read this correctly, you are skipping random characters until a
quote is found. This is not how a robust parser should be written.


> +    if (sscanf(ptr, "\"%u %u %u %u\",",
> +               &avctx->width, &avctx->height, &ncolors, &cpp) != 4) {

This is not properly checking the final quote and comma.

> +        av_log(avctx, AV_LOG_ERROR, "missing image parameters\n");
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    if ((ret = ff_set_dimensions(avctx, avctx->width, avctx->height)) < 0)
> +        return ret;
> +
> +    if ((ret = ff_get_buffer(avctx, p, 0)) < 0)
> +        return ret;
> +
> +    if (ncolors <= 0) {
> +        av_log(avctx, AV_LOG_ERROR, "invalid number of colors: %d\n", ncolors);
> +        return AVERROR_INVALIDDATA;
> +    }
> +

> +    if (cpp <= 0) {
> +        av_log(avctx, AV_LOG_ERROR, "invalid number of chars per pixel: %d\n", cpp);
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    size = 1;
> +    j = 1;
> +    for (i = 0; i < cpp; i++) {
> +        size += j*94;
> +        j *= 95;
> +    }
> +    size *= 4;

This is a DoS waiting to happen.

> +

> +    if (size < 0) {

This is deliberately invoking an undefined behaviour.

> +        av_log(avctx, AV_LOG_ERROR, "unsupported number of chars per pixel: %d\n", cpp);
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    av_fast_padded_malloc(&x->pixels, &x->pixels_size, size);
> +    if ( !x->pixels ) {
> +        return AVERROR(ENOMEM);
> +    }
> +

> +    ptr += mod_strcspn(ptr, ",") + 1;

Same remark as above: skipping random contents. Same for other uses of
mod_strcspn().

> +    for (i = 0; i < ncolors; i++) {
> +        const uint8_t *index;
> +        int len;
> +
> +        ptr += mod_strcspn(ptr, "\"") + 1;
> +        if (ptr + cpp > end)
> +            return AVERROR_INVALIDDATA;

> +        index = ptr;

index looks like a misnomer.

> +        ptr += cpp;
> +
> +        ptr = strstr(ptr, "c ");
> +        if (ptr) {
> +            ptr += 2;
> +        } else {
> +            return AVERROR_INVALIDDATA;
> +        }
> +
> +        len = strcspn(ptr, "\" ");
> +
> +        if ((ret = ascii2index(index, cpp)) < 0)
> +            return ret;
> +
> +        x->pixels[ret] = hexstring_to_rgba(ptr, len);
> +        ptr += mod_strcspn(ptr, ",") + 1;
> +    }
> +
> +    for (i = 0; i < avctx->height; i++) {
> +        dst = (uint32_t *)(p->data[0] + i * p->linesize[0]);

> +        ptr += mod_strcspn(ptr, "\"") + 1;
> +
> +        for (j = 0; j < avctx->width; j++) {
> +            if (ptr + cpp > end)
> +                return AVERROR_INVALIDDATA;
> +
> +            if ((ret = ascii2index(ptr, cpp)) < 0)
> +                return ret;
> +
> +            *dst++ = x->pixels[ret];
> +            ptr += cpp;
> +        }
> +        ptr += mod_strcspn(ptr, ",") + 1;

This whole loop can go beyond the end of the input buffer very easily.

> +    }
> +
> +    p->key_frame = 1;
> +    p->pict_type = AV_PICTURE_TYPE_I;
> +
> +    *got_frame = 1;
> +
> +    return avpkt->size;
> +}
> +
> +static av_cold int xpm_decode_close(AVCodecContext *avctx)
> +{
> +    XPMDecContext *x = avctx->priv_data;
> +    av_freep(&x->pixels);
> +
> +    return 0;
> +}
> +
> +AVCodec ff_xpm_decoder = {
> +    .name           = "xpm",
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_XPM,
> +    .priv_data_size = sizeof(XPMDecContext),
> +    .close          = xpm_decode_close,
> +    .decode         = xpm_decode_frame,
> +    .capabilities   = CODEC_CAP_DR1,
> +    .long_name      = NULL_IF_CONFIG_SMALL("XPM (X PixMap) image")
> +};
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index fc2d760..f56ef16 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -241,6 +241,7 @@ OBJS-$(CONFIG_IMAGE_SGI_PIPE_DEMUXER)     += img2dec.o img2.o
>  OBJS-$(CONFIG_IMAGE_SUNRAST_PIPE_DEMUXER) += img2dec.o img2.o
>  OBJS-$(CONFIG_IMAGE_TIFF_PIPE_DEMUXER)    += img2dec.o img2.o
>  OBJS-$(CONFIG_IMAGE_WEBP_PIPE_DEMUXER)    += img2dec.o img2.o
> +OBJS-$(CONFIG_IMAGE_XPM_PIPE_DEMUXER)     += img2dec.o img2.o
>  OBJS-$(CONFIG_INGENIENT_DEMUXER)         += ingenientdec.o rawdec.o
>  OBJS-$(CONFIG_IPMOVIE_DEMUXER)           += ipmovie.o
>  OBJS-$(CONFIG_IRCAM_DEMUXER)             += ircamdec.o ircam.o pcm.o
> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> index 132e58b..09e62c3 100644
> --- a/libavformat/allformats.c
> +++ b/libavformat/allformats.c
> @@ -372,6 +372,7 @@ static void register_all(void)
>      REGISTER_DEMUXER (IMAGE_SUNRAST_PIPE,    image_sunrast_pipe);
>      REGISTER_DEMUXER (IMAGE_TIFF_PIPE,       image_tiff_pipe);
>      REGISTER_DEMUXER (IMAGE_WEBP_PIPE,       image_webp_pipe);
> +    REGISTER_DEMUXER (IMAGE_XPM_PIPE,        image_xpm_pipe);
> 
>      /* external libraries */
>      REGISTER_MUXER   (CHROMAPRINT,      chromaprint);
> diff --git a/libavformat/img2.c b/libavformat/img2.c
> index f9f53ff..29df4f0 100644
> --- a/libavformat/img2.c
> +++ b/libavformat/img2.c
> @@ -75,6 +75,7 @@ const IdStrMap ff_img_tags[] = {
>      { AV_CODEC_ID_V210X,      "yuv10"    },
>      { AV_CODEC_ID_WEBP,       "webp"     },
>      { AV_CODEC_ID_XBM,        "xbm"      },
> +    { AV_CODEC_ID_XPM,        "xpm"      },
>      { AV_CODEC_ID_XFACE,      "xface"    },
>      { AV_CODEC_ID_XWD,        "xwd"      },
>      { AV_CODEC_ID_NONE,       NULL       }
> diff --git a/libavformat/img2dec.c b/libavformat/img2dec.c
> index c3c2cf3..b454071 100644
> --- a/libavformat/img2dec.c
> +++ b/libavformat/img2dec.c
> @@ -943,6 +943,15 @@ static int pam_probe(AVProbeData *p)
>      return pnm_magic_check(p, 7) ? pnm_probe(p) : 0;
>  }
> 
> +static int xpm_probe(AVProbeData *p)
> +{
> +    const uint8_t *b = p->buf;
> +
> +    if (AV_RB64(b) == 0x2f2a2058504d202a && *(b+8) == '/')
> +        return AVPROBE_SCORE_MAX - 1;
> +    return 0;
> +}
> +
>  #define IMAGEAUTO_DEMUXER(imgname, codecid)\
>  static const AVClass imgname ## _class = {\
>      .class_name = AV_STRINGIFY(imgname) " demuxer",\
> @@ -983,3 +992,4 @@ IMAGEAUTO_DEMUXER(sgi,     AV_CODEC_ID_SGI)
>  IMAGEAUTO_DEMUXER(sunrast, AV_CODEC_ID_SUNRAST)
>  IMAGEAUTO_DEMUXER(tiff,    AV_CODEC_ID_TIFF)
>  IMAGEAUTO_DEMUXER(webp,    AV_CODEC_ID_WEBP)
> +IMAGEAUTO_DEMUXER(xpm,     AV_CODEC_ID_XPM)

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170312/be937181/attachment.sig>


More information about the ffmpeg-devel mailing list