On Tue, 11 Aug 2020 23:38:21 +0530 Anamitra Ghorui <agho...@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.rheinha...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
