AAF Import Feature Testing

Getting Stuff Done How do I .... ?
#41

Please say a lot more about how you’ve determined this.

#42

I placed a breakpoint at this line in ‘ARDOUR_UI::new_session_from_aaf()’ :-

std::shared_ptr region = create_region (source_regions, aafAudioClip, *oneClipSources, sessionStart, samplerate_r);

I then modified the AAF import so it’d only import the 2 problematic clips and once the breakpoint got reached, I traced into the code until I saw where the value 56244787200 got generated - which was in a call to boost::multiprecision::convert_to()

#43

That seems pretty clear but also seems pretty damning … to say that we’ve found an error in a widely used multiprecision arithmetic library is quite something (not unprecedented, of course)

#44

Well I guess another possibility might be rounding. ‘PBD::muldiv_round() is getting called, so maybe it’s rounding something up where it’d be better to round it down?

In 'ardour_ui_aaf.cc’ the function ‘create_region()’ calculates clipLen to be 9565440 which later equates to 56244787200 / 5880. Whereas the correct value should’ve been 9565082 which equates to 56242682160 / 5880. And according to my calculator, 9565440 == 9565082 x 1.000037428 which could easily be a rounding issue.

Still confused though… :confounded:

[Edit…[ Interestingly, 48000 * 1.000037428 and then converted to int would give 48001, so maybe this is a rounding error somewhere? I’ve no idea how we’d find it though.

#45

All our builds using gcc have int128, so the boost code is not be used.

#46

Thanks Robin. I’ll check again tomorrow but I’m pretty sure this problem also affected your official builds. And in any case, my original AAF importer was able to import much longer clips without this issue.

So I’m wondering if there’s somewhere in @agfline’s code where he stores the sample rate in floating point format and maybe that’s introducing a rounding issue.

#47

both use PBD_IDIV_ROUNDING and PBD_IDIV_ASR macros. Perhaps the isssue lies there.

So that is the number of samples in the file at 48kHz.

Converting this to superclock ticks:

9565082 [samples] * 282240000 [sc/sec] / 48000 [samples/sec] = 56242682160 [sc]

and there are ` 282240000 [sc/sec] / 48000 samples/sec = 5880 [sc/sample]

In both cases all the values are integers. There should be no need to round.
Also the largest value (282240000 * 9565082) is still < 2^52. int128_t should be able to handle this just fine. What are the actual arguments that are passed to muldiv_round ?

#48

wait… that calls libAAF’s aafi_convertUnit.

Does the session’s sample-rate not match the AAF’s rate, and the conversion goes wrong there?

#49

libaaf stores sample rate / size as unsigned int 32 / 64. However libaaf is applying conversion between AAF’s Edit Unit to sample rate… Maybe that’s where the issue is coming from ?

You can verify the libaaf values (clip position, length and essence offset) in here ardour/gtk2_ardour/ardour_ui_aaf.cc at master · Ardour/ardour · GitHub

[EDIT] Sorry I missed a few messages… I’m almost sure the value produced by aafi_convertUnit() in create_region() are based on the AAF sample rate (samplerate_r), being the most used sample rate across all AAF audio files, which does not mean it is the correct sample rate value for that audio file… Maybe that’s where the conversion is coming from ?

[EDIT 2] I just checked with Kris file using aaftool, and all essences appear to be 48kHz. However, I got the same results as @John_E for file length (9565082 samples) and clip length (9565440 samples) using aaftool. So there is definitly an issue with either libaaf, or the AAF file itself.

#50

Ok, I got it.

AAF file stores clip position and length as edit units with an associated edit rate. In this case, edit rate is FPS (25/1). The length of that clip is 4982 EU, giving 4982*(48000/25) = 9565440.

1 Like
#51

Thanks Adrien - so do you have enough information to fix this or do you need me to do some more testing? A length of 9565440 would equalte (in hex) to 91f500 but I don’t see that value anywhere in the AAF file itself.

#52

The length of 9565440 is in audio sample unit, it is calculated by libaaf (based on the calculation in my previous message, handled by aafi_convertUnit()).

The length stored in the AAF file is expressed in edit unit, that is 4982 (0x1376 in hex).

There is no need for more test right now. I’ll make a sanitizing function to be sure that clip length and source file offset doesn’t exceed source file size.

#53

@x42 - it looks like @agfline has already implemented his sanitizing function but you’ll need to liaise to figure out how / where it needs to get implemented :+1:

#54

@John_E e already sent 2 pull requests yesterday, which were merged. I assume there’ll be more to come once a new version of libaaf is tagged and released.

#55

Yes I have, but please give me more time to do some testing :wink: Besides, the fix for locating media files inside sub-directories still has to be improved.

I’ll make PRs to update libaaf and ardour_ui_aaf.cc when I’m finished, so don’t worry with the implementation.

2 Likes
#56

This topic was automatically closed 365 days after the last reply. New replies are no longer allowed.