[FFmpeg-devel,6/6] ffplay: use AV_PKT_FLAG_DISPOSABLE in frame drop logic

Submitted by John Stebbins on Dec. 1, 2017, 12:27 a.m.

Details

Message ID 20171201002701.24592-2-jstebbins@jetheaddev.com
State New
Headers show

Commit Message

John Stebbins Dec. 1, 2017, 12:27 a.m.
---
 fftools/ffplay.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

Comments

wm4 Dec. 1, 2017, 4:25 p.m.
On Thu, 30 Nov 2017 16:27:01 -0800
John Stebbins <jstebbins@jetheaddev.com> wrote:

> ---
>  fftools/ffplay.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> index 10a917194d..152d220cdb 100644
> --- a/fftools/ffplay.c
> +++ b/fftools/ffplay.c
> @@ -198,6 +198,8 @@ typedef struct Decoder {
>      int64_t next_pts;
>      AVRational next_pts_tb;
>      SDL_Thread *decoder_tid;
> +    int drop_disposable;
> +    int frame_drops_disposable;
>  } Decoder;
>  
>  typedef struct VideoState {
> @@ -660,10 +662,16 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) {
>                      ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF);
>                  }
>              } else {
> -                if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
> -                    av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
> -                    d->packet_pending = 1;
> -                    av_packet_move_ref(&d->pkt, &pkt);
> +                if (d->avctx->codec_type == AVMEDIA_TYPE_VIDEO &&
> +                    d->drop_disposable &&
> +                    (pkt.flags & AV_PKT_FLAG_DISPOSABLE)) {
> +                    d->frame_drops_disposable++;
> +                } else {
> +                    if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
> +                        av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
> +                        d->packet_pending = 1;
> +                        av_packet_move_ref(&d->pkt, &pkt);
> +                    }
>                  }
>              }
>              av_packet_unref(&pkt);
> @@ -1622,6 +1630,7 @@ retry:
>                      frame_queue_next(&is->pictq);
>                      goto retry;
>                  }
> +                is->viddec.drop_disposable = 0;
>              }
>  
>              if (is->subtitle_st) {
> @@ -1699,7 +1708,8 @@ display:
>                     get_master_clock(is),
>                     (is->audio_st && is->video_st) ? "A-V" : (is->video_st ? "M-V" : (is->audio_st ? "M-A" : "   ")),
>                     av_diff,
> -                   is->frame_drops_early + is->frame_drops_late,
> +                   is->frame_drops_early + is->frame_drops_late +
> +                                           is->viddec.frame_drops_disposable,
>                     aqsize / 1024,
>                     vqsize / 1024,
>                     sqsize,
> @@ -1767,6 +1777,7 @@ static int get_video_frame(VideoState *is, AVFrame *frame)
>                      is->frame_drops_early++;
>                      av_frame_unref(frame);
>                      got_picture = 0;
> +                    is->viddec.drop_disposable = 1;
>                  }
>              }
>          }

Why not just make libavcodec drop disposable frames?
John Stebbins Dec. 1, 2017, 4:38 p.m.
On 12/01/2017 08:25 AM, wm4 wrote:
> On Thu, 30 Nov 2017 16:27:01 -0800
> John Stebbins <jstebbins@jetheaddev.com> wrote:
>
>> ---
>>  fftools/ffplay.c | 21 ++++++++++++++++-----
>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
>> index 10a917194d..152d220cdb 100644
>> --- a/fftools/ffplay.c
>> +++ b/fftools/ffplay.c
>> @@ -198,6 +198,8 @@ typedef struct Decoder {
>>      int64_t next_pts;
>>      AVRational next_pts_tb;
>>      SDL_Thread *decoder_tid;
>> +    int drop_disposable;
>> +    int frame_drops_disposable;
>>  } Decoder;
>>  
>>  typedef struct VideoState {
>> @@ -660,10 +662,16 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) {
>>                      ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF);
>>                  }
>>              } else {
>> -                if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
>> -                    av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
>> -                    d->packet_pending = 1;
>> -                    av_packet_move_ref(&d->pkt, &pkt);
>> +                if (d->avctx->codec_type == AVMEDIA_TYPE_VIDEO &&
>> +                    d->drop_disposable &&
>> +                    (pkt.flags & AV_PKT_FLAG_DISPOSABLE)) {
>> +                    d->frame_drops_disposable++;
>> +                } else {
>> +                    if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
>> +                        av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
>> +                        d->packet_pending = 1;
>> +                        av_packet_move_ref(&d->pkt, &pkt);
>> +                    }
>>                  }
>>              }
>>              av_packet_unref(&pkt);
>> @@ -1622,6 +1630,7 @@ retry:
>>                      frame_queue_next(&is->pictq);
>>                      goto retry;
>>                  }
>> +                is->viddec.drop_disposable = 0;
>>              }
>>  
>>              if (is->subtitle_st) {
>> @@ -1699,7 +1708,8 @@ display:
>>                     get_master_clock(is),
>>                     (is->audio_st && is->video_st) ? "A-V" : (is->video_st ? "M-V" : (is->audio_st ? "M-A" : "   ")),
>>                     av_diff,
>> -                   is->frame_drops_early + is->frame_drops_late,
>> +                   is->frame_drops_early + is->frame_drops_late +
>> +                                           is->viddec.frame_drops_disposable,
>>                     aqsize / 1024,
>>                     vqsize / 1024,
>>                     sqsize,
>> @@ -1767,6 +1777,7 @@ static int get_video_frame(VideoState *is, AVFrame *frame)
>>                      is->frame_drops_early++;
>>                      av_frame_unref(frame);
>>                      got_picture = 0;
>> +                    is->viddec.drop_disposable = 1;
>>                  }
>>              }
>>          }
> Why not just make libavcodec drop disposable frames?
>

libavcodec doesn't know anything about the current playback timeline.  I.e. it doesn't know if a frame is late.
Marton Balint Dec. 3, 2017, 9:12 p.m.
On Thu, 30 Nov 2017, John Stebbins wrote:

> ---
> fftools/ffplay.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> index 10a917194d..152d220cdb 100644
> --- a/fftools/ffplay.c
> +++ b/fftools/ffplay.c
> @@ -198,6 +198,8 @@ typedef struct Decoder {
>     int64_t next_pts;
>     AVRational next_pts_tb;
>     SDL_Thread *decoder_tid;
> +    int drop_disposable;
> +    int frame_drops_disposable;
> } Decoder;
> 
> typedef struct VideoState {
> @@ -660,10 +662,16 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) {
>                     ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF);
>                 }
>             } else {
> -                if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
> -                    av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
> -                    d->packet_pending = 1;
> -                    av_packet_move_ref(&d->pkt, &pkt);
> +                if (d->avctx->codec_type == AVMEDIA_TYPE_VIDEO &&
> +                    d->drop_disposable &&
> +                    (pkt.flags & AV_PKT_FLAG_DISPOSABLE)) {
> +                    d->frame_drops_disposable++;
> +                } else {
> +                    if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
> +                        av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
> +                        d->packet_pending = 1;
> +                        av_packet_move_ref(&d->pkt, &pkt);
> +                    }
>                 }
>             }
>             av_packet_unref(&pkt);
> @@ -1622,6 +1630,7 @@ retry:
>                     frame_queue_next(&is->pictq);
>                     goto retry;
>                 }
> +                is->viddec.drop_disposable = 0;
>             }
>
>             if (is->subtitle_st) {
> @@ -1699,7 +1708,8 @@ display:
>                    get_master_clock(is),
>                    (is->audio_st && is->video_st) ? "A-V" : (is->video_st ? "M-V" : (is->audio_st ? "M-A" : "   ")),
>                    av_diff,
> -                   is->frame_drops_early + is->frame_drops_late,
> +                   is->frame_drops_early + is->frame_drops_late +
> +                                           is->viddec.frame_drops_disposable,
>                    aqsize / 1024,
>                    vqsize / 1024,
>                    sqsize,
> @@ -1767,6 +1777,7 @@ static int get_video_frame(VideoState *is, AVFrame *frame)
>                     is->frame_drops_early++;
>                     av_frame_unref(frame);
>                     got_picture = 0;
> +                    is->viddec.drop_disposable = 1;
>                 }
>             }
>         }

The patch looks OK now, but as I mentioned earlier, please do not enable 
this kind of frame dropping by default, either introduce a separate option 
"hardframedrop", or make -framedrop an integer and use "2" for this kind 
of hard dropping.

Regards,
Marton
John Stebbins Dec. 4, 2017, 3:38 p.m.
On 12/03/2017 01:12 PM, Marton Balint wrote:
> On Thu, 30 Nov 2017, John Stebbins wrote:
>
>> ---
>> fftools/ffplay.c | 21 ++++++++++++++++-----
>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
>> index 10a917194d..152d220cdb 100644
>> --- a/fftools/ffplay.c
>> +++ b/fftools/ffplay.c
>> @@ -198,6 +198,8 @@ typedef struct Decoder {
>>     int64_t next_pts;
>>     AVRational next_pts_tb;
>>     SDL_Thread *decoder_tid;
>> +    int drop_disposable;
>> +    int frame_drops_disposable;
>> } Decoder;
>>
>> typedef struct VideoState {
>> @@ -660,10 +662,16 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) {
>>                     ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF);
>>                 }
>>             } else {
>> -                if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
>> -                    av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
>> -                    d->packet_pending = 1;
>> -                    av_packet_move_ref(&d->pkt, &pkt);
>> +                if (d->avctx->codec_type == AVMEDIA_TYPE_VIDEO &&
>> +                    d->drop_disposable &&
>> +                    (pkt.flags & AV_PKT_FLAG_DISPOSABLE)) {
>> +                    d->frame_drops_disposable++;
>> +                } else {
>> +                    if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
>> +                        av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
>> +                        d->packet_pending = 1;
>> +                        av_packet_move_ref(&d->pkt, &pkt);
>> +                    }
>>                 }
>>             }
>>             av_packet_unref(&pkt);
>> @@ -1622,6 +1630,7 @@ retry:
>>                     frame_queue_next(&is->pictq);
>>                     goto retry;
>>                 }
>> +                is->viddec.drop_disposable = 0;
>>             }
>>
>>             if (is->subtitle_st) {
>> @@ -1699,7 +1708,8 @@ display:
>>                    get_master_clock(is),
>>                    (is->audio_st && is->video_st) ? "A-V" : (is->video_st ? "M-V" : (is->audio_st ? "M-A" : "   ")),
>>                    av_diff,
>> -                   is->frame_drops_early + is->frame_drops_late,
>> +                   is->frame_drops_early + is->frame_drops_late +
>> +                                           is->viddec.frame_drops_disposable,
>>                    aqsize / 1024,
>>                    vqsize / 1024,
>>                    sqsize,
>> @@ -1767,6 +1777,7 @@ static int get_video_frame(VideoState *is, AVFrame *frame)
>>                     is->frame_drops_early++;
>>                     av_frame_unref(frame);
>>                     got_picture = 0;
>> +                    is->viddec.drop_disposable = 1;
>>                 }
>>             }
>>         }
> The patch looks OK now, but as I mentioned earlier, please do not enable 
> this kind of frame dropping by default, either introduce a separate option 
> "hardframedrop", or make -framedrop an integer and use "2" for this kind 
> of hard dropping.
>
>

I can certainly do that.  But I don't really understand the reason behind it.  Frame dropping is already happening in
the scenario where this would be enabled.  There are 2 types of frame dropping that currently happen in ffplay, "early"
and "late".  "Early" frame dropping happens when it is detected that the frame is too late for it's time slot
immediately after it is decoded.  "Late" frame dropping happens when it is detected that the frame is too late when it
is removed from the display queue. Dropping disposable frames in when "early" frame dropping is happening gives the
player a better chance of actually being able to catch up to the current time slot because it bypasses decoding those
frames.  Without it only the first few of frames get displayed with the samples I've been testing.  After those first
few, decoding is slow enough that all subsequent frames are too late and get dropped by the "early" frame drop logic.  
With disposable frame dropping, the video plays reasonably well (with a stutter here and there).
Marton Balint Dec. 5, 2017, 10:54 p.m.
On Mon, 4 Dec 2017, John Stebbins wrote:

> On 12/03/2017 01:12 PM, Marton Balint wrote:
>> On Thu, 30 Nov 2017, John Stebbins wrote:
>>
>>> ---
>>> fftools/ffplay.c | 21 ++++++++++++++++-----
>>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
>>> index 10a917194d..152d220cdb 100644
>>> --- a/fftools/ffplay.c
>>> +++ b/fftools/ffplay.c
>>> @@ -198,6 +198,8 @@ typedef struct Decoder {
>>>     int64_t next_pts;
>>>     AVRational next_pts_tb;
>>>     SDL_Thread *decoder_tid;
>>> +    int drop_disposable;
>>> +    int frame_drops_disposable;
>>> } Decoder;
>>>
>>> typedef struct VideoState {
>>> @@ -660,10 +662,16 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) {
>>>                     ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF);
>>>                 }
>>>             } else {
>>> -                if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
>>> -                    av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
>>> -                    d->packet_pending = 1;
>>> -                    av_packet_move_ref(&d->pkt, &pkt);
>>> +                if (d->avctx->codec_type == AVMEDIA_TYPE_VIDEO &&
>>> +                    d->drop_disposable &&
>>> +                    (pkt.flags & AV_PKT_FLAG_DISPOSABLE)) {
>>> +                    d->frame_drops_disposable++;
>>> +                } else {
>>> +                    if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
>>> +                        av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
>>> +                        d->packet_pending = 1;
>>> +                        av_packet_move_ref(&d->pkt, &pkt);
>>> +                    }
>>>                 }
>>>             }
>>>             av_packet_unref(&pkt);
>>> @@ -1622,6 +1630,7 @@ retry:
>>>                     frame_queue_next(&is->pictq);
>>>                     goto retry;
>>>                 }
>>> +                is->viddec.drop_disposable = 0;
>>>             }
>>>
>>>             if (is->subtitle_st) {
>>> @@ -1699,7 +1708,8 @@ display:
>>>                    get_master_clock(is),
>>>                    (is->audio_st && is->video_st) ? "A-V" : (is->video_st ? "M-V" : (is->audio_st ? "M-A" : "   ")),
>>>                    av_diff,
>>> -                   is->frame_drops_early + is->frame_drops_late,
>>> +                   is->frame_drops_early + is->frame_drops_late +
>>> +                                           is->viddec.frame_drops_disposable,
>>>                    aqsize / 1024,
>>>                    vqsize / 1024,
>>>                    sqsize,
>>> @@ -1767,6 +1777,7 @@ static int get_video_frame(VideoState *is, AVFrame *frame)
>>>                     is->frame_drops_early++;
>>>                     av_frame_unref(frame);
>>>                     got_picture = 0;
>>> +                    is->viddec.drop_disposable = 1;
>>>                 }
>>>             }
>>>         }
>> The patch looks OK now, but as I mentioned earlier, please do not enable
>> this kind of frame dropping by default, either introduce a separate option
>> "hardframedrop", or make -framedrop an integer and use "2" for this kind
>> of hard dropping.
>>
>>
>
> I can certainly do that.  But I don't really understand the reason behind it.  Frame dropping is already happening in
> the scenario where this would be enabled.  There are 2 types of frame dropping that currently happen in ffplay, "early"
> and "late".  "Early" frame dropping happens when it is detected that the frame is too late for it's time slot
> immediately after it is decoded.  "Late" frame dropping happens when it is detected that the frame is too late when it
> is removed from the display queue. Dropping disposable frames in when "early" frame dropping is happening gives the
> player a better chance of actually being able to catch up to the current time slot because it bypasses decoding those
> frames.  Without it only the first few of frames get displayed with the samples I've been testing.  After those first
> few, decoding is slow enough that all subsequent frames are too late and get dropped by the "early" frame drop logic.  
> With disposable frame dropping, the video plays reasonably well (with a stutter here and there).

Ok, let's keep the patch as is then.

Regards,
Marton

Patch hide | download patch | download mbox

diff --git a/fftools/ffplay.c b/fftools/ffplay.c
index 10a917194d..152d220cdb 100644
--- a/fftools/ffplay.c
+++ b/fftools/ffplay.c
@@ -198,6 +198,8 @@  typedef struct Decoder {
     int64_t next_pts;
     AVRational next_pts_tb;
     SDL_Thread *decoder_tid;
+    int drop_disposable;
+    int frame_drops_disposable;
 } Decoder;
 
 typedef struct VideoState {
@@ -660,10 +662,16 @@  static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) {
                     ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF);
                 }
             } else {
-                if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
-                    av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
-                    d->packet_pending = 1;
-                    av_packet_move_ref(&d->pkt, &pkt);
+                if (d->avctx->codec_type == AVMEDIA_TYPE_VIDEO &&
+                    d->drop_disposable &&
+                    (pkt.flags & AV_PKT_FLAG_DISPOSABLE)) {
+                    d->frame_drops_disposable++;
+                } else {
+                    if (avcodec_send_packet(d->avctx, &pkt) == AVERROR(EAGAIN)) {
+                        av_log(d->avctx, AV_LOG_ERROR, "Receive_frame and send_packet both returned EAGAIN, which is an API violation.\n");
+                        d->packet_pending = 1;
+                        av_packet_move_ref(&d->pkt, &pkt);
+                    }
                 }
             }
             av_packet_unref(&pkt);
@@ -1622,6 +1630,7 @@  retry:
                     frame_queue_next(&is->pictq);
                     goto retry;
                 }
+                is->viddec.drop_disposable = 0;
             }
 
             if (is->subtitle_st) {
@@ -1699,7 +1708,8 @@  display:
                    get_master_clock(is),
                    (is->audio_st && is->video_st) ? "A-V" : (is->video_st ? "M-V" : (is->audio_st ? "M-A" : "   ")),
                    av_diff,
-                   is->frame_drops_early + is->frame_drops_late,
+                   is->frame_drops_early + is->frame_drops_late +
+                                           is->viddec.frame_drops_disposable,
                    aqsize / 1024,
                    vqsize / 1024,
                    sqsize,
@@ -1767,6 +1777,7 @@  static int get_video_frame(VideoState *is, AVFrame *frame)
                     is->frame_drops_early++;
                     av_frame_unref(frame);
                     got_picture = 0;
+                    is->viddec.drop_disposable = 1;
                 }
             }
         }