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

[Anamitra Ghorui]

On Tue, 11 Aug 2020 23:38:21 +0530
Anamitra Ghorui <aghorui at teknik.io> wrote:

> This patch adds support for non animated, animated, non interlaced and
> interlaced FLIF images.
> Hopefully, all previously mentioned mistakes have been resolved.
> However, there are a few things I want to ask:
> 
> * The decoder can accept arbitary packet sizes. How do I make the
>   demuxer (specifically flif16_read_packet) produce a packet that is
>   reasonably sized? Using a static packet size (such as 4096) produces
>   errors
> * FLIF can encode ICCP color profiles as metadata. These however, as
>   per libavcodec/pngdec.c some processing is done per frame, along
>   with setting the dictionary entry. How should this be handled in the
>   case of a non-still image situation?
>   
> Test files are available here: https://0x0.st/iYs_.zip
> 
> Thanks,
> Anamitra

[...]

A few things that I have noticed since uploading this patch:

* There are places where preincrements have not been converted to 
  postincrements. I had not noticed these and will add these in to the
  next patch.
* In the initialisation of planes, av_mallocz_array has been used for
  the fill mode. This is redundant and I will remove it.
* Using GetByteContext in the RangeCoder struct is not very useful if
  one is writing an encoder as well. Keeping pointers to the start,
  current position and the end of the buffer is more sound.
* In ff_flif16_copy_cols, instead of incrementing pointers, a for loop
  and offset calculation is used. I had misunderstood the initial intent
  of the review of this part.

A few more things I want to mention that I did not in the initial post:
* Kartik has said that the ColorBuckets algorithm and datastructure have
  been improved
* In PermutePlanes, we haven't yet observed whether constant planes are
  permuted around or not (I do think that since it will only ever be 
  the alpha plane that will be a true constant plane in case multiple
  planes exist, and it being an alpha plane most likely means that it
  wouldn't be permuted with others), so we haven't used pointer
  iteration for copying around plane data.


On Tue, 11 Aug 2020 20:42:02 +0200
Andreas Rheinhardt <andreas.rheinhardt at gmail.com> wrote:

[...]

> > +
> > +
> > +static const AVOption options[] = {
> > +    { NULL }
> > +};  
> 
> You don't have options, so you don't need options or an AVClass. The
> const AVClass * in your context should then also be removed.
> 

I see. Will remove.

[...]

> 
> I haven't looked at it closely at all, yet we typically want decoders
> and demuxers to be in separate patches.
> 
> - Andreas

Will do on the next patchset.

Thanks,
Anamitra