[FFmpeg-devel] [PATCH v6 1/4] lavc: add FLIF decoding support

[Moritz Barsnick]

On Sun, Aug 23, 2020 at 00:09:32 +0530, Anamitra Ghorui wrote:
> v2: Fix faulty patch
> v3: Fix addressed errors, Add interlaced decoding support
> v4: Fix Further cosmetics, C.Bucket Transform reading errors, Atomise patch
> v5: Fix faulty patch
> v6: Address pointed out errors, use av_freep everywhere, further cosmetics,
>     redundancies.

These comments don't belong in the commit or the e-mail corresponding
to the commit. So either in the "0/4" e-mail, or below the "---" below.

> Test files are available here: https://0x0.st/iYs_.zip
>
> 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>
> ---

Place comments here.

> +    if (plane == 1 || plane == 2){

Please fix the bracket style -> ") {"

> +    if (plane != 2) {
> +      prop_ranges[top].min = mind;
> +      prop_ranges[top++].max = maxd;
> +      prop_ranges[top].min = mind;
> +      prop_ranges[top++].max = maxd;
> +    }

Incorrect indentation.

> +    for(uint8_t i = 0; i < (lookback ? MAX_PLANES : num_planes); i++) {

Bracket style -> "for ("

> +/*
> + * All constant plane pixel setting should be illegal in theory.

settings

> +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]];
> +    } else
> +        return ((FLIF16ColorVal *) frame->data[plane])[0];
> +    return 0;

Isn't this "return 0" dead code?

> +    if(bytestream2_get_bytes_left(gb) < FLIF16_RAC_MAX_RANGE_BYTES)

Bracket style -> "if ("

> +uint8_t ff_flif16_rac_read_bit(FLIF16RangeCoder *rc,
> +                               uint8_t *target)
> +{
> +    return ff_flif16_rac_get(rc, rc->range >> 1, target);
> +}

If this is called often, you may want to mark it inline.

> +uint32_t ff_flif16_rac_read_chance(FLIF16RangeCoder *rc,
> +                                   uint64_t b12, uint8_t *target)
> +{
> +    uint32_t ret = ((rc->range) * b12 + 0x800) >> 12;

I don't think rc->range needs to be bracketed.

> +    if(!ff_flif16_rac_renorm(rc))

Bracket style.

> +#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;                                                   \
> +    }

Functions are usually defined with a do{} while(0) wrapper. See the
style in libavutil/intreadwrite.h.

> +    return ret;
> +
> +}

Drop the empty line. ;-)

> +    if(!ff_flif16_rac_renorm(rc))

Bracket style.

> +        while (rc->pos > 0) {
> +            rc->pos--;
> +            rc->left >>= 1;
> +            rc->minabs1 = rc->have | (1 << rc->pos);
> +            rc->maxabs0 = rc->have | rc->left;
> +
> +            if (rc->minabs1 > rc->amax) {
> +                continue;
> +            } else if (rc->maxabs0 >= rc->amin) {
> +    case 3:
> +                RAC_NZ_GET(rc, ctx, NZ_INT_MANT(rc->pos), &temp);
> +                if (temp)
> +                    rc->have = rc->minabs1;
> +                temp = 0;
> +            } else {
> +                rc->have = rc->minabs1;
> +            }

A case label in the middle of an if() (in a while() loop) is not
readable to me. ;-)

> +    m->stack_top = 0;
> +    rc->segment2 = 0;
> +    return 0;
> +
> +    need_more_data:
> +    return AVERROR(EAGAIN);

I believe the label needs to be left-aligned.

> +        if(!m->forest[channel]->leaves)

Bracket style.

> +        if(!rc->curr_leaf) {

Ditto.

> +
> +    end:
> +    rc->curr_leaf = NULL;

"goto" label left alignment

> + * probability model. The other (simpler) model and this model ane non

*are

> +#define RAC_GET(rc, ctx, val1, val2, target, type) \
> +    if (!ff_flif16_rac_process((rc), (ctx), (val1), (val2), (target), (type))) { \
> +        goto need_more_data; \
> +    }

do{} while(0)

> +#define MANIAC_GET(rc, m, prop, channel, min, max, target) \
> +    { \
> +        int ret = ff_flif16_maniac_read_int((rc), (m), (prop), (channel), (min), (max), (target)); \
> +        if (ret < 0) { \
> +            return ret; \
> +        } else if (!ret) { \
> +            goto need_more_data; \
> +        } \
> +    }

Ditto.



I give up here. There are further 4000+ lines to review. What an
impressive patch.

Just this:

> +static void transform_palette_reverse(FLIF16Context *ctx,
> +                                     FLIF16TransformContext *t_ctx,
> +                                     FLIF16PixelData *frame,
> +                                     uint32_t stride_row,
> +                                     uint32_t stride_col)
> +{
> +    int r, c;
> +    int P;

ffmpeg doesn't use capitals in variable names.

Cheers,
Moritz