[FFmpeg-devel] mxfdec.c: Try TC from Footer if Header TC was < 1

Added support for reading Start Timecode from Footer (if any). 
Specifically targets Omneon 6.4.3.0 but also works on other Versions and Vendors, e.g. when Header is OpenIncomplete.
Function mxf_resolve_strong_ref_reverse can potentially be re-used for getting other values like Duration and 
Origin from Footer.
---
 libavformat/mxfdec.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

mån 2021-05-24 klockan 12:30 +0200 skrev emcodem:
> Added support for reading Start Timecode from Footer (if any). 
> Specifically targets Omneon 6.4.3.0 but also works on other Versions and Vendors, e.g. when Header is OpenIncomplete.
> Function mxf_resolve_strong_ref_reverse can potentially be re-used for getting other values like Duration and 
> Origin from Footer.

This needs a sample and a test

> ---
>  libavformat/mxfdec.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 3bf480a3a6..557e01f8ed 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1396,6 +1396,19 @@ static const MXFCodecUL *mxf_get_codec_ul(const MXFCodecUL *uls, UID *uid)
>      return uls;
>  }
>  
> +static void *mxf_resolve_strong_ref_reverse(MXFContext *mxf, UID *strong_ref, enum MXFMetadataSetType type)

This and mxf_resolve_strong_ref() could maybe be merged to one function
with an "int dir" option, and mxf_resolve_strong_ref() just calling it
with the value 1.

> +{
> +    int i;
> +    if (!strong_ref)
> +        return NULL;
> +    for (i = mxf->metadata_sets_count-1; i >= 0 ; i--) {
> +        if (!memcmp(*strong_ref, mxf->metadata_sets[i]->uid, 16) &&
> +            (type == AnyType || mxf->metadata_sets[i]->type == type)) {
> +            return mxf->metadata_sets[i];
> +        }
> +    }
> +    return NULL;
> +}

Missing newline, but I can add that locally

>  static void *mxf_resolve_strong_ref(MXFContext *mxf, UID *strong_ref, enum MXFMetadataSetType type)
>  {
>      int i;
> @@ -2328,8 +2341,15 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
>                  continue;
>  
>              mxf_tc = (MXFTimecodeComponent*)component;
> +            if (mxf_tc->start_frame <= 0) {

I feel like this should trigger on OpenIncomplete instead. I wouldn't
be surprised if start_frame < 0 is perfectly valid.


> +            if (mxf_tc->start_frame <= 0) {
> +                    av_log(mxf->fc, AV_LOG_TRACE, "Header Start Timecode was %d, trying reversed parsing\n",mxf_tc->start_frame);
> +                    component = mxf_resolve_strong_ref_reverse(mxf, &material_track->sequence->structural_components_refs[j], TimecodeComponent);
> +                    mxf_tc = (MXFTimecodeComponent*)component;

Wrong indent. I was also going to comment that mxf_tc might end up NULL
here but it can't since it's non-NULL when resolving in the forward
direction.

/Tomas

Am 2021-05-24 17:31, schrieb Tomas Härdin:
> mån 2021-05-24 klockan 12:30 +0200 skrev emcodem:
>> Added support for reading Start Timecode from Footer (if any).
>> Specifically targets Omneon 6.4.3.0 but also works on other Versions 
>> and Vendors, e.g. when Header is OpenIncomplete.
>> Function mxf_resolve_strong_ref_reverse can potentially be re-used for 
>> getting other values like Duration and
>> Origin from Footer.
> 
> This needs a sample and a test
Sure, Will add.

> 
>> ---
>>  libavformat/mxfdec.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>> 
>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> index 3bf480a3a6..557e01f8ed 100644
>> --- a/libavformat/mxfdec.c
>> +++ b/libavformat/mxfdec.c
>> @@ -1396,6 +1396,19 @@ static const MXFCodecUL *mxf_get_codec_ul(const 
>> MXFCodecUL *uls, UID *uid)
>>      return uls;
>>  }
>> 
>> +static void *mxf_resolve_strong_ref_reverse(MXFContext *mxf, UID 
>> *strong_ref, enum MXFMetadataSetType type)
> 
> This and mxf_resolve_strong_ref() could maybe be merged to one function
> with an "int dir" option, and mxf_resolve_strong_ref() just calling it
> with the value 1.
> 

Will delete the revers function and alter the mxf_resolve_strong_ref as 
suggested.

>> +{
>> +    int i;
>> +    if (!strong_ref)
>> +        return NULL;
>> +    for (i = mxf->metadata_sets_count-1; i >= 0 ; i--) {
>> +        if (!memcmp(*strong_ref, mxf->metadata_sets[i]->uid, 16) &&
>> +            (type == AnyType || mxf->metadata_sets[i]->type == type)) 
>> {
>> +            return mxf->metadata_sets[i];
>> +        }
>> +    }
>> +    return NULL;
>> +}
> 
> Missing newline, but I can add that locally
> 
>>  static void *mxf_resolve_strong_ref(MXFContext *mxf, UID *strong_ref, 
>> enum MXFMetadataSetType type)
>>  {
>>      int i;
>> @@ -2328,8 +2341,15 @@ static int 
>> mxf_parse_structural_metadata(MXFContext *mxf)
>>                  continue;
>> 
>>              mxf_tc = (MXFTimecodeComponent*)component;
>> +            if (mxf_tc->start_frame <= 0) {
> 
> I feel like this should trigger on OpenIncomplete instead. I wouldn't
> be surprised if start_frame < 0 is perfectly valid.

OK thats an interesting discussion, from my perspective there is a 
little gray area here and what i did should deliver the best effort. My 
thoughts are:
*) openincomplete may or may not carry the correct value, it is not 
guaranteed to carry the wrong value
*) there may not be a footer (file truncated, file growing)
*) if a footer is there, it may or may not repeat the header metadata
*) all 377m versions say "if values are unknonwn (open/incomplete), one 
must use the "distinguished value", but none of the 377m up to 2011 
define such a value for the start timecode - BUT  my guess was that this 
distinguished value shall be a valid TC, instead of e.g. "-1" (FFFF...) 
because the value -1 would potentially fail in the TC parsing code 
(thinking globally here, not only for libav*)
*) if the Start TC is actually 0 AND there is a Footer Metadata 
repetition, my code still works because it will take the value 0 from 
the footer

So after all i believe checking for "openincomplete" would be kind of 
superfluous for this usecase. Checking if there is another value than 0 
in the footer does not really hurt and as the footer is guaranteed to 
carry the correct timecode, it should never return a really wrong value.
The really correct solution i believe would be to parse ALL metadata 
from footer (if any) first but i don't want to change the mxfdec.c 
fundamentally just for this sidecase.

> 
> 
>> +            if (mxf_tc->start_frame <= 0) {
>> +                    av_log(mxf->fc, AV_LOG_TRACE, "Header Start 
>> Timecode was %d, trying reversed parsing\n",mxf_tc->start_frame);
>> +                    component = mxf_resolve_strong_ref_reverse(mxf, 
>> &material_track->sequence->structural_components_refs[j], 
>> TimecodeComponent);
>> +                    mxf_tc = (MXFTimecodeComponent*)component;
> 
> Wrong indent. I was also going to comment that mxf_tc might end up NULL
> here but it can't since it's non-NULL when resolving in the forward
> direction.

Yeah, in worst case the reverse parsing should return exactly the same 
value as the forward parsing did, so i guess we are safe here. My 
intention was to be backward compatible as far as possible.

> 
> /Tomas
> 

Hi Tomas, thanks for the quick reply.
OK i'll try to get hold of a short example file, upload to the fate 
suite and add a test. If i cannot get a very short sample (XDCAM, ~1-2 
GOP's), i'll make some up by altering values using hex editor if thats 
ok for you.
Other comments inline, please let me know your thougts - AND as i am 
pretty new to sending patches here, please also let me know if you like 
me to send ONE patch with the new code for mxfdec.c AND the test or you 
want me to send it separately?

Thanks,
-emcodem