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

[Michael Niedermayer]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170127/8ce1658f/attachment.sig>