Re: [FFmpeg-devel] [PATCH 2/2] avformat/hls: use AVIOContext new_http_request callback

[wm4]

On Sat, 30 Dec 2017 11:53:14 +0100
Nicolas George  wrote:

> Aman Gupta (2017-12-29):
> > It also makes it easier in the future to add http keepalive support to
> > other consumers like the dash demuxer and the crypto protocol.  
> 
> For that, the API does not need to be public, it just need to be clean.
> 
> I thought you were referring to using the hls demuxer with other
> protocols that may allow connection reuse. That could justify a public
> API. But we do not know what kind of parameters these other protocols
> may need; maybe some of them will require flags to select parallel /
> sequential requests; maybe one of them will need to return some kind of
> connection handle. We do not know, therefore we need to plan carefully,
> or even better: wait for an actual case. Also, naming the callback
> http_something would be a misnomer.

As usual, you block patches based on some theoretical nonsense that
will never happen anyway. Like your XML parser.

> > I'll go ahead and submit another patchset using this approach to fix
> > the immediate segfault while we consider a larger API change.  
> 
> Thanks. It seems to me a very sane course of action, and I have no
> objection to the two patches you just sent.

Well I object to the other patches.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avformat/hls: use AVIOContext new_http_request callback

[Nicolas George]

Aman Gupta (2017-12-29):
> It also makes it easier in the future to add http keepalive support to
> other consumers like the dash demuxer and the crypto protocol.

For that, the API does not need to be public, it just need to be clean.

I thought you were referring to using the hls demuxer with other
protocols that may allow connection reuse. That could justify a public
API. But we do not know what kind of parameters these other protocols
may need; maybe some of them will require flags to select parallel /
sequential requests; maybe one of them will need to return some kind of
connection handle. We do not know, therefore we need to plan carefully,
or even better: wait for an actual case. Also, naming the callback
http_something would be a misnomer.

> I'll go ahead and submit another patchset using this approach to fix
> the immediate segfault while we consider a larger API change.

Thanks. It seems to me a very sane course of action, and I have no
objection to the two patches you just sent.

Regards,

-- 
  Nicolas George


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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/hls: use AVIOContext new_http_request callback

[wm4]

On Fri, 29 Dec 2017 23:45:10 +0100
Nicolas George  wrote:

> Aman Gupta (2017-12-29):
> > From: Aman Gupta 
> > 
> > This fixes a segfault when streaming from an HLS playlist which uses
> > crypto, by ensuring ff_http_do_new_request will never be invoked on a
> > non-http URLContext.
> > 
> > Additionally it simplifies the demuxer by removing http.h and all the
> > http specific code in open_url_keepalive.  
> 
> It it awfully specific for something quite minor. And it is messing with
> the public API, that requires careful design.

Reusing a http connection is not minor. It's pretty straightforward
too, and doesn't require being stuck forever in a bikeshed carousel.

> I suggest to have ff_http_do_new_request() check that its argument is
> really a HTTP context and return EINVAL or ENOSYS otherwise.

Unclean and fragile. How many more bugs do you want to have with this?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avformat/hls: use AVIOContext new_http_request callback

[Aman Gupta]

On Fri, Dec 29, 2017 at 2:45 PM, Nicolas George  wrote:

> Aman Gupta (2017-12-29):
> > From: Aman Gupta 
> >
> > This fixes a segfault when streaming from an HLS playlist which uses
> > crypto, by ensuring ff_http_do_new_request will never be invoked on a
> > non-http URLContext.
> >
> > Additionally it simplifies the demuxer by removing http.h and all the
> > http specific code in open_url_keepalive.
>
> It it awfully specific for something quite minor. And it is messing with
> the public API, that requires careful design.
>

I understand the public API should not be changed lightly, but I believe
this approach is far
superior to the mess of http specific checks currently present all over
hls.c. It also makes it easier
in the future to add http keepalive support to other consumers like the
dash demuxer and the
crypto protocol.


>
> I suggest to have ff_http_do_new_request() check that its argument is
> really a HTTP context and return EINVAL or ENOSYS otherwise.
>

I'll go ahead and submit another patchset using this approach to fix the
immediate
segfault while we consider a larger API change.

Thanks,
Aman


>
> Regards,
>
> --
>   Nicolas George
>
> ___
> 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

Re: [FFmpeg-devel] [PATCH 2/2] avformat/hls: use AVIOContext new_http_request callback

[Nicolas George]

Aman Gupta (2017-12-29):
> From: Aman Gupta 
> 
> This fixes a segfault when streaming from an HLS playlist which uses
> crypto, by ensuring ff_http_do_new_request will never be invoked on a
> non-http URLContext.
> 
> Additionally it simplifies the demuxer by removing http.h and all the
> http specific code in open_url_keepalive.

It it awfully specific for something quite minor. And it is messing with
the public API, that requires careful design.

I suggest to have ff_http_do_new_request() check that its argument is
really a HTTP context and return EINVAL or ENOSYS otherwise.

Regards,

-- 
  Nicolas George


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

[FFmpeg-devel] [PATCH 2/2] avformat/hls: use AVIOContext new_http_request callback

[Aman Gupta]

From: Aman Gupta 

This fixes a segfault when streaming from an HLS playlist which uses
crypto, by ensuring ff_http_do_new_request will never be invoked on a
non-http URLContext.

Additionally it simplifies the demuxer by removing http.h and all the
http specific code in open_url_keepalive.

Signed-off-by: Aman Gupta 
Signed-off-by: wm4 
---
 libavformat/hls.c | 19 ++-
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/libavformat/hls.c b/libavformat/hls.c
index dccc7c7dd2..6c638537ff 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -26,7 +26,6 @@
  * http://tools.ietf.org/html/draft-pantos-http-live-streaming
  */
 
-#include "libavformat/http.h"
 #include "libavutil/avstring.h"
 #include "libavutil/avassert.h"
 #include "libavutil/intreadwrite.h"
@@ -611,19 +610,13 @@ static void update_options(char **dest, const char *name, 
void *src)
 static int open_url_keepalive(AVFormatContext *s, AVIOContext **pb,
   const char *url)
 {
-#if !CONFIG_HTTP_PROTOCOL
-return AVERROR_PROTOCOL_NOT_FOUND;
-#else
 int ret;
-URLContext *uc = ffio_geturlcontext(*pb);
-av_assert0(uc);
 (*pb)->eof_reached = 0;
-ret = ff_http_do_new_request(uc, url);
+ret = (*pb)->new_http_request(*pb, url);
 if (ret < 0) {
 ff_format_io_close(s, pb);
 }
 return ret;
-#endif
 }
 
 static int open_url(AVFormatContext *s, AVIOContext **pb, const char *url,
@@ -670,7 +663,7 @@ static int open_url(AVFormatContext *s, AVIOContext **pb, 
const char *url,
 else if (strcmp(proto_name, "file") || !strncmp(url, "file,", 5))
 return AVERROR_INVALIDDATA;
 
-if (is_http && c->http_persistent && *pb) {
+if (c->http_persistent && *pb && (*pb)->new_http_request) {
 ret = open_url_keepalive(c->ctx, pb, url);
 if (ret == AVERROR_EXIT) {
 return ret;
@@ -725,9 +718,9 @@ static int parse_playlist(HLSContext *c, const char *url,
 struct variant_info variant_info;
 char tmp_str[MAX_URL_SIZE];
 struct segment *cur_init_section = NULL;
-int is_http = av_strstart(url, "http", NULL);
 
-if (is_http && !in && c->http_persistent && c->playlist_pb) {
+if (!in && c->http_persistent &&
+c->playlist_pb && c->playlist_pb->new_http_request) {
 in = c->playlist_pb;
 ret = open_url_keepalive(c->ctx, >playlist_pb, url);
 if (ret == AVERROR_EXIT) {
@@ -761,7 +754,7 @@ static int parse_playlist(HLSContext *c, const char *url,
 if (ret < 0)
 return ret;
 
-if (is_http && c->http_persistent)
+if (c->http_persistent && in && in->new_http_request)
 c->playlist_pb = in;
 else
 close_in = 1;
@@ -1511,7 +1504,7 @@ reload:
 
 return ret;
 }
-if (c->http_persistent && av_strstart(seg->url, "http", NULL)) {
+if (c->http_persistent && v->input && v->input->new_http_request) {
 v->input_read_done = 1;
 } else {
 ff_format_io_close(v->parent, >input);
-- 
2.14.2

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