[FFmpeg-devel] [PATCH] Obey configure updates for OGG, Matroska, and MOV parsers.

[Reimar Döffinger]

On Tue, Apr 10, 2012 at 11:09:18AM -0700, dalecurtis at chromium.org wrote:
> From: Dale Curtis <dalecurtis at chromium.org>
> 
> Uses #if checks to remove code paths which are invalid or unused
> depending on configure options.
> 
> Signed-off-by: Dale Curtis <dalecurtis at chromium.org>
> ---
>  libavformat/matroskadec.c |   38 +++++++++++++++++++++++++++-----------
>  libavformat/mov.c         |    4 ++++
>  libavformat/oggdec.c      |   10 ++++++++++
>  3 files changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 6d7401b..a6fec8a 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -35,13 +35,17 @@
>  /* For ff_codec_get_id(). */
>  #include "riff.h"
>  #include "isom.h"
> +#if CONFIG_SIPR_DECODER
>  #include "rm.h"
> +#endif
>  #include "matroska.h"
>  #include "libavcodec/mpeg4audio.h"
>  #include "libavutil/intfloat.h"
>  #include "libavutil/intreadwrite.h"
>  #include "libavutil/avstring.h"
> +#if HAVE_LZO1X_999_COMPRESS
>  #include "libavutil/lzo.h"
> +#endif

We don't put headers under #if unless necessary.
And in these cases there is no point.
Also HAVE_LZO1X_999_COMPRESS is about LZO compression,
that header provides _de_compression.
And is small enough that there isn't really a point in
not including it.

> @@ -1581,8 +1590,10 @@ static int matroska_read_header(AVFormatContext *s)
>          } else if (codec_id == CODEC_ID_RA_144) {
>              track->audio.out_samplerate = 8000;
>              track->audio.channels = 1;
> -        } else if (codec_id == CODEC_ID_RA_288 || codec_id == CODEC_ID_COOK ||
> -                   codec_id == CODEC_ID_ATRAC3 || codec_id == CODEC_ID_SIPR) {
> +        } else if ((CONFIG_RA_288_DECODER && codec_id == CODEC_ID_RA_288) ||
> +                   (CONFIG_COOK_DECODER && codec_id == CODEC_ID_COOK) ||
> +                   (CONFIG_ATRAC3_DECODER && codec_id == CODEC_ID_ATRAC3) ||
> +                   (CONFIG_SIPR_DECODER && codec_id == CODEC_ID_SIPR)) {

This is just wrong. That we don't have the decoder doesn't mean it is ok
to demux it incorrectly. Like this an FFmpeg configured without these
decoders will create a broken file when remuxing with -acodec copy.
Not to mention players like MPlayer that might use libavformat for
demuxing but e.g. the original Real codec binaries for decoding.
Why should you have to build in the decoders into FFmpeg then just to
play them?

> @@ -570,6 +570,7 @@ static int mov_read_esds(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>      return ff_mov_read_esds(c->fc, pb, atom);
>  }
>  
> +#if CONFIG_AC3_DEMUXER
>  static int mov_read_dac3(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  {
>      AVStream *st;
> @@ -593,6 +594,7 @@ static int mov_read_dac3(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  
>      return 0;
>  }
> +#endif

I don't see what demuxing AC3 in mov has to do with whether the AC3
demuxer is enabled?

> diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> index bdd2c5b..731d7f2 100644
> --- a/libavformat/oggdec.c
> +++ b/libavformat/oggdec.c
> @@ -39,14 +39,24 @@
>  
>  static const struct ogg_codec * const ogg_codecs[] = {
>      &ff_skeleton_codec,
> +#if CONFIG_DIRAC_DEMUXER
>      &ff_dirac_codec,
> +#endif

DIRAC_DEMUXER enables/disables the special dirac format demuxer,
you should not disable demuxing of dirac in ogg because of it.
Also I didn't see any changes that would actually remove the
ff_dirac_codec structure and associated code.