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

[Aaron Levinson]

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 at aracnet.com>
> Signed-off-by: Steven Liu <lq at 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