Message ID | 20190712091740.95577-2-remi.achard@ymagis.com |
---|---|
State | New |
Headers | show |
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.
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); }