[FFmpeg-devel] [PATCH v2] [RFC] lavc, lavfmt: add FLIF decoding support
Kartik K. Khullar
kartikkhullar840 at gmail.com
Sun Aug 2 23:47:24 EEST 2020
My apologies, I should have removed other parts of the quoted code which I
was not replying to.
The reply is unnecesarily long now.
On Mon, 3 Aug, 2020, 12:39 am Kartik K. Khullar, <kartikkhullar840 at gmail.com>
wrote:
>
>
> On Sun, Aug 2, 2020 at 5:57 PM Nicolas George <george at nsup.org> wrote:
>
>> Anamitra Ghorui (12020-07-30):
>> > Visible errors have been fixed in libavformat/flifdec.c, however the
>> > problem regarding metadata decoding still exists. The function does work
>> > with dummy data from zlib's example program (see
>> https://0x0.st/ix_E.zip for
>> > an example with "1234" as the encoded sequence), so the problem may be
>> > in providing the appropriate parameters.
>>
>> Thanks for the patch. See a few comments below. This is so long, I was
>> not as careful at the end as in the beginning.
>>
>> >
>> > Other test files: https://0x0.st/ixSs.7z
>> >
>> > Co-authored-by: Anamitra Ghorui <aghorui at teknik.io>
>> > Co-authored-by: Kartik K Khullar <kartikkhullar840 at gmail.com>
>> >
>> > Signed-off-by: Anamitra Ghorui <aghorui at teknik.io>
>> > ---
>> > Changelog | 3 +-
>> > configure | 2 +
>> > doc/general.texi | 2 +
>> > libavcodec/Makefile | 2 +
>> > libavcodec/allcodecs.c | 1 +
>> > libavcodec/codec_desc.c | 7 +
>> > libavcodec/codec_id.h | 1 +
>> > libavcodec/flif16.c | 198 +++
>> > libavcodec/flif16.h | 278 +++
>> > libavcodec/flif16_parser.c | 189 ++
>> > libavcodec/flif16_rangecoder.c | 464 +++++
>> > libavcodec/flif16_rangecoder.h | 824 +++++++++
>> > libavcodec/flif16_transform.c | 2964 ++++++++++++++++++++++++++++++++
>> > libavcodec/flif16_transform.h | 123 ++
>> > libavcodec/flif16dec.c | 1146 ++++++++++++
>> > libavcodec/parsers.c | 1 +
>> > libavformat/Makefile | 1 +
>> > libavformat/allformats.c | 1 +
>> > libavformat/flifdec.c | 377 ++++
>> > 19 files changed, 6583 insertions(+), 1 deletion(-)
>> > create mode 100644 libavcodec/flif16.c
>> > create mode 100644 libavcodec/flif16.h
>> > create mode 100644 libavcodec/flif16_parser.c
>> > create mode 100644 libavcodec/flif16_rangecoder.c
>> > create mode 100644 libavcodec/flif16_rangecoder.h
>> > create mode 100644 libavcodec/flif16_transform.c
>> > create mode 100644 libavcodec/flif16_transform.h
>> > create mode 100644 libavcodec/flif16dec.c
>> > create mode 100644 libavformat/flifdec.c
>> >
>> > diff --git a/Changelog b/Changelog
>> > index 6f648bff2b..ac5a21b1a9 100644
>> > --- a/Changelog
>> > +++ b/Changelog
>> > @@ -10,7 +10,8 @@ version <next>:
>> > - ADPCM IMA Ubisoft APM encoder
>> > - Rayman 2 APM muxer
>> > - AV1 encoding support SVT-AV1
>> > -
>> > +- FLIF16 decoder
>> > +- FLIF16 demuxer
>> >
>> > version 4.3:
>> > - v360 filter
>> > diff --git a/configure b/configure
>> > index 169f23e17f..50936fef4a 100755
>> > --- a/configure
>> > +++ b/configure
>> > @@ -2718,6 +2718,8 @@ ffvhuff_encoder_select="huffyuv_encoder"
>> > fic_decoder_select="golomb"
>> > flac_decoder_select="flacdsp"
>> > flac_encoder_select="bswapdsp flacdsp lpc"
>> > +flif16_decoder_select="flif16dec"
>> > +flif16_encoder_select="flif16enc"
>> > flashsv2_decoder_deps="zlib"
>> > flashsv2_encoder_deps="zlib"
>> > flashsv_decoder_deps="zlib"
>> > diff --git a/doc/general.texi b/doc/general.texi
>> > index dfcfd394e6..71b61100e3 100644
>> > --- a/doc/general.texi
>> > +++ b/doc/general.texi
>> > @@ -903,6 +903,8 @@ following image formats are supported:
>> > @item Flash Screen Video v2 @tab X @tab X
>> > @item Flash Video (FLV) @tab X @tab X
>> > @tab Sorenson H.263 used in Flash
>> > + at item FLIF (Free Lossless Image Format @tab @tab X
>> > + @tab Precursor to JPEG XL and FUIF
>> > @item FM Screen Capture Codec @tab @tab X
>> > @item Forward Uncompressed @tab @tab X
>> > @item Fraps @tab @tab X
>> > diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> > index 9d4d52d048..96d06d2479 100644
>> > --- a/libavcodec/Makefile
>> > +++ b/libavcodec/Makefile
>> > @@ -328,6 +328,7 @@ OBJS-$(CONFIG_FLASHSV_ENCODER) +=
>> flashsvenc.o
>> > OBJS-$(CONFIG_FLASHSV2_ENCODER) += flashsv2enc.o
>> > OBJS-$(CONFIG_FLASHSV2_DECODER) += flashsv.o
>> > OBJS-$(CONFIG_FLIC_DECODER) += flicvideo.o
>> > +OBJS-$(CONFIG_FLIF16_DECODER) += flif16dec.o
>> flif16_rangecoder.o flif16.o flif16_transform.o
>> > OBJS-$(CONFIG_FMVC_DECODER) += fmvc.o
>> > OBJS-$(CONFIG_FOURXM_DECODER) += 4xm.o
>> > OBJS-$(CONFIG_FRAPS_DECODER) += fraps.o
>> > @@ -1069,6 +1070,7 @@ OBJS-$(CONFIG_DVD_NAV_PARSER) +=
>> dvd_nav_parser.o
>> > OBJS-$(CONFIG_DVDSUB_PARSER) += dvdsub_parser.o
>> > OBJS-$(CONFIG_FLAC_PARSER) += flac_parser.o flacdata.o
>> flac.o \
>> > vorbis_data.o
>> > +OBJS-$(CONFIG_FLIF16_PARSER) += flif16_parser.o
>> > OBJS-$(CONFIG_G723_1_PARSER) += g723_1_parser.o
>> > OBJS-$(CONFIG_G729_PARSER) += g729_parser.o
>> > OBJS-$(CONFIG_GIF_PARSER) += gif_parser.o
>> > diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
>> > index 80142899fe..032ff422f8 100644
>> > --- a/libavcodec/allcodecs.c
>> > +++ b/libavcodec/allcodecs.c
>> > @@ -119,6 +119,7 @@ extern AVCodec ff_flashsv_decoder;
>> > extern AVCodec ff_flashsv2_encoder;
>> > extern AVCodec ff_flashsv2_decoder;
>> > extern AVCodec ff_flic_decoder;
>> > +extern AVCodec ff_flif16_decoder;
>> > extern AVCodec ff_flv_encoder;
>> > extern AVCodec ff_flv_decoder;
>> > extern AVCodec ff_fmvc_decoder;
>> > diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
>> > index ced00bd34c..4ca0d1f514 100644
>> > --- a/libavcodec/codec_desc.c
>> > +++ b/libavcodec/codec_desc.c
>> > @@ -1784,6 +1784,13 @@ static const AVCodecDescriptor
>> codec_descriptors[] = {
>> > .long_name = NULL_IF_CONFIG_SMALL("PFM (Portable FloatMap)
>> image"),
>> > .props = AV_CODEC_PROP_INTRA_ONLY | AV_CODEC_PROP_LOSSLESS,
>> > },
>> > + {
>> > + .id = AV_CODEC_ID_FLIF16,
>> > + .type = AVMEDIA_TYPE_VIDEO,
>> > + .name = "flif16",
>> > + .long_name = NULL_IF_CONFIG_SMALL("FLIF16 (Free Lossless Image
>> Format)"),
>> > + .props = AV_CODEC_PROP_LOSSLESS,
>> > + },
>> >
>> > /* various PCM "codecs" */
>> > {
>> > diff --git a/libavcodec/codec_id.h b/libavcodec/codec_id.h
>> > index 896ecb0ce0..5c4f2dd7d0 100644
>> > --- a/libavcodec/codec_id.h
>> > +++ b/libavcodec/codec_id.h
>> > @@ -296,6 +296,7 @@ enum AVCodecID {
>> > AV_CODEC_ID_MV30,
>> > AV_CODEC_ID_NOTCHLC,
>> > AV_CODEC_ID_PFM,
>> > + AV_CODEC_ID_FLIF16,
>> >
>> > /* various PCM "codecs" */
>> > AV_CODEC_ID_FIRST_AUDIO = 0x10000, ///< A dummy id pointing at
>> the start of audio codecs
>> > diff --git a/libavcodec/flif16.c b/libavcodec/flif16.c
>> > new file mode 100644
>> > index 0000000000..d8ffb31c34
>> > --- /dev/null
>> > +++ b/libavcodec/flif16.c
>> > @@ -0,0 +1,198 @@
>> > +/*
>> > + * FLIF16 Image Format Definitions
>> > + * Copyright (c) 2020 Anamitra Ghorui <aghorui at teknik.io>
>> > + *
>> > + * 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
>> > + * FLIF16 format definitions and functions.
>> > + */
>> > +
>> > +#include "flif16.h"
>> > +#include "flif16_transform.h"
>> > +
>> > +/**
>> > + * Initialise property ranges for non interlaced images.
>> > + * @param[out] prop_ranges resultant ranges
>> > + * @param[in] color ranges of each channel
>> > + * @param[in] channels number of channels
>> > + */
>>
>> > +int32_t (*ff_flif16_maniac_ni_prop_ranges_init(unsigned int
>> *prop_ranges_size,
>> > + FLIF16RangesContext
>> *ranges,
>> > + uint8_t plane,
>> > + uint8_t channels))[2]
>>
>> I would prefer avoiding pointers to arrays of arrays, they are tricky to
>> use and the syntax is awful, as clearly visible here.
>>
>> I suggest to define "struct FLIFMinMax { int32_t min, max; }" and to use
>> it instead.
>>
>> > +{
>> > + int min = ff_flif16_ranges_min(ranges, plane);
>> > + int max = ff_flif16_ranges_max(ranges, plane);
>> > + int mind = min - max, maxd = max - min;
>> > + int32_t (*prop_ranges)[2];
>> > + unsigned int top = 0;
>>
>> > + unsigned int size = (((plane < 3) ? plane : 0) + 2 + 5) + ((plane
>> < 3) && (ranges->num_planes > 3));
>>
>> Am I wrong, or is it always at most 10 here and 12 in the other function?
>>
>> If so, then let us get rid of this dynamic allocation and just have:
>>
>> #define FLIF_MAX_RANGES 12
>>
>> FLIFMinMax prop_ranges[FLIF_MAX_RANGES];
>>
>> > + *prop_ranges_size = size;
>>
>> > + prop_ranges = av_mallocz(sizeof(*prop_ranges) * size);
>>
>> av_mallocz_array(), but moot if we avoid the dynamic alloc.
>>
>> > + if (!prop_ranges)
>> > + return NULL;
>> > + if (plane < 3) {
>> > + for (int i = 0; i < plane; i++) {
>> > + prop_ranges[top][0] = ff_flif16_ranges_min(ranges, i);
>> > + prop_ranges[top++][1] = ff_flif16_ranges_max(ranges, i);
>> // pixels on previous planes
>> > + }
>> > + if (ranges->num_planes > 3) {
>> > + prop_ranges[top][0] = ff_flif16_ranges_min(ranges, 3);
>> > + prop_ranges[top++][1] = ff_flif16_ranges_max(ranges, 3);
>> // pixel on alpha plane
>> > + }
>> > + }
>> > + prop_ranges[top][0] = min;
>> > + prop_ranges[top++][1] = max; // guess (median of 3)
>> > + prop_ranges[top][0] = 0;
>> > + prop_ranges[top++][1] = 2; // which predictor was it
>>
>> > + for (int i = 0; i < 5; ++i) {
>>
>> Nit: We usually write i++. At least be consistent.
>>
>> > + prop_ranges[top][0] = mind;
>> > + prop_ranges[top++][1] = maxd;
>> > + }
>> > + return prop_ranges;
>> > +}
>> > +
>> > +int32_t (*ff_flif16_maniac_prop_ranges_init(unsigned int
>> *prop_ranges_size,
>> > + FLIF16RangesContext
>> *ranges,
>> > + uint8_t property,
>> > + uint8_t channels))[2]
>> > +{
>> > + int min = ff_flif16_ranges_min(ranges, property);
>> > + int max = ff_flif16_ranges_max(ranges, property);
>> > + unsigned int top = 0, pp;
>> > + int mind = min - max, maxd = max - min;
>> > + int32_t (*prop_ranges)[2];
>>
>> > + unsigned int size = (((property < 3) ? ((ranges->num_planes > 3)
>> ? property + 1 : property) : 0) \
>> > + + ((property == 1 || property == 2) ? 1 : 0) \
>> > + + ((property != 2) ? 2 : 0) + 1 + 5);
>> > + prop_ranges = av_mallocz(sizeof(*prop_ranges) * size);
>> > + if (!prop_ranges)
>> > + return NULL;
>> > + *prop_ranges_size = size;
>> > +
>> > + if (property < 3) {
>> > + for (pp = 0; pp < property; pp++) {
>> > + prop_ranges[top][0] = ff_flif16_ranges_min(ranges, pp);
>> > + prop_ranges[top++][1] = ff_flif16_ranges_max(ranges, pp);
>> > + }
>> > + if (ranges->num_planes > 3) {
>> > + prop_ranges[top][0] = ff_flif16_ranges_min(ranges, 3);
>> > + prop_ranges[top++][1] = ff_flif16_ranges_max(ranges, 3);;
>> > + }
>> > + }
>> > +
>> > + prop_ranges[top][0] = 0;
>> > + prop_ranges[top++][0] = 2;
>> > +
>> > + if (property == 1 || property == 2){
>> > + prop_ranges[top][0] = ff_flif16_ranges_min(ranges, 0) -
>> ff_flif16_ranges_max(ranges, 0);
>> > + prop_ranges[top++][1] = ff_flif16_ranges_max(ranges, 0) -
>> ff_flif16_ranges_min(ranges, 0); // luma prediction miss
>> > + }
>> > + for (int i = 0; i < 4; ++i) {
>> > + prop_ranges[top][0] = mind;
>> > + prop_ranges[top++][1] = maxd;
>> > + }
>> > + prop_ranges[top][0] = min;
>> > + prop_ranges[top++][0] = max;
>> > +
>> > + if (property != 2) {
>> > + prop_ranges[top][0] = mind;
>> > + prop_ranges[top++][1] = maxd;
>> > + }
>> > + return prop_ranges;
>> > +}
>> > +
>> > +
>> > +int ff_flif16_planes_init(FLIF16Context *s, FLIF16PixelData *frames,
>> > + uint8_t *plane_mode, uint8_t
>> *const_plane_value,
>> > + uint8_t lookback)
>> > +{
>> > + for (int j = 0; j < s->num_frames; ++j) {
>> > + if (frames[j].seen_before >= 0)
>> > + continue;
>> > +
>>
>> > + frames[j].data = av_mallocz(sizeof(*frames->data) *
>> s->num_planes);
>>
>> Can num_planes be greater than 5? If yes, then av_mallocz_array(). If
>> no, then let us get rid of this dynamic allocation too.
>>
>> > +
>> > + if (!frames[j].data) {
>> > + return AVERROR(ENOMEM);
>> > + }
>> > +
>> > + for (int i = 0; i < (s->num_planes + lookback); ++i) {
>>
>> > + printf("Plane: %d ", i);
>>
>> Remember to get rid of all printf() for the final version.
>>
>> > + switch (plane_mode[i]) {
>> > + case FLIF16_PLANEMODE_NORMAL:
>>
>> > + frames[j].data[i] = av_mallocz(sizeof(int32_t) *
>> s->width * s->height);
>>
>> av_malloc_array() and missing error check.
>>
>> Are width and height validated against multiplication overflow? IIRC,
>> ff_set_dimensions() checks against avctx->max_pixels, which can be set
>> to more than INT_MAX.
>>
>> Is the initialization to 0 necessary? It is expensive.
>>
>> > + break;
>> > +
>> > + case FLIF16_PLANEMODE_CONSTANT:
>>
>> > + frames[j].data[i] = av_mallocz(sizeof(int32_t));
>>
>> Missing error check. And the initialization to 0 is not necessary.
>>
>> > + ((int32_t *) frames[j].data[i])[0] =
>> const_plane_value[i];
>> > + break;
>> > +
>> > + case FLIF16_PLANEMODE_FILL:
>>
>> > + frames[j].data[i] = av_mallocz(sizeof(int32_t) *
>> s->width * s->height);;
>>
>> Same as above.
>>
>> > + if (!frames[j].data[i])
>> > + return AVERROR(ENOMEM);
>> > + for (int k = 0; k < s->height * s->width; ++k)
>> > + ((int32_t *) frames[j].data[i])[k] =
>> const_plane_value[i];
>> > + break;
>> > + }
>> > + }
>> > + }
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +
>> > +static void ff_flif16_planes_free(FLIF16PixelData *frame, uint8_t
>> num_planes,
>> > + uint8_t lookback)
>> > +{
>>
>> > + for(uint8_t i = 0; i < (num_planes + lookback); ++i) {
>>
>> int i, to avoid confusing the compiler about your intent.
>>
>> > + av_free(frame->data[i]);
>> > + }
>> > + av_free(frame->data);
>> > +}
>> > +
>> > +FLIF16PixelData *ff_flif16_frames_init(FLIF16Context *s)
>> > +{
>> > + FLIF16PixelData *frames = av_mallocz(sizeof(*frames) *
>> s->num_frames);
>>
>> av_malloc_array()
>>
>> > + if (!frames)
>> > + return NULL;
>> > +
>> > + for (int i = 0; i < s->num_frames; ++i)
>> > + frames[i].seen_before = -1;
>> > + return frames;
>> > +}
>> > +
>> > +void ff_flif16_frames_free(FLIF16PixelData **frames, uint32_t
>> num_frames,
>> > + uint32_t num_planes, uint8_t lookback)
>> > +{
>> > + for (int i = 0; i < num_frames; ++i) {
>> > + if ((*frames)[i].seen_before >= 0)
>> > + continue;
>> > + ff_flif16_planes_free(&(*frames)[i], num_planes, lookback);
>> > + if ((*frames)[i].col_begin)
>> > + av_freep(&(*frames)[i].col_begin);
>> > + if ((*frames)[i].col_end)
>> > + av_freep(&(*frames)[i].col_end);
>> > + }
>> > +
>> > + av_freep(frames);
>> > +}
>> > diff --git a/libavcodec/flif16.h b/libavcodec/flif16.h
>> > new file mode 100644
>> > index 0000000000..d21cfd79a4
>> > --- /dev/null
>> > +++ b/libavcodec/flif16.h
>> > @@ -0,0 +1,278 @@
>> > +/*
>> > + * FLIF16 Image Format Definitions
>> > + * Copyright (c) 2020 Anamitra Ghorui <aghorui at teknik.io>
>> > + *
>> > + * 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
>> > + * FLIF16 format definitions and functions.
>> > + */
>> > +
>> > +#ifndef AVCODEC_FLIF16_H
>> > +#define AVCODEC_FLIF16_H
>> > +
>> > +#include <stdint.h>
>> > +#include <stdlib.h>
>> > +
>> > +#include "avcodec.h"
>> > +#include "libavutil/pixfmt.h"
>> > +#include "flif16_rangecoder.h"
>> > +
>> > +#define MAX_PLANES 5
>> > +#define MAX_PREDICTORS 2
>> > +
>> > +#define VARINT_APPEND(a,x) (a) = ((a) << 7) | (uint32_t) ((x) & 127)
>> > +#define ZOOM_ROWPIXELSIZE(zoomlevel) (1 << (((zoomlevel) + 1) / 2))
>> > +#define ZOOM_COLPIXELSIZE(zoomlevel) (1 << (((zoomlevel)) / 2))
>> > +#define ZOOM_HEIGHT(r, z) ((!z) ? 0 : (1 + ((r) - 1) /
>> ZOOM_ROWPIXELSIZE(z)))
>> > +#define ZOOM_WIDTH(w, z) ((!z) ? 0 : (1 + ((w) - 1) /
>> ZOOM_COLPIXELSIZE(z)))
>> > +#define MEDIAN3(a, b, c) (((a) < (b)) ? (((b) < (c)) ? (b) : ((a) <
>> (c) ? (c) : (a))) : (((a) < (c)) ? (a) : ((b) < (c) ? (c) : (b))))
>> > +
>> > +static const uint8_t flif16_header[4] = "FLIF";
>> > +
>> > +// Pixeldata types
>> > +static const enum AVPixelFormat flif16_out_frame_type[][2] = {
>> > + { -1, -1 }, // Padding
>> > + { AV_PIX_FMT_GRAY8, AV_PIX_FMT_GRAY16 },
>> > + { -1 , -1 }, // Padding
>> > + { AV_PIX_FMT_RGB24, AV_PIX_FMT_RGB48 },
>> > + { AV_PIX_FMT_RGB32, AV_PIX_FMT_RGBA64 }
>> > +};
>> > +
>> > +typedef enum FLIF16Plane {
>> > + FLIF16_PLANE_Y = 0,
>> > + FLIF16_PLANE_CO,
>> > + FLIF16_PLANE_CG,
>> > + FLIF16_PLANE_ALPHA,
>> > + FLIF16_PLANE_LOOKBACK, // Frame lookback
>> > + FLIF16_PLANE_GRAY = 0, // Is this needed?
>> > +} FLIF16Plane;
>> > +
>> > +typedef enum FLIF16PlaneMode {
>> > + FLIF16_PLANEMODE_CONSTANT = 0, ///< A true constant plane
>> > + FLIF16_PLANEMODE_NORMAL, ///< A normal pixel matrix
>> > + FLIF16_PLANEMODE_FILL /**< A constant plane that is
>> later manipulated
>> > + by transforms, making it
>> nonconstant and
>> > + allocating a plane for it */
>> > +
>> > +} FLIF16PlaneMode;
>> > +
>> > +typedef struct FLIF16PixelData {
>>
>> > + int8_t seen_before; // Required by FrameDup
>> > + uint32_t *col_begin; // Required by FrameShape
>> > + uint32_t *col_end; // Required by FrameShape
>> > + int s_r[MAX_PLANES];
>> > + int s_c[MAX_PLANES];
>> > + void **data;
>>
>> Nit: Larger fields first, smaller fields last, to avoid padding. Same at
>> other places.
>>
>> > +} FLIF16PixelData;
>> > +
>> > +typedef int32_t FLIF16ColorVal;
>> > +
>> > +typedef struct FLIF16Context {
>> > + GetByteContext gb;
>> > + FLIF16MANIACContext maniac_ctx;
>> > + FLIF16RangeCoder rc;
>> > +
>>
>> > + // Dimensions and other things.
>> > + uint32_t width;
>> > + uint32_t height;
>> > + uint32_t num_frames;
>> > + uint32_t meta; ///< Size of a meta chunk
>> > +
>> > + // Primary Header
>> > + uint8_t ia; ///< Is image interlaced or/and animated or
>> not
>> > + uint32_t bpc; ///< 2 ^ Bytes per channel
>> > + uint8_t num_planes; ///< Number of planes
>> > + uint8_t loops; ///< Number of times animation loops
>> > + uint16_t *framedelay; ///< Frame delay for each frame
>> > + uint8_t plane_mode[MAX_PLANES];
>> > +
>> > + // Transform flags
>> > + uint8_t framedup;
>> > + uint8_t frameshape;
>> > + uint8_t framelookback;
>> > +} FLIF16Context;
>> > +
>> > +typedef struct FLIF16RangesContext {
>> > + uint8_t r_no;
>> > + uint8_t num_planes;
>>
>> > + void* priv_data;
>>
>> Nit: "void *priv_data".
>>
>> > +} FLIF16RangesContext;
>> > +
>> > +typedef struct FLIF16Ranges {
>> > + uint8_t priv_data_size;
>> > +
>> > + FLIF16ColorVal (*min)(FLIF16RangesContext *ranges, int plane);
>> > + FLIF16ColorVal (*max)(FLIF16RangesContext *ranges, int plane);
>> > + void (*minmax)(FLIF16RangesContext *ranges, const int plane,
>> > + FLIF16ColorVal *prev_planes, FLIF16ColorVal *minv,
>> > + FLIF16ColorVal *maxv);
>> > + void (*snap)(FLIF16RangesContext*, const int, FLIF16ColorVal*,
>> > + FLIF16ColorVal*, FLIF16ColorVal*, FLIF16ColorVal*);
>> > + uint8_t is_static;
>> > + void (*close)(FLIF16RangesContext*);
>> > + void (*previous)(FLIF16RangesContext*); //TODO : Maybe remove it
>> later
>> > +} FLIF16Ranges;
>> > +
>>
>> > +typedef struct FLIF16TransformContext{
>>
>> Nit: space.
>>
>> > + uint8_t t_no;
>> > + unsigned int segment; ///< Segment the code is executing in.
>> > + int i; ///< Variable to store iteration number.
>> > + uint8_t done;
>> > + void *priv_data;
>> > +} FLIF16TransformContext;
>> > +
>> > +typedef struct FLIF16Transform {
>> > + int16_t priv_data_size;
>> > + //Functions
>> > + int (*init) (FLIF16TransformContext *t_ctx, FLIF16RangesContext
>> *r_ctx);
>> > + int (*read) (FLIF16TransformContext *t_ctx, FLIF16Context *ctx,
>> > + FLIF16RangesContext *r_ctx);
>> > + FLIF16RangesContext *(*meta) (FLIF16Context *ctx,
>> > + FLIF16PixelData *frame, uint32_t
>> frame_count,
>> > + FLIF16TransformContext *t_ctx,
>> > + FLIF16RangesContext *r_ctx);
>> > + int (*forward) (FLIF16Context *ctx, FLIF16TransformContext *t_ctx,
>> FLIF16PixelData *frame);
>> > + int (*reverse) (FLIF16Context *ctx, FLIF16TransformContext *t_ctx,
>> FLIF16PixelData *frame,
>> > + uint32_t stride_row, uint32_t stride_col);
>> > + void (*configure) (FLIF16TransformContext *, const int);
>> > + void (*close) (FLIF16TransformContext *t_ctx);
>> > +} FLIF16Transform;
>> > +
>> > +int32_t (*ff_flif16_maniac_ni_prop_ranges_init(unsigned int
>> *prop_ranges_size,
>> > + FLIF16RangesContext
>> *ranges,
>> > + uint8_t property,
>> > + uint8_t channels))[2];
>> > +
>> > +int32_t (*ff_flif16_maniac_prop_ranges_init(unsigned int
>> *prop_ranges_size,
>> > + FLIF16RangesContext
>> *ranges,
>> > + uint8_t property,
>> > + uint8_t channels))[2];
>> > +
>> > +int ff_flif16_planes_init(FLIF16Context *s, FLIF16PixelData *frames,
>> > + uint8_t *is_const, uint8_t
>> *const_plane_value,
>> > + uint8_t lookback);
>> > +
>> > +FLIF16PixelData *ff_flif16_frames_init(FLIF16Context *s);
>> > +
>> > +void ff_flif16_frames_free(FLIF16PixelData **frames, uint32_t
>> num_frames,
>> > + uint32_t num_planes, uint8_t lookback);
>> > +
>> > +
>> > +
>> > +/*
>> > + * All constant plane pixel setting should be illegal in theory.
>> > + */
>> > +
>> > +static inline void ff_flif16_pixel_set(FLIF16Context *s,
>> FLIF16PixelData *frame,
>> > + uint8_t plane, uint32_t row,
>> uint32_t col,
>> > + FLIF16ColorVal value)
>> > +{
>> > + if (s->plane_mode[plane])
>> > + ((FLIF16ColorVal *) frame->data[plane])[s->width * row + col]
>> = value;
>> > + else
>> > + ((FLIF16ColorVal *) frame->data[plane])[0] = value;
>> > +}
>> > +
>> > +static inline FLIF16ColorVal ff_flif16_pixel_get(FLIF16Context *s,
>> > + FLIF16PixelData
>> *frame,
>> > + uint8_t plane,
>> uint32_t row,
>> > + uint32_t col)
>> > +{
>> > + if (s->plane_mode[plane])
>> > + return ((FLIF16ColorVal *) frame->data[plane])[s->width * row
>> + col];
>> > + else
>> > + return ((FLIF16ColorVal *) frame->data[plane])[0];
>> > +}
>> > +
>> > +
>> > +static inline void ff_flif16_pixel_setz(FLIF16Context *s,
>> > + FLIF16PixelData *frame,
>> > + uint8_t plane, int z, uint32_t
>> row,
>> > + uint32_t col, FLIF16ColorVal
>> value)
>> > +{
>> > + if (s->plane_mode[plane])
>> > + ((FLIF16ColorVal *) frame->data[plane])[(row *
>> ZOOM_ROWPIXELSIZE(z)) * s->width +
>> > + (col *
>> ZOOM_COLPIXELSIZE(z))] = value;
>> > + else
>> > + ((FLIF16ColorVal *) frame->data[plane])[0] = value;
>> > +}
>> > +
>> > +static inline FLIF16ColorVal ff_flif16_pixel_getz(FLIF16Context *s,
>> > + FLIF16PixelData
>> *frame,
>> > + uint8_t plane, int z,
>> > + size_t row, size_t
>> col)
>> > +{
>> > + if (s->plane_mode[plane])
>> > + return ((FLIF16ColorVal *) frame->data[plane])[(row *
>> ZOOM_ROWPIXELSIZE(z)) *
>> > + s->width + (col
>> * ZOOM_COLPIXELSIZE(z))];
>> > + else
>> > + return ((FLIF16ColorVal *) frame->data[plane])[0];
>> > +}
>> > +
>> > +static inline void ff_flif16_prepare_zoomlevel(FLIF16Context *s,
>> > + FLIF16PixelData *frame,
>> > + uint8_t plane, int z)
>> > +{
>> > + frame->s_r[plane] = ZOOM_ROWPIXELSIZE(z) * s->width;
>> > + frame->s_c[plane] = ZOOM_COLPIXELSIZE(z);
>> > +}
>> > +
>> > +static inline FLIF16ColorVal ff_flif16_pixel_get_fast(FLIF16Context *s,
>> > + FLIF16PixelData
>> *frame,
>> > + uint8_t plane,
>> uint32_t row,
>> > + uint32_t col)
>> > +{
>> > + if (s->plane_mode[plane])
>> > + return ((FLIF16ColorVal *) frame->data[plane])[row *
>> frame->s_r[plane] + col * frame->s_c[plane]];
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static inline void ff_flif16_pixel_set_fast(FLIF16Context *s,
>> > + FLIF16PixelData *frame,
>> > + uint8_t plane, uint32_t
>> row,
>> > + uint32_t col,
>> FLIF16ColorVal value)
>> > +{
>> > + if (s->plane_mode[plane])
>> > + ((FLIF16ColorVal *) frame->data[plane])[row *
>> frame->s_r[plane] + col * frame->s_c[plane]] = value;
>> > +}
>> > +
>> > +static inline void ff_flif16_copy_rows(FLIF16Context *s,
>> > + FLIF16PixelData *dest,
>> > + FLIF16PixelData *src, uint8_t
>> plane,
>> > + uint32_t row, uint32_t
>> col_start,
>> > + uint32_t col_end)
>> > +{
>> > + for(uint32_t col = col_start; col < col_end; ++col) {
>>
>> > + ff_flif16_pixel_set(s, dest, plane, row, col,
>> ff_flif16_pixel_get(s, src, plane, row, col));
>>
>> ff_flif16_pixel_get() and ff_flif16_pixel_set() hide some arithmetic,
>> re-computed for each pixel. I do not trust compilers to optimize them.
>>
>> Better take a pointer to the source, a pointer to the destination, and
>> increment them by the right amount.
>>
>> > + }
>> > +}
>> > +
>> > +static inline void ff_flif16_copy_rows_stride(FLIF16Context *s,
>> > + FLIF16PixelData *dest,
>> > + FLIF16PixelData *src,
>> uint8_t plane,
>> > + uint32_t row, uint32_t
>> col_start,
>> > + uint32_t col_end,
>> uint32_t stride)
>> > +{
>> > + for(uint32_t col = col_start; col < col_end; col += stride) {
>> > + ff_flif16_pixel_set(s, dest, plane, row, col,
>> ff_flif16_pixel_get(s, src, plane, row, col));
>> > + }
>>
>> Same.
>>
>> > +}
>> > +#endif /* AVCODEC_FLIF16_H */
>> > diff --git a/libavcodec/flif16_parser.c b/libavcodec/flif16_parser.c
>> > new file mode 100644
>> > index 0000000000..c795b44b4d
>> > --- /dev/null
>> > +++ b/libavcodec/flif16_parser.c
>> > @@ -0,0 +1,189 @@
>> > +/*
>> > + * FLIF16 parser
>> > + * Copyright (c) 2020 Anamitra Ghorui <aghorui at teknik.io>
>> > + *
>> > + * 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
>> > + * FLIF16 parser
>> > + */
>> > +
>> > +#include "flif16.h"
>> > +#include "parser.h"
>> > +#include "libavutil/avassert.h"
>> > +#include "libavutil/bswap.h"
>> > +
>> > +#include <stdio.h> //remove
>> > +#include <stdint.h>
>> > +#include <stdlib.h>
>> > +
>> > +typedef enum FLIF16ParseStates {
>>
>> FLIF16_INIT_STATE = 0,
>>
>> It is not elegant to use an enum and a value that is not part of the
>> enum.
>>
>> > + FLIF16_HEADER = 1,
>> > + FLIF16_METADATA,
>> > + FLIF16_BITSTREAM
>> > +} FLIF16ParseStates;
>> > +
>> > +typedef struct FLIF16ParseContext {
>> > + ParseContext pc;
>>
>> > + int state; ///< The section of the file the parser is in
>> currently.
>>
>> FLIF16ParseStates state;
>>
>> > + unsigned int index; ///< An index based on the current state.
>> > + uint8_t animated; ///< Is image animated or not
>> > + uint8_t varint; ///< Number of varints to process in sequence
>> > + uint32_t width;
>> > + uint32_t height;
>> > + uint32_t frames;
>> > + uint32_t meta; ///< Size of a meta chunk
>> > + uint32_t count;
>> > +} FLIF16ParseContext;
>> > +
>> > +
>> > +// TODO revamp this function
>> > +static int flif16_find_frame(FLIF16ParseContext *f, const uint8_t *buf,
>> > + int buf_size)
>> > +{
>> > + int next = END_NOT_FOUND;
>> > + int index;
>> > +
>> > + for (index = 0; index < buf_size; index++) {
>> > + if (!f->state) {
>> > + if (!memcmp(flif16_header, buf + index, 4))
>> > + f->state = FLIF16_HEADER;
>> > + ++f->index;
>>
>> > + } else if (f->state == FLIF16_HEADER) {
>>
>> switch (f->state)?
>>
>> > + if (f->index == 3 + 1) {
>> > + // See whether image is animated or not
>> > + f->animated = (((buf[index] >> 4) > 4)?1:0);
>> > + } else if (f->index == (3 + 1 + 1)) {
>> > + // Start - 1 of the first varint
>> > + f->varint = 1;
>> > + } else if (f->varint) {
>> > + // Count varint
>> > + if (f->count == 5)
>>
>> > + return AVERROR(ENOMEM);
>>
>> AVERROR_INVALIDDATA
>>
>> > +
>> > + switch (f->varint) {
>> > + case 1:
>> > + VARINT_APPEND(f->width, buf[index]);
>> > + break;
>> > +
>> > + case 2:
>> > + VARINT_APPEND(f->height, buf[index]);
>> > + break;
>> > +
>> > + case 3:
>> > + VARINT_APPEND(f->frames, buf[index]);
>> > + break;
>> > + }
>> > + if (buf[index] < 128) {
>> > + if (f->varint < (2 + f->animated)) {
>> > + switch (f->varint) {
>> > + case 1: f->width++; break;
>> > + case 2: f->height++; break;
>> > + }
>> > + f->varint++;
>> > + f->count = 0;
>> > + } else {
>> > + if (f->varint == 2)
>> > + f->height++;
>> > + if (f->animated)
>> > + f->frames += 2;
>> > + else
>> > + f->frames = 1;
>> > + f->state = FLIF16_METADATA;
>> > + f->varint = 0;
>> > + f->index = 0;
>> > + f->count = 0;
>> > + continue;
>> > + }
>> > + } else {
>> > + f->count++;
>> > + }
>> > + }
>> > + f->index++;
>> > + } else if (f->state == FLIF16_METADATA) {
>> > + if (f->index == 0) {
>> > + // Identifier for the bitstream chunk is a null byte.
>> > + if (buf[index] == 0) {
>> > + f->state = FLIF16_BITSTREAM;
>> > + return buf_size;
>> > + }
>> > + } else if (f->index < 3) {
>> > + // nop
>> > + } else if (f->index == 3) {
>> > + // Handle the size varint
>> > + f->varint = 1;
>> > + } else if (f->varint) {
>> > + if (f->count == 9)
>>
>> > + return AVERROR(ENOMEM);
>>
>> AVERROR_INVALIDDATA
>>
>> > + if (buf[index] < 128) {
>> > + f->varint = 0;
>> > + f->count = 0;
>> > + }
>> > + VARINT_APPEND(f->meta, buf[index]);
>> > + f->count++;
>> > + } else if (f->meta > 1) {
>> > + // increment varint until equal to size
>> > + f->meta--;
>> > + } else {
>> > + f->meta = 0;
>> > + f->index = 0;
>> > + continue;
>> > + }
>> > + f->index++;
>> > + } else if (f->state == FLIF16_BITSTREAM) {
>> > + /* Since we cannot find the end of the bitstream without
>> any
>> > + * processing, we will simply return each read chunk as a
>> packet
>> > + * to the decoder.
>> > + */
>> > + printf("<Bitstream chunk size %dd>\n", buf_size);
>> > + return buf_size;
>> > + }
>> > + }
>> > + printf("End not found\n");
>> > + return next;
>> > +}
>> > +
>> > +static int flif16_parse(AVCodecParserContext *s, AVCodecContext *avctx,
>> > + const uint8_t **poutbuf, int *poutbuf_size,
>> > + const uint8_t *buf, int buf_size)
>> > +{
>> > + FLIF16ParseContext *fpc = s->priv_data;
>> > + int next;
>> > +
>> > + next = flif16_find_frame(fpc, buf, buf_size);
>> > +
>> > + if (ff_combine_frame(&fpc->pc, next, &buf, &buf_size) < 0) {
>> > + *poutbuf = NULL;
>> > + *poutbuf_size = 0;
>> > + return buf_size;
>> > + }
>> > + printf("Width:%u\nHeight:%u\nFrames:%u\nEnd:%d\n",
>> > + fpc->width, fpc->height, fpc->frames, buf_size);
>> > + *poutbuf = buf;
>> > + *poutbuf_size = buf_size;
>> > + return next;
>> > +}
>> > +
>> > +AVCodecParser ff_flif16_parser = {
>> > + .codec_ids = { AV_CODEC_ID_FLIF16 },
>> > + .priv_data_size = sizeof(FLIF16ParseContext),
>> > + .parser_parse = flif16_parse,
>> > + .parser_close = ff_parse_close,
>> > +};
>> > +
>> > diff --git a/libavcodec/flif16_rangecoder.c
>> b/libavcodec/flif16_rangecoder.c
>> > new file mode 100644
>> > index 0000000000..c8f1b7bbb0
>> > --- /dev/null
>> > +++ b/libavcodec/flif16_rangecoder.c
>> > @@ -0,0 +1,464 @@
>> > +/*
>> > + * Range coder for FLIF16
>>
>> > + * Copyright (c) 2004, Michael Niedermayer,
>>
>> This looks like new code. Can you explain where Michael's copyright
>> comes from?
>>
>> > + * 2010-2016, Jon Sneyers & Pieter Wuille,
>>
>> Same here.
>>
>> > + * 2020, Anamitra Ghorui <aghorui at teknik.io>
>> > + *
>> > + * 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
>> > + * Range coder for FLIF16
>> > + */
>> > +
>> > +#include "avcodec.h"
>> > +#include "libavutil/common.h"
>> > +#include "flif16_rangecoder.h"
>> > +#include "flif16.h"
>> > +
>> > +// TODO write separate function for RAC encoder
>> > +
>> > +// The coder requires a certain number of bytes for initiialization.
>> buf
>> > +// provides it. gb is used by the coder functions for actual coding.
>> > +void ff_flif16_rac_init(FLIF16RangeCoder *rc, GetByteContext *gb,
>> uint8_t *buf,
>> > + uint8_t buf_size)
>> > +{
>> > + GetByteContext gbi;
>> > + if(!rc)
>> > + return;
>> > +
>> > + if(buf_size < FLIF16_RAC_MAX_RANGE_BYTES)
>> > + return;
>> > +
>> > + bytestream2_init(&gbi, buf, buf_size);
>> > +
>> > + rc->range = FLIF16_RAC_MAX_RANGE;
>> > + rc->gb = gb;
>> > +
>>
>> > + for (uint32_t r = FLIF16_RAC_MAX_RANGE; r > 1; r >>= 8) {
>> > + rc->low <<= 8;
>> > + rc->low |= bytestream2_get_byte(&gbi);
>> > + }
>>
>> Do you need bytestream2_get_byte() for that? Testing that buf_size is
>> large enough at the beginning and directly accessing buf seems simpler
>> and more efficient.
>>
>> > +}
>> > +
>> > +void ff_flif16_rac_free(FLIF16RangeCoder *rc)
>> > +{
>> > + if (!rc)
>> > + return;
>>
>> > + av_freep(rc);
>>
>> Was this tested? av_freep() wants a pointer to pointer.
>>
>> > +}
>> > +
>> > +// TODO Maybe restructure rangecoder.c/h to fit a more generic case
>> > +static void build_table(uint16_t *zero_state, uint16_t *one_state,
>> size_t size,
>> > + uint32_t factor, unsigned int max_p)
>> > +{
>> > + const int64_t one = 1LL << 32;
>> > + int64_t p = one / 2;
>> > + unsigned int last_p8 = 0, p8;
>> > + unsigned int i;
>> > +
>> > + for (i = 0; i < size / 2; i++) {
>> > + p8 = (size * p + one / 2) >> 32;
>> > + if (p8 <= last_p8)
>> > + p8 = last_p8 + 1;
>> > + if (last_p8 && last_p8 < size && p8 <= max_p)
>> > + one_state[last_p8] = p8;
>> > + p += ((one - p) * factor + one / 2) >> 32;
>> > + last_p8 = p8;
>> > + }
>> > +
>> > + for (i = size - max_p; i <= max_p; i++) {
>> > + if (one_state[i])
>> > + continue;
>> > + p = (i * one + size / 2) / size;
>> > + p += ((one - p) * factor + one / 2) >> 32;
>> > + p8 = (size * p + one / 2) >> 32; //FIXME try without the one
>> > + if (p8 <= i)
>> > + p8 = i + 1;
>> > + if (p8 > max_p)
>> > + p8 = max_p;
>> > + one_state[i] = p8;
>> > + }
>> > +
>> > + for (i = 1; i < size; i++)
>> > + zero_state[i] = size - one_state[size - i];
>> > +}
>> > +
>> > +static inline uint32_t log4kf(int x, uint32_t base)
>> > +{
>>
>> > + int bits = 8 * sizeof(int) - ff_clz(x);
>>
>> Code relying on sizeof(int) for anything but allocating memory is very
>> suspicious.
>>
>> > + uint64_t y = ((uint64_t)x) << (32 - bits);
>> > + uint32_t res = base * (13 - bits);
>> > + uint32_t add = base;
>> > + while ((add > 1) && ((y & 0x7FFFFFFF) != 0)) {
>> > + y = (((uint64_t)y) * y + 0x40000000) >> 31;
>> > + add >>= 1;
>> > + if ((y >> 32) != 0) {
>> > + res -= add;
>> > + y >>= 1;
>> > + }
>> > + }
>> > + return res;
>> > +}
>> > +
>> > +void ff_flif16_build_log4k_table(FLIF16Log4kTable *log4k)
>> > +{
>> > + log4k->table[0] = 0;
>> > + for (int i = 1; i < 4096; i++)
>> > + log4k->table[i] = (log4kf(i, (65535UL << 16) / 12) +
>> > + (1 << 15)) >> 16;
>> > + log4k->scale = 65535 / 12;
>> > +}
>> > +
>> > +void ff_flif16_chancetable_init(FLIF16ChanceTable *ct, int alpha, int
>> cut)
>> > +{
>> > + build_table(ct->zero_state, ct->one_state, 4096, alpha, 4096 -
>> cut);
>> > +}
>> > +
>> > +void ff_flif16_chancecontext_init(FLIF16ChanceContext *ctx)
>> > +{
>>
>> > + if(!ctx)
>> > + return;
>>
>> You never call this except with &something as argument: remove this
>> useless check, and let your code crash if you get something wrong when
>> debugging.
>>
>> > + memcpy(&ctx->data, &flif16_nz_int_chances,
>> sizeof(flif16_nz_int_chances));
>> > +}
>> > +
>> > +#ifdef MULTISCALE_CHANCES_ENABLED
>> > +FLIF16MultiscaleChanceTable
>> *ff_flif16_multiscale_chancetable_init(void)
>> > +{
>> > + unsigned int len = MULTISCALE_CHANCETABLE_DEFAULT_SIZE;
>> > + FLIF16MultiscaleChanceTable *ct = av_malloc(sizeof(*ct));
>> > + if (!ct)
>> > + return null
>> > + for (int i = 0; i < len; ++i) {
>> > + ff_flif16_chancetable_init(&ct->sub_table[i],
>> > + flif16_multiscale_alphas[i],
>> > + MULTISCALE_CHANCETABLE_DEFAULT_CUT);
>> > + }
>> > + return ct;
>> > +}
>> > +
>> > +/**
>> > + * Allocate and set all chances according to flif16_nz_int_chances
>> > + */
>> > +void
>> ff_flif16_multiscale_chancecontext_init(FLIF16MultiscaleChanceContext *ctx)
>> > +{
>>
>> > + for (int i = 0; i < sizeof(flif16_nz_int_chances) /
>> > + sizeof(flif16_nz_int_chances[0]); ++i)
>>
>> FF_ARRAY_ELEMS(); possibly same in other places.
>>
>> > + ff_flif16_multiscale_chance_set(&ctx->data[i],
>> flif16_nz_int_chances[i]);
>> > + return ctx;
>> > +}
>> > +
>> > +#endif
>> > +
>> > +int ff_flif16_read_maniac_tree(FLIF16RangeCoder *rc,
>> > + FLIF16MANIACContext *m,
>> > + int32_t (*prop_ranges)[2],
>> > + unsigned int prop_ranges_size,
>> > + unsigned int channel)
>> > +{
>> > + int oldp = 0, p = 0, split_val = 0, temp;
>> > +
>> > + switch (rc->segment2) {
>>
>> > + default: case 0:
>>
>> Nit: do not indent cases more than switch().
>>
>> > + rc->segment2 = 0;
>> > + if (!(m->forest[channel])) {
>> > + m->forest[channel] =
>> av_mallocz(sizeof(*(m->forest[channel])));
>> > + if (!(m->forest[channel]))
>> > + return AVERROR(ENOMEM);
>> > + m->forest[channel]->data =
>> av_mallocz(MANIAC_TREE_BASE_SIZE *
>> > +
>> sizeof(*(m->forest[channel]->data)));
>>
>> av_mallocz_array().
>>
>> > + if (!m->forest[channel]->data)
>> > + return AVERROR(ENOMEM);
>> > +
>> > + m->stack = av_mallocz(MANIAC_TREE_BASE_SIZE *
>> sizeof(*(m->stack)));
>>
>> Same.
>>
>> > +
>> > + if (!(m->stack))
>>
>> Nit: parentheses not necessary.
>>
>> > + return AVERROR(ENOMEM);
>> > +
>> > + for (int i = 0; i < 3; ++i) {
>> > + #ifdef MULTISCALE_CHANCES_ENABLED
>> > +
>> ff_flif16_multiscale_chancecontext_init(&m->ctx[i]);
>> > + #else
>> > + ff_flif16_chancecontext_init(&m->ctx[i]);
>> > + #endif
>> > + }
>> > + m->stack_top = m->tree_top = 0;
>>
>> > + m->forest[channel]->size = MANIAC_TREE_BASE_SIZE;
>>
>> Strange spacing.
>>
>> > + m->stack_size = MANIAC_TREE_BASE_SIZE;
>> > + m->stack[m->stack_top].id = m->tree_top;
>> > + m->stack[m->stack_top].mode = 0;
>> > + ++m->stack_top;
>> > + ++m->tree_top;
>> > + }
>> > + ++rc->segment2;
>> > +
>> > + case 1:
>>
>> > + start:
>> > + if(!m->stack_top)
>> > + goto end;
>> > +
>>
>> Looks like precisely the kind of code for which the rule "don't use
>> goto" was coined. Better make it a proper loop.
>>
>> > + oldp = m->stack[m->stack_top - 1].p;
>> > + if (!m->stack[m->stack_top - 1].visited) {
>> > + switch (m->stack[m->stack_top - 1].mode) {
>> > + case 1:
>> > + prop_ranges[oldp][0] = m->stack[m->stack_top -
>> 1].min;
>> > + prop_ranges[oldp][1] = m->stack[m->stack_top -
>> 1].max;
>> > + break;
>> > +
>> > + case 2:
>> > + prop_ranges[oldp][0] = m->stack[m->stack_top -
>> 1].min;
>> > + break;
>> > + }
>> > + } else {
>> > + prop_ranges[oldp][1] = m->stack[m->stack_top - 1].max2;
>> > + --m->stack_top;
>> > + rc->segment2 = 1;
>> > + goto start;
>> > + }
>> > + m->stack[m->stack_top - 1].visited = 1;
>> > + ++rc->segment2;
>> > +
>> > + case 2:
>> > + #ifdef MULTISCALE_CHANCES_ENABLED
>> > + RAC_GET(rc, &m->ctx[0], 0, prop_ranges_size,
>> > + &m->forest[channel]->data[m->stack[m->stack_top -
>> 1].id].property,
>> > + FLIF16_RAC_GNZ_MULTISCALE_INT);
>> > + #else
>> > + RAC_GET(rc, &m->ctx[0], 0, prop_ranges_size,
>> > + &m->forest[channel]->data[m->stack[m->stack_top -
>> 1].id].property,
>> > + FLIF16_RAC_GNZ_INT);
>> > + #endif
>> > + p = --(m->forest[channel]->data[m->stack[m->stack_top -
>> 1].id].property);
>> > + if (p == -1) {
>> > + --m->stack_top;
>> > + rc->segment2 = 1;
>> > + goto start;
>> > + }
>> > +
>> > + m->forest[channel]->data[m->stack[m->stack_top -
>> 1].id].child_id = m->tree_top;
>> > + rc->oldmin = prop_ranges[p][0];
>> > + rc->oldmax = prop_ranges[p][1];
>> > + if (rc->oldmin >= rc->oldmax) {
>> > + printf("!!! rc->oldmin >= rc->oldmax\n");
>>
>> > + return AVERROR(EINVAL);
>>
>> AVERROR_INVALIDDATA
>>
>> > + }
>> > + ++rc->segment2;
>> > +
>> > + case 3:
>> > + #ifdef MULTISCALE_CHANCES_ENABLED
>> > + RAC_GET(rc, &m->ctx[1], MANIAC_TREE_MIN_COUNT,
>> MANIAC_TREE_MAX_COUNT,
>> > + &m->forest[channel]->data[m->stack[m->stack_top -
>> 1].id].count,
>> > + FLIF16_RAC_GNZ_MULTISCALE_INT);
>> > + #else
>> > + RAC_GET(rc, &m->ctx[1], MANIAC_TREE_MIN_COUNT,
>> MANIAC_TREE_MAX_COUNT,
>> > + &m->forest[channel]->data[m->stack[m->stack_top -
>> 1].id].count,
>> > + FLIF16_RAC_GNZ_INT);
>> > + #endif
>> > + ++rc->segment2;
>> > +
>> > + case 4:
>> > + #ifdef MULTISCALE_CHANCES_ENABLED
>> > + RAC_GET(rc, &m->ctx[2], rc->oldmin, rc->oldmax - 1,
>> > + &m->forest[channel]->data[m->stack[m->stack_top -
>> 1].id].split_val,
>> > + FLIF16_RAC_GNZ_MULTISCALE_INT);
>> > + #else
>> > + RAC_GET(rc, &m->ctx[2], rc->oldmin, rc->oldmax - 1,
>> > + &m->forest[channel]->data[m->stack[m->stack_top -
>> 1].id].split_val,
>> > + FLIF16_RAC_GNZ_INT);
>> > + #endif
>> > + split_val = m->forest[channel]->data[m->stack[m->stack_top
>> - 1].id].split_val;
>> > + ++rc->segment2;
>> > +
>> > + case 5:
>> > + if ((m->tree_top + 2) >= m->forest[channel]->size) {
>> > + m->forest[channel]->data =
>> av_realloc(m->forest[channel]->data,
>> > + (m->forest[channel]->size) * 2 *
>> sizeof(*(m->forest[channel]->data)));
>> > + if(!(m->forest[channel]->data))
>> > + return AVERROR(ENOMEM);
>> > + m->forest[channel]->size *= 2;
>> > + }
>> > +
>> > + if ((m->stack_top + 2) >= m->stack_size) {
>>
>> > + m->stack = av_realloc(m->stack, (m->stack_size) * 2 *
>> sizeof(*(m->stack)));
>>
>> av_realloc_array()
>>
>> > + if(!(m->stack))
>> > + return AVERROR(ENOMEM);
>>
>> This leaks the old m->stack. See av_realloc_f().
>>
>> > + m->stack_size *= 2;
>> > + }
>> > +
>> > + temp = m->forest[channel]->data[m->stack[m->stack_top -
>> 1].id].property;
>> > +
>> > + // Parent
>> > + m->stack[m->stack_top - 1].p = temp;
>> > + m->stack[m->stack_top - 1].max2 = rc->oldmax;
>> > +
>> > + // Right child
>> > + m->stack[m->stack_top].id = m->tree_top + 1;
>> > + m->stack[m->stack_top].p = temp;
>> > + m->stack[m->stack_top].min = rc->oldmin;
>> > + m->stack[m->stack_top].max = split_val;
>> > + m->stack[m->stack_top].mode = 1;
>> > + m->stack[m->stack_top].visited = 0;
>> > + ++m->stack_top;
>> > +
>> > + // Left Child
>> > + m->stack[m->stack_top].id = m->tree_top;
>> > + m->stack[m->stack_top].p = temp;
>> > + m->stack[m->stack_top].min = split_val + 1;
>> > + m->stack[m->stack_top].mode = 2;
>> > + m->stack[m->stack_top].visited = 0;
>> > + ++m->stack_top;
>> > +
>> > + m->tree_top += 2;
>> > + rc->segment2 = 1;
>> > + goto start;
>> > + }
>> > +
>> > + end:
>> > + m->forest[channel]->data = av_realloc(m->forest[channel]->data,
>> > + m->tree_top *
>> sizeof(*m->forest[channel]->data)); // Maybe replace by fast realloc
>> > + if (!m->forest[channel]->data)
>> > + return AVERROR(ENOMEM);
>> > + m->forest[channel]->size = m->tree_top;
>> > + av_freep(&m->stack);
>> > + m->stack_top = 0;
>> > + rc->segment2 = 0;
>> > + return 0;
>> > +
>> > + need_more_data:
>> > + return AVERROR(EAGAIN);
>> > +}
>> > +
>> > +void ff_flif16_maniac_close(FLIF16MANIACContext *m, uint8_t
>> num_planes)
>> > +{
>> > + for (int i = 0; i < num_planes; ++i) {
>> > + if (!m->forest[i])
>> > + continue;
>> > + if (m->forest[i]->data)
>> > + av_freep(&m->forest[i]->data);
>> > + if (m->forest[i]->leaves)
>> > + av_freep(&m->forest[i]->leaves);
>> > + av_freep(&m->forest[i]);
>> > + }
>> > +
>> > + av_freep(&m->forest);
>> > + // Should be already freed in maniac reading, but checking anyway.
>> > + if(m->stack)
>> > + av_freep(&m->stack);
>> > +}
>> > +
>> > +#ifdef MULTISCALE_CHANCES_ENABLED
>> > +FLIF16MultiscaleChanceContext
>> *ff_flif16_maniac_findleaf(FLIF16MANIACContext *m,
>> > + uint8_t
>> channel,
>> > + int32_t
>> *properties)
>> > +#else
>> > +FLIF16ChanceContext *ff_flif16_maniac_findleaf(FLIF16MANIACContext *m,
>> > + uint8_t channel,
>> > + int32_t *properties)
>> > +#endif
>> > +{
>> > + unsigned int pos = 0;
>> > + uint32_t old_leaf;
>> > + uint32_t new_leaf;
>> > + FLIF16MANIACTree *tree = m->forest[channel];
>> > + FLIF16MANIACNode *nodes = tree->data;
>> > +
>> > + if (!m->forest[channel]->leaves) {
>> > + m->forest[channel]->leaves = av_mallocz(MANIAC_TREE_BASE_SIZE *
>> > +
>> sizeof(*m->forest[channel]->leaves));
>>
>> av_mallocz_array();
>>
>> > + m->forest[channel]->leaves_size = MANIAC_TREE_BASE_SIZE;
>> > + if(!m->forest[channel]->leaves)
>> > + return NULL;
>> > + ff_flif16_chancecontext_init(&m->forest[channel]->leaves[0]);
>> > + tree->leaves_top = 1;
>> > + }
>> > +
>> > + while (nodes[pos].property != -1) {
>> > + if (nodes[pos].count < 0) {
>> > + if (properties[nodes[pos].property] > nodes[pos].split_val)
>> > + pos = nodes[pos].child_id;
>> > + else
>> > + pos = nodes[pos].child_id + 1;
>> > + } else if (nodes[pos].count > 0) {
>> > + --nodes[pos].count;
>> > + break;
>> > + } else {
>> > + --nodes[pos].count;
>> > + if ((tree->leaves_top) >= tree->leaves_size) {
>> > + m->forest[channel]->leaves =
>> av_realloc(m->forest[channel]->leaves,
>> > +
>> sizeof(*m->forest[channel]->leaves) *
>> > +
>> m->forest[channel]->leaves_size * 2);
>>
>> > + if (!m->forest[channel]->leaves)
>> > + return NULL;
>>
>> This leaks old leaves.
>>
>> > + m->forest[channel]->leaves_size *= 2;
>> > + }
>> > + old_leaf = nodes[pos].leaf_id;
>> > + new_leaf = tree->leaves_top;
>> > + memcpy(&m->forest[channel]->leaves[tree->leaves_top],
>> > + &m->forest[channel]->leaves[nodes[pos].leaf_id],
>> > + sizeof(*m->forest[channel]->leaves));
>> > + ++tree->leaves_top;
>> > + nodes[nodes[pos].child_id].leaf_id = old_leaf;
>> > + nodes[nodes[pos].child_id + 1].leaf_id = new_leaf;
>> > +
>> > + if (properties[nodes[pos].property] > nodes[pos].split_val)
>> > + return &m->forest[channel]->leaves[old_leaf];
>> > + else
>> > + return &m->forest[channel]->leaves[new_leaf];
>> > + }
>> > + }
>> > + return
>> &m->forest[channel]->leaves[m->forest[channel]->data[pos].leaf_id];
>> > +}
>> > +
>> > +int ff_flif16_maniac_read_int(FLIF16RangeCoder *rc,
>> > + FLIF16MANIACContext *m,
>> > + int32_t *properties,
>> > + uint8_t channel,
>> > + int min, int max, int *target)
>> > +{
>> > + if (!rc->maniac_ctx)
>> > + rc->segment2 = 0;
>> > +
>> > + switch(rc->segment2) {
>> > + case 0:
>> > + if (min == max) {
>> > + *target = min;
>> > + goto end;
>> > + }
>> > + rc->maniac_ctx = ff_flif16_maniac_findleaf(m, channel,
>> properties);
>> > + if(!rc->maniac_ctx) {
>> > + return AVERROR(ENOMEM);
>> > + }
>> > + ++rc->segment2;
>> > +
>> > + case 1:
>> > + #ifdef MULTISCALE_CHANCES_ENABLED
>> > + RAC_GET(rc, rc->maniac_ctx, min, max, target,
>> FLIF16_RAC_NZ_MULTISCALE_INT);
>> > + #else
>> > + RAC_GET(rc, rc->maniac_ctx, min, max, target,
>> FLIF16_RAC_NZ_INT);
>> > + #endif
>> > +
>> > + }
>> > +
>> > + end:
>> > + rc->maniac_ctx = NULL;
>> > + rc->segment2 = 0;
>> > + return 1;
>> > +
>> > + need_more_data:
>> > + return 0;
>> > +}
>> > diff --git a/libavcodec/flif16_rangecoder.h
>> b/libavcodec/flif16_rangecoder.h
>> > new file mode 100644
>> > index 0000000000..9cd2d5ee22
>> > --- /dev/null
>> > +++ b/libavcodec/flif16_rangecoder.h
>> > @@ -0,0 +1,824 @@
>> > +/*
>> > + * Range coder for FLIF16
>> > + * Copyright (c) 2020 Anamitra Ghorui <aghorui at teknik.io>
>> > + *
>> > + * 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
>> > + * Range coder for FLIF16.
>> > + */
>> > +
>> > +#ifndef FLIF16_RANGECODER_H
>> > +#define FLIF16_RANGECODER_H
>> > +
>> > +#include "libavutil/mem.h"
>> > +#include "libavutil/intmath.h"
>> > +#include "bytestream.h"
>> > +#include "rangecoder.h"
>> > +
>> > +#include <stdint.h>
>> > +
>> > +
>> > +#define FLIF16_RAC_MAX_RANGE_BITS 24
>> > +#define FLIF16_RAC_MAX_RANGE_BYTES (FLIF16_RAC_MAX_RANGE_BITS / 8)
>> > +#define FLIF16_RAC_MIN_RANGE_BITS 16
>> > +#define FLIF16_RAC_MAX_RANGE (uint32_t) 1 << FLIF16_RAC_MAX_RANGE_BITS
>> > +#define FLIF16_RAC_MIN_RANGE (uint32_t) 1 << FLIF16_RAC_MIN_RANGE_BITS
>> > +
>> > +#define CHANCETABLE_DEFAULT_ALPHA (0xFFFFFFFF / 19)
>> > +#define CHANCETABLE_DEFAULT_CUT 2
>> > +
>> > +// #define MULTISCALE_CHANCES_ENABLED
>> > +
>> > +#define MULTISCALE_CHANCETABLE_DEFAULT_SIZE 6
>> > +#define MULTISCALE_CHANCETABLE_DEFAULT_CUT 8
>> > +
>> > +#define MANIAC_TREE_BASE_SIZE 1600
>> > +#define MANIAC_TREE_MIN_COUNT 1
>> > +#define MANIAC_TREE_MAX_COUNT 512
>> > +
>> > +typedef enum FLIF16RACReader {
>> > + FLIF16_RAC_BIT = 0,
>> > + FLIF16_RAC_UNI_INT8,
>> > + FLIF16_RAC_UNI_INT16,
>> > + FLIF16_RAC_UNI_INT32,
>> > + FLIF16_RAC_CHANCE,
>> > + FLIF16_RAC_NZ_INT,
>> > + FLIF16_RAC_GNZ_INT,
>> > +#ifdef MULTISCALE_CHANCES_ENABLED
>> > + FLIF16_RAC_NZ_MULTISCALE_INT,
>> > + FLIF16_RAC_GNZ_MULTISCALE_INT
>> > +#endif
>> > +} FLIF16RACReader;
>> > +
>> > +typedef struct FLIF16ChanceTable {
>> > + uint16_t zero_state[4096];
>> > + uint16_t one_state[4096];
>> > +} FLIF16ChanceTable;
>> > +
>> > +typedef struct FLIF16MultiscaleChanceTable {
>> > + FLIF16ChanceTable sub_table[MULTISCALE_CHANCETABLE_DEFAULT_SIZE];
>> > +} FLIF16MultiscaleChanceTable;
>> > +
>> > +
>> > +typedef struct FLIF16Log4kTable {
>> > + uint16_t table[4097];
>> > + int scale;
>> > +} FLIF16Log4kTable;
>> > +
>>
>> > +static const uint32_t flif16_multiscale_alphas[] = {
>> > + 21590903, 66728412, 214748365, 7413105, 106514140, 10478104
>> > +};
>>
>> Please add a short comment to explain.
>>
>> > +
>> > +typedef struct FLIF16MultiscaleChance {
>> > + uint16_t chances[MULTISCALE_CHANCETABLE_DEFAULT_SIZE];
>> > + uint32_t quality[MULTISCALE_CHANCETABLE_DEFAULT_SIZE];
>> > + uint8_t best;
>> > +} FLIF16MultiscaleChance;
>> > +
>> > +static uint16_t flif16_nz_int_chances[] = {
>> > + 1000, // ZERO
>> > + 2048, // SIGN (0) (1)
>> > + 1000, 1000, // EXP: 0, 1
>> > + 1200, 1200, // EXP: 2, 3
>> > + 1500, 1500, // EXP: 4, 5
>> > + 1750, 1750, // EXP: 6, 7
>> > + 2000, 2000, // EXP: 8, 9
>> > + 2300, 2300, // EXP: 10, 11
>> > + 2800, 2800, // EXP: 12, 13
>> > + 2400, 2400, // EXP: 14, 15
>> > + 2300, 2300, // EXP: 16, 17
>> > + 2048, 2048, // EXP: 18, 19
>> > + 2048, 2048, // EXP: 20, 21
>> > + 2048, 2048, // EXP: 22, 23
>> > + 2048, 2048, // EXP: 24, 25
>> > + 2048, 2048, // EXP: 26, 27
>> > + 2048, 2048, // EXP: 28, 29
>> > + 2048, 2048, // EXP: 30, 31
>> > + 2048, 2048, // EXP: 32, 33
>> > + 1900, // MANT: 0
>> > + 1850, // MANT: 1
>> > + 1800, // MANT: 2
>> > + 1750, // MANT: 3
>> > + 1650, // MANT: 4
>> > + 1600, // MANT: 5
>> > + 1600, // MANT: 6
>> > + 2048, // MANT: 7
>> > + 2048, // MANT: 8
>> > + 2048, // MANT: 9
>> > + 2048, // MANT: 10
>> > + 2048, // MANT: 11
>> > + 2048, // MANT: 12
>> > + 2048, // MANT: 13
>> > + 2048, // MANT: 14
>> > + 2048, // MANT: 15
>> > + 2048, // MANT: 16
>> > + 2048 // MANT: 17
>> > +};
>> > +
>> > +#define NZ_INT_ZERO (0)
>> > +#define NZ_INT_SIGN (1)
>> > +#define NZ_INT_EXP(k) ((2 + (k)))
>> > +#define NZ_INT_MANT(k) ((36 + (k)))
>> > +
>> > +
>> > +typedef struct FLIF16MultiscaleChanceContext {
>> > + FLIF16MultiscaleChance data[sizeof(flif16_nz_int_chances) /
>> sizeof(flif16_nz_int_chances[0])];
>> > +} FLIF16MultiscaleChanceContext;
>> > +
>> > +// Maybe rename to symbol context
>> > +typedef struct FLIF16ChanceContext {
>>
>> > + uint16_t data[sizeof(flif16_nz_int_chances) /
>> sizeof(flif16_nz_int_chances[0])];
>>
>> FF_ARRAY_ELEMS()
>>
>> > +} FLIF16ChanceContext;
>> > +
>> > +typedef struct FLIF16RangeCoder {
>> > + uint_fast32_t range;
>> > + uint_fast32_t low;
>> > + uint16_t chance;
>> > + uint8_t active; ///< Is an integer reader currently active (to
>> save/
>> > + /// transfer state)
>> > +
>> > + // uni_int state management
>> > + uint32_t min;
>> > + uint32_t len;
>> > +
>> > + // nz_int state management
>> > + uint8_t segment; ///< The "segment" the function currently is in
>> > + uint8_t sign;
>> > + int amin, amax, emax, e, have, left, minabs1, maxabs0, pos;
>> > +
>> > + // maniac_int state management
>> > + uint8_t segment2;
>> > + int oldmin, oldmax;
>> > +
>> > + #ifdef MULTISCALE_CHANCES_ENABLED
>> > + FLIF16MultiscaleChanceContext *maniac_ctx;
>> > + #else
>> > + FLIF16ChanceContext *maniac_ctx;
>> > + #endif
>> > +
>> > + FLIF16ChanceTable ct;
>> > +#ifdef MULTISCALE_CHANCES_ENABLED
>> > + FLIF16MultiscaleChanceTable *mct;
>> > + FLIF16Log4kTable log4k;
>> > +#endif
>> > + GetByteContext *gb;
>> > +} FLIF16RangeCoder;
>> > +
>> > +/**
>> > + * The Stack used to construct the MANIAC tree
>> > + */
>> > +typedef struct FLIF16MANIACStack {
>> > + unsigned int id;
>> > + int p;
>> > + int min;
>> > + int max;
>> > + int max2;
>> > + uint8_t mode;
>> > + uint8_t visited;
>> > +} FLIF16MANIACStack;
>> > +
>> > +typedef struct FLIF16MANIACNode {
>> > + int32_t property;
>> > + int32_t count;
>> > + int32_t split_val;
>> > + int32_t child_id;
>> > + int32_t leaf_id;
>> > +} FLIF16MANIACNode;
>> > +
>> > +typedef struct FLIF16MANIACTree {
>> > + FLIF16MANIACNode *data;
>>
>> > +#ifdef MULTISCALE_CHANCES_ENABLED
>> > + FLIF16MultiscaleChanceContext *leaves;
>> > +#else
>> > + FLIF16ChanceContext *leaves;
>> > +#endif
>>
>> You could avoid these multiple ifdef with a single conditional typedef.
>>
>> > + unsigned int size;
>> > + unsigned int leaves_size;
>> > + unsigned int leaves_top;
>> > +} FLIF16MANIACTree;
>> > +
>> > +typedef struct FLIF16MANIACContext {
>> > + FLIF16MANIACTree **forest;
>> > + FLIF16MANIACStack *stack;
>> > +#ifdef MULTISCALE_CHANCES_ENABLED
>> > + FLIF16MultiscaleChanceContext ctx[3];
>> > +#else
>> > + FLIF16ChanceContext ctx[3];
>> > +#endif
>> > + unsigned int tree_top;
>> > + unsigned int stack_top;
>> > + unsigned int stack_size;
>> > +} FLIF16MANIACContext;
>> > +
>> > +void ff_flif16_rac_init(FLIF16RangeCoder *rc, GetByteContext *gb,
>> uint8_t *buf,
>> > + uint8_t buf_size);
>> > +
>> > +void ff_flif16_rac_free(FLIF16RangeCoder *rc);
>> > +
>> > +void ff_flif16_chancecontext_init(FLIF16ChanceContext *ctx);
>> > +
>> > +void ff_flif16_chancetable_init(FLIF16ChanceTable *ct, int alpha, int
>> cut);
>> > +
>> > +void ff_flif16_build_log4k_table(FLIF16Log4kTable *log4k);
>> > +
>> > +int ff_flif16_read_maniac_tree(FLIF16RangeCoder *rc,
>> > + FLIF16MANIACContext *m,
>> > + int32_t (*prop_ranges)[2],
>> > + unsigned int prop_ranges_size,
>> > + unsigned int channel);
>> > +
>> > +void ff_flif16_maniac_close(FLIF16MANIACContext *m, uint8_t
>> num_planes);
>> > +
>> > +#ifdef MULTISCALE_CHANCES_ENABLED
>> > +
>> > +void
>> ff_flif16_multiscale_chancecontext_init(FLIF16MultiscaleChanceContext *ctx);
>> > +
>> > +FLIF16MultiscaleChanceTable
>> *ff_flif16_multiscale_chancetable_init(void);
>> > +
>> > +FLIF16MultiscaleChanceContext
>> *ff_flif16_maniac_findleaf(FLIF16MANIACContext *m,
>> > + uint8_t
>> channel,
>> > + int32_t
>> *properties);
>> > +#else
>> > +FLIF16ChanceContext *ff_flif16_maniac_findleaf(FLIF16MANIACContext *m,
>> > + uint8_t channel,
>> > + int32_t *properties);
>> > +#endif
>> > +
>> > +int ff_flif16_maniac_read_int(FLIF16RangeCoder *rc,
>> > + FLIF16MANIACContext *m,
>> > + int32_t *properties,
>> > + uint8_t channel,
>> > + int min, int max, int *target);
>> > +
>> > +#define MANIAC_GET(rc, m, prop, channel, min, max, target) \
>> > + if (!ff_flif16_maniac_read_int((rc), (m), (prop), (channel),
>> (min), (max), (target))) {\
>> > + goto need_more_data; \
>> > + }
>> > +
>> > +// Functions
>> > +
>> > +static inline int ff_flif16_rac_renorm(FLIF16RangeCoder *rc)
>> > +{
>> > + uint32_t left;
>> > + while (rc->range <= FLIF16_RAC_MIN_RANGE) {
>> > + left = bytestream2_get_bytes_left(rc->gb);
>> > + if (!left) {
>> > + return 0;
>> > + }
>> > + rc->low <<= 8;
>> > + rc->range <<= 8;
>> > + rc->low |= bytestream2_get_byte(rc->gb);
>> > + if(!left) {
>> > + return 0;
>> > + } else {
>> > + --left;
>> > + }
>> > + }
>> > + return 1;
>> > +}
>> > +
>> > +static inline uint8_t ff_flif16_rac_get(FLIF16RangeCoder *rc, uint32_t
>> chance,
>> > + uint8_t *target)
>> > +{
>> > + if (rc->low >= rc->range - chance) {
>> > + rc->low -= rc->range - chance;
>> > + rc->range = chance;
>> > + *target = 1;
>> > + } else {
>> > + rc->range -= chance;
>> > + *target = 0;
>> > + }
>> > +
>> > + return 1;
>> > +}
>> > +
>> > +static inline uint8_t ff_flif16_rac_read_bit(FLIF16RangeCoder *rc,
>> > + uint8_t *target)
>> > +{
>> > + return ff_flif16_rac_get(rc, rc->range >> 1, target);
>> > +}
>> > +
>> > +static inline uint32_t ff_flif16_rac_read_chance(FLIF16RangeCoder *rc,
>> > + uint16_t b12, uint8_t
>> *target)
>> > +{
>> > + uint32_t ret;
>> > +
>>
>> > + if (sizeof(rc->range) > 4)
>> > + ret = ((rc->range) * b12 + 0x800) >> 12;
>> > + else
>> > + ret = (((((rc->range) & 0xFFF) * b12 + 0x800) >> 12) +
>> > + (((rc->range) >> 12) * b12));
>>
>> Cast b12 to uint64_t and let the compiler optimize this.
>>
>> > +
>> > + return ff_flif16_rac_get(rc, ret, target);
>> > +}
>> > +
>> > +/**
>> > + * Reads a Uniform Symbol Coded Integer.
>> > + */
>> > +static inline int ff_flif16_rac_read_uni_int(FLIF16RangeCoder *rc,
>> > + uint32_t min, uint32_t
>> len,
>> > + int type,
>> > + void *target)
>> > +{
>> > + int med;
>> > + uint8_t bit;
>> > +
>> > + if (!rc->active) {
>> > + rc->min = min;
>> > + rc->len = len;
>> > + rc->active = 1;
>> > + }
>> > +
>> > + if ((rc->len) > 0) {
>> > + ff_flif16_rac_read_bit(rc, &bit);
>> > + med = (rc->len) / 2;
>> > + if (bit) {
>> > + rc->min += med + 1;
>> > + rc->len -= med + 1;
>> > + } else {
>> > + rc->len = med;
>> > + }
>> > + return 0;
>> > + } else {
>> > + switch (type) {
>> > + case FLIF16_RAC_UNI_INT8:
>> > + *((uint8_t *) target) = rc->min;
>> > + break;
>> > +
>> > + case FLIF16_RAC_UNI_INT16:
>> > + *((uint16_t *) target) = rc->min;
>> > + break;
>> > +
>> > + case FLIF16_RAC_UNI_INT32:
>> > + *((uint32_t *) target) = rc->min;
>> > + break;
>> > + }
>> > + rc->active = 0;
>> > + return 1;
>> > + }
>> > +}
>> > +
>> > +// Nearzero integer definitions
>> > +
>> > +static inline void ff_flif16_chancetable_put(FLIF16RangeCoder *rc,
>> > + FLIF16ChanceContext *ctx,
>> > + uint16_t type, uint8_t
>> bit)
>> > +{
>> > + ctx->data[type] = (!bit) ? rc->ct.zero_state[ctx->data[type]]
>> > + : rc->ct.one_state[ctx->data[type]];
>> > +}
>> > +
>> > +#ifdef MULTISCALE_CHANCES_ENABLED
>> > +static inline void ff_flif16_chance_estim(FLIF16RangeCoder *rc,
>> > + uint16_t chance, uint8_t bit,
>> > + uint64_t *total)
>> > +{
>> > + *total += rc->log4k.table[bit ? chance : 4096 - chance];
>> > +}
>> > +#endif
>> > +
>> > +/**
>> > + * Reads a near-zero encoded symbol into the RAC probability
>> model/chance table
>> > + * @param type The symbol chance specified by the NZ_INT_* macros
>> > + */
>> > +// TODO remove return value
>> > +static inline uint8_t ff_flif16_rac_read_symbol(FLIF16RangeCoder *rc,
>> > + FLIF16ChanceContext
>> *ctx,
>> > + uint16_t type,
>> > + uint8_t *target)
>> > +{
>> > + ff_flif16_rac_read_chance(rc, ctx->data[type], target);
>> > + ff_flif16_chancetable_put(rc, ctx, type, *target);
>> > + return 1;
>> > +}
>> > +
>> > +// NearZero Integer Coder
>> > +
>> > +static inline int ff_flif16_rac_nz_read_internal(FLIF16RangeCoder *rc,
>> > + FLIF16ChanceContext
>> *ctx,
>> > + uint16_t type,
>> uint8_t *target)
>> > +{
>> > + int flag = 0;
>> > + while (!flag) {
>> > + if(!ff_flif16_rac_renorm(rc))
>> > + return 0; // EAGAIN condition
>> > + flag = ff_flif16_rac_read_symbol(rc, ctx, type, target);
>> > + }
>> > + return 1;
>> > +}
>> > +
>> > +#define RAC_NZ_GET(rc, ctx, chance, target)
>> \
>> > + if (!ff_flif16_rac_nz_read_internal((rc), (ctx), (chance),
>> \
>> > + (uint8_t *) (target))) {
>> \
>> > + goto need_more_data;
>> \
>> > + }
>> > +
>>
>> > +static inline int ff_flif16_rac_read_nz_int(FLIF16RangeCoder *rc,
>> > + FLIF16ChanceContext *ctx,
>> > + int min, int max, int
>> *target)
>>
>> I am worried about the size of all these inline functions that call each
>> other multiple times, growing exponentially. Bigger code will stress the
>> cache more and make everything slower. Better make them normal
>> functions.
>>
>> > +{
>> > + uint8_t temp = 0;
>> > + if (min == max) {
>> > + *target = min;
>> > + rc->active = 0;
>> > + return 1;
>> > + }
>> > +
>> > + if (!rc->active) {
>> > + rc->segment = 0;
>> > + rc->amin = 1;
>> > + rc->active = 1;
>> > + rc->sign = 0;
>> > + rc->have = 0;
>> > + }
>> > +
>>
>> > + switch (rc->segment) {
>> > + case 0:
>>
>> Nit: indentation.
>>
>> > + RAC_NZ_GET(rc, ctx, NZ_INT_ZERO, &(temp));
>> > + if (temp) {
>> > + *target = 0;
>> > + rc->active = 0;
>> > + return 1;
>> > + }
>> > + ++rc->segment;
>> > +
>> > + case 1:
>> > + if (min < 0) {
>> > + if (max > 0) {
>> > + RAC_NZ_GET(rc, ctx, NZ_INT_SIGN, &(rc->sign));
>> > + } else {
>> > + rc->sign = 0;
>> > + }
>> > + } else {
>> > + rc->sign = 1;
>> > + }
>> > + rc->amax = (rc->sign ? max : -min);
>> > + rc->emax = ff_log2(rc->amax);
>> > + rc->e = ff_log2(rc->amin);
>> > + ++rc->segment;
>> > +
>> > + case 2:
>> > + for (; (rc->e) < (rc->emax); (rc->e++)) {
>> > + RAC_NZ_GET(rc, ctx, NZ_INT_EXP((((rc->e) << 1) +
>> rc->sign)),
>> > + &(temp));
>> > + if (temp)
>> > + break;
>> > + temp = 0;
>> > + }
>> > + rc->have = (1 << (rc->e));
>> > + rc->left = rc->have - 1;
>> > + rc->pos = rc->e;
>> > + ++rc->segment;
>> > +
>> > + /*
>> > + case 3 and case 4 mimic a for loop.
>> > + This is done to separate the RAC read statement.
>> > + for(pos = e; pos > 0; --pos) ...
>> > + TODO replace entirely with an actual for loop.
>> > + */
>> > + case 3:
>>
>> > + loop: /* start for */
>>
>> Avoid goto, make it a real loop.
>>
>> > + if ((rc->pos) <= 0)
>> > + goto end;
>> > + --(rc->pos);
>> > + rc->left >>= 1;
>> > + rc->minabs1 = (rc->have) | (1 << (rc->pos));
>> > + rc->maxabs0 = (rc->have) | (rc->left);
>> > + ++rc->segment;
>> > +
>> > + case 4:
>> > + if ((rc->minabs1) > (rc->amax)) {
>> > + --rc->segment;
>> > + goto loop; /* continue; */
>> > + } else if ((rc->maxabs0) >= (rc->amin)) {
>> > + RAC_NZ_GET(rc, ctx, NZ_INT_MANT(rc->pos), &temp);
>> > + if (temp)
>> > + rc->have = rc->minabs1;
>> > + temp = 0;
>> > + } else
>> > + rc->have = rc->minabs1;
>> > + --rc->segment;
>> > + goto loop; /* end for */
>> > + }
>> > +
>> > + end:
>> > + *target = ((rc->sign) ? (rc->have) : -(rc->have));
>> > + rc->active = 0;
>> > + return 1;
>> > +
>> > + need_more_data:
>> > + return 0;
>> > +}
>> > +
>> > +static inline int ff_flif16_rac_read_gnz_int(FLIF16RangeCoder *rc,
>> > + FLIF16ChanceContext *ctx,
>> > + int min, int max, int
>> *target)
>> > +{
>> > + int ret;
>> > + if (min > 0) {
>> > + ret = ff_flif16_rac_read_nz_int(rc, ctx, 0, max - min, target);
>> > + if (ret)
>> > + *target += min;
>> > + } else if (max < 0) {
>> > + ret = ff_flif16_rac_read_nz_int(rc, ctx, min - max, 0,
>> target);
>> > + if (ret)
>> > + *target += max;
>> > + } else
>> > + ret = ff_flif16_rac_read_nz_int(rc, ctx, min, max, target);
>> > + return ret;
>> > +
>> > +}
>> > +
>> > +#ifdef MULTISCALE_CHANCES_ENABLED
>> > +// Multiscale chance definitions
>> > +
>> > +static inline void
>> ff_flif16_multiscale_chance_set(FLIF16MultiscaleChance *c,
>> > + uint16_t chance)
>> > +{
>> > + for (int i = 0; i < MULTISCALE_CHANCETABLE_DEFAULT_SIZE; i++) {
>> > + c->chances[i] = chance;
>> > + c->quality[i] = 0;
>> > + }
>> > + c->best = 0;
>> > +}
>> > +
>>
>> > +static inline uint16_t
>> ff_flif16_multiscale_chance_get(FLIF16MultiscaleChance c)
>> > +{
>> > + return c.chances[c.best];
>> > +}
>>
>> This does not look very useful.
>>
>> > +
>> > +static inline void
>> ff_flif16_multiscale_chancetable_put(FLIF16RangeCoder *rc,
>> > +
>> FLIF16MultiscaleChanceContext *ctx,
>> > + uint16_t type,
>> uint8_t bit)
>> > +{
>> > + FLIF16MultiscaleChance *c = &ctx->data[type];
>> > + uint64_t sbits, oqual;
>> > + for (int i = 0; i < MULTISCALE_CHANCETABLE_DEFAULT_SIZE; ++i) {
>> > + sbits = 0;
>> > + ff_flif16_chance_estim(rc, c->chances[i], bit, &sbits);
>> > + oqual = c->quality[i];
>> > + c->quality[i] = (oqual * 255 + sbits * 4097 + 128) >> 8;
>> > + c->chances[i] = (bit) ?
>> rc->mct->sub_table[i].one_state[c->chances[i]]
>> > + :
>> rc->mct->sub_table[i].zero_state[c->chances[i]];
>> > + }
>> > + for (int i = 0; i < MULTISCALE_CHANCETABLE_DEFAULT_SIZE; ++i)
>> > + if (c->quality[i] < c->quality[c->best])
>> > + c->best = i;
>> > +}
>> > +
>> > +static inline int
>> ff_flif16_rac_read_multiscale_symbol(FLIF16RangeCoder *rc,
>> > +
>> FLIF16MultiscaleChanceContext *ctx,
>> > + uint16_t type,
>> uint8_t *target)
>> > +{
>> > + ff_flif16_rac_read_chance(rc,
>> ff_flif16_multiscale_chance_get(ctx->data[type]), target);
>> > + ff_flif16_multiscale_chancetable_put(rc, ctx, type, *target);
>> > + return 1;
>> > +}
>> > +
>> > +static inline int
>> ff_flif16_rac_nz_read_multiscale_internal(FLIF16RangeCoder *rc,
>> > +
>> FLIF16MultiscaleChanceContext *ctx,
>> > + uint16_t
>> type, uint8_t *target)
>> > +{
>> > + int flag = 0;
>> > + // Maybe remove the while loop
>> > + while (!flag) {
>> > + if(!ff_flif16_rac_renorm(rc))
>> > + return 0; // EAGAIN condition
>> > + flag = ff_flif16_rac_read_multiscale_symbol(rc, ctx, type,
>> target);
>> > + }
>> > + return 1;
>> > +}
>> > +
>> > +#define RAC_NZ_MULTISCALE_GET(rc, ctx, chance, target)
>> \
>> > + if (!ff_flif16_rac_nz_read_multiscale_internal((rc), (ctx),
>> (chance), \
>> > + (uint8_t *)
>> (target))) { \
>> > + goto need_more_data;
>> \
>> > + }
>> > +
>> > +static inline int
>> ff_flif16_rac_read_nz_multiscale_int(FLIF16RangeCoder *rc,
>> > +
>> FLIF16MultiscaleChanceContext *ctx,
>> > + int min, int
>> max, int *target)
>> > +{
>> > + int temp = 0;
>> > +
>> > + if (min == max) {
>> > + *target = min;
>> > + goto end;
>> > + }
>> > +
>> > + if (!rc->active) {
>> > + rc->segment = 0;
>> > + rc->amin = 1;
>> > + rc->active = 1;
>> > + rc->sign = 0;
>> > + rc->have = 0;
>> > + }
>> > +
>>
>> > + switch (rc->segment) {
>> > + case 0:
>>
>> Nit: indentation.
>>
>> > + RAC_NZ_MULTISCALE_GET(rc, ctx, NZ_INT_ZERO, &(temp));
>> > + if (temp) {
>> > + *target = 0;
>> > + goto end;
>> > + }
>>
>> > + ++rc->segment;__PLN__
>>
>> What is this __PLN__?
>>
>> > +
>> > + case 1:
>> > + if (min < 0) {
>> > + if (max > 0) {
>> > +
>
>
More information about the ffmpeg-devel
mailing list