[FFmpeg-devel] [PATCH]Add Dirac support to ffmpeg via libdirac_* and Schroedinger libraries]

[Diego Biurrun]

On Fri, Apr 04, 2008 at 02:39:25PM +1100, Anuradha Suraparaju wrote:
> Sorry. Forgot to mention that the message is a patch in my earlier
> email.

Please don't top-post, you are going to become really unpopular in a
short time if you do.  I realize you are actually forwarding a message
right now.  I'm pointing this out just in case :)

> I have attached a patch to FFmpeg to enable Dirac compression system
> support via the libdirac_encoder/decoder libraries and Schroedinger
> libraries. The attached README has instructions as to how to apply the
> patch and enable Dirac support.

OK, the first question that comes to my mind is: What is the difference
between libdirac and libschrodinger and why would we need/want support
for both?

The second issue is that a native Dirac decoder and encoder was
developed during Google Summer of Code 2007, but left unfinished.
AFAIK the decoder is close to being finished.  Maybe you can help with
getting it merged into FFmpeg.

On to the actual patch...

Why is this a combined patch for both libs?  Please split it into
separate parts for libschrodinger and libdirac, this will be easier to
review

You need to change the names of the files and variables.  All external
libraries sport a "lib" prefix in their name in FFmpeg to differentiate
them from native implementations.

Please use "schrodinger" instead of "schro" as variable name and
filename.

> --- ffmpegsvn_trunk/configure	2008-04-04 09:05:46.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/configure	2008-04-04 09:06:15.000000000 +1000
> @@ -1640,6 +1658,8 @@
>  enabled mlib       && require  mediaLib mlib_types.h mlib_VectorSub_S16_U8_Mod -lmlib
> +enabled dirac  && add_extralibs `pkg-config --libs dirac` "-lstdc++" && extraincs="$extraincs `pkg-config --cflags dirac`"
> +enabled schro  && add_extralibs `pkg-config --libs schroedinger-1.0` && extraincs="$extraincs `pkg-config --cflags schroedinger-1.0`" && extraincs="$extraincs "

Please keep the columns aligned.

> @@ -1852,6 +1872,8 @@
>  enabled libtheora && append pkg_requires "theora"
>  enabled libvorbis && append pkg_requires "vorbisenc"
> +enabled dirac && append pkg_requires "dirac"
> +enabled schro && append pkg_requires "schro"

ditto

> @@ -2029,6 +2053,7 @@
>  fi
>  echo "LIB_INSTALL_EXTRA_CMD=${LIB_INSTALL_EXTRA_CMD}" >> config.mak
>  echo "EXTRALIBS=$extralibs" >> config.mak
> +echo "EXTRAINCS=$extraincs" >> config.mak

I'm suspicious of this change.

> --- ffmpegsvn_trunk/libavcodec/avcodec.h	2008-04-01 09:14:03.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavcodec/avcodec.h	2008-04-01 09:15:43.000000000 +1000
> @@ -182,6 +182,8 @@
>      CODEC_ID_8SVX_FIB,
>      CODEC_ID_ESCAPE124,
> +    CODEC_ID_SCHRO = 0xFFFE,
> +    CODEC_ID_DIRAC = 0xFFFF,

Why the number?

> --- ffmpegsvn_trunk/libavcodec/dirac.c	1970-01-01 10:00:00.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavcodec/dirac.c	2008-04-04 11:28:12.000000000 +1000
> @@ -0,0 +1,514 @@
> +/*
> + * Dirac support via Dirac libraries
> + * Copyright (c) 2005 BBC, Andrew Kennedy <dirac at rd dot bbc dot co dot uk>
> + * Copyright (c) 2006-2008 BBC, Anuradha Suraparaju <asuraparaju at gmail dot com >
> + *
> + * This file is part of FFmpeg.
> + *
> + * This library 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.
> + *
> + * This library 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 this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02111-1307 USA

Use a standard license header from other files.

> --- ffmpegsvn_trunk/libavcodec/dirac_schro_parser.c	1970-01-01 10:00:00.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavcodec/dirac_schro_parser.c	2008-04-04 11:28:45.000000000 +1000
> @@ -0,0 +1,228 @@
> +/*
> + * Common Dirac and Schroedinger parser
> + * Copyright (c) 2008 BBC, Anuradha Suraparaju <asuraparaju at gmail dot com >
> + *
> + * This file is part of FFmpeg
> + *
> + * This library 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.
> + *
> + * This library 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 this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02111-1307 USA
> + */

ditto

> +        if(p_videobuf_code[0]==0x42 && p_videobuf_code[1]==0x42 && p_videobuf_code[2]==0x43 && p_videobuf_code[3]==0x44)

Please break these ugly long lines.

> --- ffmpegsvn_trunk/libavcodec/dirac_schro_parser.h	1970-01-01 10:00:00.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavcodec/dirac_schro_parser.h	2008-04-04 11:29:23.000000000 +1000
> @@ -0,0 +1,73 @@
> +/*
> + * This library 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.
> + *
> + * This library 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 this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02111-1307 USA
> + */

license header once more

> +#ifndef FFMPEG_DIRAC_SCHRO_PARSER_H
> +#define FFMPEG_DIRAC_SCHRO_PARSER_H
> +
> + [...]
> +
> +#endif

Please comment the #endif.

> --- ffmpegsvn_trunk/libavcodec/Makefile	2008-04-03 09:32:52.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavcodec/Makefile	2008-04-03 09:33:23.000000000 +1000
> @@ -4,6 +4,8 @@
>  
> +CFLAGS += $(EXTRAINCS)

As I said, this is suspicious.

> --- ffmpegsvn_trunk/libavcodec/schroedinger.c	1970-01-01 10:00:00.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavcodec/schroedinger.c	2008-04-04 11:29:00.000000000 +1000
> @@ -0,0 +1,700 @@
> +/*
> + *
> + * This file is part of FFmpeg.
> + *
> + * This library 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.
> + *
> + * This library 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 this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02111-1307 USA
> + */

see above

> --- ffmpegsvn_trunk/libavcodec/schro_parser.c	1970-01-01 10:00:00.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavcodec/schro_parser.c	2008-04-04 11:29:11.000000000 +1000
> @@ -0,0 +1,29 @@
> +/*
> + * This file is part of FFmpeg
> + *
> + * This library 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.
> + *
> + * This library 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 this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02111-1307 USA
> + */

.. and again ..

> +#include "dirac_schro_parser.h"
> +
> +AVCodecParser schro_parser = {
> +    { CODEC_ID_SCHRO },
> +    sizeof(FfmpegDiracSchroParseContext),
> +    dirac_schro_parse_open,
> +    dirac_schro_parse,
> +    dirac_schro_parse_close,
> +};

I have a feeling this could be merge into the other file.

> --- ffmpegsvn_trunk/libavformat/allformats.c	2008-04-01 09:14:04.000000000 +1000
> +++ ffmpegsvn_trunk_dirac_schro/libavformat/allformats.c	2008-04-03 14:34:57.000000000 +1000
> @@ -68,6 +68,14 @@
> +    /* 
> +    * Register SCHRO demuxer before Dirac so that Schro is used as the default
> +    * codec to decode Dirac bytestreams
> +    */

Why?

Diego