Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-30 Thread Micah Galizia 

On 2017-05-28 08:44 PM, Michael Niedermayer wrote:

On Fri, May 26, 2017 at 09:29:04PM -0400, Micah Galizia wrote:

On Sat, May 20, 2017 at 9:36 PM, Micah Galizia  wrote:

On 2017-05-17 05:23 AM, wm4 wrote:

On Sat, 6 May 2017 14:28:10 -0400
Micah Galizia  wrote:


On 2017-05-05 09:28 PM, wm4 wrote:

On Fri,  5 May 2017 20:55:05 -0400
Micah Galizia  wrote:


Signed-off-by: Micah Galizia 
---
libavformat/hls.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/libavformat/hls.c b/libavformat/hls.c
index bac53a4350..bda9abecfa 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -630,8 +630,16 @@ static int open_url(AVFormatContext *s,
AVIOContext **pb, const char *url,
ret = s->io_open(s, pb, url, AVIO_FLAG_READ, );
if (ret >= 0) {
// update cookies on http response with setcookies.
-void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
-update_options(>cookies, "cookies", u);
+char *new_cookies = NULL;
+
+if (s->flags ^ AVFMT_FLAG_CUSTOM_IO)
+av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN,
(uint8_t**)_cookies);

Did you mean & instead of ^?

No, the original code was structured to set *u to null (and thus did not
copy cookies) iff AVFMT_FLAG_CUSTOM_IO was set in the flags.  So using ^
is logically equivalent -- cookies are copied only if
AVFMT_FLAG_CUSTOM_IO is not set.


Did you find out yet what difference AVFMT_FLAG_CUSTOM_IO is supposed
to make in the existing code?

Yes, I wrote back about it a day or two ago... here is the reference to
its original inclusion:
https://lists.ffmpeg.org/pipermail/ffmpeg-trac/2013-February/013020.html.
When the code was originally implemented we were using s->pb->opaque,
which in FFMPEG we could assume was a URLContext with options (one of
them possibly being "cookies").

Based on the email above, that wasn't true for other apps like mplayer,
and could cause crashes trying to treat opaque as a URLContext. So that
is the purpose of checking the AVFMT_FLAG_CUSTOM_IO flag (way back in
2013).

Now though, we don't seem to touch opaque at all. The options are stored
in AVIOContext's av_class, which does have options.  Based on this I
think both patches are safe: this version has less change, but the
original gets rid of the check against AVFMT_FLAG_CUSTOM_IO that I don't
think we need anymore.

I hope that clears things up.

Sorry, I missed that. I guess this code is an artifact of some severely
unclean hook up of the AVOPtion API to AVIOContext, that breaks with
custom I/O. I guess your patch is fine then.

I just wonder how an API user is supposed to pass along cookies then.
My code passes an AVDictionary via a "cookies" entry when opening the
HLS demuxer with custom I/O, so I was wondering whether the patch
changes behavior here.


I wouldn't have expected anyone to pass the cookies to the HLS demuxer
directly -- there is an http protocol AVOption that should pass it along to
the demuxer. But I guess thats the whole point of the custom IO, right? It'd
replace the http demuxer?

Even so, I don't think this is a good reason to hold up the the patch - it
corrects the problem for apps that use the http protocol and maintains the
existing behavior -- cookies are not (and were not) copied when the custom
flag is set because u was set to null. Am I wrong in that interpretation of
the existing behavior?

Hi,

What else do I need to do to get this fix checked in?

patch applied

thx


TYVM!
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-28 Thread Michael Niedermayer 
On Fri, May 26, 2017 at 09:29:04PM -0400, Micah Galizia wrote:
> On Sat, May 20, 2017 at 9:36 PM, Micah Galizia  wrote:
> > On 2017-05-17 05:23 AM, wm4 wrote:
> >>
> >> On Sat, 6 May 2017 14:28:10 -0400
> >> Micah Galizia  wrote:
> >>
> >>> On 2017-05-05 09:28 PM, wm4 wrote:
> 
>  On Fri,  5 May 2017 20:55:05 -0400
>  Micah Galizia  wrote:
> 
> >
> > Signed-off-by: Micah Galizia 
> > ---
> >libavformat/hls.c | 12 ++--
> >1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/hls.c b/libavformat/hls.c
> > index bac53a4350..bda9abecfa 100644
> > --- a/libavformat/hls.c
> > +++ b/libavformat/hls.c
> > @@ -630,8 +630,16 @@ static int open_url(AVFormatContext *s,
> > AVIOContext **pb, const char *url,
> >ret = s->io_open(s, pb, url, AVIO_FLAG_READ, );
> >if (ret >= 0) {
> >// update cookies on http response with setcookies.
> > -void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
> > -update_options(>cookies, "cookies", u);
> > +char *new_cookies = NULL;
> > +
> > +if (s->flags ^ AVFMT_FLAG_CUSTOM_IO)
> > +av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN,
> > (uint8_t**)_cookies);
> 
>  Did you mean & instead of ^?
> >>>
> >>> No, the original code was structured to set *u to null (and thus did not
> >>> copy cookies) iff AVFMT_FLAG_CUSTOM_IO was set in the flags.  So using ^
> >>> is logically equivalent -- cookies are copied only if
> >>> AVFMT_FLAG_CUSTOM_IO is not set.
> >>>
>  Did you find out yet what difference AVFMT_FLAG_CUSTOM_IO is supposed
>  to make in the existing code?
> >>>
> >>> Yes, I wrote back about it a day or two ago... here is the reference to
> >>> its original inclusion:
> >>> https://lists.ffmpeg.org/pipermail/ffmpeg-trac/2013-February/013020.html.
> >>> When the code was originally implemented we were using s->pb->opaque,
> >>> which in FFMPEG we could assume was a URLContext with options (one of
> >>> them possibly being "cookies").
> >>>
> >>> Based on the email above, that wasn't true for other apps like mplayer,
> >>> and could cause crashes trying to treat opaque as a URLContext. So that
> >>> is the purpose of checking the AVFMT_FLAG_CUSTOM_IO flag (way back in
> >>> 2013).
> >>>
> >>> Now though, we don't seem to touch opaque at all. The options are stored
> >>> in AVIOContext's av_class, which does have options.  Based on this I
> >>> think both patches are safe: this version has less change, but the
> >>> original gets rid of the check against AVFMT_FLAG_CUSTOM_IO that I don't
> >>> think we need anymore.
> >>>
> >>> I hope that clears things up.
> >>
> >> Sorry, I missed that. I guess this code is an artifact of some severely
> >> unclean hook up of the AVOPtion API to AVIOContext, that breaks with
> >> custom I/O. I guess your patch is fine then.
> >>
> >> I just wonder how an API user is supposed to pass along cookies then.
> >> My code passes an AVDictionary via a "cookies" entry when opening the
> >> HLS demuxer with custom I/O, so I was wondering whether the patch
> >> changes behavior here.
> >
> >
> > I wouldn't have expected anyone to pass the cookies to the HLS demuxer
> > directly -- there is an http protocol AVOption that should pass it along to
> > the demuxer. But I guess thats the whole point of the custom IO, right? It'd
> > replace the http demuxer?
> >
> > Even so, I don't think this is a good reason to hold up the the patch - it
> > corrects the problem for apps that use the http protocol and maintains the
> > existing behavior -- cookies are not (and were not) copied when the custom
> > flag is set because u was set to null. Am I wrong in that interpretation of
> > the existing behavior?
> 
> Hi,
> 
> What else do I need to do to get this fix checked in?

patch applied

thx

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-26 Thread Micah Galizia 
On Sat, May 20, 2017 at 9:36 PM, Micah Galizia  wrote:
> On 2017-05-17 05:23 AM, wm4 wrote:
>>
>> On Sat, 6 May 2017 14:28:10 -0400
>> Micah Galizia  wrote:
>>
>>> On 2017-05-05 09:28 PM, wm4 wrote:

 On Fri,  5 May 2017 20:55:05 -0400
 Micah Galizia  wrote:

>
> Signed-off-by: Micah Galizia 
> ---
>libavformat/hls.c | 12 ++--
>1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index bac53a4350..bda9abecfa 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -630,8 +630,16 @@ static int open_url(AVFormatContext *s,
> AVIOContext **pb, const char *url,
>ret = s->io_open(s, pb, url, AVIO_FLAG_READ, );
>if (ret >= 0) {
>// update cookies on http response with setcookies.
> -void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
> -update_options(>cookies, "cookies", u);
> +char *new_cookies = NULL;
> +
> +if (s->flags ^ AVFMT_FLAG_CUSTOM_IO)
> +av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN,
> (uint8_t**)_cookies);

 Did you mean & instead of ^?
>>>
>>> No, the original code was structured to set *u to null (and thus did not
>>> copy cookies) iff AVFMT_FLAG_CUSTOM_IO was set in the flags.  So using ^
>>> is logically equivalent -- cookies are copied only if
>>> AVFMT_FLAG_CUSTOM_IO is not set.
>>>
 Did you find out yet what difference AVFMT_FLAG_CUSTOM_IO is supposed
 to make in the existing code?
>>>
>>> Yes, I wrote back about it a day or two ago... here is the reference to
>>> its original inclusion:
>>> https://lists.ffmpeg.org/pipermail/ffmpeg-trac/2013-February/013020.html.
>>> When the code was originally implemented we were using s->pb->opaque,
>>> which in FFMPEG we could assume was a URLContext with options (one of
>>> them possibly being "cookies").
>>>
>>> Based on the email above, that wasn't true for other apps like mplayer,
>>> and could cause crashes trying to treat opaque as a URLContext. So that
>>> is the purpose of checking the AVFMT_FLAG_CUSTOM_IO flag (way back in
>>> 2013).
>>>
>>> Now though, we don't seem to touch opaque at all. The options are stored
>>> in AVIOContext's av_class, which does have options.  Based on this I
>>> think both patches are safe: this version has less change, but the
>>> original gets rid of the check against AVFMT_FLAG_CUSTOM_IO that I don't
>>> think we need anymore.
>>>
>>> I hope that clears things up.
>>
>> Sorry, I missed that. I guess this code is an artifact of some severely
>> unclean hook up of the AVOPtion API to AVIOContext, that breaks with
>> custom I/O. I guess your patch is fine then.
>>
>> I just wonder how an API user is supposed to pass along cookies then.
>> My code passes an AVDictionary via a "cookies" entry when opening the
>> HLS demuxer with custom I/O, so I was wondering whether the patch
>> changes behavior here.
>
>
> I wouldn't have expected anyone to pass the cookies to the HLS demuxer
> directly -- there is an http protocol AVOption that should pass it along to
> the demuxer. But I guess thats the whole point of the custom IO, right? It'd
> replace the http demuxer?
>
> Even so, I don't think this is a good reason to hold up the the patch - it
> corrects the problem for apps that use the http protocol and maintains the
> existing behavior -- cookies are not (and were not) copied when the custom
> flag is set because u was set to null. Am I wrong in that interpretation of
> the existing behavior?

Hi,

What else do I need to do to get this fix checked in?

Thanks
-- 
"The mark of an immature man is that he wants to die nobly for a
cause, while the mark of the mature man is that he wants to live
humbly for one."   --W. Stekel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-20 Thread Micah Galizia 

On 2017-05-17 05:23 AM, wm4 wrote:

On Sat, 6 May 2017 14:28:10 -0400
Micah Galizia  wrote:


On 2017-05-05 09:28 PM, wm4 wrote:

On Fri,  5 May 2017 20:55:05 -0400
Micah Galizia  wrote:
  

Signed-off-by: Micah Galizia 
---
   libavformat/hls.c | 12 ++--
   1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/libavformat/hls.c b/libavformat/hls.c
index bac53a4350..bda9abecfa 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -630,8 +630,16 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, 
const char *url,
   ret = s->io_open(s, pb, url, AVIO_FLAG_READ, );
   if (ret >= 0) {
   // update cookies on http response with setcookies.
-void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
-update_options(>cookies, "cookies", u);
+char *new_cookies = NULL;
+
+if (s->flags ^ AVFMT_FLAG_CUSTOM_IO)
+av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN, 
(uint8_t**)_cookies);

Did you mean & instead of ^?

No, the original code was structured to set *u to null (and thus did not
copy cookies) iff AVFMT_FLAG_CUSTOM_IO was set in the flags.  So using ^
is logically equivalent -- cookies are copied only if
AVFMT_FLAG_CUSTOM_IO is not set.


Did you find out yet what difference AVFMT_FLAG_CUSTOM_IO is supposed
to make in the existing code?

Yes, I wrote back about it a day or two ago... here is the reference to
its original inclusion:
https://lists.ffmpeg.org/pipermail/ffmpeg-trac/2013-February/013020.html.
When the code was originally implemented we were using s->pb->opaque,
which in FFMPEG we could assume was a URLContext with options (one of
them possibly being "cookies").

Based on the email above, that wasn't true for other apps like mplayer,
and could cause crashes trying to treat opaque as a URLContext. So that
is the purpose of checking the AVFMT_FLAG_CUSTOM_IO flag (way back in 2013).

Now though, we don't seem to touch opaque at all. The options are stored
in AVIOContext's av_class, which does have options.  Based on this I
think both patches are safe: this version has less change, but the
original gets rid of the check against AVFMT_FLAG_CUSTOM_IO that I don't
think we need anymore.

I hope that clears things up.

Sorry, I missed that. I guess this code is an artifact of some severely
unclean hook up of the AVOPtion API to AVIOContext, that breaks with
custom I/O. I guess your patch is fine then.

I just wonder how an API user is supposed to pass along cookies then.
My code passes an AVDictionary via a "cookies" entry when opening the
HLS demuxer with custom I/O, so I was wondering whether the patch
changes behavior here.


I wouldn't have expected anyone to pass the cookies to the HLS demuxer 
directly -- there is an http protocol AVOption that should pass it along 
to the demuxer. But I guess thats the whole point of the custom IO, 
right? It'd replace the http demuxer?


Even so, I don't think this is a good reason to hold up the the patch - 
it corrects the problem for apps that use the http protocol and 
maintains the existing behavior -- cookies are not (and were not) copied 
when the custom flag is set because u was set to null. Am I wrong in 
that interpretation of the existing behavior?


Thanks,
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-17 Thread wm4 
On Sat, 6 May 2017 14:28:10 -0400
Micah Galizia  wrote:

> On 2017-05-05 09:28 PM, wm4 wrote:
> > On Fri,  5 May 2017 20:55:05 -0400
> > Micah Galizia  wrote:
> >  
> >> Signed-off-by: Micah Galizia 
> >> ---
> >>   libavformat/hls.c | 12 ++--
> >>   1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavformat/hls.c b/libavformat/hls.c
> >> index bac53a4350..bda9abecfa 100644
> >> --- a/libavformat/hls.c
> >> +++ b/libavformat/hls.c
> >> @@ -630,8 +630,16 @@ static int open_url(AVFormatContext *s, AVIOContext 
> >> **pb, const char *url,
> >>   ret = s->io_open(s, pb, url, AVIO_FLAG_READ, );
> >>   if (ret >= 0) {
> >>   // update cookies on http response with setcookies.
> >> -void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
> >> -update_options(>cookies, "cookies", u);
> >> +char *new_cookies = NULL;
> >> +
> >> +if (s->flags ^ AVFMT_FLAG_CUSTOM_IO)
> >> +av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN, 
> >> (uint8_t**)_cookies);  
> > Did you mean & instead of ^?  
> 
> No, the original code was structured to set *u to null (and thus did not 
> copy cookies) iff AVFMT_FLAG_CUSTOM_IO was set in the flags.  So using ^ 
> is logically equivalent -- cookies are copied only if 
> AVFMT_FLAG_CUSTOM_IO is not set.
> 
> > Did you find out yet what difference AVFMT_FLAG_CUSTOM_IO is supposed
> > to make in the existing code?  
> Yes, I wrote back about it a day or two ago... here is the reference to 
> its original inclusion: 
> https://lists.ffmpeg.org/pipermail/ffmpeg-trac/2013-February/013020.html. 
> When the code was originally implemented we were using s->pb->opaque, 
> which in FFMPEG we could assume was a URLContext with options (one of 
> them possibly being "cookies").
> 
> Based on the email above, that wasn't true for other apps like mplayer, 
> and could cause crashes trying to treat opaque as a URLContext. So that 
> is the purpose of checking the AVFMT_FLAG_CUSTOM_IO flag (way back in 2013).
> 
> Now though, we don't seem to touch opaque at all. The options are stored 
> in AVIOContext's av_class, which does have options.  Based on this I 
> think both patches are safe: this version has less change, but the 
> original gets rid of the check against AVFMT_FLAG_CUSTOM_IO that I don't 
> think we need anymore.
> 
> I hope that clears things up.

Sorry, I missed that. I guess this code is an artifact of some severely
unclean hook up of the AVOPtion API to AVIOContext, that breaks with
custom I/O. I guess your patch is fine then.

I just wonder how an API user is supposed to pass along cookies then.
My code passes an AVDictionary via a "cookies" entry when opening the
HLS demuxer with custom I/O, so I was wondering whether the patch
changes behavior here.

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

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-16 Thread Micah Galizia 

On 2017-05-16 04:57 PM, Michael Niedermayer wrote:

On Sat, May 06, 2017 at 02:28:10PM -0400, Micah Galizia wrote:

On 2017-05-05 09:28 PM, wm4 wrote:


Did you mean & instead of ^?

No, the original code was structured to set *u to null (and thus did
not copy cookies) iff AVFMT_FLAG_CUSTOM_IO was set in the flags.  So
using ^ is logically equivalent -- cookies are copied only if
AVFMT_FLAG_CUSTOM_IO is not set.

it would also copy if another flag is set, is that intended ?


... I think that is what wm4 was getting at too. That is wrong and not 
intended. I'll resubmit with !(a) which is the proper logic.


Thanks,
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-16 Thread Michael Niedermayer 
On Sat, May 06, 2017 at 02:28:10PM -0400, Micah Galizia wrote:
> On 2017-05-05 09:28 PM, wm4 wrote:
> >On Fri,  5 May 2017 20:55:05 -0400
> >Micah Galizia  wrote:
> >
> >>Signed-off-by: Micah Galizia 
> >>---
> >>  libavformat/hls.c | 12 ++--
> >>  1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/libavformat/hls.c b/libavformat/hls.c
> >>index bac53a4350..bda9abecfa 100644
> >>--- a/libavformat/hls.c
> >>+++ b/libavformat/hls.c
> >>@@ -630,8 +630,16 @@ static int open_url(AVFormatContext *s, AVIOContext 
> >>**pb, const char *url,
> >>  ret = s->io_open(s, pb, url, AVIO_FLAG_READ, );
> >>  if (ret >= 0) {
> >>  // update cookies on http response with setcookies.
> >>-void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
> >>-update_options(>cookies, "cookies", u);
> >>+char *new_cookies = NULL;
> >>+
> >>+if (s->flags ^ AVFMT_FLAG_CUSTOM_IO)
> >>+av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN, 
> >>(uint8_t**)_cookies);
> >Did you mean & instead of ^?
> 
> No, the original code was structured to set *u to null (and thus did
> not copy cookies) iff AVFMT_FLAG_CUSTOM_IO was set in the flags.  So
> using ^ is logically equivalent -- cookies are copied only if
> AVFMT_FLAG_CUSTOM_IO is not set.

it would also copy if another flag is set, is that intended ?


[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-14 Thread Micah Galizia 

On 2017-05-06 02:28 PM, Micah Galizia wrote:

On 2017-05-05 09:28 PM, wm4 wrote:

On Fri,  5 May 2017 20:55:05 -0400
Micah Galizia  wrote:


Signed-off-by: Micah Galizia 
---
  libavformat/hls.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/libavformat/hls.c b/libavformat/hls.c
index bac53a4350..bda9abecfa 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -630,8 +630,16 @@ static int open_url(AVFormatContext *s, 
AVIOContext **pb, const char *url,

  ret = s->io_open(s, pb, url, AVIO_FLAG_READ, );
  if (ret >= 0) {
  // update cookies on http response with setcookies.
-void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
-update_options(>cookies, "cookies", u);
+char *new_cookies = NULL;
+
+if (s->flags ^ AVFMT_FLAG_CUSTOM_IO)
+av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN, 
(uint8_t**)_cookies);

Did you mean & instead of ^?


No, the original code was structured to set *u to null (and thus did 
not copy cookies) iff AVFMT_FLAG_CUSTOM_IO was set in the flags.  So 
using ^ is logically equivalent -- cookies are copied only if 
AVFMT_FLAG_CUSTOM_IO is not set.



Did you find out yet what difference AVFMT_FLAG_CUSTOM_IO is supposed
to make in the existing code?
Yes, I wrote back about it a day or two ago... here is the reference 
to its original inclusion: 
https://lists.ffmpeg.org/pipermail/ffmpeg-trac/2013-February/013020.html. 
When the code was originally implemented we were using s->pb->opaque, 
which in FFMPEG we could assume was a URLContext with options (one of 
them possibly being "cookies").


Based on the email above, that wasn't true for other apps like 
mplayer, and could cause crashes trying to treat opaque as a 
URLContext. So that is the purpose of checking the 
AVFMT_FLAG_CUSTOM_IO flag (way back in 2013).


Now though, we don't seem to touch opaque at all. The options are 
stored in AVIOContext's av_class, which does have options.  Based on 
this I think both patches are safe: this version has less change, but 
the original gets rid of the check against AVFMT_FLAG_CUSTOM_IO that I 
don't think we need anymore.


I hope that clears things up.


Hi, is there anything further needed to get this fix committed?

Thanks,
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-06 Thread Micah Galizia 

On 2017-05-05 09:28 PM, wm4 wrote:

On Fri,  5 May 2017 20:55:05 -0400
Micah Galizia  wrote:


Signed-off-by: Micah Galizia 
---
  libavformat/hls.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/libavformat/hls.c b/libavformat/hls.c
index bac53a4350..bda9abecfa 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -630,8 +630,16 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, 
const char *url,
  ret = s->io_open(s, pb, url, AVIO_FLAG_READ, );
  if (ret >= 0) {
  // update cookies on http response with setcookies.
-void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
-update_options(>cookies, "cookies", u);
+char *new_cookies = NULL;
+
+if (s->flags ^ AVFMT_FLAG_CUSTOM_IO)
+av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN, 
(uint8_t**)_cookies);

Did you mean & instead of ^?


No, the original code was structured to set *u to null (and thus did not 
copy cookies) iff AVFMT_FLAG_CUSTOM_IO was set in the flags.  So using ^ 
is logically equivalent -- cookies are copied only if 
AVFMT_FLAG_CUSTOM_IO is not set.



Did you find out yet what difference AVFMT_FLAG_CUSTOM_IO is supposed
to make in the existing code?
Yes, I wrote back about it a day or two ago... here is the reference to 
its original inclusion: 
https://lists.ffmpeg.org/pipermail/ffmpeg-trac/2013-February/013020.html. 
When the code was originally implemented we were using s->pb->opaque, 
which in FFMPEG we could assume was a URLContext with options (one of 
them possibly being "cookies").


Based on the email above, that wasn't true for other apps like mplayer, 
and could cause crashes trying to treat opaque as a URLContext. So that 
is the purpose of checking the AVFMT_FLAG_CUSTOM_IO flag (way back in 2013).


Now though, we don't seem to touch opaque at all. The options are stored 
in AVIOContext's av_class, which does have options.  Based on this I 
think both patches are safe: this version has less change, but the 
original gets rid of the check against AVFMT_FLAG_CUSTOM_IO that I don't 
think we need anymore.


I hope that clears things up.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-05 Thread wm4 
On Fri,  5 May 2017 20:55:05 -0400
Micah Galizia  wrote:

> Signed-off-by: Micah Galizia 
> ---
>  libavformat/hls.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index bac53a4350..bda9abecfa 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -630,8 +630,16 @@ static int open_url(AVFormatContext *s, AVIOContext 
> **pb, const char *url,
>  ret = s->io_open(s, pb, url, AVIO_FLAG_READ, );
>  if (ret >= 0) {
>  // update cookies on http response with setcookies.
> -void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
> -update_options(>cookies, "cookies", u);
> +char *new_cookies = NULL;
> +
> +if (s->flags ^ AVFMT_FLAG_CUSTOM_IO)
> +av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN, 
> (uint8_t**)_cookies);

Did you mean & instead of ^?

Did you find out yet what difference AVFMT_FLAG_CUSTOM_IO is supposed
to make in the existing code?

> +
> +if (new_cookies) {
> +av_free(c->cookies);
> +c->cookies = new_cookies;
> +}
> +
>  av_dict_set(, "cookies", c->cookies, 0);
>  }
>  

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

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-05 Thread Micah Galizia 
Hi,

This is a simpler version of the other patch that still observes 
AVFMT_FLAG_CUSTOM_IO.

Thanks in advance.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-04 Thread Micah Galizia 

On 2017-05-02 09:04 PM, wm4 wrote:

On Tue,  2 May 2017 20:47:06 -0400
Micah Galizia  wrote:


Signed-off-by: Micah Galizia 
---
  libavformat/hls.c | 12 +---
  1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/libavformat/hls.c b/libavformat/hls.c
index bac53a4350..643d50e1da 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -630,8 +630,14 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, 
const char *url,
  ret = s->io_open(s, pb, url, AVIO_FLAG_READ, );
  if (ret >= 0) {
  // update cookies on http response with setcookies.
-void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
-update_options(>cookies, "cookies", u);
+char *new_cookies = NULL;
+
+av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN, 
(uint8_t**)_cookies);
+if (new_cookies) {
+av_free(c->cookies);
+c->cookies = new_cookies;
+}
+
  av_dict_set(, "cookies", c->cookies, 0);
  }
  
@@ -1608,7 +1614,7 @@ static int hls_close(AVFormatContext *s)
  
  static int hls_read_header(AVFormatContext *s)

  {
-void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
+void *u = s->pb;
  HLSContext *c = s->priv_data;
  int ret = 0, i;
  int highest_cur_seq_no = 0;

Not comfortable with this without knowing what the purpose of this
CUSTOM_IO check was.


Sorry, I misunderstood a prior email. Regarding why we check the custom 
IO flags, I was able to track down the following: 
https://lists.ffmpeg.org/pipermail/ffmpeg-trac/2013-February/013020.html 
for the hls_read_header. It was originally added in commit 
db6e2e848b21d988fb108995387a8c7836da4dc7 back in 2013.


After reading that, I think the problem it was trying to solve was about 
treating pb->opaque as a urlcontext -- but the code no longer does that, 
unless I've misunderstood all the casting in av_opt_get. What I believe 
it does now is call av_opt_get(*pb...), which eventually winds up in 
av_opt_find2 where it pulls options out the AVClass (av_class) contained 
in AVIOContext and starts reading its options. It doesn't seem to be 
touching opaque field in AVIOContext anymore.


Given this, I _think_ what I did is still ok. However, I can also change 
the patch to leave hls_read_header alone and add an if (s->flags ^ 
AVFMT_FLAG_CUSTOM_IO) before getting the new_cookies. This, effectively, 
fixes the bug without disregarding the check against 
AVFMT_FLAG_CUSTOM_IO -- best of both worlds and a smaller patch.


If nobody can confirm my analysis that opaque is now being left alone 
(thus, no longer requiring the check against the custom IO flag) then 
I'll just put the smaller patch up.


Thanks,

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

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Observe Set-Cookie headers

2017-05-02 Thread wm4 
On Tue,  2 May 2017 20:47:06 -0400
Micah Galizia  wrote:

> Signed-off-by: Micah Galizia 
> ---
>  libavformat/hls.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index bac53a4350..643d50e1da 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -630,8 +630,14 @@ static int open_url(AVFormatContext *s, AVIOContext 
> **pb, const char *url,
>  ret = s->io_open(s, pb, url, AVIO_FLAG_READ, );
>  if (ret >= 0) {
>  // update cookies on http response with setcookies.
> -void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
> -update_options(>cookies, "cookies", u);
> +char *new_cookies = NULL;
> +
> +av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN, 
> (uint8_t**)_cookies);
> +if (new_cookies) {
> +av_free(c->cookies);
> +c->cookies = new_cookies;
> +}
> +
>  av_dict_set(, "cookies", c->cookies, 0);
>  }
>  
> @@ -1608,7 +1614,7 @@ static int hls_close(AVFormatContext *s)
>  
>  static int hls_read_header(AVFormatContext *s)
>  {
> -void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb;
> +void *u = s->pb;
>  HLSContext *c = s->priv_data;
>  int ret = 0, i;
>  int highest_cur_seq_no = 0;

Not comfortable with this without knowing what the purpose of this
CUSTOM_IO check was.

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