[FFmpeg-devel] Display decoded frame number on error

---
 fftools/ffmpeg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Thanks for the review, I updated the patch to show the correct frame number
as suggested (sorry for the double post I managed to screw up my git
send-email).

Le ven. 12 juil. 2019 à 11:18, Rémi Achard <remiachard@gmail.com> a écrit :

> ---
>  fftools/ffmpeg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 01f04103cf..aaeabe512b 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2107,7 +2107,7 @@ static void check_decode_result(InputStream *ist,
> int *got_output, int ret)
>      if (*got_output && ist) {
>          if (ist->decoded_frame->decode_error_flags ||
> (ist->decoded_frame->flags & AV_FRAME_FLAG_CORRUPT)) {
>              av_log(NULL, exit_on_error ? AV_LOG_FATAL : AV_LOG_WARNING,
> -                   "%s: corrupt decoded frame in stream %d\n",
> input_files[ist->file_index]->ctx->url, ist->st->index);
> +                   "%s: corrupt decoded frame %llu in stream %d\n",
> input_files[ist->file_index]->ctx->url, ist->frames_decoded + 1,
> ist->st->index);
>              if (exit_on_error)
>                  exit_program(1);
>          }
> --
> 2.16.2
>
>

On 12-07-2019 02:54 PM, Remi Achard wrote:
> Thanks for the review, I updated the patch to show the correct frame number
> as suggested (sorry for the double post I managed to screw up my git
> send-email).
This is not what I meant.

Let's call good frames G and bad frames B. If your input is    G G G B B 
B G G B   then for all 3 initial B frames, ist->frames_decoded will be 
3, and 5 for the last B frame. So, what your patched log tells you is 
the total number of previous _successful_ decodes. Presumably, you wish 
to see 4, 5, 6 and 9 respectively for these B frames.

Gyan

>
> Le ven. 12 juil. 2019 à 11:18, Rémi Achard <remiachard@gmail.com> a écrit :
>
>> ---
>>   fftools/ffmpeg.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>> index 01f04103cf..aaeabe512b 100644
>> --- a/fftools/ffmpeg.c
>> +++ b/fftools/ffmpeg.c
>> @@ -2107,7 +2107,7 @@ static void check_decode_result(InputStream *ist,
>> int *got_output, int ret)
>>       if (*got_output && ist) {
>>           if (ist->decoded_frame->decode_error_flags ||
>> (ist->decoded_frame->flags & AV_FRAME_FLAG_CORRUPT)) {
>>               av_log(NULL, exit_on_error ? AV_LOG_FATAL : AV_LOG_WARNING,
>> -                   "%s: corrupt decoded frame in stream %d\n",
>> input_files[ist->file_index]->ctx->url, ist->st->index);
>> +                   "%s: corrupt decoded frame %llu in stream %d\n",
>> input_files[ist->file_index]->ctx->url, ist->frames_decoded + 1,
>> ist->st->index);
>>               if (exit_on_error)
>>                   exit_program(1);
>>           }
>> --
>> 2.16.2
>>
>>
> _______________________________________________
> 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".

Rémi Achard (12019-07-12):
> ---
>  fftools/ffmpeg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 01f04103cf..aaeabe512b 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2107,7 +2107,7 @@ static void check_decode_result(InputStream *ist, int *got_output, int ret)
>      if (*got_output && ist) {
>          if (ist->decoded_frame->decode_error_flags || (ist->decoded_frame->flags & AV_FRAME_FLAG_CORRUPT)) {
>              av_log(NULL, exit_on_error ? AV_LOG_FATAL : AV_LOG_WARNING,
> -                   "%s: corrupt decoded frame in stream %d\n", input_files[ist->file_index]->ctx->url, ist->st->index);

> +                   "%s: corrupt decoded frame %llu in stream %d\n", input_files[ist->file_index]->ctx->url, ist->frames_decoded + 1, ist->st->index);

frames_decoded is uint64_t, not long long; %lld is for long long.

Also: what is the use case for this change?

>              if (exit_on_error)
>                  exit_program(1);
>          }

Regards,

> Rémi Achard (12019-07-12):
> > ---
> >  fftools/ffmpeg.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 01f04103cf..aaeabe512b 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -2107,7 +2107,7 @@ static void check_decode_result(InputStream *ist,
> int *got_output, int ret)
> >      if (*got_output && ist) {
> >          if (ist->decoded_frame->decode_error_flags ||
> (ist->decoded_frame->flags & AV_FRAME_FLAG_CORRUPT)) {
> >              av_log(NULL, exit_on_error ? AV_LOG_FATAL : AV_LOG_WARNING,
> > -                   "%s: corrupt decoded frame in stream %d\n",
> input_files[ist->file_index]->ctx->url, ist->st->index);
>
> > +                   "%s: corrupt decoded frame %llu in stream %d\n",
> input_files[ist->file_index]->ctx->url, ist->frames_decoded + 1,
> ist->st->index);
>
> frames_decoded is uint64_t, not long long; %lld is for long long.


So I'm not supposed to use %llu as is the case here (sorry if I didn't
understood your point) ?


>
>
Also: what is the use case for this change?
>

The use case would be to improve error diagnostic, eg. when you want to
check a video integrity, you might want to know at what point in time
corruption occurs and there is currently no way of knowing this if I'm not
mistaken.


>
> >              if (exit_on_error)
> >                  exit_program(1);
> >          }
>
> Regards,
>

Remi Achard (12019-07-12):
> So I'm not supposed to use %llu as is the case here (sorry if I didn't
> understood your point) ?

Exactly, you need to use the format string for uint64_t, PRIu64 from
memory; check in the code base and standard.

> The use case would be to improve error diagnostic, eg. when you want to
> check a video integrity, you might want to know at what point in time
> corruption occurs and there is currently no way of knowing this if I'm not
> mistaken.

Then I think the timestamp of the frame would be a better choice. IIUC,
the field you print depends on the way ffmpeg processes the frames,
including seek: it is not robust.

Regards,

>
> Let's call good frames G and bad frames B. If your input is    G G G B B
> B G G B   then for all 3 initial B frames, ist->frames_decoded will be
> 3, and 5 for the last B frame. So, what your patched log tells you is
> the total number of previous _successful_ decodes. Presumably, you wish
> to see 4, 5, 6 and 9 respectively for these B frames.
>
 Oh I see what you mean, I was only considering the case where you use the
exit on error flag (-xerror).
Not sure what the clean way of doing this would be, without creating
aditional counter.

>
> Exactly, you need to use the format string for uint64_t, PRIu64 from
> memory; check in the code base and standard.
>

Thanks for the pointer, didn't knew this.


> Then I think the timestamp of the frame would be a better choice. IIUC,
> the field you print depends on the way ffmpeg processes the frames,
> including seek: it is not robust.
>

Would you advice using the AVFrame::best_effort_timestamp field ?
One problem I see is that it doesn't seems to be aware of trim command (eg.
-ss flag).

Remi Achard (12019-07-12):
> Would you advice using the AVFrame::best_effort_timestamp field ?

That is the one.

> One problem I see is that it doesn't seems to be aware of trim command (eg.
> -ss flag).

Which is good: that means you get the same number with or without it.

Regards,

> Which is good: that means you get the same number with or without it.
>

Sorry I was unclear, I mean that best_effort_timestamp is relative to trim
start.
Would be nice to have the abosulte timestamp in the file, regardless of
where the -ss flag starts no ?

On 12-07-2019 04:08 PM, Remi Achard wrote:
>> Which is good: that means you get the same number with or without it.
>>
> Sorry I was unclear, I mean that best_effort_timestamp is relative to trim
> start.
> Would be nice to have the abosulte timestamp in the file, regardless of
> where the -ss flag starts no ?

Add `-copyts` for that. If you want the index of the first decode 
failure, you don't need this patch. Run with `-xerror -v verbose`. 
ist->frames_decoded will be printed for each input stream. Add 1 to 
that. The `time=` value at the end should be the timestamp + duration of 
the last successfully decoded frame, but if you have other streams or 
vsync cfr set, this may not hold.

Gyan

>
> Add `-copyts` for that. If you want the index of the first decode
> failure, you don't need this patch. Run with `-xerror -v verbose`.
> ist->frames_decoded will be printed for each input stream. Add 1 to
> that. The `time=` value at the end should be the timestamp + duration of
> the last successfully decoded frame, but if you have other streams or
> vsync cfr set, this may not hold.
>

This did the trick thanks. For the -v verbose this indeed print the last
decoded frame on normal run but not when using -xerror.
Eg.
[AVIOContext @ 0x7f7f77000080] Statistics: 52700397 bytes read, 4 seeks

>
> This did the trick thanks. For the -v verbose this indeed print the last
> decoded frame on normal run but not when using -xerror.
> Eg.
> [AVIOContext @ 0x7f7f77000080] Statistics: 52700397 bytes read, 4 seeks
>

I guess because print_report is not called when exit_program is called in
check_decode_result.
I don't see the way of printing the corrupted frame on decoding failure
(with -xerror) appart from dumping every frame statistic while decoding
with eg. showinfo filter.