diff mbox series

[FFmpeg-devel] avfilter/avfilter: Fix hardcoded input index

Message ID MN2PR04MB59816E161C01A77CFC195D61BAD19@MN2PR04MB5981.namprd04.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel] avfilter/avfilter: Fix hardcoded input index
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Soft Works Sept. 5, 2021, 4:28 a.m. UTC
This fix targets cases where multiple input pads have a
.filter_frame function. ff_request_frame_to_filter needs
to call ff_request_frame with the correct input pad
instead of the hardcoded first one.

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 libavfilter/avfilter.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Paul B Mahol Sept. 5, 2021, 9 a.m. UTC | #1
With what filters this happens?

IIUC this is forbidden and such filters should use .activate instead.
Soft Works Sept. 5, 2021, 2:59 p.m. UTC | #2
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Paul B Mahol
> Sent: Sunday, 5 September 2021 11:01
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix hardcoded
> input index
> 
> With what filters this happens?
> 
> IIUC this is forbidden and such filters should use .activate instead.

The hardcoded [0] is wrong, no matter what the use case is.

But to answer your question: It's about a new overlay_textsubs filter
where framesync cannot be used and incoming frames (video on input0,
subs on input1) need to be processed independently: the subtitle events 
are fed into the ass library (you could practically feed all events
in advance) while the overlay image generation is done based on a given
time code. 
This works fine by having a separate .filter_frame function for each
input (after applying this patch).

Thanks,
softworkz
Soft Works Sept. 5, 2021, 7:44 p.m. UTC | #3
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Soft Works
> Sent: Sunday, 5 September 2021 16:59
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix hardcoded
> input index
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Paul B Mahol
> > Sent: Sunday, 5 September 2021 11:01
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> hardcoded
> > input index
> >
> > With what filters this happens?
> >
> > IIUC this is forbidden and such filters should use .activate
> instead.
> 
> The hardcoded [0] is wrong, no matter what the use case is.
> 
> But to answer your question: It's about a new overlay_textsubs filter
> where framesync cannot be used and incoming frames (video on input0,
> subs on input1) need to be processed independently: the subtitle
> events
> are fed into the ass library (you could practically feed all events
> in advance) while the overlay image generation is done based on a
> given
> time code.
> This works fine by having a separate .filter_frame function for each
> input (after applying this patch).

BTW

> > IIUC this is forbidden and such filters should use .activate

The scale2ref filter is set up in the same way (two .filter_frame
functions, no .activate, no framesync), probably because - like in 
my case - the filter must not sync frames across inputs.

softworkz
Paul B Mahol Sept. 5, 2021, 7:51 p.m. UTC | #4
On Sun, Sep 5, 2021 at 9:44 PM Soft Works <softworkz@hotmail.com> wrote:

>
>
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Soft Works
> > Sent: Sunday, 5 September 2021 16:59
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix hardcoded
> > input index
> >
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Paul B Mahol
> > > Sent: Sunday, 5 September 2021 11:01
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> > hardcoded
> > > input index
> > >
> > > With what filters this happens?
> > >
> > > IIUC this is forbidden and such filters should use .activate
> > instead.
> >
> > The hardcoded [0] is wrong, no matter what the use case is.
> >
> > But to answer your question: It's about a new overlay_textsubs filter
> > where framesync cannot be used and incoming frames (video on input0,
> > subs on input1) need to be processed independently: the subtitle
> > events
> > are fed into the ass library (you could practically feed all events
> > in advance) while the overlay image generation is done based on a
> > given
> > time code.
> > This works fine by having a separate .filter_frame function for each
> > input (after applying this patch).
>
> BTW
>
> > > IIUC this is forbidden and such filters should use .activate
>
> The scale2ref filter is set up in the same way (two .filter_frame
> functions, no .activate, no framesync), probably because - like in
> my case - the filter must not sync frames across inputs.
>

FYI .activate does not sync frames across inputs.


>
> softworkz
>
>
> _______________________________________________
> 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".
>
Soft Works Sept. 5, 2021, 7:56 p.m. UTC | #5
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Paul B Mahol
> Sent: Sunday, 5 September 2021 21:51
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix hardcoded
> input index
> 
> On Sun, Sep 5, 2021 at 9:44 PM Soft Works <softworkz@hotmail.com>
> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Soft Works
> > > Sent: Sunday, 5 September 2021 16:59
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> hardcoded
> > > input index
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> > > > Paul B Mahol
> > > > Sent: Sunday, 5 September 2021 11:01
> > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > devel@ffmpeg.org>
> > > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> > > hardcoded
> > > > input index
> > > >
> > > > With what filters this happens?
> > > >
> > > > IIUC this is forbidden and such filters should use .activate
> > > instead.
> > >
> > > The hardcoded [0] is wrong, no matter what the use case is.
> > >
> > > But to answer your question: It's about a new overlay_textsubs
> filter
> > > where framesync cannot be used and incoming frames (video on
> input0,
> > > subs on input1) need to be processed independently: the subtitle
> > > events
> > > are fed into the ass library (you could practically feed all
> events
> > > in advance) while the overlay image generation is done based on a
> > > given
> > > time code.
> > > This works fine by having a separate .filter_frame function for
> each
> > > input (after applying this patch).
> >
> > BTW
> >
> > > > IIUC this is forbidden and such filters should use .activate
> >
> > The scale2ref filter is set up in the same way (two .filter_frame
> > functions, no .activate, no framesync), probably because - like in
> > my case - the filter must not sync frames across inputs.
> >
> 
> FYI .activate does not sync frames across inputs.

Thanks for the information, I wasn't aware of that; so far I had always
seen it used in combination with framesync, do you know an example
where it's being used without framesync?

Thanks,
softworkz
Paul B Mahol Sept. 5, 2021, 8:06 p.m. UTC | #6
On Sun, Sep 5, 2021 at 9:56 PM Soft Works <softworkz@hotmail.com> wrote:

>
>
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Paul B Mahol
> > Sent: Sunday, 5 September 2021 21:51
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix hardcoded
> > input index
> >
> > On Sun, Sep 5, 2021 at 9:44 PM Soft Works <softworkz@hotmail.com>
> > wrote:
> >
> > >
> > >
> > > > -----Original Message-----
> > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > > Soft Works
> > > > Sent: Sunday, 5 September 2021 16:59
> > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > devel@ffmpeg.org>
> > > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> > hardcoded
> > > > input index
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> > Of
> > > > > Paul B Mahol
> > > > > Sent: Sunday, 5 September 2021 11:01
> > > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > > devel@ffmpeg.org>
> > > > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> > > > hardcoded
> > > > > input index
> > > > >
> > > > > With what filters this happens?
> > > > >
> > > > > IIUC this is forbidden and such filters should use .activate
> > > > instead.
> > > >
> > > > The hardcoded [0] is wrong, no matter what the use case is.
> > > >
> > > > But to answer your question: It's about a new overlay_textsubs
> > filter
> > > > where framesync cannot be used and incoming frames (video on
> > input0,
> > > > subs on input1) need to be processed independently: the subtitle
> > > > events
> > > > are fed into the ass library (you could practically feed all
> > events
> > > > in advance) while the overlay image generation is done based on a
> > > > given
> > > > time code.
> > > > This works fine by having a separate .filter_frame function for
> > each
> > > > input (after applying this patch).
> > >
> > > BTW
> > >
> > > > > IIUC this is forbidden and such filters should use .activate
> > >
> > > The scale2ref filter is set up in the same way (two .filter_frame
> > > functions, no .activate, no framesync), probably because - like in
> > > my case - the filter must not sync frames across inputs.
> > >
> >
> > FYI .activate does not sync frames across inputs.
>
> Thanks for the information, I wasn't aware of that; so far I had always
> seen it used in combination with framesync, do you know an example
> where it's being used without framesync?
>

Numerous examples across code base, grep for .activate.


> Thanks,
> softworkz
>
>
>
>
> _______________________________________________
> 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".
>
Soft Works Sept. 5, 2021, 8:16 p.m. UTC | #7
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Paul B Mahol
> Sent: Sunday, 5 September 2021 22:06
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix hardcoded
> input index
> 
> On Sun, Sep 5, 2021 at 9:56 PM Soft Works <softworkz@hotmail.com>
> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Paul B Mahol
> > > Sent: Sunday, 5 September 2021 21:51
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> hardcoded
> > > input index
> > >
> > > On Sun, Sep 5, 2021 at 9:44 PM Soft Works <softworkz@hotmail.com>
> > > wrote:
> > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On
> Behalf Of
> > > > > Soft Works
> > > > > Sent: Sunday, 5 September 2021 16:59
> > > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > > devel@ffmpeg.org>
> > > > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> > > hardcoded
> > > > > input index
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On
> Behalf
> > > Of
> > > > > > Paul B Mahol
> > > > > > Sent: Sunday, 5 September 2021 11:01
> > > > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > > > devel@ffmpeg.org>
> > > > > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> > > > > hardcoded
> > > > > > input index
> > > > > >
> > > > > > With what filters this happens?
> > > > > >
> > > > > > IIUC this is forbidden and such filters should use
> .activate
> > > > > instead.
> > > > >
> > > > > The hardcoded [0] is wrong, no matter what the use case is.
> > > > >
> > > > > But to answer your question: It's about a new
> overlay_textsubs
> > > filter
> > > > > where framesync cannot be used and incoming frames (video on
> > > input0,
> > > > > subs on input1) need to be processed independently: the
> subtitle
> > > > > events
> > > > > are fed into the ass library (you could practically feed all
> > > events
> > > > > in advance) while the overlay image generation is done based
> on a
> > > > > given
> > > > > time code.
> > > > > This works fine by having a separate .filter_frame function
> for
> > > each
> > > > > input (after applying this patch).
> > > >
> > > > BTW
> > > >
> > > > > > IIUC this is forbidden and such filters should use
> .activate
> > > >
> > > > The scale2ref filter is set up in the same way (two
> .filter_frame
> > > > functions, no .activate, no framesync), probably because - like
> in
> > > > my case - the filter must not sync frames across inputs.
> > > >
> > >
> > > FYI .activate does not sync frames across inputs.
> >
> > Thanks for the information, I wasn't aware of that; so far I had
> always
> > seen it used in combination with framesync, do you know an example
> > where it's being used without framesync?
> >
> 
> Numerous examples across code base, grep for .activate.

Grep (lol..) - I asked because I thought you might 
have an example in mind that would be similar to the discussed case.
Nevermind, I'll find one..

But if you don't mind, could you please elaborate on why .activate 
would be a preferred approach when input frame processing is
meant to be totally independent between two inputs?`

Thanks,
softworkz
Soft Works Sept. 6, 2021, 5:23 a.m. UTC | #8
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Paul B Mahol
> Sent: Sunday, 5 September 2021 21:51
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix hardcoded
> input index
> 
> On Sun, Sep 5, 2021 at 9:44 PM Soft Works <softworkz@hotmail.com>
> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Soft Works
> > > Sent: Sunday, 5 September 2021 16:59
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> hardcoded
> > > input index
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> > > > Paul B Mahol
> > > > Sent: Sunday, 5 September 2021 11:01
> > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > devel@ffmpeg.org>
> > > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> > > hardcoded
> > > > input index
> > > >
> > > > With what filters this happens?
> > > >
> > > > IIUC this is forbidden and such filters should use .activate
> > > instead.
> > >
> > > The hardcoded [0] is wrong, no matter what the use case is.
> > >
> > > But to answer your question: It's about a new overlay_textsubs
> filter
> > > where framesync cannot be used and incoming frames (video on
> input0,
> > > subs on input1) need to be processed independently: the subtitle
> > > events
> > > are fed into the ass library (you could practically feed all
> events
> > > in advance) while the overlay image generation is done based on a
> > > given
> > > time code.
> > > This works fine by having a separate .filter_frame function for
> each
> > > input (after applying this patch).
> >
> > BTW
> >
> > > > IIUC this is forbidden and such filters should use .activate
> >
> > The scale2ref filter is set up in the same way (two .filter_frame
> > functions, no .activate, no framesync), probably because - like in
> > my case - the filter must not sync frames across inputs.
> >
> 
> FYI .activate does not sync frames across inputs.

Using .activate helped to solve another issue I had, so thanks again
for the hint.

Besides recommended usage patterns - would you disagree that this
patch is logically correct?

Regards,
softworkz
Paul B Mahol Sept. 6, 2021, 6:48 a.m. UTC | #9
On Mon, Sep 6, 2021 at 7:23 AM Soft Works <softworkz@hotmail.com> wrote:

>
>
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Paul B Mahol
> > Sent: Sunday, 5 September 2021 21:51
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix hardcoded
> > input index
> >
> > On Sun, Sep 5, 2021 at 9:44 PM Soft Works <softworkz@hotmail.com>
> > wrote:
> >
> > >
> > >
> > > > -----Original Message-----
> > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > > Soft Works
> > > > Sent: Sunday, 5 September 2021 16:59
> > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > devel@ffmpeg.org>
> > > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> > hardcoded
> > > > input index
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> > Of
> > > > > Paul B Mahol
> > > > > Sent: Sunday, 5 September 2021 11:01
> > > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > > devel@ffmpeg.org>
> > > > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> > > > hardcoded
> > > > > input index
> > > > >
> > > > > With what filters this happens?
> > > > >
> > > > > IIUC this is forbidden and such filters should use .activate
> > > > instead.
> > > >
> > > > The hardcoded [0] is wrong, no matter what the use case is.
> > > >
> > > > But to answer your question: It's about a new overlay_textsubs
> > filter
> > > > where framesync cannot be used and incoming frames (video on
> > input0,
> > > > subs on input1) need to be processed independently: the subtitle
> > > > events
> > > > are fed into the ass library (you could practically feed all
> > events
> > > > in advance) while the overlay image generation is done based on a
> > > > given
> > > > time code.
> > > > This works fine by having a separate .filter_frame function for
> > each
> > > > input (after applying this patch).
> > >
> > > BTW
> > >
> > > > > IIUC this is forbidden and such filters should use .activate
> > >
> > > The scale2ref filter is set up in the same way (two .filter_frame
> > > functions, no .activate, no framesync), probably because - like in
> > > my case - the filter must not sync frames across inputs.
> > >
> >
> > FYI .activate does not sync frames across inputs.
>
> Using .activate helped to solve another issue I had, so thanks again
> for the hint.
>
> Besides recommended usage patterns - would you disagree that this
> patch is logically correct?
>

Yes. Because .activate fixes numerous issues.
I see that only scale2ref is still using old way with multiple inputs. It
probably should be fixed,
and after that assert should be added in this file so new filters do not
expose same limitations and mistakes of the past.


> Regards,
> softworkz
>
>
> _______________________________________________
> 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".
>
Nicolas George Sept. 6, 2021, 6:53 a.m. UTC | #10
Paul B Mahol (12021-09-06):
> Yes. Because .activate fixes numerous issues.
> I see that only scale2ref is still using old way with multiple inputs. It
> probably should be fixed,
> and after that assert should be added in this file so new filters do not
> expose same limitations and mistakes of the past.

Thank you for explaining this.

A filter with several inputs but no synchronization between them is a
nonsense. If there is no synchronization, it should be two separate
filters.

It is possible to synchronize without activate, it was done before
activate; but it is harder.

Anyway, the code that is changed by this patch is the fall-back code for
filters without a request_frame() callback, and this was only ever
allowed if there is a single input.

Regards,
Soft Works Sept. 6, 2021, 7 a.m. UTC | #11
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Paul B Mahol
> Sent: Monday, 6 September 2021 08:48
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix hardcoded
> input index
> 
> On Mon, Sep 6, 2021 at 7:23 AM Soft Works <softworkz@hotmail.com>
> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Paul B Mahol
> > > Sent: Sunday, 5 September 2021 21:51
> > > To: FFmpeg development discussions and patches <ffmpeg-
> > > devel@ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> hardcoded
> > > input index
> > >
> > > On Sun, Sep 5, 2021 at 9:44 PM Soft Works <softworkz@hotmail.com>
> > > wrote:
> > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On
> Behalf Of
> > > > > Soft Works
> > > > > Sent: Sunday, 5 September 2021 16:59
> > > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > > devel@ffmpeg.org>
> > > > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> > > hardcoded
> > > > > input index
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On
> Behalf
> > > Of
> > > > > > Paul B Mahol
> > > > > > Sent: Sunday, 5 September 2021 11:01
> > > > > > To: FFmpeg development discussions and patches <ffmpeg-
> > > > > > devel@ffmpeg.org>
> > > > > > Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix
> > > > > hardcoded
> > > > > > input index
> > > > > >
> > > > > > With what filters this happens?
> > > > > >
> > > > > > IIUC this is forbidden and such filters should use
> .activate
> > > > > instead.
> > > > >
> > > > > The hardcoded [0] is wrong, no matter what the use case is.
> > > > >
> > > > > But to answer your question: It's about a new
> overlay_textsubs
> > > filter
> > > > > where framesync cannot be used and incoming frames (video on
> > > input0,
> > > > > subs on input1) need to be processed independently: the
> subtitle
> > > > > events
> > > > > are fed into the ass library (you could practically feed all
> > > events
> > > > > in advance) while the overlay image generation is done based
> on a
> > > > > given
> > > > > time code.
> > > > > This works fine by having a separate .filter_frame function
> for
> > > each
> > > > > input (after applying this patch).
> > > >
> > > > BTW
> > > >
> > > > > > IIUC this is forbidden and such filters should use
> .activate
> > > >
> > > > The scale2ref filter is set up in the same way (two
> .filter_frame
> > > > functions, no .activate, no framesync), probably because - like
> in
> > > > my case - the filter must not sync frames across inputs.
> > > >
> > >
> > > FYI .activate does not sync frames across inputs.
> >
> > Using .activate helped to solve another issue I had, so thanks
> again
> > for the hint.
> >
> > Besides recommended usage patterns - would you disagree that this
> > patch is logically correct?
> >
> 
> Yes. Because .activate fixes numerous issues.
> I see that only scale2ref is still using old way with multiple
> inputs. It
> probably should be fixed,
> and after that assert should be added in this file so new filters do
> not
> expose same limitations and mistakes of the past.

Thanks for the explanation. I will use .activate instead.

softworkz
Soft Works Sept. 6, 2021, 7:21 a.m. UTC | #12
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Nicolas George
> Sent: Monday, 6 September 2021 08:53
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] avfilter/avfilter: Fix hardcoded
> input index
> 
> Paul B Mahol (12021-09-06):
> > Yes. Because .activate fixes numerous issues.
> > I see that only scale2ref is still using old way with multiple
> inputs. It
> > probably should be fixed,
> > and after that assert should be added in this file so new filters
> do not
> > expose same limitations and mistakes of the past.
> 
> Thank you for explaining this.
> 
> A filter with several inputs but no synchronization between them is a
> nonsense. If there is no synchronization, it should be two separate
> filters.

I'm not quite sure about this. When we look at the scale2ref filter:

It has two inputs and two outputs and allows passing through two 
independent video streams:

- in0 >> out0  (main)
- in1 >> out1  (ref)

Both streams could have different frame rates. Once you would start
synchronizing (like with framesync), how would it still be possible to 
pass-through both streams at different rates?

Of course there will be a random lag between those streams without 
synchronization, but not arbitrarily large, which might be fine for 
certain use cases - like scale2ref filter.


> It is possible to synchronize without activate, it was done before
> activate; but it is harder.
> 
> Anyway, the code that is changed by this patch is the fall-back code
> for
> filters without a request_frame() callback, and this was only ever
> allowed if there is a single input.

OK understood. Thanks.

As Paul suggested, it might be reasonable to add an assert to make 
it clear that this isn't a supported way.

softworkz
diff mbox series

Patch

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index a04f8ed62f..78377ac7f9 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -461,7 +461,7 @@  static int64_t guess_status_pts(AVFilterContext *ctx, int status, AVRational lin
     return AV_NOPTS_VALUE;
 }
 
-static int ff_request_frame_to_filter(AVFilterLink *link)
+static int ff_request_frame_to_filter(AVFilterLink *link, int input_index)
 {
     int ret = -1;
 
@@ -470,8 +470,8 @@  static int ff_request_frame_to_filter(AVFilterLink *link)
     link->frame_blocked_in = 1;
     if (link->srcpad->request_frame)
         ret = link->srcpad->request_frame(link);
-    else if (link->src->inputs[0])
-        ret = ff_request_frame(link->src->inputs[0]);
+    else if (link->src->inputs[input_index])
+        ret = ff_request_frame(link->src->inputs[input_index]);
     if (ret < 0) {
         if (ret != AVERROR(EAGAIN) && ret != link->status_in)
             ff_avfilter_link_set_in_status(link, ret, guess_status_pts(link->src, ret, link->time_base));
@@ -1169,6 +1169,14 @@  static int forward_status_change(AVFilterContext *filter, AVFilterLink *in)
 {
     unsigned out = 0, progress = 0;
     int ret;
+    int input_index = 0;
+
+    for (int i = 0; i < in->dst->nb_inputs; i++) {
+        if (&in->dst->input_pads[i] == in->dstpad) {
+            input_index = i;
+            break;
+        }
+    }
 
     av_assert0(!in->status_out);
     if (!filter->nb_outputs) {
@@ -1178,7 +1186,7 @@  static int forward_status_change(AVFilterContext *filter, AVFilterLink *in)
     while (!in->status_out) {
         if (!filter->outputs[out]->status_in) {
             progress++;
-            ret = ff_request_frame_to_filter(filter->outputs[out]);
+            ret = ff_request_frame_to_filter(filter->outputs[out], input_index);
             if (ret < 0)
                 return ret;
         }
@@ -1215,7 +1223,7 @@  static int ff_filter_activate_default(AVFilterContext *filter)
     for (i = 0; i < filter->nb_outputs; i++) {
         if (filter->outputs[i]->frame_wanted_out &&
             !filter->outputs[i]->frame_blocked_in) {
-            return ff_request_frame_to_filter(filter->outputs[i]);
+            return ff_request_frame_to_filter(filter->outputs[i], 0);
         }
     }
     return FFERROR_NOT_READY;