| Message ID | 20171201002701.24592-2-jstebbins@jetheaddev.com |
|---|---|
| State | New |
| Headers | show |
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?
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.
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
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).
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
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; } } }