[FFmpeg-devel] [PATCH 2/2] avformat/mov: increase current_sample after reading, to avoid discontinuous sample

[Michael Niedermayer]

On Tue, Jun 02, 2015 at 11:15:16AM +0800, Zhang Rui wrote:
> 2015-06-02 7:26 GMT+08:00 Michael Niedermayer <michaelni at gmx.at>:
> > On Thu, May 21, 2015 at 12:31:21PM +0800, Zhang Rui wrote:
> >> Discontinuous sample could cause corrupted image if next video frame
> >> is non-key frame.
> >>
> >> Mostly, avio_seek() fails on I/O error for http/ftp/... stream.
> >> In my opinion, retry is better than skip.
> >>
> >> > -    /* must be done just before reading, to avoid infinite loop on sample */
> >> > -    sc->current_sample++;
> >>
> >> This line was first introduced in 2006 by commit b72708f8f.
> >> I'm not sure if it worth an option to enable the new behaviour,
> >> and keep the default behaviour as before.
> >
> > if its specific to avio_seek() should current_sample-- be done in
> > its error case instead ?
> 
> Also to av_get_packet(). Not sure about avpriv_dv_get_packet().
> current_sample-- would be OK to me.
> 
> > maybe with a retry count to limit retries in case the next sample
> > can be read but the current always fails
> 
> Retry count could be used up soon, during a WiFi reconnection.
> If we could recover from IO error at next packet, we also could recover
> at current packet with sane position and size for most protocol, e.g.
> http, ftp.
> 
> Skipping is also OK, but we need some flag to indicate that
> the following packet is discontinued for decoder to flush data.
> 
> > Iam not sure i fully understand the bug this fixes,
> > how can it be reproduced ?
> 
> Stream a normal H264 sample from http server.
> and close the socket connection from server side while streaming.
> ( Paused player causes idle connection in real world ).
> If client fails in the avio_seek() and make sc->current_sample++,
> the next call to av_read_frames() could cause a reconnection,
> which makes client to read next discontinued packet.

would below work ?

commit 816047eb161e804ba6312947f6bd7349cf934f80
Author: Michael Niedermayer <michaelni at gmx.at>
Date:   Wed Jun 3 13:04:37 2015 +0200

    avformat/mov: Retry same packet on IO failure to avoid loosing a packet

    Based on patch by: Zhang Rui <bbcallen at gmail.com>
    Signed-off-by: Michael Niedermayer <michaelni at gmx.at>

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 5300704..bc5743a 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4316,6 +4316,13 @@ static AVIndexEntry *mov_find_next_sample(AVFormatContext *s, AVStream **st)
     return sample;
 }

+static int should_retry(AVIOContext *pb, int error_code) {
+    if (error_code == AVERROR_EOF || avio_feof(pb))
+        return 0;
+
+    return 1;
+}
+
 static int mov_read_packet(AVFormatContext *s, AVPacket *pkt)
 {
     MOVContext *mov = s->priv_data;
@@ -4351,14 +4358,18 @@ static int mov_read_packet(AVFormatContext *s, AVPacket *pkt)
     }

     if (st->discard != AVDISCARD_ALL) {
-        if (avio_seek(sc->pb, sample->pos, SEEK_SET) != sample->pos) {
+        int64_t ret64 = avio_seek(sc->pb, sample->pos, SEEK_SET);
+        if (ret64 != sample->pos) {
             av_log(mov->fc, AV_LOG_ERROR, "stream %d, offset 0x%"PRIx64": partial file\n",
                    sc->ffindex, sample->pos);
+            sc->current_sample -= should_retry(sc->pb, ret64);
             return AVERROR_INVALIDDATA;
         }
         ret = av_get_packet(sc->pb, pkt, sample->size);
-        if (ret < 0)
+        if (ret < 0) {
+            sc->current_sample -= should_retry(sc->pb, ret);
             return ret;
+        }
         if (sc->has_palette) {
             uint8_t *pal;


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150603/c827fdaa/attachment.asc>