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

[Paul B Mahol <onemda@gmail.com>]

On 11/3/22, Paul B Mahol <onemda@gmail.com> wrote:
> On 11/3/22, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
>> 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.
>
> Than will use copy frame.
>
>>
>>>> (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".
>>
>

Will apply witjh av_copy_frame() solution.
_______________________________________________
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".