[FFmpeg-devel] [PATCH] HLS fixes: better relative path handling, larger buffer for long URIs, only send range header when necessary

[Reimar Döffinger]

On 23 Sep 2012, at 15:50, Duncan Salerno <duncan.salerno at gmail.com> wrote:
> On Sun, Sep 23, 2012 at 2:24 PM, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de> wrote:
>> On Sun, Sep 23, 2012 at 01:44:33PM +0200, Michael Niedermayer wrote:
>>> On Sun, Sep 23, 2012 at 10:50:05AM +0200, Reimar Döffinger wrote:
>>>> On 23 Sep 2012, at 02:51, Michael Niedermayer <michaelni at gmx.at> wrote:
>>>>> Hi
>>>>> 
>>>>> On Sat, Sep 22, 2012 at 09:17:36PM +0100, Duncan Salerno wrote:
>>>>>> ---
>>>>>> libavformat/http.c  |    6 +++---
>>>>>> libavformat/utils.c |   27 +++++++++++++++++++++++----
>>>>>> 2 files changed, 26 insertions(+), 7 deletions(-)
>>>>>> 
>>>>>> diff --git a/libavformat/http.c b/libavformat/http.c
>>>>>> index 376ff9e..669fdf4 100644
>>>>>> --- a/libavformat/http.c
>>>>>> +++ b/libavformat/http.c
>>>>>> @@ -33,7 +33,7 @@
>>>>>>   only a subset of it. */
>>>>>> 
>>>>>> /* used for protocol handling */
>>>>>> -#define BUFFER_SIZE 1024
>>>>>> +#define BUFFER_SIZE 4096
>>>>>> #define MAX_REDIRECTS 8
>>>>>> 
>>>>>> typedef struct {
>>>>>> @@ -380,7 +380,7 @@ static int http_connect(URLContext *h, const char *path, const char *local_path,
>>>>>> {
>>>>>>    HTTPContext *s = h->priv_data;
>>>>>>    int post, err;
>>>>>> -    char headers[1024] = "";
>>>>>> +    char headers[4096] = "";
>>>>>>    char *authstr = NULL, *proxyauthstr = NULL;
>>>>>>    int64_t off = s->off;
>>>>>>    int len = 0;
>>>>>> @@ -411,7 +411,7 @@ static int http_connect(URLContext *h, const char *path, const char *local_path,
>>>>>>    if (!has_header(s->headers, "\r\nAccept: "))
>>>>>>        len += av_strlcpy(headers + len, "Accept: */*\r\n",
>>>>>>                          sizeof(headers) - len);
>>>>>> -    if (!has_header(s->headers, "\r\nRange: ") && !post)
>>>>>> +    if (!has_header(s->headers, "\r\nRange: ") && !post && s->off > 0)
>>>>>>        len += av_strlcatf(headers + len, sizeof(headers) - len,
>>>>>>                           "Range: bytes=%"PRId64"-\r\n", s->off);
>>>>>> 
>>>>> 
>>>>> split and applied above
>>>> 
>>>> May I ask if there is a specific reason for that change?
>>>> I haven't double-checked if we use it that way, but detecting if seeking works can be difficult because many servers set the wrong things in response headers.
>>>> I believe setting a Range in the request makes those give you a few more hints whether they support it or not.
>>> 
>>> i thought the same yesterday but then after a quick look in the http
>>> spec thought its ok, a longer look in the spec now and actual testing
>>> though confirms that we loose some hints on seekability with this
>>> change, thus
>>> reverted
>> 
>> Yes, the problem is that the spec doesn't help much because so many
>> people hack their own web servers for special purposes without either
>> the will or ability to do it right (including companies like Google, so
>> it's not just your typical 3rd rate companies).
> 
> I've been testing HLS with a few commercial services, and comparing
> with iPhone. I'll send a patch with my URL testcases from the RFC -
> the current code thinks a backslash in the query part is in the path,
> so breaks some providers I've seen, for example. The iPhone, i guess
> the canonical implemantation of HLS, does not send Range headers, in
> fact I've seen one provider will always return 403 if you try (hence
> my change) - seeking in HLS doesn't really make any sense. Can you
> suggest a simple way of omitting the header when the caller is HLS?

Well, I would have suggest to instead disable seeking and retry without the Range header after a failure.
In addition HLS could then maybe trigger the code to not send Range on the first try by setting the context non-seekable from the start.