[FFmpeg-devel] [PATCH] avformat/async: Fix bug where async could not recover after seek to eof

[Bryan Huh]

Michael, can you merge this?

On Wed, Nov 11, 2015 at 6:47 PM, Zhang Rui <bbcallen at gmail.com> wrote:

> 2015-11-12 9:45 GMT+08:00 Bryan Huh <bryan at box.com>:
> >
> >
> > On Wed, Nov 11, 2015 at 3:06 AM, Zhang Rui <bbcallen at gmail.com> wrote:
> >>
> >> 2015-11-11 18:00 GMT+08:00 Bryan Huh <bryan at box.com>:
> >> > When async issues its inner seek via ffurl_seek, it treats failures as
> >> > EOF being reached. This is not consistent with the behavior of other
> >> > protocols (e.g. http, cache) which continue to tolerate reads after
> >> > failed seeks, and therefore does not interact correctly with them.
> >>
> >> I'm not sure if this is a defined behavior, although it already exists
> >> in libavformat.
> >> At least, it doesn't documented in libavformat/url.h;
> >>
> >>
> >> > A common pattern where this manifests itself is where avio_seek is
> >> > called with pos to be the end-of-file - the http range-request would
> >> > fail here, and async would set io_eof_reached to 1. The background
> >> > thread would then refuse to read more bytes, and subsequent reads
> would
> >> > only empty the fifo and end in an error.
> >>
> >> Actually, I intended to refuse to do more read after failed seeks..
> >> because some demuxers call avio_read() immediately after unchecked
> >> avio_seek().
> >> That would cause more undefined behavior, such as leading a seek to an
> >> insane position.
> >> e.g.
> >>
> http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavformat/flvdec.c;h=ca7396931582a642956c68385e94dbbaa5758f26;hb=HEAD#l989
> >>
> >> IMO, I don't think any unknown error from internal protocol should be
> >> hidden.
> >> They should be handled inside protocol (e.g. reconnect) or at high
> >> level (e.g. restart),
> >> at where there are more information about the error or the need.
> >
> >
> > My commit doesn't hide the internal protocol error. It still sets
> > c->seek_ret to be the error, so that callers can choose to reason about
> it.
> > The only effective change now is in how subsequent reads/seeks should
> > behave.
> >
> > I don't think it should be async's responsibility to protect the demuxer
> > from the strange behaviors of the inner-protocol. If the demuxer was
> written
> > to do unchecked seeks followed by reads, and those seeks fail, the
> demuxer
> > would've gotten insane data on subsequent-reads without the "async"
> protocol
> > wrapper as well. By altering async's behavior after seek-failures to stop
> > reading, you are spreading out the error-reporting later in time, which I
> > think is more confusing.
> >
> > Also, previously the behavior was still potentially giving insane data on
> > subsequent-reads, as I described below.
> >>
> >>
> >> > Presumably the code may have expected subsequent seeks to unset the
> >> > io_eof_reached but this is not guaranteed to be true - a subsequent
> seek
> >> > that lands in the AVIOContext's buffer (the fact that the
> >> > previously-failed avio_seek leaves the AVIOContext's buffer intact
> also
> >> > suggests that follow-up reads are expected to be tolerated) would not
> be
> >> > issued to the async_seek function, and when that buffer is drained
> only
> >> > async_read calls would follow, leading to the same error just
> described.
> >>
> >> Assuming reads should continue after failed seek, this patch is OK for
> me.
> >> Otherwise, I would like to add an option to switch the behavior after
> >> this patch.
> >
> > It's not clear to me that reads-succeeding-after-failed-seek is really an
> > expectation (my guess is that it's undefined behavior) and I don't mind
> the
> > work to add an option, but I am thinking that my patch is the correct
> > behavior in all cases because I think async should be a neutral protocol
> -
> > if the inner protocol can handle reads after failed-seeks, then adding
> async
> > to it should be no different. If the inner protocol cannot handle reads
> > after failed seeks, then here again, async should not change that.
> >
> > Also for reference, I looked at seek functions in all the existing
> protocols
> > - many of them don't have a seek function, and out of the ones that do
> only
> > about half (concat, cache, ftp, subfile, http, async) maintain their own
> > logical-pos pointer - the remainder just delegate to the seek and read
> > functionality of an external library, and just pass-through those
> semantics.
> > All but subfile leave the logical-pos pointer alone after a failed seek.
> And
> > it looks like none of them maintain internal state for a failed seek, so
> > subsequent reads work as normal (the read may eventually fail, but they
> pass
> > it through and let it fail at the lower protocol).
>
> Yes, this patch is correct with or without that option.
> I would prefer this patch to be merged first if no other objection.
>