[FFmpeg-devel] [PATCH] lavf/segment: add CVS escaping for CSV list file filename field

[Alexander Strasser]

Hi Stefano,

  there is a CVS vs CSV typo in the subject.

  NIT: Following might be slightly easier to grasp for the human reader:

  add escaping for filename field of the CSV list file 

Stefano Sabatini wrote:
> ---
>  doc/muxers.texi       |    4 ++--
>  libavformat/segment.c |   25 ++++++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
[...]
> --- a/libavformat/segment.c
> +++ b/libavformat/segment.c
> @@ -67,6 +67,28 @@ typedef struct {
>      double start_time, end_time;
>  } SegmentContext;
>  
> +static void avio_print_csv_escaped_str(AVIOContext *ctx, const char *str)

  While technically correct I do not think it is a good idea to have
a static routine called avio_print_csv_escaped_str, IMHO either it
should be added to avio context methods or it should drop the prefix.
ATM I think the first is better.

NIT: avio_print_csv_escaped_str ing sounds better to me, don't know
currently how this relates the names we have already given though.

> +{
> +    const char *p;
> +    int quote = 0;
> +
> +    /* check if input needs quoting */
> +    for (p = str; *p; p++)
> +        if (*p == '"' || *p == ',' || *p == '\n' || *p == '\r')
> +            quote = 1;
> +
> +    if (quote)
> +        avio_w8(ctx, '"');
> +
> +    for (p = str; *p; p++) {
> +        if (*p == '"')
> +            avio_w8(ctx, '"');
> +        avio_w8(ctx, *p);
> +    }
> +    if (quote)
> +        avio_w8(ctx, '"');
> +}


[...]

  Otherwise  patch looks fine to me.

  Alexander