[FFmpeg-devel] [PATCH] Added STL demuxer and decoder

[Clément Bœsch]

On Thu, Oct 16, 2014 at 02:26:30AM +0530, Eejya Singh wrote:
> From: Eejya <singh.eejya at gmail.com>
> 
> ---
>  Changelog                |   1 +
>  doc/general.texi         |   1 +
>  libavcodec/Makefile      |   1 +
>  libavcodec/allcodecs.c   |   1 +
>  libavcodec/avcodec.h     |   1 +
>  libavcodec/codec_desc.c  |   7 +++
>  libavcodec/textdec.c     |  18 ++++++-
>  libavformat/Makefile     |   1 +
>  libavformat/allformats.c |   1 +
>  libavformat/stldec.c     | 136 +++++++++++++++++++++++++++++++++++++++++++++++
>  10 files changed, 167 insertions(+), 1 deletion(-)
>  create mode 100644 libavformat/stldec.c
> 

Please reply in the same thread when iterating the patch, it's easier for
us to track.

> diff --git a/Changelog b/Changelog
> index b59058b..9626d4a 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -5,6 +5,7 @@ version <next>:
>  - HEVC/H.265 RTP payload format (draft v6) packetizer
>  - SUP/PGS subtitle demuxer
>  - ffprobe -show_pixel_formats option
> +- STL subtitle demuxer and decoder
>  
>  version 2.4:
>  - Icecast protocol
> diff --git a/doc/general.texi b/doc/general.texi
> index 2252f7b..681c5c9 100644
> --- a/doc/general.texi
> +++ b/doc/general.texi
> @@ -1032,6 +1032,7 @@ performance on systems without hardware floating point support).
>  @item RealText         @tab   @tab X @tab   @tab X
>  @item SAMI             @tab   @tab X @tab   @tab X
>  @item SSA/ASS          @tab X @tab X @tab X @tab X

> + at item STL              @tab   @tab X @tab   @tab X

Maybe mention Spruce here

[...]
> diff --git a/libavformat/stldec.c b/libavformat/stldec.c
> new file mode 100644
> index 0000000..9e79720
> --- /dev/null
> +++ b/libavformat/stldec.c
> @@ -0,0 +1,136 @@
> +/*
> + * Copyright (c) 2014 Eejya Singh
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/**
> + * @file
> + * STL subtitles format demuxer
> + */

Maybe add a "@see" entry with a link to the specifications here (git grep
'@see http' for examples)

Please also add a line breaks after the "*/"

> +#include "avformat.h"
> +#include "internal.h"
> +#include "subtitles.h"
> +#include "libavutil/intreadwrite.h"
> +#include "libavutil/avstring.h"
> +
> +typedef struct {
> +    FFDemuxSubtitlesQueue q;
> +} STLContext;
> +
> +static int stl_probe(AVProbeData *p)
> +{
> +    char c;
> +    const unsigned char *ptr = p->buf;
> +
> +    if (AV_RB24(ptr) == 0xEFBBBF)
> +        ptr += 3;  /* skip UTF-8 BOM */

> +    while(*ptr=='\r' || *ptr=='\n' || *ptr=='$' || !strncmp(ptr,"//",2))
> +        ptr+=ff_subtitles_next_line(ptr);

Here and several times later in the file, the style is wrong. Please check
how spaces are supposed to be.

See http://ffmpeg.org/developer.html#toc-Coding-Rules-1

> +    if (sscanf(ptr, "%*d:%*d:%*d:%*d , %*d:%*d:%*d:%*d , %c", &c) == 1)
> +        return AVPROBE_SCORE_MAX;
> +    return 0;

> +}

One more line break here to separate the functions

> +static int64_t get_pts(char **buf, int *duration)
> +{
> +        int hh1, mm1, ss1, ms1;
> +        int hh2, mm2, ss2, ms2;
> +        int len=0;
> +        if (sscanf(*buf, "%2d:%2d:%2d:%2d , %2d:%2d:%2d:%2d , %n",
> +                   &hh1, &mm1, &ss1, &ms1,
> +                   &hh2, &mm2, &ss2, &ms2, &len) >= 8 && len>0) {
> +            int64_t start = (hh1*3600LL + mm1*60LL + ss1) * 100LL + ms1;
> +            int64_t end   = (hh2*3600LL + mm2*60LL + ss2) * 100LL + ms2;
> +            *duration = end - start;
> +            *buf+=len;
> +            return start;
> +         }
> +        return AV_NOPTS_VALUE;

The indent of this function is wrong

[...]

Rest of the code LGTM.

It would be nice if you could add a test for this. Pick a .stl file, try
to add weird cases like timestamps with and without spaces, with an UTF-8
BOM, with random line breaks, use of '|', and with style
changes (for later). Not need for a huge file, just a sample that covers
most of the code.

Move your samples in fate-suite/sub/, then add an entry in
tests/fate/subtitles.mak and use make fate-sub-stl GEN=1 so it creates the
reference file. If we agree with the test, we'll upload the sample.

Thanks.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141016/52358645/attachment.asc>