[FFmpeg-devel] [PATCH] avformat/hls: copy rw_timeout from parent to child AVIOContexts.

[Richard Shaffer]

On Tue, Apr 3, 2018 at 12:13 AM, Steven Liu <lq at chinaffmpeg.org> wrote:

>
>
> > On 3 Apr 2018, at 14:06, Richard Shaffer <rshaffer at tunein.com> wrote:
> >
> > On Mon, Apr 2, 2018 at 10:15 PM, Steven Liu <lq at chinaffmpeg.org> wrote:
> >>
> >>
> >>> On 3 Apr 2018, at 12:33, Richard Shaffer <rshaffer at tunein.com> wrote:
> >>>
> >>> On Mon, Apr 2, 2018 at 8:31 PM, Steven Liu <lq at chinaffmpeg.org> wrote:
> >>>>
> >>>>
> >>>>> On 3 Apr 2018, at 09:12, rshaffer at tunein.com wrote:
> >>>>>
> >>>>> From: Richard Shaffer <rshaffer at tunein.com>
> >>>>>
> >>>>> The rw_timeout option is currently not applied when opening media
> playlist,
> >>>>> segment, or encryption key URLs. This can cause the HLS demuxer to
> block
> >>>>> indefinitely, even when the rw_timeout option has been specified.
> This change
> >>>>> simply enables carrying over the rw_timeout option when the demuxer
> opens these
> >>>>> URLs.
> >>>>> ---
> >>>>> libavformat/hls.c | 2 +-
> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
> >>>>> index c578bf86e3..6663244ddf 100644
> >>>>> --- a/libavformat/hls.c
> >>>>> +++ b/libavformat/hls.c
> >>>>> @@ -1661,7 +1661,7 @@ static int save_avio_options(AVFormatContext
> *s)
> >>>>> {
> >>>>>   HLSContext *c = s->priv_data;
> >>>>>   static const char * const opts[] = {
> >>>>> -        "headers", "http_proxy", "user_agent", "user-agent",
> "cookies", "referer", NULL };
> >>>>> +        "headers", "http_proxy", "user_agent", "user-agent",
> "cookies", "referer", "rw_timeout", NULL };
> >>>> This table is used for http header.
> >>>> You could add the option into hls_options.
> >>>
> >>> Thanks for looking at the change. While the options currently in the
> >>> table are related to HTTP and rw_timeout is more general, I'm not
> >>> aware of a reason not to preserve the rw_timeout option here as well.
> >>> It seems unnecessary to define another HLS-specific option for
> >>> rw_timeout when the existing option exists and does what is intended.
> >>> I'm not sure whether you're objecting to the change and/or have a
> >>> different suggestion. Do you mind elaborating on your comment?
> >> Is the rw_timeout in to HTTP RFC? If yes, this is ok, If not, i think
> that is a ffmpeg option , not a http header content.
> >
> > http_proxy isn't tied to an HTTP header value, either. There isn't any
> > indication that avio_opts is intended for specifically HTTP options,
> > or that using it to store other avio options would break something. If
> > you have a reason why this shouldn't be used for other (non-HTTP)
> > options, can you please help me understand your thinking?
> >
> > I also want to spend some more time working on this, because I see
> > that there are multiple fields in HLSContext dealing with avio
> > options: There is the avio_opts field, but also referer, user_agent,
> > cookies, headers and http_proxy. avio_opts seems to be used when
> > opening segments and encryption key URIs, but the other fields are
> > used when reloading a playlist or parsing variant playlist URLs from a
> > master playlist. It's not clear why the options are stored in separate
> > places or whether that is necessary. If you have any insight into that
> > as well, it would be appreciated.
>
> Look at the code:
>
>  205     char *referer;                       ///< holds HTTP referer set
> as an AVOption to the HTTP protocol context
>  206     char *user_agent;                    ///< holds HTTP user agent
> set as an AVOption to the HTTP protocol context
>  207     char *cookies;                       ///< holds HTTP cookie
> values set in either the initial response or as an AVOption to the HTTP
> protocol context
>  208     char *headers;                       ///< holds HTTP headers set
> as an AVOption to the HTTP protocol context
>  209     char *http_proxy;                    ///< holds the address of
> the HTTP proxy server
>
> There have some comment for the options.
> and reference the code in: hls_read_header / open_input and use the
> options.
>
>
That's pretty clear. What I was asking is why the options are stored both
in these fields as well as avio_opts, and this doesn't answer my question.
I was also asking why you object to storing the timeout option in
avio_opts, but I'm not understanding the logic in your responses.


> >
> >>>
> >>>>>   const char * const * opt = opts;
> >>>>>   uint8_t *buf;
> >>>>>   int ret = 0;
> >>>>> --
> >>>>> 2.15.1 (Apple Git-101)
> >>>>>
> >>>>> _______________________________________________
> >>>>> ffmpeg-devel mailing list
> >>>>> ffmpeg-devel at ffmpeg.org
> >>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>
> >>>> Thanks
> >>>> Steven
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>> _______________________________________________
> >>> ffmpeg-devel mailing list
> >>> ffmpeg-devel at ffmpeg.org
> >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >> Thanks
> >> Steven
> >>
> >>
> >>
> >>
> >>
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel at ffmpeg.org
> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> Thanks
> Steven
>
>
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>