Michael, can you merge this? On Wed, Nov 11, 2015 at 6:47 PM, Zhang Rui <bbcal...@gmail.com> wrote:
> 2015-11-12 9:45 GMT+08:00 Bryan Huh <br...@box.com>: > > > > > > On Wed, Nov 11, 2015 at 3:06 AM, Zhang Rui <bbcal...@gmail.com> wrote: > >> > >> 2015-11-11 18:00 GMT+08:00 Bryan Huh <br...@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. > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
