[FFmpeg-devel] [PATCH v2] [RFC] lavc, lavfmt: add FLIF decoding support

[Anamitra Ghorui]

Hello,
Thanks a lot for the code review. We will make sure to correct all of
these mistakes after we are done with all of the features in about a
week, before we post our (supposed) final patch for the decoder.

For some reason my mail client has truncated the indentation (and any
series of multiple spaces) in the blockquotes. I apologise for that.

August 2, 2020 5:58 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.
> 

[...]

>> 
>> +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.

Will do.

>> +{
>> + 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];

Will do. Seems more efficient.

>> + *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.

Will do.

>> + 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.

No. It is at maximum of size 5. Will remove.

>> +
>> + 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.
> 

Will do.

>> + 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.

No. Initialisation to 0 was done for testing purposes. Ideally, only
initialisation in the "fill" mode should involve preliminiary setting.

>> + 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.

Will do.

>> + 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()

Will do.

>> + if (!frames)
>> + return NULL;
>> +

[...]

>> +
>> +} 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.

Will do.

>> +} 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".

Will do

>> +} 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.

Will do.

>> + uint8_t t_no;
>> + unsigned int segment; ///< Segment the code is executing in.
>> + int i; ///< Variable to store iteration number.
>> + uint8_t done;

[...]

>> + 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.

Will do.

>> + }
>> +}
>> +
>> +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.

Will do.

>> + 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;

Will do.

>> + 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)?
>

Will do. I hadn't touched this code since I wrote it so it was mostly
overlooked.

>> + 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
>

Will do.

>> +
>> + switch (f->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.
>

Please see:
* https://github.com/FLIF-hub/FLIF/blob/master/src/maniac/chance.cpp ,
  function `build_table()`
* libavcodec/rangecoder.c, function ff_build_rac_states()
* and function below, build_table()

You will see that these two functions are about 1:1, and the code in
FLIF's reference decoder has been adapted for a more generic case.
The credits were added out of suspicion over who is the original author
of the function. I don't know whether this is the only way to write the
function. A few more functions were more or less copied over from the 
reference decoder (such as log4kf below) but I don't think they are 
useful anymore. Please see below (Multiscale Bitchances).

[...]

>> +
>> +// 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.
> 

True. Will fix.

>> +}
>> +
>> +void ff_flif16_rac_free(FLIF16RangeCoder *rc)
>> +{
>> + if (!rc)
>> + return;
>> 
>> + av_freep(rc);
> 
> Was this tested? av_freep() wants a pointer to pointer.
>

Will fix. I have put the rangecoder struct in the decoder context itself
because of which this error was not caught.

[...]

>> + 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.
>

This may not be requred anymore. Please see below.

>> + 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.

Oh, whoops. Will fix.

>> + 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.
> 

Will do.

>> + 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().
> 

Will do.

>> + 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.
> 

Will do.

>> +
>> + 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.
> 

Will do. 

[...]

>> + --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
> 

Will do.

[...]

>> + 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().
> 

Will do.

>> + m->stack_size *= 2;
>> + }
>> +
>> + temp = m->forest[channel]->data[m->stack[m->stack_top - 1].id].property;
>> +

[...]

>> + 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();
> 

Will do.

[...]

>> +} FLIF16Log4kTable;
>> +
>> 
>> +static const uint32_t flif16_multiscale_alphas[] = {
>> + 21590903, 66728412, 214748365, 7413105, 106514140, 10478104
>> +};
> 
> Please add a short comment to explain.
> 

FLIF's reference sourcecode has 2 probability models (or chances) which
are non interchangeable. This is the multiscale version of the
probability model (as opposed to the simple one). However, FLIF's
upstream sourcecode does not have this enabled by default (This is a 
compile tine option). This means that all FLIF encoded files in
circulation do not use the multiscale chances/probability table for
their encoded data, and since FLIF development is dead, this is not
going to change. Should I remove this?

[...]

>> 
>> + uint16_t data[sizeof(flif16_nz_int_chances) / sizeof(flif16_nz_int_chances[0])];
> 
> FF_ARRAY_ELEMS()
> 

Will do.


>> +#ifdef MULTISCALE_CHANCES_ENABLED
>> + FLIF16MultiscaleChanceContext *leaves;
>> +#else
>> + FLIF16ChanceContext *leaves;
>> +#endif
> 
> You could avoid these multiple ifdef with a single conditional typedef.
> 

Will do if I decide to keep it. Please see above (Multiscale Bitchances)

>> + 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.
> 

Will do.

>> +
>> + 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.
> 

Will do.

>> +{
>> + 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.
> 

Will do.

>> + 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.
> 

Will do.

>> + 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.
> 

Will remove if I decide to keep it. Please see above. (Multiscale
Bitchances)

>> +
>> +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.
> 

Will do.

>> + RAC_NZ_MULTISCALE_GET(rc, ctx, NZ_INT_ZERO, &(temp));
>> + if (temp) {
>> + *target = 0;
>> + goto end;
>> + }
>> 
>> + ++rc->segment;__PLN__
> 
> What is this __PLN__?
> 

"Print Line Number". Will remove it. Was there for debugging purposes.

>> +
>> + case 1:
>> + if (min < 0) {
>> + if (max > 0) {
>> + RAC_NZ_MULTISCALE_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;__PLN__
>> +
>> + case 2:
>> + for (; (rc->e) < (rc->emax); (rc->e++)) {
>> + RAC_NZ_MULTISCALE_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;__PLN__
>> +
>> + /*
>> + * 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 with actual for loop.
>> + */
>> + case 3:
>> 
>> + loop: /* start for */
> 
> Make it a real loop.
> 

Will do.

>> + 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;__PLN__
>> +
>> + case 4:
>> + if ((rc->minabs1) > (rc->amax)) {
>> + --rc->segment;
>> + goto loop; /* continue; */
>> + } else if ((rc->maxabs0) >= (rc->amin)) {
>> + RAC_NZ_MULTISCALE_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_multiscale_int(FLIF16RangeCoder *rc,
>> + FLIF16MultiscaleChanceContext *ctx,
>> + int min, int max, int *target)
>> +{
>> + int ret;
>> + if (min > 0) {
>> + ret = ff_flif16_rac_read_nz_multiscale_int(rc, ctx, 0, max - min, target);
>> + if (ret)
>> + *target += min;
>> + } else if (max < 0) {
>> + ret = ff_flif16_rac_read_nz_multiscale_int(rc, ctx, min - max, 0, target);
>> + if (ret)
>> + *target += max;
>> + } else
>> + ret = ff_flif16_rac_read_nz_multiscale_int(rc, ctx, min, max, target);
>> + return ret;
>> +
>> +}
>> +#endif
>> +
>> +/*
>> +RAC_NZ_DEFINE(, FLIF16ChanceContext)
>> +RAC_GNZ_DEFINE(, FLIF16ChanceContext)
>> +
>> +#ifdef MULTISCALE_CHANCES_ENABLED
>> +
>> +#undef RAC_NZ_GET
>> +
>> +#define RAC_NZ_GET(rc, ctx, chance, target) \
>> + if (!ff_flif16_rac_nz_read_multiscale_internal((rc), (ctx), (chance), \
>> + (uint8_t *) (target))) { \
>> + goto need_more_data; \
>> + }
>> +
>> +RAC_NZ_DEFINE(_multiscale, FLIF16MultiscaleChanceContext)
>> +RAC_GNZ_DEFINE(_multiscale, FLIF16MultiscaleChanceContext)
>> +
>> +#endif
>> +*/
>> +
>> +/**
>> + * Reads an integer encoded by FLIF's RAC.
>> + * @param[in] val1 A generic value, chosen according to the required type
>> + * @param[in] val2 Same as val1
>> + * @param[out] target The place where the resultant value should be written to
>> + * @param[in] type The type of the integer to be decoded specified by
>> + * FLIF16RACTypes
>> + * @return 0 on bytestream empty, 1 on successful decoding.
>> + */
>> +static inline int ff_flif16_rac_process(FLIF16RangeCoder *rc,
>> + void *ctx,
>> + int val1, int val2, void *target,
>> + int type)
>> +{
>> + int flag = 0;
>> + while (!flag) {
>> + if(!ff_flif16_rac_renorm(rc)) {
>> + return 0; // EAGAIN condition
>> + }
>> +
>> 
>> + switch (type) {
>> + case FLIF16_RAC_BIT:
> 
> Nit: indentation.
> 

Will do.

>> + flag = ff_flif16_rac_read_bit(rc, (uint8_t *) target);
>> + break;
>> +
>> + case FLIF16_RAC_UNI_INT8:
>> + case FLIF16_RAC_UNI_INT16:
>> + case FLIF16_RAC_UNI_INT32:
>> + flag = ff_flif16_rac_read_uni_int(rc, val1, val2, type, target);
>> + break;
>> +
>> + case FLIF16_RAC_CHANCE:
>> + flag = ff_flif16_rac_read_chance(rc, val1, (uint8_t *) target);
>> + break;
>> +
>> + case FLIF16_RAC_NZ_INT:
>> + // handle nz_ints
>> + flag = ff_flif16_rac_read_nz_int(rc, (FLIF16ChanceContext *) ctx,
>> + val1, val2, (int *) target);
>> + break;
>> +
>> + case FLIF16_RAC_GNZ_INT:
>> + // handle gnz_ints
>> + flag = ff_flif16_rac_read_gnz_int(rc, (FLIF16ChanceContext *) ctx,
>> + val1, val2, (int *) target);
>> + break;
>> +#ifdef MULTISCALE_CHANCES_ENABLED
>> + case FLIF16_RAC_NZ_MULTISCALE_INT:
>> + // handle nz_ints
>> + flag = ff_flif16_rac_read_nz_multiscale_int(rc, (FLIF16MultiscaleChanceContext *) ctx,
>> + val1, val2, (int *) target);
>> + break;
>> +
>> + case FLIF16_RAC_GNZ_MULTISCALE_INT:
>> + // handle multiscale nz_ints
>> + flag = ff_flif16_rac_read_gnz_multiscale_int(rc, (FLIF16MultiscaleChanceContext *) ctx,
>> + val1, val2, (int *) target);
>> + break;
>> +#endif
>> + default:
>> + // MSG("unknown rac reader\n");
>> + break;
>> + }
>> + }
>> + return 1;
>> +}
>> +
>> +#define RAC_GET(rc, ctx, val1, val2, target, type) \
>> + if (!ff_flif16_rac_process((rc), (ctx), (val1), (val2), (target), (type))) {\
>> + goto need_more_data; \
>> + }
>> +
>> +#endif /* FLIF16_RANGECODER_H */
>> diff --git a/libavcodec/flif16_transform.c b/libavcodec/flif16_transform.c
>> new file mode 100644
>> index 0000000000..7b6cdef070
>> --- /dev/null
>> +++ b/libavcodec/flif16_transform.c
>> @@ -0,0 +1,2964 @@
>> +/*
>> + * Transforms for FLIF16.
>> + * Copyright (c) 2020 Kartik K. Khullar <kartikkhullar840 at gmail.com>
>> + *
>> + * 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
>> + */

This part is for Kartik to respond. In hindsight I should have gone
through this sourcecode more and suggest more changes.

[...]

>> @@ -0,0 +1,1146 @@
>> +/*
>> + * FLIF16 Decoder
>> + * 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
>> + */
>> +
>> +/**

[...]

>> + if (bytestream2_size(&s->gb) < 8) {
>> + av_log(avctx, AV_LOG_ERROR, "buf size too small (%d)\n",
>> + bytestream2_size(&s->gb));
>> 
>> + return AVERROR(EINVAL);
> 
> AVERROR_INVALIDDATA
> 

Will do.

>> + }
>> +
>> 
>> + if (bytestream2_get_le32(&s->gb) != (*((uint32_t *) flif16_header))) {
> 
> You can't do that. Was it tested on big endian?
> 

I thought I would be able to get away from using strcmp, but I haven't
tested on big endian machines. I thought the byte order would be the
same if I used bytestream2_get_le32. Will see.

>> + av_log(avctx, AV_LOG_ERROR, "bad magic number\n");
>> 
>> + return AVERROR(EINVAL);
> 
> AVERROR_INVALIDDATA
> 

Will do.

[...]

>> 
>> + return AVERROR(ENOMEM);
> 
> AVERROR_INVALIDDATA
> 

Will do.

[...]

>> 
>> + av_log(avctx, AV_LOG_ERROR, "failed to initialise transform %u\n", temp);
>> + return AVERROR(ENOMEM);
> 
> If the reason is ENOMEM, no need for an extra message.
> 

Will do.


>> + case FLIF16_TRANSFORM_DUPLICATEFRAME:
>> + s->framedup = 1;
>> + if(s->num_frames < 2)
>> + return AVERROR(EINVAL);
> 
> AVERROR_INVALIDDATA
> 

Will do.

>> + ff_flif16_transform_configure(s->transforms[s->transform_top],
>> + s->num_frames);
>> + break;
>> +
>> + case FLIF16_TRANSFORM_FRAMESHAPE:
>> + s->frameshape = 1;
>> 
>> + if (s->num_frames < 2)
>> + return AVERROR(EINVAL);
> 
> AVERROR_INVALIDDATA
> 

Will do.

>> + unique_frames = s->num_frames - 1;
>> + for (unsigned int i = 0; i < s->num_frames; i++) {
>> + if(s->frames[i].seen_before >= 0)
>> + unique_frames--;
>> + }
>> + if (unique_frames < 1)
>> + return AVERROR(EINVAL);
> 
> AVERROR_INVALIDDATA
> 

Will do.

>> + ff_flif16_transform_configure(s->transforms[s->transform_top],
>> + (unique_frames) * s->height);
>> + ff_flif16_transform_configure(s->transforms[s->transform_top],
>> + s->width);
>> + break;
>> +
>> + case FLIF16_TRANSFORM_FRAMELOOKBACK:
>> + if(s->num_frames < 2)
>> + return AVERROR(EINVAL);
> 
> AVERROR_INVALIDDATA
> 

Will do.

>> + }
>> +
>> + if (ff_flif16_planes_init(CTX_CAST(s), s->frames, s->plane_mode,
>> + const_plane_value, s->framelookback) < 0) {
>> 
>> + av_log(avctx, AV_LOG_ERROR, "could not allocate planes\n");
>> + return AVERROR(ENOMEM);
> 
> Redundant error message.
> 

Will do.

>> + }
>> +
>> + // if (!(s->ia % 2))
>> + // s->state = FLIF16_ROUGH_PIXELDATA;
>> + // else
>> + // s->state = FLIF16_MANIAC;
>> + s->state = FLIF16_MANIAC;
>> + s->segment = 0;
>> + return 0;
>> +
>> + need_more_data:
>> + return AVERROR(EAGAIN);
>> +}
>> +
>> +static int flif16_read_maniac_forest(AVCodecContext *avctx)
>> +{
>> + int ret;
>> + FLIF16DecoderContext *s = avctx->priv_data;
>> + if (!s->maniac_ctx.forest) {
>> + s->maniac_ctx.forest = av_mallocz((s->num_planes) * sizeof(*(s->maniac_ctx.forest)));
>> + if (!s->maniac_ctx.forest) {
>> + return AVERROR(ENOMEM);
>> + }
>> + s->segment = s->i = 0; // Remove later
>> + }
>> + switch (s->segment) {
>> + case 0:
>> 
>> + loop:
> 
> Proper loop please.
> 

Will do.

>> + uint8_t p, uint32_t r,
>> + uint32_t c, FLIF16ColorVal *min,
>> + FLIF16ColorVal *max,
>> + const FLIF16ColorVal fallback,
>> + uint8_t nobordercases)
> 
> Functions with that many parameters would often do better with fewer
> structures.
> 
> For example, all the calls below are with "s->c, s->min, s->max", they
> need not be parameters.
> 

Yes. The function parameters are not needed at all. Had put this on hold
for other components. Will fix.

>> 
>> + for (; s->i2 < s->height; ++s->i2) {
>> + for (; s->i3 < s->num_frames; ++s->i3) {
>> + case 1:
>> + // TODO maybe put this in dec ctx
> 
> Urgh.
> 

Will fix.

>> + min_p = ff_flif16_ranges_min(s->range, s->curr_plane);
>> + ret = flif16_read_ni_plane(s, s->range, s->properties,
>> + s->curr_plane,
>> + s->i3,
>> + s->i2,
>> + s->grays[s->curr_plane],
>> + min_p);
>> +
>> + if (ret) {
>> + goto error;
>> + }
>> + } // End for
>> + s->i3 = 0;
>> + } // End for
>> + if (s->properties)
>> + av_freep(&s->properties);
>> + s->i2 = 0;
>> + } // End for
>> +
>> + } // End switch
>> +
>> + for (int i = 0; i < s->num_frames; i++) {
>> + if (s->frames[i].seen_before >= 0)
>> + continue;
>> + for (int j = s->transform_top - 1; j >= 0; --j) {
>> + ff_flif16_transform_reverse(CTX_CAST(s), s->transforms[j], &s->frames[i], 1, 1);
>> + }
>> + }
>> +
>> + if (s->grays)
>> + av_freep(&s->grays);
>> +
>> + s->state = FLIF16_OUTPUT;
>> + return 0;
>> +
>> + error:
>> + return ret;
>> +}
>> +
>> +/* ============================================================================
>> + * Interlaced plane decoding
>> + * ============================================================================
>> + *
>> + * This is how the data is organised here:
>> + * 1. uni_int: rough_zoomlevel
>> + * 2. (repeat num_planes times) values of top left pixels of each channel
>> + * 3. Rough Pixeldata max_zoomlevel to rough_zoomlevel + 1
>> + * For this case, the MANIAC forest is initialised with a single node per
>> + * channel. This is nused with the maniac integer reader.
>> + * 4. Actual Encoded MANIAC trees
>> + * 5. Rest of the pixeldata rough_zoomlevel to 0
>> + *
>> + * TODO
>> + */
>> +
>> +static int flif16_read_pixeldata(AVCodecContext *avctx)
>> +{
>> + FLIF16DecoderContext *s = avctx->priv_data;
>> + int ret;
>> + if((s->ia % 2))
>> + ret = flif16_read_ni_image(avctx);
>> + else
>> + return AVERROR(EINVAL);
>> +
>> + if(!ret)
>> + s->state = FLIF16_OUTPUT;
>> +
>> + return ret;
>> +}
>> +
>> +static int flif16_write_frame(AVCodecContext *avctx, AVFrame *data)
>> +{
>> + uint32_t target_frame;
>> + int ret;
>> 
>> + FLIF16DecoderContext *s = avctx->priv_data;
> 
> Why a decoder context for an encoder?

This is a decoder.

>> + ff_set_dimensions(avctx, s->width, s->height);
> 
> Missing error check.
> 

Will fix.

[...]

>> + // Looping is done to change states in between functions.
>> + // Function will either exit on AVERROR(EAGAIN) or AVERROR_EOF
>> 
>> + do {
>> + switch(s->state) {
> 
> Now, this is a proper loop with a state machine!
> 
>> + case FLIF16_HEADER:
>> + ret = flif16_read_header(avctx);
>> + break;
>> +

[...]

>> --- /dev/null
>> +++ b/libavformat/flifdec.c
>> @@ -0,0 +1,377 @@
>> +/*
>> + * FLIF16 demuxer
>> + * 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

[...]

>> + int64_t duration;
>> +} FLIFDemuxContext;
>> +
>> +
>> +#if CONFIG_ZLIB
>> +static int flif_inflate(FLIFDemuxContext *s, uint8_t *buf, int buf_size,
>> + uint8_t **out_buf, int *out_buf_size)
>> +{
>> + int ret;
>> + z_stream *stream = &s->stream;
>> +
>> + if (!s->active) {
>> + s->active = 1;
>> + stream->zalloc = Z_NULL;
>> + stream->zfree = Z_NULL;
>> + stream->opaque = Z_NULL;
>> + stream->avail_in = 0;
>> + stream->next_in = Z_NULL;
>> + ret = inflateInit(stream);
>> +
>> + if (ret != Z_OK)
>> + return ret;
>> +
>> + *out_buf_size = buf_size;
>> + *out_buf = av_realloc(*out_buf, *out_buf_size);
>> + if (!*out_buf)
>> + return AVERROR(ENOMEM);
>> + }
>> +
>> + stream->next_in = buf;
>> + stream->avail_in = buf_size;
>> + while (stream->total_out >= *out_buf_size) {
>> 
>> + *out_buf = av_realloc(*out_buf, (*out_buf_size) * 2);
>> + if (!out_buf)
>> + return AVERROR(ENOMEM);
> 
> Leaks original out_buf.
> 

How so?

>> + *out_buf_size *= 2;
>> + }
>> +
>> + stream->next_out = *out_buf + stream->total_out;
>> + stream->avail_out = *out_buf_size - stream->total_out - 1; // Last byte should be NULL char
>> + printf("First 10 bytes: ");
>> + for (int i = 0; i < FFMIN(10, buf_size); ++i)
>> + printf("%x ", buf[i]);
>> + printf("\n");
>> + printf("Buf size: %d, Outbuf size: %d\n", buf_size, *out_buf_size);
>> + ret = inflate(stream, Z_NO_FLUSH);
>> + printf("Return: %d Message: %s \nZ_NEED_DICT: %d\nZ_DATA_ERROR: %d\n"
>> + "Z_MEM_ERROR: %d\n", ret, stream->msg, Z_NEED_DICT, Z_DATA_ERROR,
>> + Z_MEM_ERROR);
>> + switch (ret) {
>> + case Z_NEED_DICT:
>> + case Z_DATA_ERROR:
>> + ret = inflateSync(stream);
>> + printf("Sync ret: %d\n", ret);
>> + printf("Buf size: %d, Outbuf size: %d\n", buf_size, *out_buf_size);
>> + (void)inflateEnd(stream);
>> + return AVERROR(EINVAL);
>> + case Z_MEM_ERROR:
>> + (void)inflateEnd(stream);
>> 
>> + return AVERROR(ENOMEM);
> 
> Looks incorrect.
> 

Will check.

[...]

>> 
>> + st = avformat_new_stream(s, NULL);
> 
> Missing error check.
> 

Will fix

>> + flag = avio_r8(pb);
>> + animated = (flag >> 4) > 4;
>> + duration = !animated;
>> + bpc = avio_r8(pb); // Bytes per channel
>> +
>> + num_planes = flag & 0x0F;
>> +
>> + for (int i = 0; i < (2 + animated); ++i) {
>> + while ((temp = avio_r8(pb)) > 127) {
>> + if (!(count--))
>> + return AVERROR_INVALIDDATA;
>> + VARINT_APPEND(vlist[i], temp);
>> + }
>> + VARINT_APPEND(vlist[i], temp);
>> + count = 4;
>> + }
>> +
>> +
>> + ++vlist[0];
>> + ++vlist[1];
>> + if (animated)
>> + vlist[2] += 2;
>> + else
>> + vlist[2] = 1;
>> +
>> + num_frames = vlist[2];
>> +
>> + while ((temp = avio_r8(pb))) {
>> + // Get metadata identifier
>> 
>> + #if CONFIG_ZLIB
>> + tag[0] = temp;
>> + for(int i = 1; i <= 3; ++i)
>> + tag[i] = avio_r8(pb);
>> 
>> + #else
>> + avio_skip(pb, 3);
>> + #endif
> 
> Unnecessary: you can fill tag even if zlib is not available.
> 

Should the dictionary be populated with empty values then?

>> +
>> + // Read varint
>> + while ((temp = avio_r8(pb)) > 127) {
>> + if (!(count--))
>> + return AVERROR_INVALIDDATA;
>> + VARINT_APPEND(metadata_size, temp);
>> + }
>> + VARINT_APPEND(metadata_size, temp);
>> + count = 4;
>> +
>> + #if CONFIG_ZLIB
>> + // Decompression Routines
>> + while (metadata_size > 0) {
>> + ret = avio_read(pb, buf, FFMIN(BUF_SIZE, metadata_size));
>> + metadata_size -= ret;
>> + if((ret = flif_inflate(dc, buf, ret, &out_buf, &out_buf_size)) < 0 &&
>> + ret != AVERROR(EAGAIN)) {
>> + av_log(s, AV_LOG_ERROR, "could not decode metadata\n");
>> + return ret;
>> + }
>> + }
>> + av_dict_set(&s->metadata, tag, out_buf, 0);
>> + #else
>> + avio_skip(pb, metadata_size);
>> + #endif
>> + }
>> +
>> 
>> + #if CONFIG_ZLIB
> 
> Unnecessary.
> 

Will do.

>> + if (out_buf)
> 
> Unnecessary, av_freep() accepts NULL.
> 

Will do.

[...]

> 
> Regards,
> 
> --
> Nicolas George
> 

Thanks,
Anamitra