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

[Dale Curtis]

On Tue, Apr 10, 2012 at 11:46 AM, Reimar Döffinger <Reimar.Doeffinger at gmx.de
> wrote:

> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Thanks for the comments Reimar. I'm in the process of reviewing all of the
Chrome patches accumulated over the years and trying to un-fork us from
mainline FFmpeg; this is the first of those patches.

After digging through the history on this patch, it looks like this patch
was added to reduces the size of the ffmpeg library used by Chrome.
Retesting this assumption with the current code base and our configure
options shows an ~4kb uncompressed improvement with these patches and ~0kb
compressed improvement. Which is not worth the trouble.

While digging through the above, I also found the build problem with
ac3/mov was on our end. This patch can be abandoned.

- dale