diff mbox

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

Message ID 20180403011246.30530-1-rshaffer@tunein.com
State Accepted
Commit 6a1be7561c870a8cd3cee86a57aabdffb19e3870
Headers show

Commit Message

rshaffer@tunein.com April 3, 2018, 1:12 a.m. UTC
From: Richard Shaffer <rshaffer@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(-)

Comments

Steven Liu April 3, 2018, 3:31 a.m. UTC | #1
> On 3 Apr 2018, at 09:12, rshaffer@tunein.com wrote:
> 
> From: Richard Shaffer <rshaffer@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.
>     const char * const * opt = opts;
>     uint8_t *buf;
>     int ret = 0;
> -- 
> 2.15.1 (Apple Git-101)
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Thanks
Steven
rshaffer@tunein.com April 3, 2018, 4:33 a.m. UTC | #2
On Mon, Apr 2, 2018 at 8:31 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
>
>
>> On 3 Apr 2018, at 09:12, rshaffer@tunein.com wrote:
>>
>> From: Richard Shaffer <rshaffer@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?

>>     const char * const * opt = opts;
>>     uint8_t *buf;
>>     int ret = 0;
>> --
>> 2.15.1 (Apple Git-101)
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> Thanks
> Steven
>
>
>
>
>
Steven Liu April 3, 2018, 5:15 a.m. UTC | #3
> On 3 Apr 2018, at 12:33, Richard Shaffer <rshaffer@tunein.com> wrote:
> 
> On Mon, Apr 2, 2018 at 8:31 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
>> 
>> 
>>> On 3 Apr 2018, at 09:12, rshaffer@tunein.com wrote:
>>> 
>>> From: Richard Shaffer <rshaffer@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.
> 
>>>    const char * const * opt = opts;
>>>    uint8_t *buf;
>>>    int ret = 0;
>>> --
>>> 2.15.1 (Apple Git-101)
>>> 
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> Thanks
>> Steven
>> 
>> 
>> 
>> 
>> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Thanks
Steven
rshaffer@tunein.com April 3, 2018, 6:06 a.m. UTC | #4
On Mon, Apr 2, 2018 at 10:15 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
>
>
>> On 3 Apr 2018, at 12:33, Richard Shaffer <rshaffer@tunein.com> wrote:
>>
>> On Mon, Apr 2, 2018 at 8:31 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
>>>
>>>
>>>> On 3 Apr 2018, at 09:12, rshaffer@tunein.com wrote:
>>>>
>>>> From: Richard Shaffer <rshaffer@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.

>>
>>>>    const char * const * opt = opts;
>>>>    uint8_t *buf;
>>>>    int ret = 0;
>>>> --
>>>> 2.15.1 (Apple Git-101)
>>>>
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> Thanks
>>> Steven
>>>
>>>
>>>
>>>
>>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> Thanks
> Steven
>
>
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Steven Liu April 3, 2018, 7:13 a.m. UTC | #5
> On 3 Apr 2018, at 14:06, Richard Shaffer <rshaffer@tunein.com> wrote:
> 
> On Mon, Apr 2, 2018 at 10:15 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
>> 
>> 
>>> On 3 Apr 2018, at 12:33, Richard Shaffer <rshaffer@tunein.com> wrote:
>>> 
>>> On Mon, Apr 2, 2018 at 8:31 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
>>>> 
>>>> 
>>>>> On 3 Apr 2018, at 09:12, rshaffer@tunein.com wrote:
>>>>> 
>>>>> From: Richard Shaffer <rshaffer@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.


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

Thanks
Steven
rshaffer@tunein.com April 3, 2018, 4:46 p.m. UTC | #6
On Tue, Apr 3, 2018 at 12:13 AM, Steven Liu <lq@chinaffmpeg.org> wrote:

>
>
> > On 3 Apr 2018, at 14:06, Richard Shaffer <rshaffer@tunein.com> wrote:
> >
> > On Mon, Apr 2, 2018 at 10:15 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
> >>
> >>
> >>> On 3 Apr 2018, at 12:33, Richard Shaffer <rshaffer@tunein.com> wrote:
> >>>
> >>> On Mon, Apr 2, 2018 at 8:31 PM, Steven Liu <lq@chinaffmpeg.org> wrote:
> >>>>
> >>>>
> >>>>> On 3 Apr 2018, at 09:12, rshaffer@tunein.com wrote:
> >>>>>
> >>>>> From: Richard Shaffer <rshaffer@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@ffmpeg.org
> >>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>
> >>>> Thanks
> >>>> Steven
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>> _______________________________________________
> >>> ffmpeg-devel mailing list
> >>> ffmpeg-devel@ffmpeg.org
> >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >> Thanks
> >> Steven
> >>
> >>
> >>
> >>
> >>
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel@ffmpeg.org
> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> Thanks
> Steven
>
>
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Steven Liu April 3, 2018, 11:53 p.m. UTC | #7
>>
>> 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.

no logic problem, just that option be used to HTTP, is that ok?

BTW, how should i reproduce the problem you said?

"
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.
"

>
>
Steven Liu April 4, 2018, 12:11 a.m. UTC | #8
2018-04-04 7:53 GMT+08:00 Steven Liu <lingjiujianke@gmail.com>:
>>>
>>> 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.
>
> no logic problem, just that option be used to HTTP, is that ok?
>
> BTW, how should i reproduce the problem you said?
>
> "
> 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.
> "

So i think add option timeout to do it maybe better than this way. you
can find another formats do it like this.
>
>>
>>
rshaffer@tunein.com April 10, 2018, 7:14 p.m. UTC | #9
On Tue, Apr 3, 2018 at 5:11 PM, Steven Liu <lingjiujianke@gmail.com> wrote:

> 2018-04-04 7:53 GMT+08:00 Steven Liu <lingjiujianke@gmail.com>:
> >>>
> >>> 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.
> >
> > no logic problem, just that option be used to HTTP, is that ok?
>

This is a logic problem for me, because I'm not understanding your logic.
What is the reason that avio_opts should only be used for HTTP options?


> >
> > BTW, how should i reproduce the problem you said?
>

If you pass the rw_timeout option to either the command-line or to
avformat_open_input, it will be applied only to the URL passed on the
command line or to the function. When the HLS demuxer tries to open other
URLs, such as keying URIs, media playlists or segments, it will not apply
the rw_timeout option. As a result, if we fail to receive data from the
remote server, it can block the process instead of returning the expected
AVERROR(ETIMEDOUT) error.


> >
> > "
> > 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.
> > "
>
> So i think add option timeout to do it maybe better than this way. you
> can find another formats do it like this.
>

I think the HLS demuxer is pretty unique. Except for dash, I don't know
which other demuxers have to open additional avio to do their work. If
there is an example you can show me that illustrates your point, that would
be appreciated.


> >
> >>
> >>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Hi Steven,

I apologize for the late response. I have been traveling.

I understand that you might be busy, and that English is not everyone's
first language. However, if you could take the time to give a more thorough
response, I'd really appreciate it. It's a little bit frustrating for me:
I'm asking why avio_opts needs to be restricted to HTTP options, and the
answer I seem to keep getting back is "because it's for HTTP options." That
answer isn't very satisfying, and doesn't help me understand /why/ you
think it should only be used for HTTP options. It may be that you have a
good reason, but if I can't understand what it is, I'm not going to be able
to provide a better fix.

I can't force anyone to accept my changes or provide more detailed
feedback. However, if we can't understand each other, then at some point I
will have to give up. I'll apply a patch that fixes my problem to a local
fork of ffmpeg. Other people outside of my company won't benefit from the
fix. My company will also have to maintain our own copy of ffmpeg. This
could make it less likely for us to share our changes with the community,
and it will also make it harder for us to benefit from improvements and
fixes to ffmpeg. I don't want to do that, because nobody wins in that
scenario. That is why I'm asking for your help to understand your objection
and suggestion.

Thanks,

-Richard
Steven Liu April 10, 2018, 11 p.m. UTC | #10
> 在 2018年4月11日,上午3:14,Richard Shaffer <rshaffer@tunein.com> 写道:
> 
> On Tue, Apr 3, 2018 at 5:11 PM, Steven Liu <lingjiujianke@gmail.com> wrote:
> 
>> 2018-04-04 7:53 GMT+08:00 Steven Liu <lingjiujianke@gmail.com>:
>>>>> 
>>>>> 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.
>>> 
>>> no logic problem, just that option be used to HTTP, is that ok?
>> 
> 
> This is a logic problem for me, because I'm not understanding your logic.
> What is the reason that avio_opts should only be used for HTTP options?
> 
> 
>>> 
>>> BTW, how should i reproduce the problem you said?
>> 
> 
> If you pass the rw_timeout option to either the command-line or to
> avformat_open_input, it will be applied only to the URL passed on the
> command line or to the function. When the HLS demuxer tries to open other
> URLs, such as keying URIs, media playlists or segments, it will not apply
> the rw_timeout option. As a result, if we fail to receive data from the
> remote server, it can block the process instead of returning the expected
> AVERROR(ETIMEDOUT) error.
> 
> 
>>> 
>>> "
>>> 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.
>>> "
>> 
>> So i think add option timeout to do it maybe better than this way. you
>> can find another formats do it like this.
>> 
> 
> I think the HLS demuxer is pretty unique. Except for dash, I don't know
> which other demuxers have to open additional avio to do their work. If
> there is an example you can show me that illustrates your point, that would
> be appreciated.
> 
> 
>>> 
>>>> 
>>>> 
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
> 
> Hi Steven,
> 
> I apologize for the late response. I have been traveling.
> 
> I understand that you might be busy, and that English is not everyone's
> first language. However, if you could take the time to give a more thorough
> response, I'd really appreciate it. It's a little bit frustrating for me:
> I'm asking why avio_opts needs to be restricted to HTTP options, and the
> answer I seem to keep getting back is "because it's for HTTP options." That
> answer isn't very satisfying, and doesn't help me understand /why/ you
> think it should only be used for HTTP options. It may be that you have a
> good reason, but if I can't understand what it is, I'm not going to be able
> to provide a better fix.
> 
> I can't force anyone to accept my changes or provide more detailed
> feedback. However, if we can't understand each other, then at some point I
> will have to give up. I'll apply a patch that fixes my problem to a local
> fork of ffmpeg. Other people outside of my company won't benefit from the
> fix. My company will also have to maintain our own copy of ffmpeg. This
> could make it less likely for us to share our changes with the community,
> and it will also make it harder for us to benefit from improvements and
> fixes to ffmpeg. I don't want to do that, because nobody wins in that
> scenario. That is why I'm asking for your help to understand your objection
> and suggestion.

Sorry for my poor English, English is not my first language, thanks, you can contribute.
After the last patch you submit(https://patchwork.ffmpeg.org/patch/8378/), i think i can understand this patch.


> 
> Thanks,
> 
> -Richard
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
rshaffer@tunein.com April 10, 2018, 11:13 p.m. UTC | #11
On Tue, Apr 10, 2018 at 4:00 PM, Liu Steven <lq@chinaffmpeg.org> wrote:

>
> > 在 2018年4月11日,上午3:14,Richard Shaffer <rshaffer@tunein.com> 写道:
> >
> > On Tue, Apr 3, 2018 at 5:11 PM, Steven Liu <lingjiujianke@gmail.com>
> wrote:
> >
> >> 2018-04-04 7:53 GMT+08:00 Steven Liu <lingjiujianke@gmail.com>:
> >>>>>
> >>>>> 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.
> >>>
> >>> no logic problem, just that option be used to HTTP, is that ok?
> >>
> >
> > This is a logic problem for me, because I'm not understanding your logic.
> > What is the reason that avio_opts should only be used for HTTP options?
> >
> >
> >>>
> >>> BTW, how should i reproduce the problem you said?
> >>
> >
> > If you pass the rw_timeout option to either the command-line or to
> > avformat_open_input, it will be applied only to the URL passed on the
> > command line or to the function. When the HLS demuxer tries to open other
> > URLs, such as keying URIs, media playlists or segments, it will not apply
> > the rw_timeout option. As a result, if we fail to receive data from the
> > remote server, it can block the process instead of returning the expected
> > AVERROR(ETIMEDOUT) error.
> >
> >
> >>>
> >>> "
> >>> 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.
> >>> "
> >>
> >> So i think add option timeout to do it maybe better than this way. you
> >> can find another formats do it like this.
> >>
> >
> > I think the HLS demuxer is pretty unique. Except for dash, I don't know
> > which other demuxers have to open additional avio to do their work. If
> > there is an example you can show me that illustrates your point, that
> would
> > be appreciated.
> >
> >
> >>>
> >>>>
> >>>>
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel@ffmpeg.org
> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >
> > Hi Steven,
> >
> > I apologize for the late response. I have been traveling.
> >
> > I understand that you might be busy, and that English is not everyone's
> > first language. However, if you could take the time to give a more
> thorough
> > response, I'd really appreciate it. It's a little bit frustrating for me:
> > I'm asking why avio_opts needs to be restricted to HTTP options, and the
> > answer I seem to keep getting back is "because it's for HTTP options."
> That
> > answer isn't very satisfying, and doesn't help me understand /why/ you
> > think it should only be used for HTTP options. It may be that you have a
> > good reason, but if I can't understand what it is, I'm not going to be
> able
> > to provide a better fix.
> >
> > I can't force anyone to accept my changes or provide more detailed
> > feedback. However, if we can't understand each other, then at some point
> I
> > will have to give up. I'll apply a patch that fixes my problem to a local
> > fork of ffmpeg. Other people outside of my company won't benefit from the
> > fix. My company will also have to maintain our own copy of ffmpeg. This
> > could make it less likely for us to share our changes with the community,
> > and it will also make it harder for us to benefit from improvements and
> > fixes to ffmpeg. I don't want to do that, because nobody wins in that
> > scenario. That is why I'm asking for your help to understand your
> objection
> > and suggestion.
>
> Sorry for my poor English, English is not my first language, thanks, you
> can contribute.
> After the last patch you submit(https://patchwork.ffmpeg.org/patch/8378/),
> i think i can understand this patch.
>
> Thanks, Steven. I will wait a few days. If there are no additional
comments, I will ask you or another maintainer to apply the patches.

>
> >
> > Thanks,
> >
> > -Richard
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Steven Liu April 17, 2018, 6:43 a.m. UTC | #12
> On 11 Apr 2018, at 07:13, Richard Shaffer <rshaffer@tunein.com> wrote:
> 
> 
> 
> On Tue, Apr 10, 2018 at 4:00 PM, Liu Steven <lq@chinaffmpeg.org> wrote:
> 
> > 在 2018年4月11日,上午3:14,Richard Shaffer <rshaffer@tunein.com> 写道:
> >
> > On Tue, Apr 3, 2018 at 5:11 PM, Steven Liu <lingjiujianke@gmail.com> wrote:
> >
> >> 2018-04-04 7:53 GMT+08:00 Steven Liu <lingjiujianke@gmail.com>:
> >>>>>
> >>>>> 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.
> >>>
> >>> no logic problem, just that option be used to HTTP, is that ok?
> >>
> >
> > This is a logic problem for me, because I'm not understanding your logic.
> > What is the reason that avio_opts should only be used for HTTP options?
> >
> >
> >>>
> >>> BTW, how should i reproduce the problem you said?
> >>
> >
> > If you pass the rw_timeout option to either the command-line or to
> > avformat_open_input, it will be applied only to the URL passed on the
> > command line or to the function. When the HLS demuxer tries to open other
> > URLs, such as keying URIs, media playlists or segments, it will not apply
> > the rw_timeout option. As a result, if we fail to receive data from the
> > remote server, it can block the process instead of returning the expected
> > AVERROR(ETIMEDOUT) error.
> >
> >
> >>>
> >>> "
> >>> 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.
> >>> "
> >>
> >> So i think add option timeout to do it maybe better than this way. you
> >> can find another formats do it like this.
> >>
> >
> > I think the HLS demuxer is pretty unique. Except for dash, I don't know
> > which other demuxers have to open additional avio to do their work. If
> > there is an example you can show me that illustrates your point, that would
> > be appreciated.
> >
> >
> >>>
> >>>>
> >>>>
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel@ffmpeg.org
> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >
> > Hi Steven,
> >
> > I apologize for the late response. I have been traveling.
> >
> > I understand that you might be busy, and that English is not everyone's
> > first language. However, if you could take the time to give a more thorough
> > response, I'd really appreciate it. It's a little bit frustrating for me:
> > I'm asking why avio_opts needs to be restricted to HTTP options, and the
> > answer I seem to keep getting back is "because it's for HTTP options." That
> > answer isn't very satisfying, and doesn't help me understand /why/ you
> > think it should only be used for HTTP options. It may be that you have a
> > good reason, but if I can't understand what it is, I'm not going to be able
> > to provide a better fix.
> >
> > I can't force anyone to accept my changes or provide more detailed
> > feedback. However, if we can't understand each other, then at some point I
> > will have to give up. I'll apply a patch that fixes my problem to a local
> > fork of ffmpeg. Other people outside of my company won't benefit from the
> > fix. My company will also have to maintain our own copy of ffmpeg. This
> > could make it less likely for us to share our changes with the community,
> > and it will also make it harder for us to benefit from improvements and
> > fixes to ffmpeg. I don't want to do that, because nobody wins in that
> > scenario. That is why I'm asking for your help to understand your objection
> > and suggestion.
> 
> Sorry for my poor English, English is not my first language, thanks, you can contribute.
> After the last patch you submit(https://patchwork.ffmpeg.org/patch/8378/), i think i can understand this patch.
> 
> Thanks, Steven. I will wait a few days. If there are no additional comments, I will ask you or another maintainer to apply the patches.
pushed
> 
> >
> > Thanks,
> >
> > -Richard
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Thanks
Steven
diff mbox

Patch

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 };
     const char * const * opt = opts;
     uint8_t *buf;
     int ret = 0;