[FFmpeg-devel] [PATCH] avcodec: add Cintel RAW decoder

Andreas Rheinhardt andreas.rheinhardt at gmail.com
Thu Oct 1 20:58:02 EEST 2020


Paul B Mahol:
> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> ---
>  libavcodec/Makefile     |   1 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/codec_desc.c |   7 +
>  libavcodec/codec_id.h   |   1 +
>  libavcodec/cri.c        | 300 ++++++++++++++++++++++++++++++++++++++++
>  libavformat/img2.c      |   1 +
>  6 files changed, 311 insertions(+)
>  create mode 100644 libavcodec/cri.c
> 
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 816d87ba60..488cb158ef 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -269,6 +269,7 @@ OBJS-$(CONFIG_COMFORTNOISE_DECODER)    += cngdec.o celp_filters.o
>  OBJS-$(CONFIG_COMFORTNOISE_ENCODER)    += cngenc.o
>  OBJS-$(CONFIG_COOK_DECODER)            += cook.o
>  OBJS-$(CONFIG_CPIA_DECODER)            += cpia.o
> +OBJS-$(CONFIG_CRI_DECODER)             += cri.o
>  OBJS-$(CONFIG_CSCD_DECODER)            += cscd.o
>  OBJS-$(CONFIG_CYUV_DECODER)            += cyuv.o
>  OBJS-$(CONFIG_DCA_DECODER)             += dcadec.o dca.o dcadata.o dcahuff.o \
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index 2b580b66cf..52fc415815 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -82,6 +82,7 @@ extern AVCodec ff_cllc_decoder;
>  extern AVCodec ff_comfortnoise_encoder;
>  extern AVCodec ff_comfortnoise_decoder;
>  extern AVCodec ff_cpia_decoder;
> +extern AVCodec ff_cri_decoder;
>  extern AVCodec ff_cscd_decoder;
>  extern AVCodec ff_cyuv_decoder;
>  extern AVCodec ff_dds_decoder;
> diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
> index 1246dc2b96..01c0eca7b4 100644
> --- a/libavcodec/codec_desc.c
> +++ b/libavcodec/codec_desc.c
> @@ -1812,6 +1812,13 @@ static const AVCodecDescriptor codec_descriptors[] = {
>          .long_name = NULL_IF_CONFIG_SMALL("Argonaut Games Video"),
>          .props     = AV_CODEC_PROP_LOSSY,
>      },
> +    {
> +        .id        = AV_CODEC_ID_CRI,
> +        .type      = AVMEDIA_TYPE_VIDEO,
> +        .name      = "cri",
> +        .long_name = NULL_IF_CONFIG_SMALL("Cintel RAW"),
> +        .props     = AV_CODEC_PROP_INTRA_ONLY | AV_CODEC_PROP_LOSSY | AV_CODEC_PROP_LOSSLESS,
> +    },
>  
>      /* various PCM "codecs" */
>      {
> diff --git a/libavcodec/codec_id.h b/libavcodec/codec_id.h
> index 21444f9ce3..e933f7664a 100644
> --- a/libavcodec/codec_id.h
> +++ b/libavcodec/codec_id.h
> @@ -300,6 +300,7 @@ enum AVCodecID {
>      AV_CODEC_ID_PHOTOCD,
>      AV_CODEC_ID_IPU,
>      AV_CODEC_ID_ARGO,
> +    AV_CODEC_ID_CRI,
>  
>      /* various PCM "codecs" */
>      AV_CODEC_ID_FIRST_AUDIO = 0x10000,     ///< A dummy id pointing at the start of audio codecs
> diff --git a/libavcodec/cri.c b/libavcodec/cri.c
> new file mode 100644
> index 0000000000..4bd0262255
> --- /dev/null
> +++ b/libavcodec/cri.c
> @@ -0,0 +1,300 @@
> +/*
> + * CRI image decoder
> + *
> + * Copyright (c) 2020 Paul B Mahol
> + *
> + * 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
> + */
> +
> +/**
> + * @file
> + * Cintel RAW image decoder
> + */
> +
> +#define BITSTREAM_READER_LE
> +
> +#include "libavutil/avassert.h"
> +#include "libavutil/intreadwrite.h"
> +#include "libavutil/opt.h"

These three headers seem to be unnecessary.

> +#include "avcodec.h"
> +#include "bytestream.h"
> +#include "get_bits.h"
> +#include "internal.h"
> +#include "thread.h"
> +
> +typedef struct CRIContext {
> +    AVCodecContext *jpeg_avctx;   // wrapper context for MJPEG
> +    AVFrame *jpgframe;            // decoded JPEG tile
> +
> +    GetByteContext gb;
> +    int color_model;
> +    const uint8_t *data;
> +    unsigned data_size;
> +    uint64_t tile_size[4];
> +} CRIContext;
> +
> +static av_cold int cri_decode_init(AVCodecContext *avctx)
> +{
> +    CRIContext *s = avctx->priv_data;
> +    const AVCodec *codec;
> +    int ret;
> +
> +    s->jpgframe = av_frame_alloc();

This leaks if an error happens later.

> +    if (!s->jpgframe)
> +        return AVERROR(ENOMEM);
> +
> +    codec = avcodec_find_decoder(AV_CODEC_ID_MJPEG);
> +    if (!codec)
> +        return AVERROR_BUG;
> +    s->jpeg_avctx = avcodec_alloc_context3(codec);
> +    if (!s->jpeg_avctx)
> +        return AVERROR(ENOMEM);
> +    s->jpeg_avctx->flags = avctx->flags;
> +    s->jpeg_avctx->flags2 = avctx->flags2;
> +    s->jpeg_avctx->dct_algo = avctx->dct_algo;
> +    s->jpeg_avctx->idct_algo = avctx->idct_algo;
> +    ret = ff_codec_open2_recursive(s->jpeg_avctx, codec, NULL);
> +    if (ret < 0)
> +        return ret;
> +
> +    return 0;
> +}
> +
> +static int cri_decode_frame(AVCodecContext *avctx, void *data,
> +                            int *got_frame, AVPacket *avpkt)
> +{
> +    CRIContext *s = avctx->priv_data;
> +    GetByteContext *gb = &s->gb;
> +    ThreadFrame frame = { .f = data };
> +    int ret, bps, hflip = 0, vflip = 0;
> +    int compressed = 0;
> +    GetBitContext gbit;
> +    AVFrame *p = data;
> +
> +    bytestream2_init(gb, avpkt->data, avpkt->size);
> +
> +    while (bytestream2_get_bytes_left(gb) > 8) {
> +        char codec_name[1024];
> +        uint32_t key, length;
> +
> +        key    = bytestream2_get_le32(gb);
> +        length = bytestream2_get_le32(gb);
> +
> +        switch (key) {
> +        case 1:
> +            if (length != 4)
> +                return AVERROR_INVALIDDATA;
> +
> +            if (bytestream2_get_le32(gb) != MKTAG('D', 'V', 'C', 'C'))
> +                return AVERROR_INVALIDDATA;
> +            break;
> +        case 100:
> +            if (length < 16)
> +                return AVERROR_INVALIDDATA;
> +            avctx->width   = bytestream2_get_le32(gb);
> +            avctx->height  = bytestream2_get_le32(gb);
> +            s->color_model = bytestream2_get_le32(gb);
> +            if (bytestream2_get_le32(gb) != 1)
> +                return AVERROR_INVALIDDATA;
> +            length -= 16;
> +            goto skip;
> +        case 101:
> +            if (length != 4)
> +                return AVERROR_INVALIDDATA;
> +
> +            if (bytestream2_get_le32(gb) != 0)
> +                return AVERROR_INVALIDDATA;
> +            break;
> +        case 102:
> +            bytestream2_get_buffer(gb, codec_name, FFMIN(length, sizeof(codec_name) - 1));
> +            length -= FFMIN(length, sizeof(codec_name) - 1);
> +            if (strncmp(codec_name, "cintel_craw", FFMIN(length, sizeof(codec_name) - 1)))
> +                return AVERROR_INVALIDDATA;
> +            compressed = 1;
> +            goto skip;
> +        case 103:
> +            if (bytestream2_get_bytes_left(gb) < length)
> +                return AVERROR_INVALIDDATA;
> +            s->data = gb->buffer;
> +            s->data_size = length;

The rest of the code seems to be based on the assumption that this case
is always triggered, yet there is no check for this. If it isn't, you
are using dangling pointers below.

> +            goto skip;
> +        case 105:
> +            hflip = bytestream2_get_byte(gb) != 0;
> +            length--;
> +            goto skip;
> +        case 106:
> +            vflip = bytestream2_get_byte(gb) != 0;

The *flip values are set-but-unused.

> +            length--;
> +            goto skip;
> +        case 119:
> +            if (length != 32)
> +                return AVERROR_INVALIDDATA;
> +
> +            for (int i = 0; i < 4; i++)
> +                s->tile_size[i] = bytestream2_get_le64(gb);
> +            break;
> +        default:
> +skip:
> +            bytestream2_skip(gb, length);
> +        }
> +    }
> +
> +    switch (s->color_model) {
> +    case 76:
> +    case 88:
> +        avctx->pix_fmt = AV_PIX_FMT_BAYER_BGGR16;
> +        break;
> +    case 77:
> +    case 89:
> +        avctx->pix_fmt = AV_PIX_FMT_BAYER_GBRG16;
> +        break;
> +    case 78:
> +    case 90:
> +        avctx->pix_fmt = AV_PIX_FMT_BAYER_RGGB16;
> +        break;
> +    case 45:
> +    case 79:
> +    case 91:
> +        avctx->pix_fmt = AV_PIX_FMT_BAYER_GRBG16;
> +        break;
> +    }
> +
> +    switch (s->color_model) {
> +    case 45:
> +        bps = 10;
> +        break;
> +    case 76:
> +    case 77:
> +    case 78:
> +    case 79:
> +        bps = 12;
> +        break;
> +    case 88:
> +    case 89:
> +    case 90:
> +    case 91:
> +        bps = 16;
> +        break;
> +    default:
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    if (compressed) {
> +        for (int i = 0; i < 4; i++) {
> +            if (s->tile_size[i] >= s->data_size)
> +                return AVERROR_INVALIDDATA;
> +        }
> +
> +        if (s->tile_size[0] + s->tile_size[1] + s->tile_size[2] + s->tile_size[3] !=
> +            s->data_size)
> +            return AVERROR_INVALIDDATA;
> +    }
> +
> +    if ((ret = ff_thread_get_buffer(avctx, &frame, 0)) < 0)
> +        return ret;
> +
> +    if (!compressed) {
> +        int shift = 16 - bps;
> +
> +        ret = init_get_bits8(&gbit, s->data, s->data_size);
> +        if (ret < 0)
> +            return ret;
> +
> +        for (int y = 0; y < avctx->height; y++) {
> +            uint16_t *dst = (uint16_t *)(p->data[0] + y * p->linesize[0]);
> +
> +            for (int x = 0; x < avctx->width; x++)
> +                dst[x] = get_bits(&gbit, bps) << shift;
> +        }
> +    } else {
> +        unsigned offset = 0;
> +
> +        for (int tile = 0; tile < 4; tile++) {
> +            AVPacket jpkt;
> +
> +            av_init_packet(&jpkt);
> +            jpkt.data = (uint8_t *)s->data + offset;
> +            jpkt.size = s->tile_size[tile];
> +
> +            ret = avcodec_send_packet(s->jpeg_avctx, &jpkt);

This will copy the data. av_packet_ref() followed by modifying the
packet's data and size would probably be cheaper.

> +            if (ret < 0) {
> +                av_log(avctx, AV_LOG_ERROR, "Error submitting a packet for decoding\n");
> +                return ret;
> +            }
> +
> +            ret = avcodec_receive_frame(s->jpeg_avctx, s->jpgframe);
> +            if (ret < 0 || s->jpgframe->format != AV_PIX_FMT_GRAY16 ||
> +                s->jpeg_avctx->width  * 2 != avctx->width ||
> +                s->jpeg_avctx->height * 2 != avctx->height) {
> +                av_log(avctx, AV_LOG_ERROR,
> +                       "JPEG decoding error (%d).\n", ret);

This message will also be emitted if ret == 0, although it makes no
sense then.

> +
> +                /* Normally skip, if error explode */
> +                if (avctx->err_recognition & AV_EF_EXPLODE)
> +                    return AVERROR_INVALIDDATA;
> +                else
> +                    return 0;

If ret != 0, why are you not forwarding this error?

> +            }
> +
> +            for (int y = 0; y < s->jpeg_avctx->height; y++) {
> +                const int hw =  s->jpgframe->width / 2;
> +                uint16_t *dst = (uint16_t *)(p->data[0] + (y * 2) * p->linesize[0] + tile * hw * 2);
> +                const uint16_t *src = (const uint16_t *)(s->jpgframe->data[0] + y * s->jpgframe->linesize[0]);
> +
> +                for (int x = 0; x < s->jpeg_avctx->width / 2; x++)
> +                    dst[x] = src[x];

memcpy?

> +
> +                src += hw;
> +                dst += p->linesize[0] / 2;
> +                for (int x = 0; x < s->jpeg_avctx->width / 2; x++)
> +                    dst[x] = src[x];

memcpy?

> +            }
> +
> +            av_frame_unref(s->jpgframe);
> +            offset += s->tile_size[tile];
> +        }
> +    }
> +
> +    p->pict_type = AV_PICTURE_TYPE_I;
> +    p->key_frame = 1;
> +
> +    *got_frame = 1;
> +
> +    return 0;
> +}
> +
> +static av_cold int cri_decode_close(AVCodecContext *avctx)
> +{
> +    CRIContext *s = avctx->priv_data;
> +
> +    av_frame_free(&s->jpgframe);
> +    avcodec_free_context(&s->jpeg_avctx);
> +
> +    return 0;
> +}
> +
> +AVCodec ff_cri_decoder = {
> +    .name           = "cri",
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_CRI,
> +    .priv_data_size = sizeof(CRIContext),
> +    .init           = cri_decode_init,
> +    .decode         = cri_decode_frame,
> +    .close          = cri_decode_close,
> +    .capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_FRAME_THREADS,
> +    .long_name      = NULL_IF_CONFIG_SMALL("Cintel RAW"),
> +};
> diff --git a/libavformat/img2.c b/libavformat/img2.c
> index db37aa7228..6bdd7efe26 100644
> --- a/libavformat/img2.c
> +++ b/libavformat/img2.c
> @@ -41,6 +41,7 @@ const IdStrMap ff_img_tags[] = {
>      { AV_CODEC_ID_PBM,        "pbm"      },
>      { AV_CODEC_ID_PAM,        "pam"      },
>      { AV_CODEC_ID_PFM,        "pfm"      },
> +    { AV_CODEC_ID_CRI,        "cri"      },
>      { AV_CODEC_ID_ALIAS_PIX,  "pix"      },
>      { AV_CODEC_ID_DDS,        "dds"      },
>      { AV_CODEC_ID_MPEG1VIDEO, "mpg1-img" },
> 



More information about the ffmpeg-devel mailing list