[FFmpeg-devel] [PATCH] mxfdec: do not fail on error parsing footer.

[Tomas Härdin]

On Tue, 2012-01-10 at 20:16 +0100, Reimar Döffinger wrote:
> On Tue, Jan 10, 2012 at 03:24:33PM +0100, Tomas Härdin wrote:
> > On Thu, 2012-01-05 at 22:06 +0100, Reimar Döffinger wrote:
> > 
> > Oops, didn't notice this thread until Michael pointed it out to me.
> > 
> > > Failing it play a file just because some metadata was not parseable
> > > is overkill.
> > > This should fix trac issue #879.
> > 
> > Uhm, 10bit_yuv_j2k.mxf plays fine with current master and
> > --enable-libopenjpeg.
> > It seems we can't fix the ticket properly without the whole sample file.
> > I'd ask of the reporter to upload it somewhere.
> 
> Yes, I commented out the code that checks for seek failure with that
> file, which has kind of the same effect as a corrupted footer.
> 
> > > Signed-off-by: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
> > > ---
> > >  libavformat/mxfdec.c |    7 ++++---
> > >  1 files changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index 10512ef..66af2e4 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -1634,7 +1634,7 @@ static int mxf_read_header(AVFormatContext *s, AVFormatParameters *ap)
> > >  
> > >              if (!mxf->current_partition) {
> > >                  av_log(mxf->fc, AV_LOG_ERROR, "found essence prior to first PartitionPack\n");
> > > -                return AVERROR_INVALIDDATA;
> > > +                goto handle_essence;
> > 
> > #879 doesn't list this error. Files that trigger this violate the spec
> > hard. Also, mxf_parse_structural_metadata() would create zero streams
> > due to no metadata, so definately not OK.
> 
> I can't see that, it seems to me it is perfectly possibly for
> mxf_read_content_storage etc. being called without any
> mxf_read_partition_pack.

True, it's definately possible.

> Anyway it seemed way easier to me instead of overthinking it
> to just follow the simple rule of not failing when there's
> no need to.

I'd me more open to this if you could find a not-too-contrived sample. I
know of no writer that outputs files like this (and I'd send an angry
mail to its developers if I did).

> > >              if (!mxf->current_partition->essence_offset) {
> > > @@ -1688,14 +1688,14 @@ static int mxf_read_header(AVFormatContext *s, AVFormatParameters *ap)
> > >                      if (avio_tell(s->pb) > next) {
> > >                          av_log(s, AV_LOG_ERROR, "read past end of KLV @ %#"PRIx64"\n",
> > >                                 klv.offset);
> > > -                        return AVERROR_INVALIDDATA;
> > > +                        goto handle_essence;
> > 
> > Not relevant to the ticket AFAICT. I'd rather be more stringent with
> > what the demuxer accepts, but I guess trying to make sense of the file
> > anyway doesn't hurt..
> 
> Hm, we do have a strict flag. I wouldn't mind at all making everything
> hard failures by default (even if it's not what I prefer) but I really
> dislike not giving the user a chance to ignore errors that really
> don't need to be fatal.

Strictness flag is a pretty good idea actually..

> > >                  if (res < 0) {
> > >                      av_log(s, AV_LOG_ERROR, "error reading header metadata\n");
> > > -                    return -1;
> > > +                    goto handle_essence;
> > 
> > This is the wrong place to fix #879 - my guess is
> > mxf_read_partition_pack() needs to be a bit more clever. Might be OK on
> > its own though, just like the previous hunk.
> 
> I agree with you I was a bit silly to stop handling the bug, the user
> should try to provide the sample file with the middle zero'd out or
> such.

dd:ing off the header and footer and filling with zeroes should work
well.

> I'd still like to see something similar to this patch in.

As long as it's an option that's off by default. I'd like any hacks for
noncompliant files to be justified with at least one sample. Then we can
enable the needed hacks by default.

For all of these I think a good rule of thumb is "if mxfdump/mxfsplit
are able to make sense of the file then we should too".

/Tomas