From patchwork Thu Jan 26 16:44:33 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Joel Cunningham X-Patchwork-Id: 2328 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.89.21 with SMTP id n21csp269439vsb; Thu, 26 Jan 2017 08:45:16 -0800 (PST) X-Received: by 10.28.13.16 with SMTP id 16mr3392812wmn.101.1485449115968; Thu, 26 Jan 2017 08:45:15 -0800 (PST) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id g48si2632702wrg.164.2017.01.26.08.45.14; Thu, 26 Jan 2017 08:45:15 -0800 (PST) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@me.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=me.com Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 0922B68A84A; Thu, 26 Jan 2017 18:45:11 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from pv38p43im-ztdg05061201.me.com (pv38p43im-ztdg05061201.me.com [17.133.183.25]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 9D2D568A83C for ; Thu, 26 Jan 2017 18:45:03 +0200 (EET) Received: from process-dkim-sign-daemon.pv38p43im-ztdg05061201.me.com by pv38p43im-ztdg05061201.me.com (Oracle Communications Messaging Server 7.0.5.38.0 64bit (built Feb 26 2016)) id <0OKE00G00BTD0900@pv38p43im-ztdg05061201.me.com> for ffmpeg-devel@ffmpeg.org; Thu, 26 Jan 2017 16:44:35 +0000 (GMT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=me.com; s=4d515a; t=1485449075; bh=/R37AhENccf/TsxcCU9CJj283s8Wak0UGy3adENvOP4=; h=From:Content-type:MIME-version:Subject:Date:To:Message-id; b=IxtqqrHmtll+FyIJvrOXt8zjhGuj4Jyi3ZWTUv3dg+m+OB8/QpduLyiFf8eUWsCnJ U7w8IzwDpdJcJ+kZBLlPpy6dCrNI/KXxrHPuqKyD3nEelBHAhorKleaaBg895kMewM l2sk5sCDe0iqqJrT5wMqbFPcTLpCEU8I2ikwG0goTD17oWOS6a8LvKZTOqI1ZM3YPR Yp8dGii5sdP/iys2m5bupSy3nB1dPWGobUHTT6yKlDMtG0BBflkc7diEZYIz0zuTOm 5ZjFcV1vKYQ7/weLFWKwMB23Ni5Ah+C+3Yq/94fxY2OYNM0KKx/WydbA7xTP9ttxze cToiZCBRpyHYQ== Received: from icloud.com ([127.0.0.1]) by pv38p43im-ztdg05061201.me.com (Oracle Communications Messaging Server 7.0.5.38.0 64bit (built Feb 26 2016)) with ESMTPSA id <0OKE00A2ABU9SX50@pv38p43im-ztdg05061201.me.com> for ffmpeg-devel@ffmpeg.org; Thu, 26 Jan 2017 16:44:35 +0000 (GMT) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-01-26_11:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 clxscore=1034 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1603290000 definitions=main-1701260162 From: Joel Cunningham MIME-version: 1.0 (Mac OS X Mail 10.2 \(3259\)) Date: Thu, 26 Jan 2017 10:44:33 -0600 References: <5C6D12FC-B117-4EF8-AA7C-320426C12D5D@me.com> <20170112125342.GA416006@phare.normalesup.org> <879BB226-8B35-4CE6-8F7B-038CAE7A207E@me.com> <081DBAED-2A10-4E48-963B-E1595F05375A@me.com> <3BA7C6AB-FE7B-458F-B070-A5A9DED29717@me.com> <20170126015425.GK4698@nb4> To: FFmpeg development discussions and patches In-reply-to: <20170126015425.GK4698@nb4> Message-id: <82AD7217-05B8-4D7F-859C-B271BD47EB70@me.com> X-Mailer: Apple Mail (2.3259) Subject: Re: [FFmpeg-devel] [PATCH] HTTP: optimize forward seek performance X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Michael, Thanks for the review and testing! New patch included, see comments below > > 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 > I found I wasn’t setting s->short_seek_get to NULL in ffio_init_context() and was relying on the zeroing aspect of av_mallocz() in avio_alloc_context(). From a grep of the source, it looks like fifo_init_context() can be called from other functions without having zero’d the AVIOContext first. Is the fuzz test exercising the AVIOContext in this manner? I’d also like to reproduce this test if there are instructions >> >> 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 Fixed >> + 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() > */ > I moved short_seek_get to the end of AVIOContext. I’m not sure if the minor version bump referenced in the comment requires any in-code changes. Is this an acceptable magnitude of change for this feature? > [...] >> --- 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 Both formatting issues have been corrected From bac0f351afd14c56c56376d20557b7330bcea23e Mon Sep 17 00:00:00 2001 From: Joel Cunningham 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 initial 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 | 19 ++++++++++++++++++- libavformat/http.c | 8 ++++++++ libavformat/tcp.c | 21 +++++++++++++++++++++ libavformat/url.h | 8 ++++++++ 6 files changed, 68 insertions(+), 1 deletion(-) diff --git a/libavformat/avio.c b/libavformat/avio.c index 3606eb0..62233a6 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 AVERROR(ENOSYS); + 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..a088f05 100644 --- a/libavformat/avio.h +++ b/libavformat/avio.h @@ -313,6 +313,12 @@ typedef struct AVIOContext { */ enum AVIODataMarkerType current_type; int64_t last_time; + + /** + * 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); } AVIOContext; /** diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index 134d627..b102510 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -119,6 +119,7 @@ int ffio_init_context(AVIOContext *s, s->ignore_boundary_point = 0; s->current_type = AVIO_DATA_MARKER_UNKNOWN; s->last_time = AV_NOPTS_VALUE; + s->short_seek_get = NULL; return 0; } @@ -233,6 +234,7 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int whence) int64_t pos; int force = whence & AVSEEK_FORCE; int buffer_size; + int short_seek; whence &= ~AVSEEK_FORCE; if(!s) @@ -254,13 +256,21 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int whence) if (offset < 0) return AVERROR(EINVAL); + if (s->short_seek_get) { + short_seek = s->short_seek_get(s->opaque); + /* fallback to default short seek */ + if (short_seek <= 0) + short_seek = s->short_seek_threshold; + } else + short_seek = s->short_seek_threshold; + offset1 = offset - pos; // "offset1" is the relative offset from the beginning of s->buffer if (!s->must_flush && (!s->direct || !s->seek) && offset1 >= 0 && offset1 <= buffer_size - s->write_flag) { /* can do the seek inside the buffer */ s->buf_ptr = s->buffer + offset1; } else if ((!s->seekable || - offset1 <= buffer_size + s->short_seek_threshold) && + offset1 <= buffer_size + short_seek) && !s->write_flag && offset1 >= 0 && (!s->direct || !s->seek) && (whence != SEEK_END || force)) { @@ -858,6 +868,12 @@ static int64_t io_seek(void *opaque, int64_t offset, int whence) return ffurl_seek(internal->h, offset, whence); } +static int io_short_seek(void *opaque) +{ + AVIOInternal *internal = opaque; + return ffurl_get_short_seek(internal->h); +} + static int io_read_pause(void *opaque, int pause) { AVIOInternal *internal = opaque; @@ -919,6 +935,7 @@ int ffio_fdopen(AVIOContext **s, URLContext *h) (*s)->read_pause = io_read_pause; (*s)->read_seek = io_read_seek; } + (*s)->short_seek_get = io_short_seek; (*s)->av_class = &ff_avio_class; return 0; fail: diff --git a/libavformat/http.c b/libavformat/http.c index 944a6cf..b354c87 100644 --- a/libavformat/http.c +++ b/libavformat/http.c @@ -1524,6 +1524,12 @@ static int http_get_file_handle(URLContext *h) return ffurl_get_file_handle(s->hd); } +static int http_get_short_seek(URLContext *h) +{ + HTTPContext *s = h->priv_data; + return ffurl_get_short_seek(s->hd); +} + #define HTTP_CLASS(flavor) \ static const AVClass flavor ## _context_class = { \ .class_name = # flavor, \ @@ -1545,6 +1551,7 @@ const URLProtocol ff_http_protocol = { .url_seek = http_seek, .url_close = http_close, .url_get_file_handle = http_get_file_handle, + .url_get_short_seek = http_get_short_seek, .url_shutdown = http_shutdown, .priv_data_size = sizeof(HTTPContext), .priv_data_class = &http_context_class, @@ -1564,6 +1571,7 @@ const URLProtocol ff_https_protocol = { .url_seek = http_seek, .url_close = http_close, .url_get_file_handle = http_get_file_handle, + .url_get_short_seek = http_get_short_seek, .url_shutdown = http_shutdown, .priv_data_size = sizeof(HTTPContext), .priv_data_class = &https_context_class, diff --git a/libavformat/tcp.c b/libavformat/tcp.c index 25abafc..bfed5f5 100644 --- 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 + /* 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(); + } + return avail; +} + const URLProtocol ff_tcp_protocol = { .name = "tcp", .url_open = tcp_open, @@ -273,6 +293,7 @@ const URLProtocol ff_tcp_protocol = { .url_write = tcp_write, .url_close = tcp_close, .url_get_file_handle = tcp_get_file_handle, + .url_get_short_seek = tcp_get_window_size, .url_shutdown = tcp_shutdown, .priv_data_size = sizeof(TCPContext), .flags = URL_PROTOCOL_FLAG_NETWORK, diff --git a/libavformat/url.h b/libavformat/url.h index 5c50245..910f1e0 100644 --- a/libavformat/url.h +++ b/libavformat/url.h @@ -84,6 +84,7 @@ typedef struct URLProtocol { int (*url_get_file_handle)(URLContext *h); int (*url_get_multi_file_handle)(URLContext *h, int **handles, int *numhandles); + int (*url_get_short_seek)(URLContext *h); int (*url_shutdown)(URLContext *h, int flags); int priv_data_size; const AVClass *priv_data_class; @@ -249,6 +250,13 @@ int ffurl_get_file_handle(URLContext *h); int ffurl_get_multi_file_handle(URLContext *h, int **handles, int *numhandles); /** + * Return the current short seek threshold value for this URL. + * + * @return threshold (>0) on success or <=0 on error. + */ +int ffurl_get_short_seek(URLContext *h); + +/** * Signal the URLContext that we are done reading or writing the stream. * * @param h pointer to the resource