[FFmpeg-devel] [PATCH v5 1/3] [RFC] lavc: add FLIF decoding support

[Anamitra Ghorui]

August 21, 2020 8:27 PM, "Paul B Mahol" <onemda at gmail.com> wrote:

> On 8/20/20, Anamitra Ghorui <aghorui at teknik.io> wrote:
> 
>> Hello,
>> 
>> On Wed, 19 Aug 2020 21:05:41 +0200
>> Paul B Mahol <onemda at gmail.com> wrote:
>> 
>> [...]
>> 
>>>> 
>>>> 
>>>> All these corrections have been made by me locally, but I would
>>>> like to wait for some further review (self and otherwise) before
>>>> posting again.
>>> 
>>> Please make use of av_clip and FFMIN/FFMAX macros.
>> 
>> We have looked through the code again and have not been able to notice a
>> situation where these macros will be required aside from where they are
>> already used. They have been largely used in the transform code and
>> sparsely used everywhere else. Can you please point to a portion where
>> their use is warranted?
> 
> ff_bound_snap.

Thanks for the pointer. Didn't see that most of these patterns such as:
    if (a < b)
        a = b;
could be converted into a = FFMAX(a, b).

> Also av_freep() is inconsistently used in code. You do not need to
> check for NULL, prior to calling it.

Ah. I don't know why I didn't check all the code for it after Nicolas
George's prompting. Fixed locally. Thanks.

A few more things noticed:
* A few av_assert0()s in transform reverse functions should be rewritten
  to av_assert1() as they are speed critical.
* ff_static_snap calls minmax functions, a lot of which use av_assert0.
  Snapping functions are called in speed critical code therefore they
  should likely be converted into av_assert1()s.
* There is some cosmetic asymmetry between flif16_ni_predict_calcprops
  and flif16_predict_calcprops (variable aliases used). Fixed locally.

Regards,
Anamitra