[FFmpeg-devel] [WIP][RFC][PATCH] avcodec: add IMM4 decoder

[Moritz Barsnick]

Hi,

On Mon, Aug 06, 2018 at 12:35:15 +0200, Paul B Mahol wrote:
> patch attached.

I can't judge on most of the procedural stuff, but here's some 2 cents:

>  libavcodec/Makefile     |   1 +
>  libavcodec/allcodecs.c  |   1 +
>  libavcodec/avcodec.h    |   1 +
>  libavcodec/codec_desc.c |   7 +
>  libavcodec/imm4.c       | 399 ++++++++++++++++++++++++++++++++++++++++++++++++
>  libavformat/riff.c      |   1 +

Changelog and docs missing.

>  OBJS-$(CONFIG_INTERPLAY_VIDEO_DECODER) += interplayvideo.o
> +OBJS-$(CONFIG_IMM4_DECODER)            += imm4.o
>  OBJS-$(CONFIG_JACOSUB_DECODER)         += jacosubdec.o ass.o

Alphabetical order?

>  extern AVCodec ff_interplay_video_decoder;
> +extern AVCodec ff_imm4_decoder;
>  extern AVCodec ff_jpeg2000_encoder;

Alphabetical order?

> +static uint16_t table_7[304] = {
> +  0u, 0u, 0u, 0u, 0u, 0u, 0u, 0u, 16514u, 16514u, 16387u, 16387u,

Inconsisent to declare these explicitly with 'u',

> +static uint8_t table_8[] = {
> +    0,  12,  11,  11,  11,  11,  11,  11,  12,  12,

but not these, but it doesn't really matter.

Shouldn't they be const, BTW?

> +    if (x)
> +        return table_5[2 * value];
> +    else
> +        return 15 - table_5[2 * value];

I've seen requests on this list to "simplify" such terms to
       return x ? table_5[2 * value] : 15 - table_5[2 * value];
but I don't care, and consider your syntax more readable.

> +        if (!c) {
> +            f |= 2;
> +            av_assert0(0);
> +        }

If it always aborts, what's the use of the operation on f?
           av_assert0(c); ?

> +    printf("gb: %d\n", get_bits_left(gb));

Debug code?

Cheers,
Moritz