| Message ID | eab9ecb0-5699-f8d7-747d-52afbbcd6184@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 21 Apr 2017 11:34:39 +0100 Amine kabab <kabab1993@gmail.com> wrote: > Hi Ben, thanks for reviewing the patch, > > > On 21/04/2017 08:13, Benoit Fouet wrote: > > Hi, > > > > > > On 20/04/2017 18:18, Amine kabab wrote: > >> From 5079f9b7114589626a4c9fff0fbb8f6e0d2f4fd9 Mon Sep 17 00:00:00 2001 > >> From: amine kabab <kabab1993@gmail.com> > >> Date: Thu, 20 Apr 2017 15:59:42 +0000 > >> Subject: [PATCH] HLS skip down streams > >> > >> --- > >> libavformat/hls.c | 14 +++++++++++++- > >> 1 file changed, 13 insertions(+), 1 deletion(-) > >> > >> diff --git a/libavformat/hls.c b/libavformat/hls.c > >> index bac53a4..26d7679 100644 > >> --- a/libavformat/hls.c > >> +++ b/libavformat/hls.c > >> @@ -194,6 +194,7 @@ typedef struct HLSContext { > >> > >> int cur_seq_no; > >> int live_start_index; > >> + int skip_down_streams; > >> int first_packet; > >> int64_t first_timestamp; > >> int64_t cur_timestamp; > >> @@ -1652,11 +1653,20 @@ static int hls_read_header(AVFormatContext *s) > >> /* If the playlist only contained playlists (Master Playlist), > >> * parse each individual playlist. */ > >> if (c->n_playlists > 1 || c->playlists[0]->n_segments == 0) { > >> + int all_failed = 1; > >> for (i = 0; i < c->n_playlists; i++) { > >> struct playlist *pls = c->playlists[i]; > >> - if ((ret = parse_playlist(c, pls->url, pls, NULL)) < 0) > >> + av_log(NULL, AV_LOG_WARNING, "Trying %s\n", pls->url); > > Do you really want to keep this trace? > I think the log is very helpful in this part, otherwise if the playlist is too long the user will get no feedback. > > > >> + ret = parse_playlist(c, pls->url, pls, NULL); > >> + if (c->skip_down_streams && ret >= 0) { > >> + all_failed = 0; > >> + } else if (!c->skip_down_streams && ret < 0){ > >> goto fail; > >> + } > > If I understand correctly, that means that if you do not set > > skip_down_streams and the playlist parsing is OK, you will not unset > > all_failed, and bail out below. Is this really what you want? > I wanted to keep the normal behavior, I've updated the patch to fix the issue > > > >> } > >> + > >> + if (all_failed) > >> + goto fail; > >> } > >> > >> if (c->variants[0]->playlists[0]->n_segments == 0) { > >> @@ -2126,6 +2136,8 @@ static int hls_probe(AVProbeData *p) > >> static const AVOption hls_options[] = { > >> {"live_start_index", "segment index to start live streams at (negative values are from the end)", > >> OFFSET(live_start_index), AV_OPT_TYPE_INT, {.i64 = -3}, INT_MIN, INT_MAX, FLAGS}, > >> + {"skip_down_streams", "continue playback of HLS when one of the variant streams are down", > >> + OFFSET(skip_down_streams), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, FLAGS}, > >> {NULL} > >> }; > Does this code affect only selected streams, or all of them?
Normally, all the streams On 21/04/2017 12:49, wm4 wrote: > On Fri, 21 Apr 2017 11:34:39 +0100 > Amine kabab <kabab1993@gmail.com> wrote: > >> Hi Ben, thanks for reviewing the patch, >> >> >> On 21/04/2017 08:13, Benoit Fouet wrote: >>> Hi, >>> >>> >>> On 20/04/2017 18:18, Amine kabab wrote: >>>> From 5079f9b7114589626a4c9fff0fbb8f6e0d2f4fd9 Mon Sep 17 00:00:00 2001 >>>> From: amine kabab <kabab1993@gmail.com> >>>> Date: Thu, 20 Apr 2017 15:59:42 +0000 >>>> Subject: [PATCH] HLS skip down streams >>>> >>>> --- >>>> libavformat/hls.c | 14 +++++++++++++- >>>> 1 file changed, 13 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/libavformat/hls.c b/libavformat/hls.c >>>> index bac53a4..26d7679 100644 >>>> --- a/libavformat/hls.c >>>> +++ b/libavformat/hls.c >>>> @@ -194,6 +194,7 @@ typedef struct HLSContext { >>>> >>>> int cur_seq_no; >>>> int live_start_index; >>>> + int skip_down_streams; >>>> int first_packet; >>>> int64_t first_timestamp; >>>> int64_t cur_timestamp; >>>> @@ -1652,11 +1653,20 @@ static int hls_read_header(AVFormatContext *s) >>>> /* If the playlist only contained playlists (Master Playlist), >>>> * parse each individual playlist. */ >>>> if (c->n_playlists > 1 || c->playlists[0]->n_segments == 0) { >>>> + int all_failed = 1; >>>> for (i = 0; i < c->n_playlists; i++) { >>>> struct playlist *pls = c->playlists[i]; >>>> - if ((ret = parse_playlist(c, pls->url, pls, NULL)) < 0) >>>> + av_log(NULL, AV_LOG_WARNING, "Trying %s\n", pls->url); >>> Do you really want to keep this trace? >> I think the log is very helpful in this part, otherwise if the playlist is too long the user will get no feedback. >>> >>>> + ret = parse_playlist(c, pls->url, pls, NULL); >>>> + if (c->skip_down_streams && ret >= 0) { >>>> + all_failed = 0; >>>> + } else if (!c->skip_down_streams && ret < 0){ >>>> goto fail; >>>> + } >>> If I understand correctly, that means that if you do not set >>> skip_down_streams and the playlist parsing is OK, you will not unset >>> all_failed, and bail out below. Is this really what you want? >> I wanted to keep the normal behavior, I've updated the patch to fix the issue >>> >>>> } >>>> + >>>> + if (all_failed) >>>> + goto fail; >>>> } >>>> >>>> if (c->variants[0]->playlists[0]->n_segments == 0) { >>>> @@ -2126,6 +2136,8 @@ static int hls_probe(AVProbeData *p) >>>> static const AVOption hls_options[] = { >>>> {"live_start_index", "segment index to start live streams at (negative values are from the end)", >>>> OFFSET(live_start_index), AV_OPT_TYPE_INT, {.i64 = -3}, INT_MIN, INT_MAX, FLAGS}, >>>> + {"skip_down_streams", "continue playback of HLS when one of the variant streams are down", >>>> + OFFSET(skip_down_streams), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, FLAGS}, >>>> {NULL} >>>> }; > Does this code affect only selected streams, or all of them? > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Fri, Apr 21, 2017 at 11:34:39 +0100, Amine kabab wrote: > + {"skip_down_streams", "continue playback of HLS when one of the variant streams are down", > + OFFSET(skip_down_streams), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, FLAGS}, "one" is singular, so this would be "... streams is down". I would prefer: "continue playback of HLS if any of the variant streams is down" as "one" is wrong, strictly speaking. Moritz
On Fri, 21 Apr 2017 15:31:55 +0100 Amine kabab <kabab1993@gmail.com> wrote: > Normally, all the streams > Wouldn't it be better to only consider the selected streams for this?
diff --git a/libavformat/hls.c b/libavformat/hls.c index bac53a4..8cea34e 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -194,6 +194,7 @@ typedef struct HLSContext { int cur_seq_no; int live_start_index; + int skip_down_streams; int first_packet; int64_t first_timestamp; int64_t cur_timestamp; @@ -1652,11 +1653,20 @@ static int hls_read_header(AVFormatContext *s) /* If the playlist only contained playlists (Master Playlist), * parse each individual playlist. */ if (c->n_playlists > 1 || c->playlists[0]->n_segments == 0) { + int all_failed = 1; for (i = 0; i < c->n_playlists; i++) { struct playlist *pls = c->playlists[i]; - if ((ret = parse_playlist(c, pls->url, pls, NULL)) < 0) + av_log(NULL, AV_LOG_WARNING, "Trying %s\n", pls->url); + ret = parse_playlist(c, pls->url, pls, NULL); + if (c->skip_down_streams && ret >= 0) { + all_failed = 0; + } else if (!c->skip_down_streams && ret < 0){ goto fail; + } } + + if (c->skip_down_streams && all_failed) + goto fail; } if (c->variants[0]->playlists[0]->n_segments == 0) { @@ -2126,6 +2136,8 @@ static int hls_probe(AVProbeData *p) static const AVOption hls_options[] = { {"live_start_index", "segment index to start live streams at (negative values are from the end)", OFFSET(live_start_index), AV_OPT_TYPE_INT, {.i64 = -3}, INT_MIN, INT_MAX, FLAGS}, + {"skip_down_streams", "continue playback of HLS when one of the variant streams are down", + OFFSET(skip_down_streams), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, FLAGS}, {NULL} };