diff mbox series

[FFmpeg-devel,1/2] fftools/ffmpeg: exit application when decoding returns AVERROR_EXIT

Message ID MN2PR04MB5981036E3F53F3BA24E992A9BABC9@MN2PR04MB5981.namprd04.prod.outlook.com
State Withdrawn, archived
Headers show
Series [FFmpeg-devel,1/2] fftools/ffmpeg: exit application when decoding returns AVERROR_EXIT
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 Oct. 18, 2021, 11:24 p.m. UTC
Introduce a way for decoders to request application exit via error return

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 fftools/ffmpeg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Gyan Doshi Oct. 19, 2021, 4 a.m. UTC | #1
On 2021-10-19 04:54 am, Soft Works wrote:
> Introduce a way for decoders to request application exit via error return

Why? The ffmpeg app may be processing multiple inputs and outputs. At 
most, you can close the codec and end the stream.

> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
>   fftools/ffmpeg.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 9d4f9d7a2b..dbbe670a0a 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2727,7 +2727,7 @@ static int process_input_packet(InputStream *ist, const AVPacket *pkt, int no_eo
>                   av_log(NULL, AV_LOG_FATAL, "Error while processing the decoded "
>                          "data for stream #%d:%d\n", ist->file_index, ist->st->index);
>               }
> -            if (!decode_failed || exit_on_error)
> +            if (!decode_failed || exit_on_error || ret == AVERROR_EXIT)
>                   exit_program(1);
>               break;
>           }
Soft Works Oct. 19, 2021, 4:08 a.m. UTC | #2
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Gyan Doshi
> Sent: Tuesday, October 19, 2021 6:01 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> application when decoding returns AVERROR_EXIT
> 
> 
> 
> On 2021-10-19 04:54 am, Soft Works wrote:
> > Introduce a way for decoders to request application exit via error
> return
> 
> Why? The ffmpeg app may be processing multiple inputs and outputs. At
> most, you can close the codec and end the stream.

When a hardware device fails unexpectedly, why should the app continue
running? In this case, it's clear that things do not work as intended.

softworkz
Gyan Doshi Oct. 19, 2021, 4:20 a.m. UTC | #3
On 2021-10-19 09:38 am, Soft Works wrote:
>
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Gyan Doshi
>> Sent: Tuesday, October 19, 2021 6:01 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
>> application when decoding returns AVERROR_EXIT
>>
>>
>>
>> On 2021-10-19 04:54 am, Soft Works wrote:
>>> Introduce a way for decoders to request application exit via error
>> return
>>
>> Why? The ffmpeg app may be processing multiple inputs and outputs. At
>> most, you can close the codec and end the stream.
> When a hardware device fails unexpectedly, why should the app continue
> running? In this case, it's clear that things do not work as intended.

Because the application may be doing other things and the hardware 
function may not be central to its task.

See the condition just before the one you added. It is `exit_on_error` 
which implies that the user can *choose* to exit, or not, upon error. 
Your patch lets the decoder unilaterally decide to quit without user input.

In fact, I don't think your patch is necessary. You added the condition 
in process_input_packet() . During decoding, check_decode_result() is 
called, which has

     if (ret < 0 && exit_on_error)
         exit_program(1);

So, the user can choose to set exit_on_error and get this behaviour.

Regards,
Gyan
Soft Works Oct. 19, 2021, 4:41 a.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Gyan Doshi
> Sent: Tuesday, October 19, 2021 6:21 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> application when decoding returns AVERROR_EXIT
> 
> 
> 
> On 2021-10-19 09:38 am, Soft Works wrote:
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Gyan Doshi
> >> Sent: Tuesday, October 19, 2021 6:01 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> >> application when decoding returns AVERROR_EXIT
> >>
> >>
> >>
> >> On 2021-10-19 04:54 am, Soft Works wrote:
> >>> Introduce a way for decoders to request application exit via
> error
> >> return
> >>
> >> Why? The ffmpeg app may be processing multiple inputs and outputs.
> At
> >> most, you can close the codec and end the stream.
> > When a hardware device fails unexpectedly, why should the app
> continue
> > running? In this case, it's clear that things do not work as
> intended.
> 
> Because the application may be doing other things and the hardware
> function may not be central to its task.

Is that really a realistic scenario that a user wants ffmpeg to
continue even though his setup doesn't work?
All over the code there are so many less critical conditions that
lead ffmpeg to exit.


> See the condition just before the one you added. It is
> `exit_on_error`
> which implies that the user can *choose* to exit, or not, upon error.
> Your patch lets the decoder unilaterally decide to quit without user
> input.

Yes, exactly.

> In fact, I don't think your patch is necessary. You added the
> condition
> in process_input_packet() . During decoding, check_decode_result() is
> called, which has
> 
>      if (ret < 0 && exit_on_error)
>          exit_program(1);
> 
> So, the user can choose to set exit_on_error and get this behaviour.

Well, but usually nobody sets this.

So when it isn't set, we should keep ffmpeg 
alive to let it write a 10 GB logfile?

Maybe there's a better way than my patch, but what happens is 
a bug IMO, and I wouldn't say that it would be a fix to say that 
a certain option needs to be set by the user to "fix it".

Kind regards,
softworkz
Gyan Doshi Oct. 19, 2021, 4:50 a.m. UTC | #5
On 2021-10-19 10:11 am, Soft Works wrote:

> Maybe there's a better way than my patch, but what happens is
> a bug IMO, and I wouldn't say that it would be a fix to say that
> a certain option needs to be set by the user to "fix it".

What's ideally required is for user to designate essential and 
non-essential parts of the task. Then some errors can be ignored, and 
some are fatal.

Regards,
Gyan
Soft Works Oct. 19, 2021, 5:07 a.m. UTC | #6
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Gyan Doshi
> Sent: Tuesday, October 19, 2021 6:51 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> application when decoding returns AVERROR_EXIT
> 
> 
> 
> On 2021-10-19 10:11 am, Soft Works wrote:
> 
> > Maybe there's a better way than my patch, but what happens is
> > a bug IMO, and I wouldn't say that it would be a fix to say that
> > a certain option needs to be set by the user to "fix it".
> 
> What's ideally required is for user to designate essential and
> non-essential parts of the task. Then some errors can be ignored, and
> some are fatal.

Yes, and we know when this will happen - haha.

I guess I'll keep my fix and ffmpeg keeps the bug.

softworkz
Paul B Mahol Oct. 19, 2021, 6:41 a.m. UTC | #7
On Tue, Oct 19, 2021 at 7:07 AM Soft Works <softworkz@hotmail.com> wrote:

>
>
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Gyan Doshi
> > Sent: Tuesday, October 19, 2021 6:51 AM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> > application when decoding returns AVERROR_EXIT
> >
> >
> >
> > On 2021-10-19 10:11 am, Soft Works wrote:
> >
> > > Maybe there's a better way than my patch, but what happens is
> > > a bug IMO, and I wouldn't say that it would be a fix to say that
> > > a certain option needs to be set by the user to "fix it".
> >
> > What's ideally required is for user to designate essential and
> > non-essential parts of the task. Then some errors can be ignored, and
> > some are fatal.
>
> Yes, and we know when this will happen - haha.
>
> I guess I'll keep my fix and ffmpeg keeps the bug.
>

Please refrain from such communication here.


> 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".
>
James Almer Oct. 19, 2021, 2:38 p.m. UTC | #8
On 10/19/2021 2:07 AM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Gyan Doshi
>> Sent: Tuesday, October 19, 2021 6:51 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
>> application when decoding returns AVERROR_EXIT
>>
>>
>>
>> On 2021-10-19 10:11 am, Soft Works wrote:
>>
>>> Maybe there's a better way than my patch, but what happens is
>>> a bug IMO, and I wouldn't say that it would be a fix to say that
>>> a certain option needs to be set by the user to "fix it".
>>
>> What's ideally required is for user to designate essential and
>> non-essential parts of the task. Then some errors can be ignored, and
>> some are fatal.
> 
> Yes, and we know when this will happen - haha.
> 
> I guess I'll keep my fix and ffmpeg keeps the bug.

You could also just call ffmpeg with the -xerror option in this scenario 
where cuda is failing. It should abort the process at the exact same 
point your patch tries to.

> 
> 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 Oct. 19, 2021, 6:59 p.m. UTC | #9
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> James Almer
> Sent: Tuesday, October 19, 2021 4:39 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> application when decoding returns AVERROR_EXIT
> 
> On 10/19/2021 2:07 AM, Soft Works wrote:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Gyan Doshi
> >> Sent: Tuesday, October 19, 2021 6:51 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> >> application when decoding returns AVERROR_EXIT
> >>
> >>
> >>
> >> On 2021-10-19 10:11 am, Soft Works wrote:
> >>
> >>> Maybe there's a better way than my patch, but what happens is
> >>> a bug IMO, and I wouldn't say that it would be a fix to say that
> >>> a certain option needs to be set by the user to "fix it".
> >>
> >> What's ideally required is for user to designate essential and
> >> non-essential parts of the task. Then some errors can be ignored,
> and
> >> some are fatal.
> >
> > Yes, and we know when this will happen - haha.
> >
> > I guess I'll keep my fix and ffmpeg keeps the bug.
> 
> You could also just call ffmpeg with the -xerror option in this
> scenario
> where cuda is failing. It should abort the process at the exact same
> point your patch tries to.

I try to contribute patches based on experience when working 
with ffmpeg for the benefit of other ffmpeg users.
Though, I have no need for these to get merged as we're always
running ffmppeg builds from a custom fork, besides that it would
be nice to keep the differences at a moderate level.

-xerror is a widely unknown parameter that nobody is using, so it 
doesn't help for dealing with this situation. Also it's not 
a fix for a bug to say that it doesn't occur when a certain parameter
is specified.

This is not the first situation where my fixes for obvious and 
clear bugs are talked away with great effort, but with zero
effort and zero interest to find a solution for the actual 
problems they are meant to address.

I am open to suggestions to fix the issue in a different way.
Otherwise - if you prefer to keep things as they are, we 
get just the result I described above: 

It's fixed for me, but the bugs remain unfixed in ffmpeg.

@Paul: I don't see why this would be bad communication, it's just
a simple factual assessment.

Kind regards,
softworkz
Paul B Mahol Oct. 19, 2021, 7:51 p.m. UTC | #10
Feel free to keep hacks for yourself.

No need to send it to us, it is considered as spam as more useful patches
are forgotten this way.
James Almer Oct. 19, 2021, 8:06 p.m. UTC | #11
On 10/19/2021 4:51 PM, Paul B Mahol wrote:
> Feel free to keep hacks for yourself.
> 
> No need to send it to us, it is considered as spam as more useful patches
> are forgotten this way.

No, you keep this kind of reply to yourself. Don't badmouth contributors.
Soft Works Oct. 19, 2021, 8:17 p.m. UTC | #12
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Paul B Mahol
> Sent: Tuesday, October 19, 2021 9:52 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> application when decoding returns AVERROR_EXIT
> 
> Feel free to keep hacks for yourself.
> 
> No need to send it to us, it is considered as spam as more useful
> patches
> are forgotten this way.

Paul, let me respond with your very own words:

[Paul Mahol to Nicolas George, Sat 11 Sep 2021 10:22]

> Please stop immediately with your immature and childish behavior!
> 
> Patches are welcome, if you do not like them, just leave project for
> once.
> 
> Not welcoming patch - not welcomed to project.

softworkz
James Almer Oct. 19, 2021, 8:50 p.m. UTC | #13
On 10/19/2021 3:59 PM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> James Almer
>> Sent: Tuesday, October 19, 2021 4:39 PM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
>> application when decoding returns AVERROR_EXIT
>>
>> On 10/19/2021 2:07 AM, Soft Works wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>>>> Gyan Doshi
>>>> Sent: Tuesday, October 19, 2021 6:51 AM
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
>>>> application when decoding returns AVERROR_EXIT
>>>>
>>>>
>>>>
>>>> On 2021-10-19 10:11 am, Soft Works wrote:
>>>>
>>>>> Maybe there's a better way than my patch, but what happens is
>>>>> a bug IMO, and I wouldn't say that it would be a fix to say that
>>>>> a certain option needs to be set by the user to "fix it".
>>>>
>>>> What's ideally required is for user to designate essential and
>>>> non-essential parts of the task. Then some errors can be ignored,
>> and
>>>> some are fatal.
>>>
>>> Yes, and we know when this will happen - haha.
>>>
>>> I guess I'll keep my fix and ffmpeg keeps the bug.
>>
>> You could also just call ffmpeg with the -xerror option in this
>> scenario
>> where cuda is failing. It should abort the process at the exact same
>> point your patch tries to.
> 
> I try to contribute patches based on experience when working
> with ffmpeg for the benefit of other ffmpeg users.
> Though, I have no need for these to get merged as we're always
> running ffmppeg builds from a custom fork, besides that it would
> be nice to keep the differences at a moderate level.
> 
> -xerror is a widely unknown parameter that nobody is using, so it

It exists, and is documented in both texigen and the ffmpeg CLI help 
output. People not being familiar with it doesn't mean fixes should be 
designed in a way as if it didn't exist.

> doesn't help for dealing with this situation. Also it's not
> a fix for a bug to say that it doesn't occur when a certain parameter
> is specified.
> 
> This is not the first situation where my fixes for obvious and
> clear bugs are talked away with great effort, but with zero
> effort and zero interest to find a solution for the actual
> problems they are meant to address.

You didn't ask for alternatives until now, only submitted this patch and 
argued in favor of it. Gyan explained to you why letting a module 
overide a user settable option like xerror is not ok.

If you look at the code where you added the check for AVERROR_EXIT, it 
explicitly ensures it's *not* a decode error before deciding to abort or 
not, because decoders can fail at decoding a packet but easily recover 
after receiving new ones.
The exit_on_error option was added among other reason to override this 
behavior, so i again recommend you to use it.

> 
> I am open to suggestions to fix the issue in a different way.

If the issue is that a CUDA module like cuviddec gets stuck in an 
unrecoverable state with decode/receive_frame calls, regardless of what 
packet or how many you pass to it, then an option could be to somehow 
prevent said module to get stuck like that, by for example adding some 
sort of retry count to ffmpeg CLI, with infinite as default value to 
maintain the current behavior, and make it user configurable. But then 
you could also just use the existing xerror option if you're fine with 
aborting as soon as a single packet fails to decode.

> Otherwise - if you prefer to keep things as they are, we
> get just the result I described above:
> 
> It's fixed for me, but the bugs remain unfixed in ffmpeg.

Why do you think it's a bug? ffmpeg, by design, is explicitly not 
stopping on decoding errors. Your issue is that it prints way too many 
error log lines and you apparently never stop sending it new packets, so 
ffmpeg will keep passing them to cuviddec and it will keep barfing them.
Do you need ffmpeg to output a log at all? You could disable logging 
with -loglevel, or add a way for to ffmpeg not print >= error level log 
messages, if there isn't one already.

> 
> @Paul: I don't see why this would be bad communication, it's just
> a simple factual assessment.
> 
> Kind 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".
>
Soft Works Oct. 20, 2021, 12:42 a.m. UTC | #14
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> James Almer
> Sent: Tuesday, October 19, 2021 10:51 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> application when decoding returns AVERROR_EXIT
> 
> On 10/19/2021 3:59 PM, Soft Works wrote:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> James Almer
> >> Sent: Tuesday, October 19, 2021 4:39 PM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> >> application when decoding returns AVERROR_EXIT
> >>
> >> On 10/19/2021 2:07 AM, Soft Works wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> >>>> Gyan Doshi
> >>>> Sent: Tuesday, October 19, 2021 6:51 AM
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> >>>> application when decoding returns AVERROR_EXIT
> >>>>
> >>>>
> >>>>
> >>>> On 2021-10-19 10:11 am, Soft Works wrote:
> >>>>
> >>>>> Maybe there's a better way than my patch, but what happens is
> >>>>> a bug IMO, and I wouldn't say that it would be a fix to say
> that
> >>>>> a certain option needs to be set by the user to "fix it".
> >>>>
> >>>> What's ideally required is for user to designate essential and
> >>>> non-essential parts of the task. Then some errors can be
> ignored,
> >> and
> >>>> some are fatal.
> >>>
> >>> Yes, and we know when this will happen - haha.
> >>>
> >>> I guess I'll keep my fix and ffmpeg keeps the bug.
> >>
> >> You could also just call ffmpeg with the -xerror option in this
> >> scenario
> >> where cuda is failing. It should abort the process at the exact
> same
> >> point your patch tries to.
> >
> > I try to contribute patches based on experience when working
> > with ffmpeg for the benefit of other ffmpeg users.
> > Though, I have no need for these to get merged as we're always
> > running ffmppeg builds from a custom fork, besides that it would
> > be nice to keep the differences at a moderate level.
> >
> > -xerror is a widely unknown parameter that nobody is using, so it
> 
> It exists, and is documented in both texigen and the ffmpeg CLI help
> output. People not being familiar with it doesn't mean fixes should
> be
> designed in a way as if it didn't exist.

Yes, but were not talking about a transient error here, where there
would be a choice about whether to continue.

We're talking about an unrecoverable decoder error and currently,
this is causing ffmpeg to get into an ENDLESS LOOP (or at least
almost endless) where ffmpeg will only exit once the HD is full 
from its log output.

And you want to tell that this behavior is correct and that a user
needs to be aware of the -xerror parameter and specify it to 
prevent this from happening?

When there's an error in a filter => ffmpeg exits
When there's an error in an encoder => ffmpeg exits
When there's an error initializing a decoder or an encoder
=> ffmpeg exits

So why shouldn't ffmpeg exit when there's an unrecoverable error 
in a decoder?


In our use case of ffmpeg, we are auto-building an extremely wide
range of ffmpeg commands with and without hw accelerations (and 
from these - most of which ffmpeg supports).

Over the years, I have looked at more than a thousand log files
from users.
And I haven't seen a single case like this before, where ffmpeg 
doesn't exit when an error occurs with some hw acceleration and
continues to output millions of times the same 2 lines to the log.

softworkz
Soft Works Oct. 20, 2021, 12:53 a.m. UTC | #15
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> James Almer
> Sent: Tuesday, October 19, 2021 10:51 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> application when decoding returns AVERROR_EXIT


> You didn't ask for alternatives until now, only submitted this patch
> and
> argued in favor of it. 

No that's not true, I already said 

"Maybe there's a better way than my patch, .."


> explicitly ensures it's *not* a decode error before deciding to abort
> or
> not, because decoders can fail at decoding a packet but easily
> recover
> after receiving new ones.

Yes, that's why I'm returning an error code for this case that
clearly indicates that it is an unrecoverable error.


> > I am open to suggestions to fix the issue in a different way.
> 
> If the issue is that a CUDA module like cuviddec gets stuck in an
> unrecoverable state with decode/receive_frame calls, regardless of
> what
> packet or how many you pass to it, then an option could be to somehow
> prevent said module to get stuck like that, by for example adding
> some
> sort of retry count to ffmpeg CLI, with infinite as default 

Why does an unrecoverable error need retries?


> Why do you think it's a bug? ffmpeg, by design, is explicitly not
> stopping on decoding errors.

This is not a decoding error, it's a decoder error and surely not
the intended kind of case where ffmpeg should continue.

> Do you need ffmpeg to output a log at all?

How should I be able to diagnose and discover such bugs without
outputting a log?

Kind regards,
softworkz
Nicolas George Oct. 20, 2021, 2:56 p.m. UTC | #16
Soft Works (12021-10-18):
> Introduce a way for decoders to request application exit via error return
> 
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
>  fftools/ffmpeg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 9d4f9d7a2b..dbbe670a0a 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2727,7 +2727,7 @@ static int process_input_packet(InputStream *ist, const AVPacket *pkt, int no_eo
>                  av_log(NULL, AV_LOG_FATAL, "Error while processing the decoded "
>                         "data for stream #%d:%d\n", ist->file_index, ist->st->index);
>              }
> -            if (!decode_failed || exit_on_error)
> +            if (!decode_failed || exit_on_error || ret == AVERROR_EXIT)
>                  exit_program(1);
>              break;
>          }

On top of everything else that has been said about it, this is not the
semantic of AVERROR_EXIT, which is meant for communication within a
library.
Soft Works Oct. 20, 2021, 3 p.m. UTC | #17
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Nicolas George
> Sent: Wednesday, October 20, 2021 4:57 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> application when decoding returns AVERROR_EXIT
> 
> Soft Works (12021-10-18):
> > Introduce a way for decoders to request application exit via error
> return
> >
> > Signed-off-by: softworkz <softworkz@hotmail.com>
> > ---
> >  fftools/ffmpeg.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 9d4f9d7a2b..dbbe670a0a 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -2727,7 +2727,7 @@ static int process_input_packet(InputStream
> *ist, const AVPacket *pkt, int no_eo
> >                  av_log(NULL, AV_LOG_FATAL, "Error while processing
> the decoded "
> >                         "data for stream #%d:%d\n", ist-
> >file_index, ist->st->index);
> >              }
> > -            if (!decode_failed || exit_on_error)
> > +            if (!decode_failed || exit_on_error || ret ==
> AVERROR_EXIT)
> >                  exit_program(1);
> >              break;
> >          }
> 
> On top of everything else that has been said about it, this is not
> the
> semantic of AVERROR_EXIT, which is meant for communication within a
> library.

Sure. What would be a better way?
James Almer Oct. 25, 2021, 3:33 p.m. UTC | #18
On 10/20/2021 12:00 PM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Nicolas George
>> Sent: Wednesday, October 20, 2021 4:57 PM
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
>> application when decoding returns AVERROR_EXIT
>>
>> Soft Works (12021-10-18):
>>> Introduce a way for decoders to request application exit via error
>> return
>>>
>>> Signed-off-by: softworkz <softworkz@hotmail.com>
>>> ---
>>>   fftools/ffmpeg.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>>> index 9d4f9d7a2b..dbbe670a0a 100644
>>> --- a/fftools/ffmpeg.c
>>> +++ b/fftools/ffmpeg.c
>>> @@ -2727,7 +2727,7 @@ static int process_input_packet(InputStream
>> *ist, const AVPacket *pkt, int no_eo
>>>                   av_log(NULL, AV_LOG_FATAL, "Error while processing
>> the decoded "
>>>                          "data for stream #%d:%d\n", ist-
>>> file_index, ist->st->index);
>>>               }
>>> -            if (!decode_failed || exit_on_error)
>>> +            if (!decode_failed || exit_on_error || ret ==
>> AVERROR_EXIT)
>>>                   exit_program(1);
>>>               break;
>>>           }
>>
>> On top of everything else that has been said about it, this is not
>> the
>> semantic of AVERROR_EXIT, which is meant for communication within a
>> library.
> 
> Sure. What would be a better way?

What is this scenario that makes cuviddec always return this 
CUDA_ERROR_UNKNOWN error when calling cuvidParseVideoData()? Why did it 
not happen during init()? If there's a problem with the underlying 
hardware, I'd expect the process to not even be able to reach the point 
where you pass packets to it.
Soft Works Oct. 26, 2021, 3:33 a.m. UTC | #19
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> James Almer
> Sent: Monday, October 25, 2021 5:33 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> application when decoding returns AVERROR_EXIT
> 
> On 10/20/2021 12:00 PM, Soft Works wrote:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Nicolas George
> >> Sent: Wednesday, October 20, 2021 4:57 PM
> >> To: FFmpeg development discussions and patches <ffmpeg-
> >> devel@ffmpeg.org>
> >> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
> >> application when decoding returns AVERROR_EXIT
> >>
> >> Soft Works (12021-10-18):
> >>> Introduce a way for decoders to request application exit via
> error
> >> return
> >>>
> >>> Signed-off-by: softworkz <softworkz@hotmail.com>
> >>> ---
> >>>   fftools/ffmpeg.c | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> >>> index 9d4f9d7a2b..dbbe670a0a 100644
> >>> --- a/fftools/ffmpeg.c
> >>> +++ b/fftools/ffmpeg.c
> >>> @@ -2727,7 +2727,7 @@ static int process_input_packet(InputStream
> >> *ist, const AVPacket *pkt, int no_eo
> >>>                   av_log(NULL, AV_LOG_FATAL, "Error while
> processing
> >> the decoded "
> >>>                          "data for stream #%d:%d\n", ist-
> >>> file_index, ist->st->index);
> >>>               }
> >>> -            if (!decode_failed || exit_on_error)
> >>> +            if (!decode_failed || exit_on_error || ret ==
> >> AVERROR_EXIT)
> >>>                   exit_program(1);
> >>>               break;
> >>>           }
> >>
> >> On top of everything else that has been said about it, this is not
> >> the
> >> semantic of AVERROR_EXIT, which is meant for communication within
> a
> >> library.
> >
> > Sure. What would be a better way?
> 
> What is this scenario that makes cuviddec always return this
> CUDA_ERROR_UNKNOWN error when calling cuvidParseVideoData()? Why did
> it
> not happen during init()? If there's a problem with the underlying
> hardware, I'd expect the process to not even be able to reach the
> point
> where you pass packets to it.

My approach so far was on a more general side: I've seen that this
can happen => find a way to prevent this from happening, no matter
what input file has caused this and no matter whether that file is
valid or not.

But if you would like to reproduce the issue, I could get the file from
the user who encountered this. I can also provide the 10 GB log (80MB
compressed)

Do you want those files?

Thanks for replying,
softworkz
James Almer Oct. 26, 2021, 11:34 a.m. UTC | #20
On 10/26/2021 12:33 AM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> James Almer
>> Sent: Monday, October 25, 2021 5:33 PM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
>> application when decoding returns AVERROR_EXIT
>>
>> On 10/20/2021 12:00 PM, Soft Works wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>>>> Nicolas George
>>>> Sent: Wednesday, October 20, 2021 4:57 PM
>>>> To: FFmpeg development discussions and patches <ffmpeg-
>>>> devel@ffmpeg.org>
>>>> Subject: Re: [FFmpeg-devel] [PATCH 1/2] fftools/ffmpeg: exit
>>>> application when decoding returns AVERROR_EXIT
>>>>
>>>> Soft Works (12021-10-18):
>>>>> Introduce a way for decoders to request application exit via
>> error
>>>> return
>>>>>
>>>>> Signed-off-by: softworkz <softworkz@hotmail.com>
>>>>> ---
>>>>>    fftools/ffmpeg.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>>>>> index 9d4f9d7a2b..dbbe670a0a 100644
>>>>> --- a/fftools/ffmpeg.c
>>>>> +++ b/fftools/ffmpeg.c
>>>>> @@ -2727,7 +2727,7 @@ static int process_input_packet(InputStream
>>>> *ist, const AVPacket *pkt, int no_eo
>>>>>                    av_log(NULL, AV_LOG_FATAL, "Error while
>> processing
>>>> the decoded "
>>>>>                           "data for stream #%d:%d\n", ist-
>>>>> file_index, ist->st->index);
>>>>>                }
>>>>> -            if (!decode_failed || exit_on_error)
>>>>> +            if (!decode_failed || exit_on_error || ret ==
>>>> AVERROR_EXIT)
>>>>>                    exit_program(1);
>>>>>                break;
>>>>>            }
>>>>
>>>> On top of everything else that has been said about it, this is not
>>>> the
>>>> semantic of AVERROR_EXIT, which is meant for communication within
>> a
>>>> library.
>>>
>>> Sure. What would be a better way?
>>
>> What is this scenario that makes cuviddec always return this
>> CUDA_ERROR_UNKNOWN error when calling cuvidParseVideoData()? Why did
>> it
>> not happen during init()? If there's a problem with the underlying
>> hardware, I'd expect the process to not even be able to reach the
>> point
>> where you pass packets to it.
> 
> My approach so far was on a more general side: I've seen that this
> can happen => find a way to prevent this from happening, no matter
> what input file has caused this and no matter whether that file is
> valid or not.
> 
> But if you would like to reproduce the issue, I could get the file from
> the user who encountered this. I can also provide the 10 GB log (80MB
> compressed)
> 
> Do you want those files?

The sample would help find a solution, yes. The logs not really.

> 
> Thanks for replying,
> 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".
>
diff mbox series

Patch

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 9d4f9d7a2b..dbbe670a0a 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -2727,7 +2727,7 @@  static int process_input_packet(InputStream *ist, const AVPacket *pkt, int no_eo
                 av_log(NULL, AV_LOG_FATAL, "Error while processing the decoded "
                        "data for stream #%d:%d\n", ist->file_index, ist->st->index);
             }
-            if (!decode_failed || exit_on_error)
+            if (!decode_failed || exit_on_error || ret == AVERROR_EXIT)
                 exit_program(1);
             break;
         }