[FFmpeg-devel,UPDATE] HLS, add option to skip down streams,

Submitted by Amine KABAB on April 21, 2017, 10:34 a.m.

Details

Message ID eab9ecb0-5699-f8d7-747d-52afbbcd6184@gmail.com
State New
Headers show

Commit Message

Amine KABAB April 21, 2017, 10:34 a.m.
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}
>>  };
From 277650244439cd28ba7aa068364ed5358ead0546 Mon Sep 17 00:00:00 2001
From: amine kabab <kabab1993@gmail.com>
Date: Fri, 21 Apr 2017 10:27:18 +0000
Subject: [PATCH] HLS, add option to skip down streams

---
 libavformat/hls.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

wm4 April 21, 2017, 11:49 a.m.
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?
Amine KABAB April 21, 2017, 2:31 p.m.
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
Moritz Barsnick April 22, 2017, 1:11 p.m.
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
wm4 April 22, 2017, 1:16 p.m.
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?

Patch hide | download patch | download mbox

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}
 };