[FFmpeg-devel,2/2] fftools/ffmpeg: log skipped initial non-keyframes

Submitted by Stephan Hilb on June 6, 2019, 8:31 p.m.

Details

Message ID 20190606203145.22217-2-stephan@ecshi.net
State New
Headers show

Commit Message

Stephan Hilb June 6, 2019, 8:31 p.m.
If `AV_PKT_FLAG_KEY` stays unset on `pkt->flags`, the output stream
stays empty with little information about what is going on.
This change makes it easier to debug the situation for the user who
could then choose to use the `-copyinkf` option.
---
 fftools/ffmpeg.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer June 7, 2019, 3:15 p.m.
On Thu, Jun 06, 2019 at 10:31:45PM +0200, Stephan Hilb wrote:
> If `AV_PKT_FLAG_KEY` stays unset on `pkt->flags`, the output stream
> stays empty with little information about what is going on.
> This change makes it easier to debug the situation for the user who
> could then choose to use the `-copyinkf` option.
> ---
>  fftools/ffmpeg.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 01f04103cf..446439e285 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2006,8 +2006,10 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
>      }
>  
>      if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
> -        !ost->copy_initial_nonkeyframes)
> +        !ost->copy_initial_nonkeyframes) {
> +        av_log(NULL, AV_LOG_DEBUG, "skipping initial non-keyframe\n");

This should contain some information so the stream (in case of multiple) can
be identified

[...]
Moritz Barsnick June 7, 2019, 4:19 p.m.
On Thu, Jun 06, 2019 at 22:31:45 +0200, Stephan Hilb wrote:
> If `AV_PKT_FLAG_KEY` stays unset on `pkt->flags`, the output stream
> stays empty with little information about what is going on.
> This change makes it easier to debug the situation for the user who
> could then choose to use the `-copyinkf` option.
[...]
> -        !ost->copy_initial_nonkeyframes)
> +        !ost->copy_initial_nonkeyframes) {
> +        av_log(NULL, AV_LOG_DEBUG, "skipping initial non-keyframe\n");
>          return;

Incorrect indentation.

More seriously: I welcome this message. There was a question on
ffmpeg-user[1] recently where a user provided a file without any
keyframes at all, which could be played by ffplay, but not copy-coded
by ffmpeg.

I suggest this message should be at INFO level, but appear only once.
Unfortunately, the file said user provided is no longer available (and
I have deleted my copy).

I believe ffprobe also suffers from such files and never displays
anyhing with -show_frames. In fact, does ffprobe perhaps always begin
from, or at least wait for, the first keyframe?

Cheers,
Moritz

[1] http://ffmpeg.org/pipermail/ffmpeg-user/2019-June/044526.html
Stephan Hilb June 7, 2019, 8:35 p.m.
>> -        !ost->copy_initial_nonkeyframes)
>> +        !ost->copy_initial_nonkeyframes) {
>> +        av_log(NULL, AV_LOG_DEBUG, "skipping initial
>> non-keyframe\n"); return;  
> 
> Incorrect indentation.

It's actually the same indentation as in other places in the same file,
what would be the correct way then?

> I suggest this message should be at INFO level, but appear only once.

I'm fine with INFO level. Repeated messages get supressed anyways, does
that suffice?

> I believe ffprobe also suffers from such files and never displays
> anyhing with -show_frames. In fact, does ffprobe perhaps always begin
> from, or at least wait for, the first keyframe?

Cannot really comment on that. For me it happens when stream copying
from v4l2, since the keyframe flag is not being set there, Probing
works just fine.
Moritz Barsnick June 8, 2019, 8:19 p.m.
On Fri, Jun 07, 2019 at 22:35:03 +0200, Stephan Hilb wrote:
> > Incorrect indentation.
>
> It's actually the same indentation as in other places in the same file,
> what would be the correct way then?

Sorry, my mind went flaky, you're corrent.

> > I suggest this message should be at INFO level, but appear only once.
>
> I'm fine with INFO level. Repeated messages get supressed anyways, does
> that suffice?

If it only appears once, then fine. I couldn't test (see below), so
couldn't see whether this comes for every skipped frame.

> > I believe ffprobe also suffers from such files and never displays
> > anyhing with -show_frames. In fact, does ffprobe perhaps always begin
> > from, or at least wait for, the first keyframe?
>
> Cannot really comment on that. For me it happens when stream copying
> from v4l2, since the keyframe flag is not being set there, Probing
> works just fine.

Okay, I may have a look at ffprobe myself.

Thanks for the v4l2 hint, I can use that to produce a sample.

Moritz
Michael Niedermayer June 9, 2019, 5 a.m.
On Fri, Jun 07, 2019 at 10:35:03PM +0200, Stephan Hilb wrote:
> >> -        !ost->copy_initial_nonkeyframes)
> >> +        !ost->copy_initial_nonkeyframes) {
> >> +        av_log(NULL, AV_LOG_DEBUG, "skipping initial
> >> non-keyframe\n"); return;  
> > 
> > Incorrect indentation.
> 
> It's actually the same indentation as in other places in the same file,
> what would be the correct way then?
> 
> > I suggest this message should be at INFO level, but appear only once.
> 
> I'm fine with INFO level. Repeated messages get supressed anyways, does
> that suffice?

Repeated messages only get supressed if there is no interspaced message
if 2 things generate a message per frame, neither will be supressed

[...]
Stephan Hilb June 9, 2019, 9:26 a.m.
> Repeated messages only get supressed if there is no interspaced
> message if 2 things generate a message per frame, neither will be
> supressed

So should I leave it at DEBUG level or implement a custom log_once?
The "cur_dts invalid" debug message is currently being printed at least
as often.
Michael Niedermayer June 12, 2019, 7:57 a.m.
On Sun, Jun 09, 2019 at 11:26:14AM +0200, Stephan Hilb wrote:
> > Repeated messages only get supressed if there is no interspaced
> > message if 2 things generate a message per frame, neither will be
> > supressed
> 
> So should I leave it at DEBUG level or implement a custom log_once?

i agree that the message might be usefull to be seen by the end user.
maybe as this is already discussed, the first could be a more vissible
level and the following then debug or lower level


> The "cur_dts invalid" debug message is currently being printed at least
> as often.

yes, this should be investigated and fixed by someone ...


[...]

Patch hide | download patch | download mbox

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 01f04103cf..446439e285 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -2006,8 +2006,10 @@  static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
     }
 
     if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
-        !ost->copy_initial_nonkeyframes)
+        !ost->copy_initial_nonkeyframes) {
+        av_log(NULL, AV_LOG_DEBUG, "skipping initial non-keyframe\n");
         return;
+    }
 
     if (!ost->frame_number && !ost->copy_prior_start) {
         int64_t comp_start = start_time;