Message ID | 20210524103027.30367-1-emcodem@ffastrans.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] mxfdec.c: Try TC from Footer if Header TC was < 1 | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
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
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) +{ + 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; +} 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) { + 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; + } + flags = mxf_tc->drop_frame == 1 ? AV_TIMECODE_FLAG_DROPFRAME : 0; if (av_timecode_init(&tc, mxf_tc->rate, flags, mxf_tc->start_frame, mxf->fc) == 0) { + av_log(mxf->fc, AV_LOG_TRACE, "Setting Start Timecode: %d \n",mxf_tc->start_frame); mxf_add_timecode_metadata(&mxf->fc->metadata, "timecode", &tc); break; }