| Message ID | 20170505165045.4154-1-lq@chinaffmpeg.org |
|---|---|
| State | New |
| Headers | show |
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?
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
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
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 --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)
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(-)