[FFmpeg-devel] http: honor response headers in redirect caching

[Ronald S. Bultje]

Hi,

On Mon, Jan 17, 2022 at 7:10 AM Ronald S. Bultje <rsbultje at gmail.com> wrote:

> Hi,
>
> On Tue, Jan 11, 2022 at 9:43 AM Eran Kornblau <eran.kornblau at kaltura.com>
> wrote:
>
>> Hi all,
>>
>> Recently I’ve submitted a patch that adds a config option to disable the
>> caching of http redirects.
>> We planned this as a workaround to the fact there’s a limit on the
>> expiration that can be set on S3 pre-signed URLs.
>> The idea was to have a service that generates signed S3 URLs and
>> redirects to them, ffmpeg would hit this service
>> on every seek/disconnect, and get a fresh S3 URL.
>>
>> We found that in some cases, ffmpeg seeks on every frame (probably due to
>> some gap between the positions of
>> video/audio frames in the file). In these cases, completely disabling the
>> redirect caching becomes quite inefficient -
>> our S3 signing service gets called ~20 times/sec by a single encoding
>> task.
>>
>> The attached patch provides a more complete/correct implementation of
>> redirect caching – the decision whether
>> to cache/for how long is now determined according to the response from
>> the server - status code (e.g. 301 vs 302),
>> expires header & cache-control header. The behavior (detailed in the
>> comment of the commit) is more aligned
>> with how browsers handle it.
>>
>> In high level, these are the changes that were implemented in the patch –
>>
>>   1.  Added a dictionary on HTTPContext for keeping the cache – since I
>> need to save both the target URL
>> and the expiration, I’m formatting them together on a single string
>> (“expiry;target-url”)
>>   2.  Added a string on HTTPContext to save the value of the location
>> header – this makes the existing
>> flags new_location/location_changed redundant, and they were removed
>>   3.  Added functions for parsing Cache-Control/Expires – for Expires I
>> used the existing function for
>> parsing cookie expiration time. The result is saved on a new integer on
>> HTTPContent -
>> a zero value means that no such header was encountered yet, a negative
>> value means the response
>> should not be cached.
>> Once this patch is merged, IMHO we can remove the flag that I added in
>> the previous patch (=always treat it as 0,
>> and have the caching work based on the new dictionary). I will happily
>> submit another patch for this.
>>
>> Thank you!
>>
>
> Will merge this in 1-2 days if there's no further comments.
>

Merged.

Ronald