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

[Nicolas George]

Anamitra Ghorui (12020-08-02):
> 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.

Thanks.

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

This is a good reason to consider going for a better mail client ;-)

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

You can consider using --enable-memory-poisoning instead.

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

I think you should add a comment near the functions that were taken
there.

/*
 * Ported from code by ...
 * https://github.com/FLIF-hub/FLIF/blob/master/src/maniac/chance.cpp
 */

But beware. This is Apache 2.0, and therefore incompatible with GPL 2
(according to https://www.gnu.org/licenses/license-list.en.html), and
probably not LGPL 2 either. Since these are the default for FFmpeg, it
will require configure checks.

Another possibility is to rewrite it from scratch, using a different
enough style.

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

I'm just saying saying it would be a good thing to know where these
magic-looking numbers come from.

/* Copied from the reference implementation, but I have no clue why they
   use precisely these numbers. */

for example. (Numbers do not warrant copyright, no license problem
here.)

> > What is this __PLN__?
> "Print Line Number". Will remove it. Was there for debugging purposes.

Is it a feature of your compiler? If not, I suggest you avoid using
__-prefix for macros, as they are strictly reserved for the compiler and
libc.

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

strcmp() should be inlined here. But you can avoid it by using either
MKTAG or AV_RL32.

> >> +static int flif16_write_frame(AVCodecContext *avctx, AVFrame *data)

> > Why a decoder context for an encoder?
> This is a decoder.

My bad, I saw write_frame().

> >> + *out_buf = av_realloc(*out_buf, (*out_buf_size) * 2);
> >> + if (!out_buf)
> >> + return AVERROR(ENOMEM);
> > Leaks original out_buf.
> How so?

With:

	b = realloc(a, size);

if the allocation fails, the original buffer is preserved, and still
accessible through variable a. Most (correct) programs will either exit
immediately, letting a be freed by the system, or free it and return an
error.

But some programs could continue without the extra memory (if it's a
cache, it will just continue slower), or want to save the contents of a
in emergency. This is why a is not freed if the reallocation fails.

If you write

	a = realloc(a, size);

and the reallocation fails, a is not freed, but the pointer to it is
immediately overwritten with the NULL return value. It leaks.

BSD has reallocf(), that does like realloc() but frees the memory in
case of failure. FFmpeg has av_realloc_f() for the same task, with the
extra bonus that it has the two parameters form to avoid multiplication
overflows.

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

No, the other hunk is fine. I mean just this hunk: you can read the 3
chars into tag and then do nothing with it, that's completely equivalent
to discarding them.

Although, if zlib is missing, printing a warning to notify the user that
some data was lost due to a missing library would probably be a good
idea.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200802/8a25686a/attachment.sig>