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

Avoids breaking output file dump report.
---
 fftools/ffmpeg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

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,

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

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,

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

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,

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

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,

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