Re: [FFmpeg-devel] [PATCH] avfilter: add backgroundkey video filter

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Paul B Mahol:
> On 11/2/22, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
>> Paul B Mahol:
>>> +static int filter_frame(AVFilterLink *link, AVFrame *frame)
>>> +{
>>> +    AVFilterContext *avctx = link->dst;
>>> +    BackgroundkeyContext *s = avctx->priv;
>>> +    int64_t sum = 0;
>>> +    int ret;
>>> +
>>> +    if (!s->background) {
>>> +        s->background = av_frame_clone(frame);
>>> +        if (!s->background)
>>> +            return AVERROR(ENOMEM);
>>> +        av_frame_make_writable(s->background);
>>
>> You are never writing to the background frame, so there is no point in
>> making it writable; what you actually want to achieve here is making
>> frame writable again and to achieve this you should make frame, not
>> background writable (and of course you should check said call).
>>
> 
> This is invalid, input pad receives always writable frames as there is flag.
> 

But in case this branch here is executed, the av_frame_clone() makes
frame non-writable, so it needs to be made writable again.

>> (Actually, you never
>>
>>> +    }
>>> +
>>> +    if (ret = ff_filter_execute(avctx, s->do_slice, frame, NULL,
>>> +                                FFMIN(frame->height, s->nb_threads)))
>>> +        return ret;
>>> +
>>> +    for (int n = 0; n < s->nb_threads; n++)
>>> +        sum += s->sums[n];
>>> +    if (s->max_sum * s->threshold < sum) {
>>> +        av_frame_free(&s->background);
>>> +        s->background = av_frame_clone(frame);
>>> +        if (!s->background)
>>> +            return AVERROR(ENOMEM);
>>> +        av_frame_make_writable(s->background);
>>
>> Given that you never write to background, there is no need to make it
>> writable. This time, there is also no need to make frame writable (the
>> next filter in the chain may not need a writable frame anyway), so just
>> remove this.
>> And the av_frame_free+av_frame_clone can become an
>> av_frame_unref+av_frame_ref (this will necessitate modifying the check
>> above to not only check for to existence of s->background).
>>
>>> +    }
>>> +
>>> +    return ff_filter_frame(avctx->outputs[0], frame);
>>> +}
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".