diff mbox series

[FFmpeg-devel] ffmpeg: add 100ms delay for first stats

Message ID 20201223045809.709-1-ffmpeg@gyani.pro
State New
Headers show
Series [FFmpeg-devel] ffmpeg: add 100ms delay for first stats
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Gyan Doshi Dec. 23, 2020, 4:58 a.m. UTC
Avoids breaking output file dump report.
---
 fftools/ffmpeg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Gyan Doshi Dec. 23, 2020, 9:07 a.m. UTC | #1
On 23-12-2020 10:28 am, Gyan Doshi wrote:
> Avoids breaking output file dump report.
> ---
>   fftools/ffmpeg.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 2c0820aacf..449da484d9 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -1699,7 +1699,8 @@ static void print_report(int is_last_report, int64_t timer_start, int64_t cur_ti
>           if (last_time == -1) {
>               last_time = cur_time;
>           }
> -        if ((cur_time - last_time) < stats_period && !first_report)
> +        if (((cur_time - last_time) < stats_period && !first_report) ||
> +            ((cur_time - last_time) < 100000 && first_report))
>               return;
>           last_time = cur_time;
>       }

Looks fixed here. Can anyone else do a quick check?

Thanks,
Gyan
Nicolas George Dec. 23, 2020, 9:42 a.m. UTC | #2
Gyan Doshi (12020-12-23):
> Avoids breaking output file dump report.
> ---
>  fftools/ffmpeg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

This looks broken. Why 100ms? Because on your setup, 80ms is not enough
but 100ms is? On a slower system, it would need to be 120 or 200.

Test if the info to be printed is already valid.

Regards,
Gyan Doshi Dec. 23, 2020, 10:01 a.m. UTC | #3
On 23-12-2020 03:12 pm, Nicolas George wrote:
> Gyan Doshi (12020-12-23):
>> Avoids breaking output file dump report.
>> ---
>>   fftools/ffmpeg.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
> This looks broken. Why 100ms? Because on your setup, 80ms is not enough
> but 100ms is? On a slower system, it would need to be 120 or 200.
>
> Test if the info to be printed is already valid.

Which piece of info or object would indicate that? At the stage of this 
check, I don't see any available.

Earlier, it was hardcoded to 500 ms. I chose something more apt for 
2020, but 500 ms is fine as well.

Regards,
Gyan
Nicolas George Dec. 23, 2020, 10:05 a.m. UTC | #4
Gyan Doshi (12020-12-23):
> Which piece of info or object would indicate that? At the stage of this
> check, I don't see any available.

Read Michael's mail:

Output #0, avi, to 'file.avi':=       0kB time=-577014:32:22.77 bitrate= -0.0kbits/s speed=N/A

The problem is obviously the time. The large negative number looks like
it was converted from AV_NOPTS_VALUE, so test if the PTS being printed
is AV_NOPTS_VALUE or not.

> Earlier, it was hardcoded to 500 ms. I chose something more apt for 2020,
> but 500 ms is fine as well.

If you fix it, fix it properly, please.

Regards,
Gyan Doshi Dec. 23, 2020, 10:19 a.m. UTC | #5
On 23-12-2020 03:35 pm, Nicolas George wrote:
> Gyan Doshi (12020-12-23):
>> Which piece of info or object would indicate that? At the stage of this
>> check, I don't see any available.
> Read Michael's mail:
>
> Output #0, avi, to 'file.avi':=       0kB time=-577014:32:22.77 bitrate= -0.0kbits/s speed=N/A
>
> The problem is obviously the time. The large negative number looks like
> it was converted from AV_NOPTS_VALUE, so test if the PTS being printed
> is AV_NOPTS_VALUE or not.

That's not the issue. It's the mangled line - the output from 
lavf/dump.c. mangled with stats line

AV_NOPTS_VALUE is an acceptable value and already checked for at line 
1815. As is <=0 output size.

Indicating to the user that ffmpeg processing has stalled at the 
beginning is a valid and acceptable outcome, even a necessary one.
The delay here is just to avoid the mangling. Some hardcoded value is 
the best that I see we can do, but if you have an analytical solution, 
I'm all ears.

Regards,
Gyan
Nicolas George Dec. 23, 2020, 10:23 a.m. UTC | #6
Gyan Doshi (12020-12-23):
> That's not the issue. It's the mangled line - the output from lavf/dump.c.
> mangled with stats line
> 
> AV_NOPTS_VALUE is an acceptable value and already checked for at line 1815.
> As is <=0 output size.
> 
> Indicating to the user that ffmpeg processing has stalled at the beginning
> is a valid and acceptable outcome, even a necessary one.
> The delay here is just to avoid the mangling. Some hardcoded value is the
> best that I see we can do, but if you have an analytical solution, I'm all
> ears.

Ah, I see. But in that case, the proper solution seems obvious: set a
flag when av_dump_format() has returned, and delay the printing until
that flag is set. Or am I still missing something?

Regards,
Gyan Doshi Dec. 23, 2020, 10:28 a.m. UTC | #7
On 23-12-2020 03:53 pm, Nicolas George wrote:
> Gyan Doshi (12020-12-23):
>> That's not the issue. It's the mangled line - the output from lavf/dump.c.
>> mangled with stats line
>>
>> AV_NOPTS_VALUE is an acceptable value and already checked for at line 1815.
>> As is <=0 output size.
>>
>> Indicating to the user that ffmpeg processing has stalled at the beginning
>> is a valid and acceptable outcome, even a necessary one.
>> The delay here is just to avoid the mangling. Some hardcoded value is the
>> best that I see we can do, but if you have an analytical solution, I'm all
>> ears.
> Ah, I see. But in that case, the proper solution seems obvious: set a
> flag when av_dump_format() has returned, and delay the printing until
> that flag is set. Or am I still missing something?

It's    void av_dump_format(...)

Seems too trivial to change it , or add + initialize yet another global 
variable just for this.

Regards,
Gyan
Nicolas George Dec. 23, 2020, 10:33 a.m. UTC | #8
Gyan Doshi (12020-12-23):
> It's    void av_dump_format(...)
> 
> Seems too trivial to change it , or add + initialize yet another global
> variable just for this.

Exactly. Dumping the output is a global affair, it can warrant a global
variable. Something like this:

    of->header_written = 1;

    av_dump_format(of->ctx, file_index, of->ctx->url, 1);
+   output_dumped++;

...

+   if (output_dumped >= nb_output_files) {
        ...
+   }

That way, the initial stat is printed as early as possible, but will not
mangle the dump. And if the dump is very late, it is acceptable to have
the 500ms timeout too:

+   if (output_dumped >= nb_output_files || blah >= 500000) {

Regards,
Gyan Doshi Dec. 23, 2020, 10:48 a.m. UTC | #9
On 23-12-2020 04:03 pm, Nicolas George wrote:
> Gyan Doshi (12020-12-23):
>> It's    void av_dump_format(...)
>>
>> Seems too trivial to change it , or add + initialize yet another global
>> variable just for this.
> Exactly. Dumping the output is a global affair, it can warrant a global
> variable. Something like this:
>
>      of->header_written = 1;
>
>      av_dump_format(of->ctx, file_index, of->ctx->url, 1);
> +   output_dumped++;
>
> ...
>
> +   if (output_dumped >= nb_output_files) {
>          ...
> +   }
>
> That way, the initial stat is printed as early as possible, but will not
> mangle the dump. And if the dump is very late, it is acceptable to have
> the 500ms timeout too:
>
> +   if (output_dumped >= nb_output_files || blah >= 500000) {

Ok. Will work on this.

But I still think I should post an immediate fix that handles it for 90% 
of cases.

Thanks,
Gyan
diff mbox series

Patch

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 2c0820aacf..449da484d9 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -1699,7 +1699,8 @@  static void print_report(int is_last_report, int64_t timer_start, int64_t cur_ti
         if (last_time == -1) {
             last_time = cur_time;
         }
-        if ((cur_time - last_time) < stats_period && !first_report)
+        if (((cur_time - last_time) < stats_period && !first_report) ||
+            ((cur_time - last_time) < 100000 && first_report))
             return;
         last_time = cur_time;
     }