[FFmpeg-devel] avcodec: add IMM5 decoder

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 configure               |   1 +
 libavcodec/Makefile     |   1 +
 libavcodec/allcodecs.c  |   1 +
 libavcodec/avcodec.h    |   1 +
 libavcodec/codec_desc.c |   7 ++
 libavcodec/imm5.c       | 175 ++++++++++++++++++++++++++++++++++++++++
 libavformat/riff.c      |   1 +
 7 files changed, 187 insertions(+)
 create mode 100644 libavcodec/imm5.c

On Tue, Jul 16, 2019 at 11:57:36AM +0200, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  configure               |   1 +
>  libavcodec/Makefile     |   1 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/avcodec.h    |   1 +
>  libavcodec/codec_desc.c |   7 ++
>  libavcodec/imm5.c       | 175 ++++++++++++++++++++++++++++++++++++++++
>  libavformat/riff.c      |   1 +
>  7 files changed, 187 insertions(+)
>  create mode 100644 libavcodec/imm5.c
> 
> diff --git a/configure b/configure

> +
> +        if (index >= 1 && index <= 12) {
> +            index -= 1;
> +            off = offset = IMM5_units[index].len;
> +            if (codec_type == 2) {
> +                offset += IMM5_units[12].len;
> +            } else {
> +                offset += IMM5_units[13].len;
> +            }
> +
> +            buf += 24 - offset;
> +            buf_size = new_size + offset;
> +
> +            memcpy(buf, IMM5_units[index].bits, IMM5_units[index].len);
> +            if (codec_type == 2) {
> +                memcpy(buf + off, IMM5_units[12].bits, IMM5_units[12].len);
> +            } else {
> +                memcpy(buf + off, IMM5_units[13].bits, IMM5_units[13].len);
> +            }

this all looks pretty good.
without trying to nitpick it forever, i suggest replacing the (codec_type==2) blocks.

index2 = codec_type2 == 2 ? 12 : 13;

offset += IMM5_units[index2].len;
...
memcpy(buf + off, IMM5_units[index2].bits, IMM5_units[index2].len);


-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)

Paul B Mahol:
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  configure               |   1 +
>  libavcodec/Makefile     |   1 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/avcodec.h    |   1 +
>  libavcodec/codec_desc.c |   7 ++
>  libavcodec/imm5.c       | 175 ++++++++++++++++++++++++++++++++++++++++
>  libavformat/riff.c      |   1 +
>  7 files changed, 187 insertions(+)
>  create mode 100644 libavcodec/imm5.c
> 
> diff --git a/configure b/configure
> index 5a4f507246..d03cf43350 100755
> --- a/configure
> +++ b/configure
> @@ -2714,6 +2714,7 @@ huffyuv_encoder_select="bswapdsp huffman huffyuvencdsp llvidencdsp"
>  hymt_decoder_select="huffyuv_decoder"
>  iac_decoder_select="imc_decoder"
>  imc_decoder_select="bswapdsp fft mdct sinewin"
> +imm5_decoder_select="h264_decoder hevc_decoder"
>  indeo3_decoder_select="hpeldsp"
>  indeo4_decoder_select="ividsp"
>  indeo5_decoder_select="ividsp"
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 3cd73fbcc6..39f4d9118c 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -393,6 +393,7 @@ OBJS-$(CONFIG_IFF_ILBM_DECODER)        += iff.o
>  OBJS-$(CONFIG_ILBC_DECODER)            += ilbcdec.o
>  OBJS-$(CONFIG_IMC_DECODER)             += imc.o
>  OBJS-$(CONFIG_IMM4_DECODER)            += imm4.o
> +OBJS-$(CONFIG_IMM5_DECODER)            += imm5.o
>  OBJS-$(CONFIG_INDEO2_DECODER)          += indeo2.o
>  OBJS-$(CONFIG_INDEO3_DECODER)          += indeo3.o
>  OBJS-$(CONFIG_INDEO4_DECODER)          += indeo4.o ivi.o
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index d2f9a39ce5..fe7f773925 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -158,6 +158,7 @@ extern AVCodec ff_hymt_decoder;
>  extern AVCodec ff_idcin_decoder;
>  extern AVCodec ff_iff_ilbm_decoder;
>  extern AVCodec ff_imm4_decoder;
> +extern AVCodec ff_imm5_decoder;
>  extern AVCodec ff_indeo2_decoder;
>  extern AVCodec ff_indeo3_decoder;
>  extern AVCodec ff_indeo4_decoder;
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 2528bd89ab..da6f92b443 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -457,6 +457,7 @@ enum AVCodecID {
>      AV_CODEC_ID_AGM,
>      AV_CODEC_ID_LSCR,
>      AV_CODEC_ID_VP4,
> +    AV_CODEC_ID_IMM5,
>  
>      /* 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 4d033c20ff..e6373be504 100644
> --- a/libavcodec/codec_desc.c
> +++ b/libavcodec/codec_desc.c
> @@ -1726,6 +1726,13 @@ static const AVCodecDescriptor codec_descriptors[] = {
>          .long_name = NULL_IF_CONFIG_SMALL("On2 VP4"),
>          .props     = AV_CODEC_PROP_LOSSY,
>      },
> +    {
> +        .id        = AV_CODEC_ID_IMM5,
> +        .type      = AVMEDIA_TYPE_VIDEO,
> +        .name      = "imm5",
> +        .long_name = NULL_IF_CONFIG_SMALL("Infinity IMM5"),
> +        .props     = AV_CODEC_PROP_LOSSY,
> +    },
>  
>      /* various PCM "codecs" */
>      {
> diff --git a/libavcodec/imm5.c b/libavcodec/imm5.c
> new file mode 100644
> index 0000000000..9161c13ffc
> --- /dev/null
> +++ b/libavcodec/imm5.c
> @@ -0,0 +1,175 @@
> +/*
> + * Copyright (c) 2019 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
> + */
> +
> +#include "avcodec.h"
> +#include "bytestream.h"

This header seems unnecessary.

> +#include "internal.h"
> +
> +typedef struct IMM5Context {
> +    AVCodecContext *h264_avctx;   // wrapper context for H264
> +    AVCodecContext *hevc_avctx;   // wrapper context for HEVC
> +} IMM5Context;
> +
> +static const struct IMM5_unit {
> +    uint8_t bits[14];
> +    uint8_t len;
> +} IMM5_units[14] = {
> +    { { 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x80, 0x1E, 0xF4, 0x0B, 0x0F, 0x88 }, 12 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x80, 0x1E, 0xF4, 0x05, 0x83, 0xE2 }, 12 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x80, 0x1E, 0xF4, 0x05, 0x81, 0xE8, 0x80 }, 13 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x80, 0x1E, 0xF4, 0x0B, 0x04, 0xA2 }, 12 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x80, 0x1E, 0xF4, 0x05, 0x81, 0x28, 0x80 }, 13 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x80, 0x1E, 0xF4, 0x05, 0x80, 0x92, 0x20 }, 13 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x00, 0x1E, 0x9A, 0x74, 0x0B, 0x0F, 0xC8 }, 13 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x00, 0x1E, 0x9A, 0x74, 0x05, 0x83, 0xF2 }, 13 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x00, 0x1E, 0x9A, 0x74, 0x05, 0x81, 0xEC, 0x80 }, 14 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x00, 0x1E, 0x9A, 0x74, 0x0B, 0x04, 0xB2 }, 13 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x00, 0x1E, 0x9A, 0x74, 0x05, 0x81, 0x2C, 0x80 }, 14 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x00, 0x1E, 0x9A, 0x74, 0x05, 0x80, 0x93, 0x20 }, 14 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x68, 0xDE, 0x3C, 0x80 }, 8 },
> +    { { 0x00, 0x00, 0x00, 0x01, 0x68, 0xCE, 0x32, 0x28 }, 8 },
> +};
> +
> +static av_cold int imm5_init(AVCodecContext *avctx)
> +{
> +    IMM5Context *ctx = avctx->priv_data;
> +    const AVCodec *codec;
> +    int ret;
> +
> +    codec = avcodec_find_decoder(AV_CODEC_ID_H264);
> +    if (!codec)
> +        return AVERROR_BUG;
> +    ctx->h264_avctx = avcodec_alloc_context3(codec);
> +    if (!ctx->h264_avctx)
> +        return AVERROR(ENOMEM);
> +    ctx->h264_avctx->thread_count = 1;
> +    ctx->h264_avctx->flags = avctx->flags;
> +    ctx->h264_avctx->flags2 = avctx->flags2;

The flags could be aligned.
> +    ret = ff_codec_open2_recursive(ctx->h264_avctx, codec, NULL);
> +    if (ret < 0)
> +        return ret;
> +
> +    codec = avcodec_find_decoder(AV_CODEC_ID_HEVC);
> +    if (!codec)
> +        return AVERROR_BUG;
> +    ctx->hevc_avctx = avcodec_alloc_context3(codec);
> +    if (!ctx->hevc_avctx)
> +        return AVERROR(ENOMEM);
> +    ctx->hevc_avctx->thread_count = 1;
> +    ctx->hevc_avctx->flags = avctx->flags;
> +    ctx->hevc_avctx->flags2 = avctx->flags2;
> +    ret = ff_codec_open2_recursive(ctx->hevc_avctx, codec, NULL);
> +    if (ret < 0)
> +        return ret;
> +
> +    return 0;
> +}
> +
> +static int imm5_decode_frame(AVCodecContext *avctx, void *data,
> +                             int *got_frame, AVPacket *avpkt)
> +{
> +    IMM5Context *ctx = avctx->priv_data;
> +    AVFrame *frame = data;
> +    AVCodecContext *codec_avctx = ctx->h264_avctx;
> +    int buf_size = avpkt->size;
> +    uint8_t *buf = avpkt->data;
> +    int ret;
> +
> +    if (avpkt->size > 24 && avpkt->data[8] <= 1 && AV_RL32(avpkt->data + 4) + 24ULL <= avpkt->size) {
> +        int codec_type = avpkt->data[1];
> +        int index = avpkt->data[10];
> +        int new_size = AV_RL32(avpkt->data + 4);
> +        int offset, off;
> +
> +        if (codec_type == 0xA)
> +            codec_avctx = ctx->hevc_avctx;
> +
There used to be a switch here and most of it was unnecessary, but if
index was 18, index would be modified as follows:
index = codec_type ? 11 : 5;
Has this been intentionally removed?
> +        ret = av_packet_make_writable(avpkt);
Placing this call here has several drawbacks:
1. The packet will be made writable even in the else case below when
there is no need to make it writable.
2. The copying that this call might implicitly initiate will copy the
whole big buffer and not just the smaller one.
This is besides the fact that the way you do it here is completely
wrong: Your buf still points into the original, possibly non-writable
buffer of avpkt and in one branch you are copying into this buffer
that you don't own.
Therefore you should put this call into the the if statement, after
you have modified avpkt->data and avpkt->size directly (i.e. simply
remove buf and buf_size).
> +        if (ret < 0)
> +            return ret;
> +
> +        if (index >= 1 && index <= 12) {
> +            index -= 1;
> +            off = offset = IMM5_units[index].len;
> +            if (codec_type == 2) {
> +                offset += IMM5_units[12].len;
> +            } else {
> +                offset += IMM5_units[13].len;
> +            }
> +
> +            buf += 24 - offset;
> +            buf_size = new_size + offset;
> +
> +            memcpy(buf, IMM5_units[index].bits, IMM5_units[index].len);
> +            if (codec_type == 2) {
> +                memcpy(buf + off, IMM5_units[12].bits, IMM5_units[12].len);
> +            } else {
> +                memcpy(buf + off, IMM5_units[13].bits, IMM5_units[13].len);
> +            }
> +        } else {
> +            buf += 24;
> +            buf_size -= 24;
> +        }
> +
> +        avpkt->size = buf_size;
> +        avpkt->data = buf;
> +    }
> +
> +    ret = avcodec_send_packet(codec_avctx, avpkt);
> +    if (ret < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "Error submitting a packet for decoding\n");
> +        return ret;
> +    }
> +
> +    ret = avcodec_receive_frame(codec_avctx, frame);
> +    if (ret < 0)
> +        return ret;
> +
> +    avctx->pix_fmt = codec_avctx->pix_fmt;
> +    avctx->width = codec_avctx->width;
> +    avctx->height = codec_avctx->height;

This could be aligned.
> +
> +    *got_frame = 1;
> +
> +    return avpkt->size;
> +}
> +
> +static av_cold int imm5_close(AVCodecContext *avctx)
> +{
> +    IMM5Context *ctx = avctx->priv_data;
> +
> +    avcodec_free_context(&ctx->h264_avctx);
> +    avcodec_free_context(&ctx->hevc_avctx);
> +
> +    return 0;
> +}
> +
> +AVCodec ff_imm5_decoder = {
> +    .name           = "imm5",
> +    .long_name      = NULL_IF_CONFIG_SMALL("Infinity IMM5"),
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_IMM5,
> +    .init           = imm5_init,
> +    .decode         = imm5_decode_frame,
> +    .close          = imm5_close,
> +    .priv_data_size = sizeof(IMM5Context),
> +    .caps_internal  = FF_CODEC_CAP_INIT_THREADSAFE |
> +                      FF_CODEC_CAP_INIT_CLEANUP,
> +};
> diff --git a/libavformat/riff.c b/libavformat/riff.c
> index e755ad8d5f..610974ebf0 100644
> --- a/libavformat/riff.c
> +++ b/libavformat/riff.c
> @@ -488,6 +488,7 @@ const AVCodecTag ff_codec_bmp_tags[] = {
>      { AV_CODEC_ID_AGM,          MKTAG('A', 'G', 'M', '6') },
>      { AV_CODEC_ID_AGM,          MKTAG('A', 'G', 'M', '7') },
>      { AV_CODEC_ID_LSCR,         MKTAG('L', 'S', 'C', 'R') },
> +    { AV_CODEC_ID_IMM5,         MKTAG('I', 'M', 'M', '5') },
>      { AV_CODEC_ID_NONE,         0 }
>  };
>  
>