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

[Reimar Döffinger]

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.
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.

> >              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.

> >                  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.
I'd still like to see something similar to this patch in.