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 <alevi...@aracnet.com>
Signed-off-by: Steven Liu <l...@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
