[FFmpeg-devel] [PATCH] mxfdec.c: prefer metadata from Footer

[Tomas Härdin]

sön 2021-06-27 klockan 12:03 +0200 skrev Marton Balint:
> 
> On Sat, 26 Jun 2021, emcodem wrote:
> 
> > In case there is a Footer, regarding to SMPTE 377 all versions, the metadata in Footer shall be correct (where in Header it can be incomplete)..
> > If there is no footer (stream, truncated...) it will still work as usual.
> > Tested with a huge set of files and compared old/new ffprobes, it will not change lots of metadata, mainly duration and in some cases start timecode.
> > Without this change, especially Duration would be often inaccurate because it is unknown in header and calculated from bitrate.
> > 
> > The new sample files should be added to \fate-suite\mxf, as i do not have an account to upload the files, i shared them on wetransfer:
> > 
> > https://we.tl/t-MVmyG2mZHq
> > 
> > omneon_6.4.1.0.1_xdcam_truncated.mxf
> > An original Omneon File from an older Version, file is truncated. It shall prove that Metadata is being parsed even when there is no Footer.
> > 
> > omneon_8.3.0.0_xdcam_startc_footer.mxf
> > An original Omneon File from a recent Version with "better" Metadata in Footer than in Header. I needed to hexedit this file and set the MP and SP start timecode in header to 0.
> > This test is for proving that metadata from Footer is preferred.
> > 
> > ---
> > libavformat/mxfdec.c                      |   2 +-
> > tests/fate/mxf.mak                        |  10 +
> > tests/ref/fate/mxf-probe-xdcamhd-oit      | 442 ++++++++++++++++++++++
> > tests/ref/fate/mxf-probe-xdcamhd-tcfooter | 442 ++++++++++++++++++++++
> > 4 files changed, 895 insertions(+), 1 deletion(-)
> > create mode 100644 tests/ref/fate/mxf-probe-xdcamhd-oit
> > create mode 100644 tests/ref/fate/mxf-probe-xdcamhd-tcfooter
> > 
> > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > index 7b40076fb4..a7f552c753 100644
> > --- a/libavformat/mxfdec.c
> > +++ b/libavformat/mxfdec.c
> > @@ -1402,7 +1402,7 @@ static void *mxf_resolve_strong_ref(MXFContext *mxf, UID *strong_ref, enum MXFMe
> > 
> >     if (!strong_ref)
> >         return NULL;
> > -    for (i = 0; i < mxf->metadata_sets_count; i++) {
> > +    for (i = mxf->metadata_sets_count-1; i >= 0; i--) {
> 
> Do we have to store all metadata in the first place? Why not do the 
> decision which one is better when adding the metadata set?

This could be done, but it entails rewriting much of the demuxer

> Order of 
> parsing depends on the file, we usually do header, footer and then 
> backwards the partitions. So after your patch metadata in the second 
> partition will have priority over footer, which is also against spec I 
> believe.

I was going to say this is not right because mxf_read_partition_pack()
will reverse the order in which partitions are added when parsing
backwards, so everything ends up in the correct order. But this doesn't
apply to mxf->metadata_sets, only mxf->partitions..

We've had a similar discussion on here roughly a month ago. There's a
bit of finessing around the order in which to prefer any one metadata
set. IIRC something like: FooterPartition, then any Complete non-footer 
Partition, then the latest OpenPartition

/Tomas