| Message ID | 20170315024758.8875-2-cus@passwd.hu |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 15 Mar 2017, Marton Balint wrote: > Since subtitles are not yet supported with the new API, CODEC_CAP_DELAY > subtitle codecs (only libzvbi so far) may loose the last few buffered frames in > the end of the stream. > > The impact of this is so limited, it seemded better to accept it than losing > the simplification benefits of the new API. Hi Wallak, Have you had a chance to test this ffplay patch, and see if it fixes CrystalHD decoding in ffplay or not? Thanks, Marton > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > ffplay.c | 92 +++++++++++++++++++++++++++++----------------------------------- > 1 file changed, 41 insertions(+), 51 deletions(-) > > diff --git a/ffplay.c b/ffplay.c > index cf138dc..06d1d22 100644 > --- a/ffplay.c > +++ b/ffplay.c > @@ -186,13 +186,10 @@ enum { > }; > > typedef struct Decoder { > - AVPacket pkt; > - AVPacket pkt_temp; > PacketQueue *queue; > AVCodecContext *avctx; > int pkt_serial; > int finished; > - int packet_pending; > SDL_cond *empty_queue_cond; > int64_t start_pts; > AVRational start_pts_tb; > @@ -551,40 +548,24 @@ static void decoder_init(Decoder *d, AVCodecContext *avctx, PacketQueue *queue, > d->queue = queue; > d->empty_queue_cond = empty_queue_cond; > d->start_pts = AV_NOPTS_VALUE; > + d->pkt_serial = -1; > } > > static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { > - int got_frame = 0; > + int ret = AVERROR(EAGAIN); > > - do { > - int ret = -1; > + for (;;) { > + AVPacket pkt; > > + if (d->queue->serial == d->pkt_serial) { > + do { > if (d->queue->abort_request) > return -1; > > - if (!d->packet_pending || d->queue->serial != d->pkt_serial) { > - AVPacket pkt; > - do { > - if (d->queue->nb_packets == 0) > - SDL_CondSignal(d->empty_queue_cond); > - if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0) > - return -1; > - if (pkt.data == flush_pkt.data) { > - avcodec_flush_buffers(d->avctx); > - d->finished = 0; > - d->next_pts = d->start_pts; > - d->next_pts_tb = d->start_pts_tb; > - } > - } while (pkt.data == flush_pkt.data || d->queue->serial != d->pkt_serial); > - av_packet_unref(&d->pkt); > - d->pkt_temp = d->pkt = pkt; > - d->packet_pending = 1; > - } > - > switch (d->avctx->codec_type) { > case AVMEDIA_TYPE_VIDEO: > - ret = avcodec_decode_video2(d->avctx, frame, &got_frame, &d->pkt_temp); > - if (got_frame) { > + ret = avcodec_receive_frame(d->avctx, frame); > + if (ret >= 0) { > if (decoder_reorder_pts == -1) { > frame->pts = av_frame_get_best_effort_timestamp(frame); > } else if (!decoder_reorder_pts) { > @@ -593,8 +574,8 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { > } > break; > case AVMEDIA_TYPE_AUDIO: > - ret = avcodec_decode_audio4(d->avctx, frame, &got_frame, &d->pkt_temp); > - if (got_frame) { > + ret = avcodec_receive_frame(d->avctx, frame); > + if (ret >= 0) { > AVRational tb = (AVRational){1, frame->sample_rate}; > if (frame->pts != AV_NOPTS_VALUE) > frame->pts = av_rescale_q(frame->pts, av_codec_get_pkt_timebase(d->avctx), tb); > @@ -606,37 +587,46 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { > } > } > break; > - case AVMEDIA_TYPE_SUBTITLE: > - ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &d->pkt_temp); > - break; > + } > + if (ret == AVERROR_EOF) { > + d->finished = d->pkt_serial; > + avcodec_flush_buffers(d->avctx); > + return 0; > + } > + if (ret >= 0) > + return 1; > + } while (ret != AVERROR(EAGAIN)); > } > > - if (ret < 0) { > - d->packet_pending = 0; > + do { > + if (d->queue->nb_packets == 0) > + SDL_CondSignal(d->empty_queue_cond); > + if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0) > + return -1; > + } while (d->queue->serial != d->pkt_serial); > + > + if (pkt.data == flush_pkt.data) { > + avcodec_flush_buffers(d->avctx); > + d->finished = 0; > + d->next_pts = d->start_pts; > + d->next_pts_tb = d->start_pts_tb; > } else { > - d->pkt_temp.dts = > - d->pkt_temp.pts = AV_NOPTS_VALUE; > - if (d->pkt_temp.data) { > - if (d->avctx->codec_type != AVMEDIA_TYPE_AUDIO) > - ret = d->pkt_temp.size; > - d->pkt_temp.data += ret; > - d->pkt_temp.size -= ret; > - if (d->pkt_temp.size <= 0) > - d->packet_pending = 0; > + if (d->avctx->codec_type == AVMEDIA_TYPE_SUBTITLE) { > + int got_frame = 0; > + ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &pkt); > + if (ret < 0) > + ret = AVERROR(EAGAIN); > + else > + ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF); > } else { > - if (!got_frame) { > - d->packet_pending = 0; > - d->finished = d->pkt_serial; > - } > + av_assert0(avcodec_send_packet(d->avctx, &pkt) != AVERROR(EAGAIN)); > } > + av_packet_unref(&pkt); > } > - } while (!got_frame && !d->finished); > - > - return got_frame; > + } > } > > static void decoder_destroy(Decoder *d) { > - av_packet_unref(&d->pkt); > avcodec_free_context(&d->avctx); > } > > -- > 2.10.2 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
I tried the patch. Once av_assert0 test is removed, the crystalhd decoder is running till send_packet triggers AVERROR(EAGAIN), now the crystalhd buffer is likely overflowed, the video freezes. A few seconds later the decoder displays a few frames and the cycle goes on. Im not sure how to properly implement the function to stop or resend send_packet frames, this is likely the only mechanism missing. Best Regards, Wallak. ----- Mail original ----- De: "Marton Balint" <cus@passwd.hu> À: "FFmpeg development discussions and patches" <ffmpeg-devel@ffmpeg.org> Cc: wallak@free.fr Envoyé: Jeudi 16 Mars 2017 23:25:18 Objet: Re: [FFmpeg-devel] [PATCH 2/3] ffplay: convert to new decode API On Wed, 15 Mar 2017, Marton Balint wrote: > Since subtitles are not yet supported with the new API, CODEC_CAP_DELAY > subtitle codecs (only libzvbi so far) may loose the last few buffered frames in > the end of the stream. > > The impact of this is so limited, it seemded better to accept it than losing > the simplification benefits of the new API. Hi Wallak, Have you had a chance to test this ffplay patch, and see if it fixes CrystalHD decoding in ffplay or not? Thanks, Marton > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > ffplay.c | 92 +++++++++++++++++++++++++++++----------------------------------- > 1 file changed, 41 insertions(+), 51 deletions(-) > > diff --git a/ffplay.c b/ffplay.c > index cf138dc..06d1d22 100644 > --- a/ffplay.c > +++ b/ffplay.c > @@ -186,13 +186,10 @@ enum { > }; > > typedef struct Decoder { > - AVPacket pkt; > - AVPacket pkt_temp; > PacketQueue *queue; > AVCodecContext *avctx; > int pkt_serial; > int finished; > - int packet_pending; > SDL_cond *empty_queue_cond; > int64_t start_pts; > AVRational start_pts_tb; > @@ -551,40 +548,24 @@ static void decoder_init(Decoder *d, AVCodecContext *avctx, PacketQueue *queue, > d->queue = queue; > d->empty_queue_cond = empty_queue_cond; > d->start_pts = AV_NOPTS_VALUE; > + d->pkt_serial = -1; > } > > static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { > - int got_frame = 0; > + int ret = AVERROR(EAGAIN); > > - do { > - int ret = -1; > + for (;;) { > + AVPacket pkt; > > + if (d->queue->serial == d->pkt_serial) { > + do { > if (d->queue->abort_request) > return -1; > > - if (!d->packet_pending || d->queue->serial != d->pkt_serial) { > - AVPacket pkt; > - do { > - if (d->queue->nb_packets == 0) > - SDL_CondSignal(d->empty_queue_cond); > - if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0) > - return -1; > - if (pkt.data == flush_pkt.data) { > - avcodec_flush_buffers(d->avctx); > - d->finished = 0; > - d->next_pts = d->start_pts; > - d->next_pts_tb = d->start_pts_tb; > - } > - } while (pkt.data == flush_pkt.data || d->queue->serial != d->pkt_serial); > - av_packet_unref(&d->pkt); > - d->pkt_temp = d->pkt = pkt; > - d->packet_pending = 1; > - } > - > switch (d->avctx->codec_type) { > case AVMEDIA_TYPE_VIDEO: > - ret = avcodec_decode_video2(d->avctx, frame, &got_frame, &d->pkt_temp); > - if (got_frame) { > + ret = avcodec_receive_frame(d->avctx, frame); > + if (ret >= 0) { > if (decoder_reorder_pts == -1) { > frame->pts = av_frame_get_best_effort_timestamp(frame); > } else if (!decoder_reorder_pts) { > @@ -593,8 +574,8 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { > } > break; > case AVMEDIA_TYPE_AUDIO: > - ret = avcodec_decode_audio4(d->avctx, frame, &got_frame, &d->pkt_temp); > - if (got_frame) { > + ret = avcodec_receive_frame(d->avctx, frame); > + if (ret >= 0) { > AVRational tb = (AVRational){1, frame->sample_rate}; > if (frame->pts != AV_NOPTS_VALUE) > frame->pts = av_rescale_q(frame->pts, av_codec_get_pkt_timebase(d->avctx), tb); > @@ -606,37 +587,46 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { > } > } > break; > - case AVMEDIA_TYPE_SUBTITLE: > - ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &d->pkt_temp); > - break; > + } > + if (ret == AVERROR_EOF) { > + d->finished = d->pkt_serial; > + avcodec_flush_buffers(d->avctx); > + return 0; > + } > + if (ret >= 0) > + return 1; > + } while (ret != AVERROR(EAGAIN)); > } > > - if (ret < 0) { > - d->packet_pending = 0; > + do { > + if (d->queue->nb_packets == 0) > + SDL_CondSignal(d->empty_queue_cond); > + if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0) > + return -1; > + } while (d->queue->serial != d->pkt_serial); > + > + if (pkt.data == flush_pkt.data) { > + avcodec_flush_buffers(d->avctx); > + d->finished = 0; > + d->next_pts = d->start_pts; > + d->next_pts_tb = d->start_pts_tb; > } else { > - d->pkt_temp.dts = > - d->pkt_temp.pts = AV_NOPTS_VALUE; > - if (d->pkt_temp.data) { > - if (d->avctx->codec_type != AVMEDIA_TYPE_AUDIO) > - ret = d->pkt_temp.size; > - d->pkt_temp.data += ret; > - d->pkt_temp.size -= ret; > - if (d->pkt_temp.size <= 0) > - d->packet_pending = 0; > + if (d->avctx->codec_type == AVMEDIA_TYPE_SUBTITLE) { > + int got_frame = 0; > + ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &pkt); > + if (ret < 0) > + ret = AVERROR(EAGAIN); > + else > + ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF); > } else { > - if (!got_frame) { > - d->packet_pending = 0; > - d->finished = d->pkt_serial; > - } > + av_assert0(avcodec_send_packet(d->avctx, &pkt) != AVERROR(EAGAIN)); > } > + av_packet_unref(&pkt); > } > - } while (!got_frame && !d->finished); > - > - return got_frame; > + } > } > > static void decoder_destroy(Decoder *d) { > - av_packet_unref(&d->pkt); > avcodec_free_context(&d->avctx); > } > > -- > 2.10.2 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Fri, 17 Mar 2017, wallak@free.fr wrote: > I tried the patch. Once av_assert0 test is removed, the crystalhd > decoder is running till send_packet triggers AVERROR(EAGAIN), now the > crystalhd buffer is likely overflowed, the video freezes. A few seconds > later the decoder displays a few frames and the cycle goes on. > > Im not sure how to properly implement the function to stop or resend > send_packet frames, this is likely the only mechanism missing. Hmm. Theoretically an assertion at send_packet can only happen, if the CrystalHD decoder is is returning AVERROR(EAGAIN) for receive_frame and for send_packet as well, which is forbidden. Could you add some logging, and log the return values of both send_packet and receive_frame to make sure that is the case, and the crystalHD decoder is buggy, not my implementation of the new decode API? I tested h264_cuvid decoder with ffplay (the only decoder with the new API), and that worked so I am inclined to think CrystalHD is buggy. And the behaviour you described looks like once the buffers of CrystalHD is full, but no decoded frame is available yet, it simply returns AVERROR(EAGAIN) instead of blocking receive_frame call which it should do. Thanks, Marton > Best Regards, > Wallak. > > > > ----- Mail original ----- > De: "Marton Balint" <cus@passwd.hu> > À: "FFmpeg development discussions and patches" <ffmpeg-devel@ffmpeg.org> > Cc: wallak@free.fr > Envoyé: Jeudi 16 Mars 2017 23:25:18 > Objet: Re: [FFmpeg-devel] [PATCH 2/3] ffplay: convert to new decode API > > > > On Wed, 15 Mar 2017, Marton Balint wrote: > >> Since subtitles are not yet supported with the new API, CODEC_CAP_DELAY >> subtitle codecs (only libzvbi so far) may loose the last few buffered frames in >> the end of the stream. >> >> The impact of this is so limited, it seemded better to accept it than losing >> the simplification benefits of the new API. > > Hi Wallak, > > Have you had a chance to test this ffplay patch, and see if it fixes > CrystalHD decoding in ffplay or not? > > Thanks, > Marton > >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> ffplay.c | 92 +++++++++++++++++++++++++++++----------------------------------- >> 1 file changed, 41 insertions(+), 51 deletions(-) >> >> diff --git a/ffplay.c b/ffplay.c >> index cf138dc..06d1d22 100644 >> --- a/ffplay.c >> +++ b/ffplay.c >> @@ -186,13 +186,10 @@ enum { >> }; >> >> typedef struct Decoder { >> - AVPacket pkt; >> - AVPacket pkt_temp; >> PacketQueue *queue; >> AVCodecContext *avctx; >> int pkt_serial; >> int finished; >> - int packet_pending; >> SDL_cond *empty_queue_cond; >> int64_t start_pts; >> AVRational start_pts_tb; >> @@ -551,40 +548,24 @@ static void decoder_init(Decoder *d, AVCodecContext *avctx, PacketQueue *queue, >> d->queue = queue; >> d->empty_queue_cond = empty_queue_cond; >> d->start_pts = AV_NOPTS_VALUE; >> + d->pkt_serial = -1; >> } >> >> static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { >> - int got_frame = 0; >> + int ret = AVERROR(EAGAIN); >> >> - do { >> - int ret = -1; >> + for (;;) { >> + AVPacket pkt; >> >> + if (d->queue->serial == d->pkt_serial) { >> + do { >> if (d->queue->abort_request) >> return -1; >> >> - if (!d->packet_pending || d->queue->serial != d->pkt_serial) { >> - AVPacket pkt; >> - do { >> - if (d->queue->nb_packets == 0) >> - SDL_CondSignal(d->empty_queue_cond); >> - if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0) >> - return -1; >> - if (pkt.data == flush_pkt.data) { >> - avcodec_flush_buffers(d->avctx); >> - d->finished = 0; >> - d->next_pts = d->start_pts; >> - d->next_pts_tb = d->start_pts_tb; >> - } >> - } while (pkt.data == flush_pkt.data || d->queue->serial != d->pkt_serial); >> - av_packet_unref(&d->pkt); >> - d->pkt_temp = d->pkt = pkt; >> - d->packet_pending = 1; >> - } >> - >> switch (d->avctx->codec_type) { >> case AVMEDIA_TYPE_VIDEO: >> - ret = avcodec_decode_video2(d->avctx, frame, &got_frame, &d->pkt_temp); >> - if (got_frame) { >> + ret = avcodec_receive_frame(d->avctx, frame); >> + if (ret >= 0) { >> if (decoder_reorder_pts == -1) { >> frame->pts = av_frame_get_best_effort_timestamp(frame); >> } else if (!decoder_reorder_pts) { >> @@ -593,8 +574,8 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { >> } >> break; >> case AVMEDIA_TYPE_AUDIO: >> - ret = avcodec_decode_audio4(d->avctx, frame, &got_frame, &d->pkt_temp); >> - if (got_frame) { >> + ret = avcodec_receive_frame(d->avctx, frame); >> + if (ret >= 0) { >> AVRational tb = (AVRational){1, frame->sample_rate}; >> if (frame->pts != AV_NOPTS_VALUE) >> frame->pts = av_rescale_q(frame->pts, av_codec_get_pkt_timebase(d->avctx), tb); >> @@ -606,37 +587,46 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { >> } >> } >> break; >> - case AVMEDIA_TYPE_SUBTITLE: >> - ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &d->pkt_temp); >> - break; >> + } >> + if (ret == AVERROR_EOF) { >> + d->finished = d->pkt_serial; >> + avcodec_flush_buffers(d->avctx); >> + return 0; >> + } >> + if (ret >= 0) >> + return 1; >> + } while (ret != AVERROR(EAGAIN)); >> } >> >> - if (ret < 0) { >> - d->packet_pending = 0; >> + do { >> + if (d->queue->nb_packets == 0) >> + SDL_CondSignal(d->empty_queue_cond); >> + if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0) >> + return -1; >> + } while (d->queue->serial != d->pkt_serial); >> + >> + if (pkt.data == flush_pkt.data) { >> + avcodec_flush_buffers(d->avctx); >> + d->finished = 0; >> + d->next_pts = d->start_pts; >> + d->next_pts_tb = d->start_pts_tb; >> } else { >> - d->pkt_temp.dts = >> - d->pkt_temp.pts = AV_NOPTS_VALUE; >> - if (d->pkt_temp.data) { >> - if (d->avctx->codec_type != AVMEDIA_TYPE_AUDIO) >> - ret = d->pkt_temp.size; >> - d->pkt_temp.data += ret; >> - d->pkt_temp.size -= ret; >> - if (d->pkt_temp.size <= 0) >> - d->packet_pending = 0; >> + if (d->avctx->codec_type == AVMEDIA_TYPE_SUBTITLE) { >> + int got_frame = 0; >> + ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &pkt); >> + if (ret < 0) >> + ret = AVERROR(EAGAIN); >> + else >> + ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF); >> } else { >> - if (!got_frame) { >> - d->packet_pending = 0; >> - d->finished = d->pkt_serial; >> - } >> + av_assert0(avcodec_send_packet(d->avctx, &pkt) != AVERROR(EAGAIN)); >> } >> + av_packet_unref(&pkt); >> } >> - } while (!got_frame && !d->finished); >> - >> - return got_frame; >> + } >> } >> >> static void decoder_destroy(Decoder *d) { >> - av_packet_unref(&d->pkt); >> avcodec_free_context(&d->avctx); >> } >> >> -- >> 2.10.2 >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
The logs: http://pastebin.com/1duYR0Ui Wallak. ----- Mail original ----- De: "Marton Balint" <cus@passwd.hu> À: "FFmpeg development discussions and patches" <ffmpeg-devel@ffmpeg.org> Cc: wallak@free.fr Envoyé: Vendredi 17 Mars 2017 23:38:31 Objet: Re: [FFmpeg-devel] [PATCH 2/3] ffplay: convert to new decode API On Fri, 17 Mar 2017, wallak@free.fr wrote: > I tried the patch. Once av_assert0 test is removed, the crystalhd > decoder is running till send_packet triggers AVERROR(EAGAIN), now the > crystalhd buffer is likely overflowed, the video freezes. A few seconds > later the decoder displays a few frames and the cycle goes on. > > Im not sure how to properly implement the function to stop or resend > send_packet frames, this is likely the only mechanism missing. Hmm. Theoretically an assertion at send_packet can only happen, if the CrystalHD decoder is is returning AVERROR(EAGAIN) for receive_frame and for send_packet as well, which is forbidden. Could you add some logging, and log the return values of both send_packet and receive_frame to make sure that is the case, and the crystalHD decoder is buggy, not my implementation of the new decode API? I tested h264_cuvid decoder with ffplay (the only decoder with the new API), and that worked so I am inclined to think CrystalHD is buggy. And the behaviour you described looks like once the buffers of CrystalHD is full, but no decoded frame is available yet, it simply returns AVERROR(EAGAIN) instead of blocking receive_frame call which it should do. Thanks, Marton > Best Regards, > Wallak. > > > > ----- Mail original ----- > De: "Marton Balint" <cus@passwd.hu> > À: "FFmpeg development discussions and patches" <ffmpeg-devel@ffmpeg.org> > Cc: wallak@free.fr > Envoyé: Jeudi 16 Mars 2017 23:25:18 > Objet: Re: [FFmpeg-devel] [PATCH 2/3] ffplay: convert to new decode API > > > > On Wed, 15 Mar 2017, Marton Balint wrote: > >> Since subtitles are not yet supported with the new API, CODEC_CAP_DELAY >> subtitle codecs (only libzvbi so far) may loose the last few buffered frames in >> the end of the stream. >> >> The impact of this is so limited, it seemded better to accept it than losing >> the simplification benefits of the new API. > > Hi Wallak, > > Have you had a chance to test this ffplay patch, and see if it fixes > CrystalHD decoding in ffplay or not? > > Thanks, > Marton > >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> ffplay.c | 92 +++++++++++++++++++++++++++++----------------------------------- >> 1 file changed, 41 insertions(+), 51 deletions(-) >> >> diff --git a/ffplay.c b/ffplay.c >> index cf138dc..06d1d22 100644 >> --- a/ffplay.c >> +++ b/ffplay.c >> @@ -186,13 +186,10 @@ enum { >> }; >> >> typedef struct Decoder { >> - AVPacket pkt; >> - AVPacket pkt_temp; >> PacketQueue *queue; >> AVCodecContext *avctx; >> int pkt_serial; >> int finished; >> - int packet_pending; >> SDL_cond *empty_queue_cond; >> int64_t start_pts; >> AVRational start_pts_tb; >> @@ -551,40 +548,24 @@ static void decoder_init(Decoder *d, AVCodecContext *avctx, PacketQueue *queue, >> d->queue = queue; >> d->empty_queue_cond = empty_queue_cond; >> d->start_pts = AV_NOPTS_VALUE; >> + d->pkt_serial = -1; >> } >> >> static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { >> - int got_frame = 0; >> + int ret = AVERROR(EAGAIN); >> >> - do { >> - int ret = -1; >> + for (;;) { >> + AVPacket pkt; >> >> + if (d->queue->serial == d->pkt_serial) { >> + do { >> if (d->queue->abort_request) >> return -1; >> >> - if (!d->packet_pending || d->queue->serial != d->pkt_serial) { >> - AVPacket pkt; >> - do { >> - if (d->queue->nb_packets == 0) >> - SDL_CondSignal(d->empty_queue_cond); >> - if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0) >> - return -1; >> - if (pkt.data == flush_pkt.data) { >> - avcodec_flush_buffers(d->avctx); >> - d->finished = 0; >> - d->next_pts = d->start_pts; >> - d->next_pts_tb = d->start_pts_tb; >> - } >> - } while (pkt.data == flush_pkt.data || d->queue->serial != d->pkt_serial); >> - av_packet_unref(&d->pkt); >> - d->pkt_temp = d->pkt = pkt; >> - d->packet_pending = 1; >> - } >> - >> switch (d->avctx->codec_type) { >> case AVMEDIA_TYPE_VIDEO: >> - ret = avcodec_decode_video2(d->avctx, frame, &got_frame, &d->pkt_temp); >> - if (got_frame) { >> + ret = avcodec_receive_frame(d->avctx, frame); >> + if (ret >= 0) { >> if (decoder_reorder_pts == -1) { >> frame->pts = av_frame_get_best_effort_timestamp(frame); >> } else if (!decoder_reorder_pts) { >> @@ -593,8 +574,8 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { >> } >> break; >> case AVMEDIA_TYPE_AUDIO: >> - ret = avcodec_decode_audio4(d->avctx, frame, &got_frame, &d->pkt_temp); >> - if (got_frame) { >> + ret = avcodec_receive_frame(d->avctx, frame); >> + if (ret >= 0) { >> AVRational tb = (AVRational){1, frame->sample_rate}; >> if (frame->pts != AV_NOPTS_VALUE) >> frame->pts = av_rescale_q(frame->pts, av_codec_get_pkt_timebase(d->avctx), tb); >> @@ -606,37 +587,46 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { >> } >> } >> break; >> - case AVMEDIA_TYPE_SUBTITLE: >> - ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &d->pkt_temp); >> - break; >> + } >> + if (ret == AVERROR_EOF) { >> + d->finished = d->pkt_serial; >> + avcodec_flush_buffers(d->avctx); >> + return 0; >> + } >> + if (ret >= 0) >> + return 1; >> + } while (ret != AVERROR(EAGAIN)); >> } >> >> - if (ret < 0) { >> - d->packet_pending = 0; >> + do { >> + if (d->queue->nb_packets == 0) >> + SDL_CondSignal(d->empty_queue_cond); >> + if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0) >> + return -1; >> + } while (d->queue->serial != d->pkt_serial); >> + >> + if (pkt.data == flush_pkt.data) { >> + avcodec_flush_buffers(d->avctx); >> + d->finished = 0; >> + d->next_pts = d->start_pts; >> + d->next_pts_tb = d->start_pts_tb; >> } else { >> - d->pkt_temp.dts = >> - d->pkt_temp.pts = AV_NOPTS_VALUE; >> - if (d->pkt_temp.data) { >> - if (d->avctx->codec_type != AVMEDIA_TYPE_AUDIO) >> - ret = d->pkt_temp.size; >> - d->pkt_temp.data += ret; >> - d->pkt_temp.size -= ret; >> - if (d->pkt_temp.size <= 0) >> - d->packet_pending = 0; >> + if (d->avctx->codec_type == AVMEDIA_TYPE_SUBTITLE) { >> + int got_frame = 0; >> + ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &pkt); >> + if (ret < 0) >> + ret = AVERROR(EAGAIN); >> + else >> + ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF); >> } else { >> - if (!got_frame) { >> - d->packet_pending = 0; >> - d->finished = d->pkt_serial; >> - } >> + av_assert0(avcodec_send_packet(d->avctx, &pkt) != AVERROR(EAGAIN)); >> } >> + av_packet_unref(&pkt); >> } >> - } while (!got_frame && !d->finished); >> - >> - return got_frame; >> + } >> } >> >> static void decoder_destroy(Decoder *d) { >> - av_packet_unref(&d->pkt); >> avcodec_free_context(&d->avctx); >> } >> >> -- >> 2.10.2 >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Sat, 18 Mar 2017, wallak@free.fr wrote: > The logs: http://pastebin.com/1duYR0Ui > Log with video only (run ffplay with -an -sn) might show it more clearly, but even from the logs above it looks like the CrystalHD codec is returning EAGAINs at the same time for both receive_frame and send_packet. If it indeed does work in ffmpeg, then I suspect a lot of busy waiting there. Philip, you mind taking a look? :) Thanks, Marton > Wallak. > > > ----- Mail original ----- > De: "Marton Balint" <cus@passwd.hu> > À: "FFmpeg development discussions and patches" <ffmpeg-devel@ffmpeg.org> > Cc: wallak@free.fr > Envoyé: Vendredi 17 Mars 2017 23:38:31 > Objet: Re: [FFmpeg-devel] [PATCH 2/3] ffplay: convert to new decode API > > > > On Fri, 17 Mar 2017, wallak@free.fr wrote: > >> I tried the patch. Once av_assert0 test is removed, the crystalhd >> decoder is running till send_packet triggers AVERROR(EAGAIN), now the >> crystalhd buffer is likely overflowed, the video freezes. A few seconds >> later the decoder displays a few frames and the cycle goes on. >> >> Im not sure how to properly implement the function to stop or resend >> send_packet frames, this is likely the only mechanism missing. > > Hmm. Theoretically an assertion at send_packet can only happen, if the > CrystalHD decoder is is returning AVERROR(EAGAIN) for receive_frame > and for send_packet as well, which is forbidden. > > Could you add some logging, and log the return values of both send_packet > and receive_frame to make sure that is the case, and the crystalHD decoder > is buggy, not my implementation of the new decode API? > > I tested h264_cuvid decoder with ffplay (the only decoder with the new > API), and that worked so I am inclined to think CrystalHD is buggy. And > the behaviour you described looks like once the buffers of CrystalHD > is full, but no decoded frame is available yet, it simply returns > AVERROR(EAGAIN) instead of blocking receive_frame call which it should do. > > Thanks, > Marton > >> Best Regards, >> Wallak. >> >> >> >> ----- Mail original ----- >> De: "Marton Balint" <cus@passwd.hu> >> À: "FFmpeg development discussions and patches" <ffmpeg-devel@ffmpeg.org> >> Cc: wallak@free.fr >> Envoyé: Jeudi 16 Mars 2017 23:25:18 >> Objet: Re: [FFmpeg-devel] [PATCH 2/3] ffplay: convert to new decode API >> >> >> >> On Wed, 15 Mar 2017, Marton Balint wrote: >> >>> Since subtitles are not yet supported with the new API, CODEC_CAP_DELAY >>> subtitle codecs (only libzvbi so far) may loose the last few buffered frames in >>> the end of the stream. >>> >>> The impact of this is so limited, it seemded better to accept it than losing >>> the simplification benefits of the new API. >> >> Hi Wallak, >> >> Have you had a chance to test this ffplay patch, and see if it fixes >> CrystalHD decoding in ffplay or not? >> >> Thanks, >> Marton >> >>> >>> Signed-off-by: Marton Balint <cus@passwd.hu> >>> --- >>> ffplay.c | 92 +++++++++++++++++++++++++++++----------------------------------- >>> 1 file changed, 41 insertions(+), 51 deletions(-) >>> >>> diff --git a/ffplay.c b/ffplay.c >>> index cf138dc..06d1d22 100644 >>> --- a/ffplay.c >>> +++ b/ffplay.c >>> @@ -186,13 +186,10 @@ enum { >>> }; >>> >>> typedef struct Decoder { >>> - AVPacket pkt; >>> - AVPacket pkt_temp; >>> PacketQueue *queue; >>> AVCodecContext *avctx; >>> int pkt_serial; >>> int finished; >>> - int packet_pending; >>> SDL_cond *empty_queue_cond; >>> int64_t start_pts; >>> AVRational start_pts_tb; >>> @@ -551,40 +548,24 @@ static void decoder_init(Decoder *d, AVCodecContext *avctx, PacketQueue *queue, >>> d->queue = queue; >>> d->empty_queue_cond = empty_queue_cond; >>> d->start_pts = AV_NOPTS_VALUE; >>> + d->pkt_serial = -1; >>> } >>> >>> static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { >>> - int got_frame = 0; >>> + int ret = AVERROR(EAGAIN); >>> >>> - do { >>> - int ret = -1; >>> + for (;;) { >>> + AVPacket pkt; >>> >>> + if (d->queue->serial == d->pkt_serial) { >>> + do { >>> if (d->queue->abort_request) >>> return -1; >>> >>> - if (!d->packet_pending || d->queue->serial != d->pkt_serial) { >>> - AVPacket pkt; >>> - do { >>> - if (d->queue->nb_packets == 0) >>> - SDL_CondSignal(d->empty_queue_cond); >>> - if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0) >>> - return -1; >>> - if (pkt.data == flush_pkt.data) { >>> - avcodec_flush_buffers(d->avctx); >>> - d->finished = 0; >>> - d->next_pts = d->start_pts; >>> - d->next_pts_tb = d->start_pts_tb; >>> - } >>> - } while (pkt.data == flush_pkt.data || d->queue->serial != d->pkt_serial); >>> - av_packet_unref(&d->pkt); >>> - d->pkt_temp = d->pkt = pkt; >>> - d->packet_pending = 1; >>> - } >>> - >>> switch (d->avctx->codec_type) { >>> case AVMEDIA_TYPE_VIDEO: >>> - ret = avcodec_decode_video2(d->avctx, frame, &got_frame, &d->pkt_temp); >>> - if (got_frame) { >>> + ret = avcodec_receive_frame(d->avctx, frame); >>> + if (ret >= 0) { >>> if (decoder_reorder_pts == -1) { >>> frame->pts = av_frame_get_best_effort_timestamp(frame); >>> } else if (!decoder_reorder_pts) { >>> @@ -593,8 +574,8 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { >>> } >>> break; >>> case AVMEDIA_TYPE_AUDIO: >>> - ret = avcodec_decode_audio4(d->avctx, frame, &got_frame, &d->pkt_temp); >>> - if (got_frame) { >>> + ret = avcodec_receive_frame(d->avctx, frame); >>> + if (ret >= 0) { >>> AVRational tb = (AVRational){1, frame->sample_rate}; >>> if (frame->pts != AV_NOPTS_VALUE) >>> frame->pts = av_rescale_q(frame->pts, av_codec_get_pkt_timebase(d->avctx), tb); >>> @@ -606,37 +587,46 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { >>> } >>> } >>> break; >>> - case AVMEDIA_TYPE_SUBTITLE: >>> - ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &d->pkt_temp); >>> - break; >>> + } >>> + if (ret == AVERROR_EOF) { >>> + d->finished = d->pkt_serial; >>> + avcodec_flush_buffers(d->avctx); >>> + return 0; >>> + } >>> + if (ret >= 0) >>> + return 1; >>> + } while (ret != AVERROR(EAGAIN)); >>> } >>> >>> - if (ret < 0) { >>> - d->packet_pending = 0; >>> + do { >>> + if (d->queue->nb_packets == 0) >>> + SDL_CondSignal(d->empty_queue_cond); >>> + if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0) >>> + return -1; >>> + } while (d->queue->serial != d->pkt_serial); >>> + >>> + if (pkt.data == flush_pkt.data) { >>> + avcodec_flush_buffers(d->avctx); >>> + d->finished = 0; >>> + d->next_pts = d->start_pts; >>> + d->next_pts_tb = d->start_pts_tb; >>> } else { >>> - d->pkt_temp.dts = >>> - d->pkt_temp.pts = AV_NOPTS_VALUE; >>> - if (d->pkt_temp.data) { >>> - if (d->avctx->codec_type != AVMEDIA_TYPE_AUDIO) >>> - ret = d->pkt_temp.size; >>> - d->pkt_temp.data += ret; >>> - d->pkt_temp.size -= ret; >>> - if (d->pkt_temp.size <= 0) >>> - d->packet_pending = 0; >>> + if (d->avctx->codec_type == AVMEDIA_TYPE_SUBTITLE) { >>> + int got_frame = 0; >>> + ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &pkt); >>> + if (ret < 0) >>> + ret = AVERROR(EAGAIN); >>> + else >>> + ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF); >>> } else { >>> - if (!got_frame) { >>> - d->packet_pending = 0; >>> - d->finished = d->pkt_serial; >>> - } >>> + av_assert0(avcodec_send_packet(d->avctx, &pkt) != AVERROR(EAGAIN)); >>> } >>> + av_packet_unref(&pkt); >>> } >>> - } while (!got_frame && !d->finished); >>> - >>> - return got_frame; >>> + } >>> } >>> >>> static void decoder_destroy(Decoder *d) { >>> - av_packet_unref(&d->pkt); >>> avcodec_free_context(&d->avctx); >>> } >>> >>> -- >>> 2.10.2 >>> >>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Sat, 18 Mar 2017 01:51:39 +0100 (CET) Marton Balint <cus@passwd.hu> wrote: > On Sat, 18 Mar 2017, wallak@free.fr wrote: > > > The logs: http://pastebin.com/1duYR0Ui > > > > Log with video only (run ffplay with -an -sn) might show it more > clearly, but even from the logs above it looks like the CrystalHD > codec is returning EAGAINs at the same time for both receive_frame > and send_packet. If it indeed does work in ffmpeg, then I suspect a > lot of busy waiting there. Philip, you mind taking a look? :) My first reply got HTML-ised by my client. Weird. Wallak, Can I get a sample from you to ensure I can repro. Also: are you using a 12 or a 15? I only have a 15 and have never tested the code with a 12. Also, it would be great if you can test playback with mpv. As I said, that's what I've used for testing. --phil
On Fri, 17 Mar 2017 19:42:07 -0700 Philip Langdale <philipl@overt.org> wrote: > On Sat, 18 Mar 2017 01:51:39 +0100 (CET) > Marton Balint <cus@passwd.hu> wrote: > > > On Sat, 18 Mar 2017, wallak@free.fr wrote: > > > > > The logs: http://pastebin.com/1duYR0Ui > > > > > > > Log with video only (run ffplay with -an -sn) might show it more > > clearly, but even from the logs above it looks like the CrystalHD > > codec is returning EAGAINs at the same time for both receive_frame > > and send_packet. If it indeed does work in ffmpeg, then I suspect a > > lot of busy waiting there. Philip, you mind taking a look? :) > > My first reply got HTML-ised by my client. Weird. > > Wallak, > > Can I get a sample from you to ensure I can repro. Also: are you using > a 12 or a 15? I only have a 15 and have never tested the code with a > 12. Also, it would be great if you can test playback with mpv. As I > said, that's what I've used for testing. I did take a brief look, and I confirmed that mpv works fine but ffplay is hitting this deadlock between send and receive. My hardware is getting flaky, and my actually be dying, so it's hard for me to do any kind of systematic testing. So right now my only useful advice is to look at how mpv is running the loop and see if there's a significant difference. --phil
On Sun, 19 Mar 2017 11:51:28 -0700 Philip Langdale <philipl@overt.org> wrote: > On Fri, 17 Mar 2017 19:42:07 -0700 > Philip Langdale <philipl@overt.org> wrote: > > > On Sat, 18 Mar 2017 01:51:39 +0100 (CET) > > Marton Balint <cus@passwd.hu> wrote: > > > > > On Sat, 18 Mar 2017, wallak@free.fr wrote: > > > > > > > The logs: http://pastebin.com/1duYR0Ui > > > > > > > > > > Log with video only (run ffplay with -an -sn) might show it more > > > clearly, but even from the logs above it looks like the CrystalHD > > > codec is returning EAGAINs at the same time for both receive_frame > > > and send_packet. If it indeed does work in ffmpeg, then I suspect a > > > lot of busy waiting there. Philip, you mind taking a look? :) > > > > My first reply got HTML-ised by my client. Weird. > > > > Wallak, > > > > Can I get a sample from you to ensure I can repro. Also: are you using > > a 12 or a 15? I only have a 15 and have never tested the code with a > > 12. Also, it would be great if you can test playback with mpv. As I > > said, that's what I've used for testing. > > I did take a brief look, and I confirmed that mpv works fine but ffplay > is hitting this deadlock between send and receive. My hardware is > getting flaky, and my actually be dying, so it's hard for me to do any > kind of systematic testing. > > So right now my only useful advice is to look at how mpv is running the > loop and see if there's a significant difference. Currently, mpv pretty strictly does essentially: while (1) [ if (!packet) packet = read_new_packet(); if (send_packet(packet) == ok) packet = NULL; receive_frame(); } which I guess is a bit unusual for API usage. The API should be able to deal with any of that, though.
----- Mail original ----- De: "wm4" <nfxjfg@googlemail.com> À: ffmpeg-devel@ffmpeg.org Envoyé: Lundi 20 Mars 2017 04:23:43 Objet: Re: [FFmpeg-devel] [PATCH 2/3] ffplay: convert to new decode API On Sun, 19 Mar 2017 11:51:28 -0700 Philip Langdale <philipl@overt.org> wrote: > On Fri, 17 Mar 2017 19:42:07 -0700 > Philip Langdale <philipl@overt.org> wrote: > > > On Sat, 18 Mar 2017 01:51:39 +0100 (CET) > > Marton Balint <cus@passwd.hu> wrote: > > > > > On Sat, 18 Mar 2017, wallak@free.fr wrote: > > > > > > > The logs: http://pastebin.com/1duYR0Ui > > > > > > > > > > Log with video only (run ffplay with -an -sn) might show it more > > > clearly, but even from the logs above it looks like the CrystalHD > > > codec is returning EAGAINs at the same time for both receive_frame > > > and send_packet. If it indeed does work in ffmpeg, then I suspect a > > > lot of busy waiting there. Philip, you mind taking a look? :) > > > > My first reply got HTML-ised by my client. Weird. > > > > Wallak, > > > > Can I get a sample from you to ensure I can repro. Also: are you using > > a 12 or a 15? I only have a 15 and have never tested the code with a > > 12. Also, it would be great if you can test playback with mpv. As I > > said, that's what I've used for testing. > > I did take a brief look, and I confirmed that mpv works fine but ffplay > is hitting this deadlock between send and receive. My hardware is > getting flaky, and my actually be dying, so it's hard for me to do any > kind of systematic testing. > > So right now my only useful advice is to look at how mpv is running the > loop and see if there's a significant difference. Currently, mpv pretty strictly does essentially: while (1) [ if (!packet) packet = read_new_packet(); if (send_packet(packet) == ok) packet = NULL; receive_frame(); } which I guess is a bit unusual for API usage. The API should be able to deal with any of that, though.
On Mon, 20 Mar 2017, wm4 wrote: > On Sun, 19 Mar 2017 11:51:28 -0700 > Philip Langdale <philipl@overt.org> wrote: > >> On Fri, 17 Mar 2017 19:42:07 -0700 >> Philip Langdale <philipl@overt.org> wrote: >> >> > On Sat, 18 Mar 2017 01:51:39 +0100 (CET) >> > Marton Balint <cus@passwd.hu> wrote: >> > >> > > On Sat, 18 Mar 2017, wallak@free.fr wrote: >> > > >> > > > The logs: http://pastebin.com/1duYR0Ui >> > > > >> > > >> > > Log with video only (run ffplay with -an -sn) might show it more >> > > clearly, but even from the logs above it looks like the CrystalHD >> > > codec is returning EAGAINs at the same time for both receive_frame >> > > and send_packet. If it indeed does work in ffmpeg, then I suspect a >> > > lot of busy waiting there. Philip, you mind taking a look? :) >> > >> > My first reply got HTML-ised by my client. Weird. >> > >> > Wallak, >> > >> > Can I get a sample from you to ensure I can repro. Also: are you using >> > a 12 or a 15? I only have a 15 and have never tested the code with a >> > 12. Also, it would be great if you can test playback with mpv. As I >> > said, that's what I've used for testing. >> >> I did take a brief look, and I confirmed that mpv works fine but ffplay >> is hitting this deadlock between send and receive. My hardware is >> getting flaky, and my actually be dying, so it's hard for me to do any >> kind of systematic testing. >> >> So right now my only useful advice is to look at how mpv is running the >> loop and see if there's a significant difference. > > Currently, mpv pretty strictly does essentially: > > while (1) [ > if (!packet) > packet = read_new_packet(); > if (send_packet(packet) == ok) > packet = NULL; > receive_frame(); > } > > which I guess is a bit unusual for API usage. > > The API should be able to deal with any of that, though. ffplay is: while (1) { while (receive_frame() != EAGAIN) { } send_packet(read_new_packet()) } I can change it to buffer the packet if send_packet() returns EAGAIN, even if it is should not be neede, but let's see if it makes a difference or not. Regards, Marton
On Mon, 20 Mar 2017 21:49:14 +0100 (CET) Marton Balint <cus@passwd.hu> wrote: > On Mon, 20 Mar 2017, wm4 wrote: > > > On Sun, 19 Mar 2017 11:51:28 -0700 > > Philip Langdale <philipl@overt.org> wrote: > > > >> On Fri, 17 Mar 2017 19:42:07 -0700 > >> Philip Langdale <philipl@overt.org> wrote: > >> > >> > On Sat, 18 Mar 2017 01:51:39 +0100 (CET) > >> > Marton Balint <cus@passwd.hu> wrote: > >> > > >> > > On Sat, 18 Mar 2017, wallak@free.fr wrote: > >> > > > >> > > > The logs: http://pastebin.com/1duYR0Ui > >> > > > > >> > > > >> > > Log with video only (run ffplay with -an -sn) might show it more > >> > > clearly, but even from the logs above it looks like the CrystalHD > >> > > codec is returning EAGAINs at the same time for both receive_frame > >> > > and send_packet. If it indeed does work in ffmpeg, then I suspect a > >> > > lot of busy waiting there. Philip, you mind taking a look? :) > >> > > >> > My first reply got HTML-ised by my client. Weird. > >> > > >> > Wallak, > >> > > >> > Can I get a sample from you to ensure I can repro. Also: are you using > >> > a 12 or a 15? I only have a 15 and have never tested the code with a > >> > 12. Also, it would be great if you can test playback with mpv. As I > >> > said, that's what I've used for testing. > >> > >> I did take a brief look, and I confirmed that mpv works fine but ffplay > >> is hitting this deadlock between send and receive. My hardware is > >> getting flaky, and my actually be dying, so it's hard for me to do any > >> kind of systematic testing. > >> > >> So right now my only useful advice is to look at how mpv is running the > >> loop and see if there's a significant difference. > > > > Currently, mpv pretty strictly does essentially: > > > > while (1) [ > > if (!packet) > > packet = read_new_packet(); > > if (send_packet(packet) == ok) > > packet = NULL; > > receive_frame(); > > } > > > > which I guess is a bit unusual for API usage. > > > > The API should be able to deal with any of that, though. > > ffplay is: > > while (1) { > while (receive_frame() != EAGAIN) { > } > send_packet(read_new_packet()) > } > > I can change it to buffer the packet if send_packet() returns EAGAIN, even > if it is should not be neede, but let's see if it makes a difference or > not. Yes, that should indeed not be needed according to the API. ffmpeg.c doesn't do that either.
diff --git a/ffplay.c b/ffplay.c index cf138dc..06d1d22 100644 --- a/ffplay.c +++ b/ffplay.c @@ -186,13 +186,10 @@ enum { }; typedef struct Decoder { - AVPacket pkt; - AVPacket pkt_temp; PacketQueue *queue; AVCodecContext *avctx; int pkt_serial; int finished; - int packet_pending; SDL_cond *empty_queue_cond; int64_t start_pts; AVRational start_pts_tb; @@ -551,40 +548,24 @@ static void decoder_init(Decoder *d, AVCodecContext *avctx, PacketQueue *queue, d->queue = queue; d->empty_queue_cond = empty_queue_cond; d->start_pts = AV_NOPTS_VALUE; + d->pkt_serial = -1; } static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { - int got_frame = 0; + int ret = AVERROR(EAGAIN); - do { - int ret = -1; + for (;;) { + AVPacket pkt; + if (d->queue->serial == d->pkt_serial) { + do { if (d->queue->abort_request) return -1; - if (!d->packet_pending || d->queue->serial != d->pkt_serial) { - AVPacket pkt; - do { - if (d->queue->nb_packets == 0) - SDL_CondSignal(d->empty_queue_cond); - if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0) - return -1; - if (pkt.data == flush_pkt.data) { - avcodec_flush_buffers(d->avctx); - d->finished = 0; - d->next_pts = d->start_pts; - d->next_pts_tb = d->start_pts_tb; - } - } while (pkt.data == flush_pkt.data || d->queue->serial != d->pkt_serial); - av_packet_unref(&d->pkt); - d->pkt_temp = d->pkt = pkt; - d->packet_pending = 1; - } - switch (d->avctx->codec_type) { case AVMEDIA_TYPE_VIDEO: - ret = avcodec_decode_video2(d->avctx, frame, &got_frame, &d->pkt_temp); - if (got_frame) { + ret = avcodec_receive_frame(d->avctx, frame); + if (ret >= 0) { if (decoder_reorder_pts == -1) { frame->pts = av_frame_get_best_effort_timestamp(frame); } else if (!decoder_reorder_pts) { @@ -593,8 +574,8 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { } break; case AVMEDIA_TYPE_AUDIO: - ret = avcodec_decode_audio4(d->avctx, frame, &got_frame, &d->pkt_temp); - if (got_frame) { + ret = avcodec_receive_frame(d->avctx, frame); + if (ret >= 0) { AVRational tb = (AVRational){1, frame->sample_rate}; if (frame->pts != AV_NOPTS_VALUE) frame->pts = av_rescale_q(frame->pts, av_codec_get_pkt_timebase(d->avctx), tb); @@ -606,37 +587,46 @@ static int decoder_decode_frame(Decoder *d, AVFrame *frame, AVSubtitle *sub) { } } break; - case AVMEDIA_TYPE_SUBTITLE: - ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &d->pkt_temp); - break; + } + if (ret == AVERROR_EOF) { + d->finished = d->pkt_serial; + avcodec_flush_buffers(d->avctx); + return 0; + } + if (ret >= 0) + return 1; + } while (ret != AVERROR(EAGAIN)); } - if (ret < 0) { - d->packet_pending = 0; + do { + if (d->queue->nb_packets == 0) + SDL_CondSignal(d->empty_queue_cond); + if (packet_queue_get(d->queue, &pkt, 1, &d->pkt_serial) < 0) + return -1; + } while (d->queue->serial != d->pkt_serial); + + if (pkt.data == flush_pkt.data) { + avcodec_flush_buffers(d->avctx); + d->finished = 0; + d->next_pts = d->start_pts; + d->next_pts_tb = d->start_pts_tb; } else { - d->pkt_temp.dts = - d->pkt_temp.pts = AV_NOPTS_VALUE; - if (d->pkt_temp.data) { - if (d->avctx->codec_type != AVMEDIA_TYPE_AUDIO) - ret = d->pkt_temp.size; - d->pkt_temp.data += ret; - d->pkt_temp.size -= ret; - if (d->pkt_temp.size <= 0) - d->packet_pending = 0; + if (d->avctx->codec_type == AVMEDIA_TYPE_SUBTITLE) { + int got_frame = 0; + ret = avcodec_decode_subtitle2(d->avctx, sub, &got_frame, &pkt); + if (ret < 0) + ret = AVERROR(EAGAIN); + else + ret = got_frame ? 0 : (pkt.data ? AVERROR(EAGAIN) : AVERROR_EOF); } else { - if (!got_frame) { - d->packet_pending = 0; - d->finished = d->pkt_serial; - } + av_assert0(avcodec_send_packet(d->avctx, &pkt) != AVERROR(EAGAIN)); } + av_packet_unref(&pkt); } - } while (!got_frame && !d->finished); - - return got_frame; + } } static void decoder_destroy(Decoder *d) { - av_packet_unref(&d->pkt); avcodec_free_context(&d->avctx); }
Since subtitles are not yet supported with the new API, CODEC_CAP_DELAY subtitle codecs (only libzvbi so far) may loose the last few buffered frames in the end of the stream. The impact of this is so limited, it seemded better to accept it than losing the simplification benefits of the new API. Signed-off-by: Marton Balint <cus@passwd.hu> --- ffplay.c | 92 +++++++++++++++++++++++++++++----------------------------------- 1 file changed, 41 insertions(+), 51 deletions(-)