On Fri, Jan 13, 2017 at 11:36:41AM -0600, Joel Cunningham wrote: > > > On Jan 12, 2017, at 10:59 AM, Joel Cunningham <joel.cunning...@me.com> > > wrote: > > > > Nicolas, > > > > I’ve found existing “read-ahead logic” in avio_seek to do what I’ve > > implemented in http_stream_forward(). This is controlled by > > SHORT_SEEK_THRESHOLD, currently set to 4KB. I proto-typed increasing this > > to the 256KB (matches the initial TCP window in my test setup) and saw the > > same number of reduction in HTTP GETs and the number of seeks! Thanks for > > the heads up, this should reduce my patch size! > > > > I could plumb this setting (s->short_seek_threshold) to a URL function that > > would get the desired value from HTTP/TCP. Looking at how > > ffio_init_context() is implemented, it doesn’t appear to have access to the > > URLContext pointer. Any guidance on how I can plumb the protocol layer > > with aviobuf without a public API change? > > > > Thanks, > > > > Joel > > > I managed to figure the plumbing out in a way that doesn’t modify any public > APIs. I added a new callback to struct AVIOContext that gets a short seek > threshold value from URLContext if available (otherwise fallback to > s->short_seek_threshold). This allows using the read-ahead/discard logic in > avio_seek and eliminates my forwarding logic in the HTTP layer (thanks to > Nicolas for pointing me in this direction). See new patch below > > I also updated my commit message to include assumptions/considerations about > congestion control on the sender (thanks to Andy for calling out the > discussion on it). Lastly, I have upload new captures/log in dropbox for > those that want to take a look at my testing output (see > ffplay_short_seek_output.log and mac-ffplay-short-seek-patch.pcapng) > > https://www.dropbox.com/sh/4q4ru8isdv22joj/AABU3XyXmgLMiEFqucf1LdZ3a?dl=0 > > Thanks for the feedback so far, > > Joel > > From 9bb2f184591c2d6e6a91d3760e63b013ca4c95e5 Mon Sep 17 00:00:00 2001 > From: Joel Cunningham <joel.cunning...@me.com> > Date: Fri, 13 Jan 2017 10:52:25 -0600 > Subject: [PATCH] HTTP: improve performance by reducing forward seeks > > This commit optimizes HTTP performance by reducing forward seeks, instead > favoring a read-ahead and discard on the current connection (referred to > as a short seek) for seeks that are within a TCP window's worth of data. > This improves performance because with TCP flow control, a window's worth > of data will be in the local socket buffer already or in-flight from the > sender once congestion control on the sender is fully utilizing the window. > > Note: this approach doesn't attempt to differentiate from a newly opened > connection which may not be fully utilizing the window due to congestion > control vs one that is. The receiver can't get at this information, so we > assume worst case; that full window is in use (we did advertise it after all) > and that data could be in-flight > > The previous behavior of closing the connection, then opening a new > with a new HTTP range value results in a massive amounts of discarded > and re-sent data when large TCP windows are used. This has been observed > on MacOS/iOS which starts with an inital window of 256KB and grows up to > 1MB depending on the bandwidth-product delay. > > When seeking within a window's worth of data and we close the connection, > then open a new one within the same window's worth of data, we discard > from the current offset till the end of the window. Then on the new > connection the server ends up re-sending the previous data from new > offset till the end of old window. > > Example (assumes full window utilization): > > TCP window size: 64KB > Position: 32KB > Forward seek position: 40KB > > * (Next window) > 32KB |--------------| 96KB |---------------| 160KB > * > 40KB |---------------| 104KB > > Re-sent amount: 96KB - 40KB = 56KB > > For a real world test example, I have MP4 file of ~25MB, which ffplay > only reads ~16MB and performs 177 seeks. With current ffmpeg, this results > in 177 HTTP GETs and ~73MB worth of TCP data communication. With this > patch, ffmpeg issues 4 HTTP GETs and 3 seeks for a total of ~22MB of TCP data > communication. > > To support this feature, the short seek logic in avio_seek() has been > extended to call a function to get the short seek threshold value. This > callback has been plumbed to the URLProtocol structure, which now has > infrastructure in HTTP and TCP to get the underlying receiver window size > via SO_RCVBUF. If the underlying URL and protocol don't support returning > a short seek threshold, the default s->short_seek_threshold is used > > This feature has been tested on Windows 7 and MacOS/iOS. Windows support > is slightly complicated by the fact that when TCP window auto-tuning is > enabled, SO_RCVBUF doesn't report the real window size, but it does if > SO_RCVBUF was manually set (disabling auto-tuning). So we can only use > this optimization on Windows in the later case > --- > libavformat/avio.c | 7 +++++++ > libavformat/avio.h | 6 ++++++ > libavformat/aviobuf.c | 18 +++++++++++++++++- > libavformat/http.c | 8 ++++++++ > libavformat/tcp.c | 21 +++++++++++++++++++++ > libavformat/url.h | 8 ++++++++ > 6 files changed, 67 insertions(+), 1 deletion(-)
this segfaults with many fuzzed files
backtrace looks like this:
#0 0x00007fffffffb368 in ?? ()
#1 0x00000000005f9280 in avio_seek (s=0x7fffffffb278, offset=31, whence=0) at
libavformat/aviobuf.c:259
>
> diff --git a/libavformat/avio.c b/libavformat/avio.c
> index 3606eb0..ecf6801 100644
> --- a/libavformat/avio.c
> +++ b/libavformat/avio.c
> @@ -645,6 +645,13 @@ int ffurl_get_multi_file_handle(URLContext *h, int
> **handles, int *numhandles)
> return h->prot->url_get_multi_file_handle(h, handles, numhandles);
> }
>
> +int ffurl_get_short_seek(URLContext *h)
> +{
> + if (!h->prot->url_get_short_seek)
> + return -1;
should be some AVERROR code
> + return h->prot->url_get_short_seek(h);
> +}
> +
> int ffurl_shutdown(URLContext *h, int flags)
> {
> if (!h->prot->url_shutdown)
> diff --git a/libavformat/avio.h b/libavformat/avio.h
> index b1ce1d1..0480981 100644
> --- a/libavformat/avio.h
> +++ b/libavformat/avio.h
> @@ -287,6 +287,12 @@ typedef struct AVIOContext {
> int short_seek_threshold;
>
> /**
> + * A callback that is used instead of short_seek_threshold.
> + * This is current internal only, do not use from outside.
> + */
> + int (*short_seek_get)(void *opaque);
> +
> + /**
> * ',' separated list of allowed protocols.
> */
> const char *protocol_whitelist;
thats not ok to add this way, the docs say this:
/**
* Bytestream IO Context.
* New fields can be added to the end with minor version bumps.
* Removal, reordering and changes to existing fields require a major
* version bump.
* sizeof(AVIOContext) must not be used outside libav*.
*
* @note None of the function pointers in AVIOContext should be called
* directly, they should only be set by the client application
* when implementing custom I/O. Normally these are set to the
* function pointers specified in avio_alloc_context()
*/
[...]
> --- a/libavformat/tcp.c
> +++ b/libavformat/tcp.c
> @@ -265,6 +265,26 @@ static int tcp_get_file_handle(URLContext *h)
> return s->fd;
> }
>
> +static int tcp_get_window_size(URLContext *h)
> +{
> + TCPContext *s = h->priv_data;
> + int avail;
> + int avail_len = sizeof(avail);
> +
> + #if HAVE_WINSOCK2_H
this formating is IIRC not allowed in C the # must be in the first
column
> + /* SO_RCVBUF with winsock only reports the actual TCP window size when
> + auto-tuning has been disabled via setting SO_RCVBUF */
> + if (s->recv_buffer_size < 0) {
> + return AVERROR(ENOSYS);
> + }
> + #endif
> +
> + if (getsockopt(s->fd, SOL_SOCKET, SO_RCVBUF, &avail, &avail_len)) {
> + return ff_neterrno();
> + }
the indention is inconsisntent
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
It is what and why we do it that matters, not just one of them.
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
