[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