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

[Alexander Strasser]

Hi,

Stefano Sabatini wrote:
> On date Sunday 2012-09-02 11:16:54 +0200, Alexander Strasser encoded:
[...]
> > 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.
> 
> I don't want to bloat the API with a specific function. We may want to
> add an escaping/de-escaping API for that, such for example:
> 
> int av_escape(AVBprint *dst, const char *str, enum escaping_type)
> or
> int avio_print_escaped_str(...)
> 
> And since I want to get the work done, I'd go with the simpler
> route and have a local implementation rather than refantoning the
> whole universe.

  OK, no problem. If you consider it not suited for public API
keeping it local without the prefix is fine.

[...]
> --- a/doc/muxers.texi
> +++ b/doc/muxers.texi
> @@ -498,8 +498,8 @@ each line matching the format:
>  @end example
>  
>  @var{segment_filename} is the name of the output file generated by the
> -muxer according to the provided pattern, and should not contain the
> -"," character for simplifying parsing operations.
> +muxer according to the provided pattern. CSV escaping (according to
> +RFC4180) is applied if required.
>  
>  @var{segment_start_time} and @var{segment_end_time} specify
>  the segment start and end time expressed in seconds.
> diff --git a/libavformat/segment.c b/libavformat/segment.c
> index 1dda310..2312c5d 100644
> --- a/libavformat/segment.c
> +++ b/libavformat/segment.c
> @@ -67,6 +67,28 @@ typedef struct {
>      double start_time, end_time;
>  } SegmentContext;
>  
> +static void print_csv_escaped_str(AVIOContext *ctx, const char *str)
> +{
> +    const char *p;
> +    int quote = 0;
> +
> +    /* check if input needs quoting */
> +    for (p = str; *p; p++)
> +        if (strchr("\",\n\r", *p))
> +            quote = 1;

  I just woke up, so I really might be missing something.
But do you really still need the loop?

> +
> +    if (quote)
> +        avio_w8(ctx, '"');
> +
> +    for (p = str; *p; p++) {
> +        if (*p == '"')
> +            avio_w8(ctx, '"');
> +        avio_w8(ctx, *p);
> +    }
> +    if (quote)
> +        avio_w8(ctx, '"');
> +}
[...]

  Alexander