[FFmpeg-devel] [PATCH] HTTP: optimize forward seek performance

Joel Cunningham joel.cunningham at me.com
Fri Jan 27 05:16:01 EET 2017


Attached is a git format-patch file

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-HTTP-improve-performance-by-reducing-forward-seeks.patch
Type: application/octet-stream
Size: 10122 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170126/dd4bb1d1/attachment.obj>
-------------- next part --------------

> On Jan 26, 2017, at 6:52 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> 
> On Thu, Jan 26, 2017 at 10:44:33AM -0600, Joel Cunningham wrote:
>> 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 <joel.cunningham at me.com>
>> Date: Fri, 13 Jan 2017 10:52:25 -0600
>> Subject: [PATCH] HTTP: improve performance by reducing forward seeks
> 
> 
> please attach a proper git format-patch or use git send-email
> 
> applying this is not as smooth and easy as it should be
> 
> [...]
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> If you fake or manipulate statistics in a paper in physics you will never
> get a job again.
> If you fake or manipulate statistics in a paper in medicin you will get
> a job for life at the pharma industry.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



More information about the ffmpeg-devel mailing list