| Message ID | 20170506005505.3945-2-micahgalizia@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, 5 May 2017 20:55:05 -0400 Micah Galizia <micahgalizia@gmail.com> wrote: > Signed-off-by: Micah Galizia <micahgalizia@gmail.com> > --- > 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, &tmp); > if (ret >= 0) { > // update cookies on http response with setcookies. > - void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb; > - update_options(&c->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**)&new_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(&opts, "cookies", c->cookies, 0); > } >
On 2017-05-05 09:28 PM, wm4 wrote: > On Fri, 5 May 2017 20:55:05 -0400 > Micah Galizia <micahgalizia@gmail.com> wrote: > >> Signed-off-by: Micah Galizia <micahgalizia@gmail.com> >> --- >> 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, &tmp); >> if (ret >= 0) { >> // update cookies on http response with setcookies. >> - void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb; >> - update_options(&c->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**)&new_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.
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 <micahgalizia@gmail.com> wrote: >> >>> Signed-off-by: Micah Galizia <micahgalizia@gmail.com> >>> --- >>> 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, &tmp); >>> if (ret >= 0) { >>> // update cookies on http response with setcookies. >>> - void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb; >>> - update_options(&c->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**)&new_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,
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 <micahgalizia@gmail.com> wrote: > > > >>Signed-off-by: Micah Galizia <micahgalizia@gmail.com> > >>--- > >> 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, &tmp); > >> if (ret >= 0) { > >> // update cookies on http response with setcookies. > >>- void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb; > >>- update_options(&c->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**)&new_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 ? [...]
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&b) which is the proper logic. Thanks,
On Sat, 6 May 2017 14:28:10 -0400 Micah Galizia <micahgalizia@gmail.com> wrote: > On 2017-05-05 09:28 PM, wm4 wrote: > > On Fri, 5 May 2017 20:55:05 -0400 > > Micah Galizia <micahgalizia@gmail.com> wrote: > > > >> Signed-off-by: Micah Galizia <micahgalizia@gmail.com> > >> --- > >> 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, &tmp); > >> if (ret >= 0) { > >> // update cookies on http response with setcookies. > >> - void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb; > >> - update_options(&c->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**)&new_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.
On 2017-05-17 05:23 AM, wm4 wrote: > On Sat, 6 May 2017 14:28:10 -0400 > Micah Galizia <micahgalizia@gmail.com> wrote: > >> On 2017-05-05 09:28 PM, wm4 wrote: >>> On Fri, 5 May 2017 20:55:05 -0400 >>> Micah Galizia <micahgalizia@gmail.com> wrote: >>> >>>> Signed-off-by: Micah Galizia <micahgalizia@gmail.com> >>>> --- >>>> 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, &tmp); >>>> if (ret >= 0) { >>>> // update cookies on http response with setcookies. >>>> - void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb; >>>> - update_options(&c->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**)&new_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,
On Sat, May 20, 2017 at 9:36 PM, Micah Galizia <micahgalizia@gmail.com> wrote: > On 2017-05-17 05:23 AM, wm4 wrote: >> >> On Sat, 6 May 2017 14:28:10 -0400 >> Micah Galizia <micahgalizia@gmail.com> wrote: >> >>> On 2017-05-05 09:28 PM, wm4 wrote: >>>> >>>> On Fri, 5 May 2017 20:55:05 -0400 >>>> Micah Galizia <micahgalizia@gmail.com> wrote: >>>> >>>>> >>>>> Signed-off-by: Micah Galizia <micahgalizia@gmail.com> >>>>> --- >>>>> 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, &tmp); >>>>> if (ret >= 0) { >>>>> // update cookies on http response with setcookies. >>>>> - void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb; >>>>> - update_options(&c->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**)&new_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
On Fri, May 26, 2017 at 09:29:04PM -0400, Micah Galizia wrote: > On Sat, May 20, 2017 at 9:36 PM, Micah Galizia <micahgalizia@gmail.com> wrote: > > On 2017-05-17 05:23 AM, wm4 wrote: > >> > >> On Sat, 6 May 2017 14:28:10 -0400 > >> Micah Galizia <micahgalizia@gmail.com> wrote: > >> > >>> On 2017-05-05 09:28 PM, wm4 wrote: > >>>> > >>>> On Fri, 5 May 2017 20:55:05 -0400 > >>>> Micah Galizia <micahgalizia@gmail.com> wrote: > >>>> > >>>>> > >>>>> Signed-off-by: Micah Galizia <micahgalizia@gmail.com> > >>>>> --- > >>>>> 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, &tmp); > >>>>> if (ret >= 0) { > >>>>> // update cookies on http response with setcookies. > >>>>> - void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb; > >>>>> - update_options(&c->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**)&new_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 [...]
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 <micahgalizia@gmail.com> wrote: >>> On 2017-05-17 05:23 AM, wm4 wrote: >>>> On Sat, 6 May 2017 14:28:10 -0400 >>>> Micah Galizia <micahgalizia@gmail.com> wrote: >>>> >>>>> On 2017-05-05 09:28 PM, wm4 wrote: >>>>>> On Fri, 5 May 2017 20:55:05 -0400 >>>>>> Micah Galizia <micahgalizia@gmail.com> wrote: >>>>>> >>>>>>> Signed-off-by: Micah Galizia <micahgalizia@gmail.com> >>>>>>> --- >>>>>>> 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, &tmp); >>>>>>> if (ret >= 0) { >>>>>>> // update cookies on http response with setcookies. >>>>>>> - void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb; >>>>>>> - update_options(&c->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**)&new_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!
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, &tmp); if (ret >= 0) { // update cookies on http response with setcookies. - void *u = (s->flags & AVFMT_FLAG_CUSTOM_IO) ? NULL : s->pb; - update_options(&c->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**)&new_cookies); + + if (new_cookies) { + av_free(c->cookies); + c->cookies = new_cookies; + } + av_dict_set(&opts, "cookies", c->cookies, 0); }
Signed-off-by: Micah Galizia <micahgalizia@gmail.com> --- libavformat/hls.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)