[FFmpeg-devel] [PATCH] SAMI demuxer and decoder.

[Stefano Sabatini]

On date Sunday 2012-06-10 21:57:42 +0200, Clément Bœsch encoded:
> FIXME: bump minor in lavc & lavf
> ---
>  doc/general.texi         |    1 +
>  libavcodec/Makefile      |    1 +
>  libavcodec/allcodecs.c   |    1 +
>  libavcodec/avcodec.h     |    1 +
>  libavcodec/samidec.c     |  152 ++++++++++++++++++++++++++++++++++++++++++
>  libavformat/Makefile     |    1 +
>  libavformat/allformats.c |    1 +
>  libavformat/samidec.c    |  164 ++++++++++++++++++++++++++++++++++++++++++++++
>  8 files changed, 322 insertions(+)
>  create mode 100644 libavcodec/samidec.c
>  create mode 100644 libavformat/samidec.c
> 
> diff --git a/doc/general.texi b/doc/general.texi
> index 92961ff..bfdf69b 100644
> --- a/doc/general.texi
> +++ b/doc/general.texi
> @@ -842,6 +842,7 @@ performance on systems without hardware floating point support).
>  @item JACOsub      @tab X @tab X @tab   @tab X
>  @item MicroDVD     @tab X @tab X @tab   @tab X
>  @item PGS          @tab   @tab   @tab   @tab X
> + at item SAMI         @tab   @tab X @tab   @tab X
>  @item SubRip (SRT) @tab X @tab X @tab X @tab X
>  @item XSUB         @tab   @tab   @tab X @tab X
>  @end multitable
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index f5b66f8..05ecf64 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -393,6 +393,7 @@ OBJS-$(CONFIG_RV30_DECODER)            += rv30.o rv34.o rv30dsp.o rv34dsp.o \
>                                            mpegvideo.o error_resilience.o
>  OBJS-$(CONFIG_RV40_DECODER)            += rv40.o rv34.o rv34dsp.o rv40dsp.o \
>                                            mpegvideo.o error_resilience.o
> +OBJS-$(CONFIG_SAMI_DECODER)            += samidec.o ass.o
>  OBJS-$(CONFIG_S302M_DECODER)           += s302m.o
>  OBJS-$(CONFIG_SGI_DECODER)             += sgidec.o
>  OBJS-$(CONFIG_SGI_ENCODER)             += sgienc.o rle.o
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index 4067537..1eda64f 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -402,6 +402,7 @@ void avcodec_register_all(void)
>      REGISTER_DECODER (JACOSUB, jacosub);
>      REGISTER_DECODER (MICRODVD, microdvd);
>      REGISTER_DECODER (PGSSUB, pgssub);
> +    REGISTER_DECODER (SAMI, sami);
>      REGISTER_ENCDEC  (SRT, srt);
>      REGISTER_ENCDEC  (XSUB, xsub);
>  
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index e2b754f..952d846 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -426,6 +426,7 @@ enum CodecID {
>      CODEC_ID_MICRODVD   = MKBETAG('m','D','V','D'),
>      CODEC_ID_EIA_608    = MKBETAG('c','6','0','8'),
>      CODEC_ID_JACOSUB    = MKBETAG('J','S','U','B'),
> +    CODEC_ID_SAMI       = MKBETAG('S','A','M','I'),
>  
>      /* other specific kind of codecs (generally used for attachments) */
>      CODEC_ID_FIRST_UNKNOWN = 0x18000,           ///< A dummy ID pointing at the start of various fake codecs.
> diff --git a/libavcodec/samidec.c b/libavcodec/samidec.c
> new file mode 100644
> index 0000000..1a0da8d
> --- /dev/null
> +++ b/libavcodec/samidec.c
> @@ -0,0 +1,152 @@
> +/*
> + * Copyright (c) 2012 Clément Bœsch
> + *
> + * 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
> + * SAMI subtitle decoder
> + * @see http://msdn.microsoft.com/en-us/library/ms971327.aspx
> + */
> +
> +#include "ass.h"
> +#include "libavutil/avstring.h"
> +#include "libavutil/bprint.h"
> +
> +typedef struct {
> +    AVBPrint source;
> +    AVBPrint content;
> +    AVBPrint full;
> +} SAMIContext;
> +

> +static int sami_to_ass(AVCodecContext *avctx, const char *src)

maybe mention "sami_paragraph"

[...]
> diff --git a/libavformat/samidec.c b/libavformat/samidec.c
> new file mode 100644
> index 0000000..5245b63
> --- /dev/null
> +++ b/libavformat/samidec.c
> @@ -0,0 +1,164 @@
> +/*
> + * Copyright (c) 2012 Clément Bœsch
> + *
> + * 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
> + * SAMI subtitle demuxer
> + * @see http://msdn.microsoft.com/en-us/library/ms971327.aspx
> + */
> +
> +#include "avformat.h"
> +#include "internal.h"
> +#include "libavutil/avstring.h"
> +#include "libavutil/bprint.h"
> +#include "libavutil/intreadwrite.h"
> +
> +typedef struct {
> +    char buffer[1024];
> +    int buf_pos;
> +    AVPacket pkt;
> +    int pkt_cached;
> +} SAMIContext;
> +
> +static int sami_probe(AVProbeData *p)
> +{
> +    const unsigned char *ptr = p->buf;
> +
> +    if (AV_RB24(ptr) == 0xEFBBBF)
> +        ptr += 3;  /* skip UTF-8 BOM */
> +    return !strncmp(ptr, "<SAMI>", 6) ? AVPROBE_SCORE_MAX : 0;
> +}
> +
> +static int sami_read_header(AVFormatContext *s)
> +{
> +    SAMIContext *sami = s->priv_data;
> +    AVStream *st = avformat_new_stream(s, NULL);
> +
> +    if (!st)
> +        return -1;

AVERROR(ENOMEM)

[...]

Overall considerations: I see you're using ff_ass_to_subrect() to
convert SAMI paragraph -> ASS subtitle -> subrect.

The intermediary conversion SAMI -> ASS -> subrect though looks a bit
awkward so I would like you to shortly comment on that (e.g. why not a
direct conversion?).

Also I currently don't know how you're dealing with styles (SAMI seems
use CSS for definining the style of the various elements), especially
I don't know how FFmpeg keeps track of style in its internal
representation (if at all). I suppose full styling support may require
a full-featured CSS rendering engine but this would be overkill in
this case, but would make sense to annotate the current limitations in
a dedicated section in doc/decoders.texi.

As for what regards the parsing, I see that a bison/lex parsing tool
may help but again this will require external dependencies and thus
would be overkill just for this (same as an internal ff* parsing
engine).
-- 
FFmpeg = Fanciful and Forgiving MultiPurpose Elitist Geek