diff mbox

[FFmpeg-devel,PATCHv2,2/3] ffplay: convert to new decode API

Message ID 20170320205149.5672-1-cus@passwd.hu
State Superseded
Headers show

Commit Message

Marton Balint March 20, 2017, 8:51 p.m. UTC
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 | 96 +++++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 49 insertions(+), 47 deletions(-)

Comments

Philip Langdale March 21, 2017, 2:34 p.m. UTC | #1
On Mon, 20 Mar 2017 21:51:49 +0100
Marton Balint <cus@passwd.hu> 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.

Tried this v2 patch out and it's still stalling on the crystalhd input.
I also learned, disturbingly, that mpv will fail if I turn on debug
messages, so clearly there's serious timing sensitivity even there. I
can look into blocking on the output side, but I've been very scared of
doing this before, because I've had cases where it deadlocks; "sure I
submitted 250 frames to the hardware - but I'm sure as hell not getting
anything back...". I'd much rather yield to the application to decide
when to give up, but if the API doesn't support that, then so be it.

--phil
wm4 March 21, 2017, 3:05 p.m. UTC | #2
On Tue, 21 Mar 2017 07:34:32 -0700
Philip Langdale <philipl@overt.org> wrote:

> On Mon, 20 Mar 2017 21:51:49 +0100
> Marton Balint <cus@passwd.hu> 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.  
> 
> Tried this v2 patch out and it's still stalling on the crystalhd input.
> I also learned, disturbingly, that mpv will fail if I turn on debug
> messages, so clearly there's serious timing sensitivity even there. I
> can look into blocking on the output side, but I've been very scared of
> doing this before, because I've had cases where it deadlocks; "sure I
> submitted 250 frames to the hardware - but I'm sure as hell not getting
> anything back...". I'd much rather yield to the application to decide
> when to give up, but if the API doesn't support that, then so be it.

Well, we _could_ add an async mode to the API to deal with such cases,
but of course it would still need either a notification mechanism
(which the hardware's decode API would have to provide), or evil
polling.

Though both of these mechanisms could be used to emulate a sync API, so
this wouldn't actually give too much.
Philip Langdale March 22, 2017, 9:57 p.m. UTC | #3
On Tue, 21 Mar 2017 07:34:32 -0700
Philip Langdale <philipl@overt.org> wrote:

> On Mon, 20 Mar 2017 21:51:49 +0100
> Marton Balint <cus@passwd.hu> 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.  
> 
> Tried this v2 patch out and it's still stalling on the crystalhd
> input. I also learned, disturbingly, that mpv will fail if I turn on
> debug messages, so clearly there's serious timing sensitivity even
> there. I can look into blocking on the output side, but I've been
> very scared of doing this before, because I've had cases where it
> deadlocks; "sure I submitted 250 frames to the hardware - but I'm
> sure as hell not getting anything back...". I'd much rather yield to
> the application to decide when to give up, but if the API doesn't
> support that, then so be it.

Marton,

Although there's an existence proof with mpv that the crystalhd decoder
can be used through the API, it's clearly more fragile than it should
be, so I'm fine with you pushing as is, and I'll see what I can do to
make the decoder do some kind of blocking to smooth this over.

--phil
diff mbox

Patch

diff --git a/ffplay.c b/ffplay.c
index cf138dc..e5120d7 100644
--- a/ffplay.c
+++ b/ffplay.c
@@ -187,7 +187,6 @@  enum {
 
 typedef struct Decoder {
     AVPacket pkt;
-    AVPacket pkt_temp;
     PacketQueue *queue;
     AVCodecContext *avctx;
     int pkt_serial;
@@ -551,40 +550,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 +576,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,33 +589,52 @@  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 (d->packet_pending) {
+                av_packet_move_ref(&pkt, &d->pkt);
+                d->packet_pending = 0;
+            } else {
+                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;
+                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);
         }
-    } while (!got_frame && !d->finished);
-
-    return got_frame;
+    }
 }
 
 static void decoder_destroy(Decoder *d) {