diff mbox

[FFmpeg-devel] avformat/hlsenc: support TAG span multiple lines when parse playlist

Message ID 20170505165045.4154-1-lq@chinaffmpeg.org
State New
Headers show

Commit Message

Steven Liu May 5, 2017, 4:50 p.m. UTC
refer to: https://developer.apple.com/library/content/technotes/tn2288/_index.html

support to parse the EXT-X-KEY span multiple lines:
 #EXTM3U
 #EXT-X-VERSION:3
 #EXT-X-TARGETDURATION:10
 #EXT-X-MEDIA-SEQUENCE:0
 #EXT-X-KEY:METHOD=AES-128,URI="/file.key", \
 IV=0x498c8325965f907960e3d94d20c4d138
 #EXTINF:10.000000,
 out0.ts
 #EXTINF:8.640000,
 out1.ts
 #EXTINF:9.160000,
 out2.ts

Reported-by: Aaron Levinson <alevinsn@aracnet.com>
Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/hlsenc.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

Comments

Steven Liu May 5, 2017, 8:55 a.m. UTC | #1
2017-05-06 0:50 GMT+08:00 Steven Liu <lq@chinaffmpeg.org>:

> refer to: https://developer.apple.com/library/content/technotes/
> tn2288/_index.html
>
> support to parse the EXT-X-KEY span multiple lines:
>  #EXTM3U
>  #EXT-X-VERSION:3
>  #EXT-X-TARGETDURATION:10
>  #EXT-X-MEDIA-SEQUENCE:0
>  #EXT-X-KEY:METHOD=AES-128,URI="/file.key", \
>  IV=0x498c8325965f907960e3d94d20c4d138
>  #EXTINF:10.000000,
>  out0.ts
>  #EXTINF:8.640000,
>  out1.ts
>  #EXTINF:9.160000,
>  out2.ts
>
> Reported-by: Aaron Levinson <alevinsn@aracnet.com>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavformat/hlsenc.c |   21 +++++++++++++++++++--
>  1 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 221089c..c167624 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -500,10 +500,27 @@ static int hls_encryption_start(AVFormatContext *s)
>
>  static int read_chomp_line(AVIOContext *s, char *buf, int maxlen)
>  {
> -    int len = ff_get_line(s, buf, maxlen);
> +    int len = 0;
> +    int total_len = 0;
> +
> +re_get_line:
> +    if (maxlen > total_len) {
> +        len = ff_get_line(s, buf, maxlen - total_len);
> +    } else {
> +        av_log(s, AV_LOG_WARNING, "The tag is too long, context only can
> get %s\n", buf);
> +        return maxlen;
> +    }
>      while (len > 0 && av_isspace(buf[len - 1]))
>          buf[--len] = '\0';
> -    return len;
> +
> +    if (buf[len - 1] == '\\') {
> +        buf += len - 1;
> +        total_len += len - 1;
> +        goto re_get_line;
> +    }
> +
> +    total_len += len;
> +    return total_len;
>  }
>
>  static int hls_mux_init(AVFormatContext *s)
> --
> 1.7.1
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


Hi Aaron Levinson

What about this one?
Steven Liu May 9, 2017, 12:43 a.m. UTC | #2
2017-05-05 16:55 GMT+08:00 Steven Liu <lingjiujianke@gmail.com>:

>
>
> 2017-05-06 0:50 GMT+08:00 Steven Liu <lq@chinaffmpeg.org>:
>
>> refer to: https://developer.apple.com/library/content/technotes/tn2288
>> /_index.html
>>
>> support to parse the EXT-X-KEY span multiple lines:
>>  #EXTM3U
>>  #EXT-X-VERSION:3
>>  #EXT-X-TARGETDURATION:10
>>  #EXT-X-MEDIA-SEQUENCE:0
>>  #EXT-X-KEY:METHOD=AES-128,URI="/file.key", \
>>  IV=0x498c8325965f907960e3d94d20c4d138
>>  #EXTINF:10.000000,
>>  out0.ts
>>  #EXTINF:8.640000,
>>  out1.ts
>>  #EXTINF:9.160000,
>>  out2.ts
>>
>> Reported-by: Aaron Levinson <alevinsn@aracnet.com>
>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>> ---
>>  libavformat/hlsenc.c |   21 +++++++++++++++++++--
>>  1 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>> index 221089c..c167624 100644
>> --- a/libavformat/hlsenc.c
>> +++ b/libavformat/hlsenc.c
>> @@ -500,10 +500,27 @@ static int hls_encryption_start(AVFormatContext *s)
>>
>>  static int read_chomp_line(AVIOContext *s, char *buf, int maxlen)
>>  {
>> -    int len = ff_get_line(s, buf, maxlen);
>> +    int len = 0;
>> +    int total_len = 0;
>> +
>> +re_get_line:
>> +    if (maxlen > total_len) {
>> +        len = ff_get_line(s, buf, maxlen - total_len);
>> +    } else {
>> +        av_log(s, AV_LOG_WARNING, "The tag is too long, context only can
>> get %s\n", buf);
>> +        return maxlen;
>> +    }
>>      while (len > 0 && av_isspace(buf[len - 1]))
>>          buf[--len] = '\0';
>> -    return len;
>> +
>> +    if (buf[len - 1] == '\\') {
>> +        buf += len - 1;
>> +        total_len += len - 1;
>> +        goto re_get_line;
>> +    }
>> +
>> +    total_len += len;
>> +    return total_len;
>>  }
>>
>>  static int hls_mux_init(AVFormatContext *s)
>> --
>> 1.7.1
>>
>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
>
> Hi Aaron Levinson
>
> What about this one?
>

ping
Aaron Levinson May 9, 2017, 8:01 p.m. UTC | #3
I would rewrite the commit message as:  "avformat/hlsenc:  support 
multi-line TAG spans when parsing a playlist".  Also, I thought you were 
going to have a common implementation for both hlsenc and hls?  There 
are a duplicate implementations of read_chomp_line() in hls.c, hlsenc.c, 
and hlsproto.c.  This probably ought to be done in another patch, and I 
suggest making this functionality generic by moving to 
internal.h/aviobuf.c.  The function could be called 
ff_get_multi_line_span().

On 5/5/2017 9:50 AM, Steven Liu wrote:
> refer to: https://developer.apple.com/library/content/technotes/tn2288/_index.html

Note, the actual specification can be found at 
https://tools.ietf.org/html/draft-pantos-http-live-streaming-19 .  This 
makes it clear how a '\' character is used to extend a tag declaration 
to the next line.  I recommend referring to the draft spec instead of 
the link that I previously posted.

>
> support to parse the EXT-X-KEY span multiple lines:
>  #EXTM3U
>  #EXT-X-VERSION:3
>  #EXT-X-TARGETDURATION:10
>  #EXT-X-MEDIA-SEQUENCE:0
>  #EXT-X-KEY:METHOD=AES-128,URI="/file.key", \
>  IV=0x498c8325965f907960e3d94d20c4d138
>  #EXTINF:10.000000,
>  out0.ts
>  #EXTINF:8.640000,
>  out1.ts
>  #EXTINF:9.160000,
>  out2.ts
>
> Reported-by: Aaron Levinson <alevinsn@aracnet.com>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavformat/hlsenc.c |   21 +++++++++++++++++++--
>  1 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 221089c..c167624 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -500,10 +500,27 @@ static int hls_encryption_start(AVFormatContext *s)
>
>  static int read_chomp_line(AVIOContext *s, char *buf, int maxlen)
>  {
> -    int len = ff_get_line(s, buf, maxlen);
> +    int len = 0;

Should probably move the setting of len to 0 below the label (or within 
the while loop as I suggest later) to make it clear that the last value 
isn't relevant for further iterations of the loop.  It is fine to 
declare len outside of the loop, although I think it would be cleaner if 
you declared it within the loop and adjusted the code accordingly to 
only increment total_len within the loop.  Obviously, the declaration 
within the loop is only possible if you convert to a while loop.

> +    int total_len = 0;
> +
> +re_get_line:

This can easily be done with a while loop instead of using goto.  In 
fact, what you are doing is basically a while loop, so I think it would 
be appropriate to do this as a while loop.

> +    if (maxlen > total_len) {

This is not quite right.  According to the documentation for 
ff_get_line(), the length returned is "the length of the string written 
in the buffer, not including the final \\0".  If, say, with the first 
call to ff_get_line(), it fills up the entire buffer, the length 
returned will be maxlen - 1.  If this is a multi-line span, you'll 
iterate through the loop, see maxlen > total_len, and try again.  Should 
likely be "if ((maxlen - 1) > total_len) {".

> +        len = ff_get_line(s, buf, maxlen - total_len);

This line is correct, since you do want to use the total buffer space 
for the call to ff_get_line().

> +    } else {
> +        av_log(s, AV_LOG_WARNING, "The tag is too long, context only can get %s\n", buf);

For this log message to work properly for multi-line spans, need to do 
char *buf2 = buf and work with buf2.  With your current patch, if there 
is a multi-line span, buf will not point to the original, and the log 
message will incomplete in that case.

> +        return maxlen;
> +    }
>      while (len > 0 && av_isspace(buf[len - 1]))
>          buf[--len] = '\0';

This whitespace removal while loop won't do much in the case of a 
multi-line span.  According to the spec, it is important to remove any 
trailing whitespace before the '\\':  "A '\' is used to indicate that 
the tag continues on the following line with whitespace removed."  While 
in the examples in the spec it might not be needed, a multi-line span 
could look something like the following:

#EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="aac",NAME="Commentary", DEF \
       AULT=NO,AUTOSELECT=NO,LANGUAGE="en", \
       URI="commentary/audio-only.m3u8"

As implemented in your patch, the whitespace removal code won't be 
triggered in the case that the last character in the line is a '\\', 
because in that case, it will immediately fail the check and move on to 
the "if (buf[len - 1] == '\\') {" line.  And with the above example, you 
won't get the desired result.

But, you could in theory have whitespace both before _and_ after the 
'\', and to handle that correctly, you'll want the whitespace removal 
loop both before and after the '\\' detection code.

> -    return len;
> +
> +    if (buf[len - 1] == '\\') {
> +        buf += len - 1;

I suggest using a temporary variable for increment purposes.  This will 
make it easier to examine the entire string while debugging, for 
example.  That is, don't alter buf and set char *buf2 = buf, or 
something like that, and utilize buf2.

Also, after doing buf += len - 1 (or rather, buf2), should then do *buf 
= '\0'; to make absolutely certain that the '\\' does not show up in the 
output.  If the line is too long and it doesn't call ff_get_line() 
another time in the next iteration of the loop, '\\' will be left in the 
output.

> +        total_len += len - 1;
> +        goto re_get_line;
> +    }
> +
> +    total_len += len;
> +    return total_len;
>  }
>
>  static int hls_mux_init(AVFormatContext *s)
>

Aaron Levinson
Aaron Levinson May 9, 2017, 8:31 p.m. UTC | #4
Based on a conversation that I had on IRC with Martin Storsjö, I 
misinterpreted the Apple documentation, and the only reason why '\' 
shows up in these documents is so that lines won't appear too long, 
particularly in the RFC.  According to Martin, Apple's tools can't 
handle .m3u8 files that use '\' for multi-line TAG spans, and in 
addition, there is no mention of '\' anywhere in the actual grammar 
documented in the specification.  So, it seems that that this patch 
isn't needed.

Aaron Levinson

On 5/9/2017 1:01 PM, Aaron Levinson wrote:
> I would rewrite the commit message as:  "avformat/hlsenc:  support
> multi-line TAG spans when parsing a playlist".  Also, I thought you were
> going to have a common implementation for both hlsenc and hls?  There
> are a duplicate implementations of read_chomp_line() in hls.c, hlsenc.c,
> and hlsproto.c.  This probably ought to be done in another patch, and I
> suggest making this functionality generic by moving to
> internal.h/aviobuf.c.  The function could be called
> ff_get_multi_line_span().
>
> On 5/5/2017 9:50 AM, Steven Liu wrote:
>> refer to:
>> https://developer.apple.com/library/content/technotes/tn2288/_index.html
>
> Note, the actual specification can be found at
> https://tools.ietf.org/html/draft-pantos-http-live-streaming-19 .  This
> makes it clear how a '\' character is used to extend a tag declaration
> to the next line.  I recommend referring to the draft spec instead of
> the link that I previously posted.
>
>>
>> support to parse the EXT-X-KEY span multiple lines:
>>  #EXTM3U
>>  #EXT-X-VERSION:3
>>  #EXT-X-TARGETDURATION:10
>>  #EXT-X-MEDIA-SEQUENCE:0
>>  #EXT-X-KEY:METHOD=AES-128,URI="/file.key", \
>>  IV=0x498c8325965f907960e3d94d20c4d138
>>  #EXTINF:10.000000,
>>  out0.ts
>>  #EXTINF:8.640000,
>>  out1.ts
>>  #EXTINF:9.160000,
>>  out2.ts
>>
>> Reported-by: Aaron Levinson <alevinsn@aracnet.com>
>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>> ---
>>  libavformat/hlsenc.c |   21 +++++++++++++++++++--
>>  1 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>> index 221089c..c167624 100644
>> --- a/libavformat/hlsenc.c
>> +++ b/libavformat/hlsenc.c
>> @@ -500,10 +500,27 @@ static int hls_encryption_start(AVFormatContext *s)
>>
>>  static int read_chomp_line(AVIOContext *s, char *buf, int maxlen)
>>  {
>> -    int len = ff_get_line(s, buf, maxlen);
>> +    int len = 0;
>
> Should probably move the setting of len to 0 below the label (or within
> the while loop as I suggest later) to make it clear that the last value
> isn't relevant for further iterations of the loop.  It is fine to
> declare len outside of the loop, although I think it would be cleaner if
> you declared it within the loop and adjusted the code accordingly to
> only increment total_len within the loop.  Obviously, the declaration
> within the loop is only possible if you convert to a while loop.
>
>> +    int total_len = 0;
>> +
>> +re_get_line:
>
> This can easily be done with a while loop instead of using goto.  In
> fact, what you are doing is basically a while loop, so I think it would
> be appropriate to do this as a while loop.
>
>> +    if (maxlen > total_len) {
>
> This is not quite right.  According to the documentation for
> ff_get_line(), the length returned is "the length of the string written
> in the buffer, not including the final \\0".  If, say, with the first
> call to ff_get_line(), it fills up the entire buffer, the length
> returned will be maxlen - 1.  If this is a multi-line span, you'll
> iterate through the loop, see maxlen > total_len, and try again.  Should
> likely be "if ((maxlen - 1) > total_len) {".
>
>> +        len = ff_get_line(s, buf, maxlen - total_len);
>
> This line is correct, since you do want to use the total buffer space
> for the call to ff_get_line().
>
>> +    } else {
>> +        av_log(s, AV_LOG_WARNING, "The tag is too long, context only
>> can get %s\n", buf);
>
> For this log message to work properly for multi-line spans, need to do
> char *buf2 = buf and work with buf2.  With your current patch, if there
> is a multi-line span, buf will not point to the original, and the log
> message will incomplete in that case.
>
>> +        return maxlen;
>> +    }
>>      while (len > 0 && av_isspace(buf[len - 1]))
>>          buf[--len] = '\0';
>
> This whitespace removal while loop won't do much in the case of a
> multi-line span.  According to the spec, it is important to remove any
> trailing whitespace before the '\\':  "A '\' is used to indicate that
> the tag continues on the following line with whitespace removed."  While
> in the examples in the spec it might not be needed, a multi-line span
> could look something like the following:
>
> #EXT-X-MEDIA:TYPE=AUDIO,GROUP-ID="aac",NAME="Commentary", DEF \
>       AULT=NO,AUTOSELECT=NO,LANGUAGE="en", \
>       URI="commentary/audio-only.m3u8"
>
> As implemented in your patch, the whitespace removal code won't be
> triggered in the case that the last character in the line is a '\\',
> because in that case, it will immediately fail the check and move on to
> the "if (buf[len - 1] == '\\') {" line.  And with the above example, you
> won't get the desired result.
>
> But, you could in theory have whitespace both before _and_ after the
> '\', and to handle that correctly, you'll want the whitespace removal
> loop both before and after the '\\' detection code.
>
>> -    return len;
>> +
>> +    if (buf[len - 1] == '\\') {
>> +        buf += len - 1;
>
> I suggest using a temporary variable for increment purposes.  This will
> make it easier to examine the entire string while debugging, for
> example.  That is, don't alter buf and set char *buf2 = buf, or
> something like that, and utilize buf2.
>
> Also, after doing buf += len - 1 (or rather, buf2), should then do *buf
> = '\0'; to make absolutely certain that the '\\' does not show up in the
> output.  If the line is too long and it doesn't call ff_get_line()
> another time in the next iteration of the loop, '\\' will be left in the
> output.
>
>> +        total_len += len - 1;
>> +        goto re_get_line;
>> +    }
>> +
>> +    total_len += len;
>> +    return total_len;
>>  }
>>
>>  static int hls_mux_init(AVFormatContext *s)
>>
>
> Aaron Levinson
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index 221089c..c167624 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -500,10 +500,27 @@  static int hls_encryption_start(AVFormatContext *s)
 
 static int read_chomp_line(AVIOContext *s, char *buf, int maxlen)
 {
-    int len = ff_get_line(s, buf, maxlen);
+    int len = 0;
+    int total_len = 0;
+
+re_get_line:
+    if (maxlen > total_len) {
+        len = ff_get_line(s, buf, maxlen - total_len);
+    } else {
+        av_log(s, AV_LOG_WARNING, "The tag is too long, context only can get %s\n", buf);
+        return maxlen;
+    }
     while (len > 0 && av_isspace(buf[len - 1]))
         buf[--len] = '\0';
-    return len;
+
+    if (buf[len - 1] == '\\') {
+        buf += len - 1;
+        total_len += len - 1;
+        goto re_get_line;
+    }
+
+    total_len += len;
+    return total_len;
 }
 
 static int hls_mux_init(AVFormatContext *s)