library prints to stdout/err

[x42]

It is bad practice for a library to print to stdout/stderr.

Say you write an application that can directly to output to stdout. This will not be possible if any shared library will also produce output there.

Common practice is to either only optionally enable debug messages for the shared-lib, or alternatively add a logger callback. Then the front-end application could set a log callback to receive any messages.

[agfline]

You're right, it's already in my todo list ;) Is this a priority issue for Ardour ?

[x42]

Great to hear that it's already on your list! and no, it is not high priority, I have just reported it so that it won't be forgotten.

[johne53]

Could this get solved by building libaaf as a static library, rather than a dynamic one (the printouts would then be coming from the relevant executable...)

I haven't reached the stage yet of creating a libaaf build project for MSVC but I must admit, I'd been considering whether to use the static build model anyway, for other reasons.

[johne53]

I haven't reached the stage yet of creating a libaaf build project for MSVC

I might give this a try during the weekend. @agfline - could you maybe give me a list of which of your 'C' files I'll need when building libAAF?

[Update...] It's okay, I figured it out from reading CMakeLists.txt and so far, it's all looking pretty good - however... I suspect that Ardour's version of the AAF library stuff might be a commit or two out of date. Basically, I can build libAAF.lib here and it seems to work - except that I'm back to seeing those strange concatenation issues and the directory separator issue that you just fixed. @agfline - would you mind letting me know please, once you've had a chance to bring Ardour's aaf branch is up to date? Thanks, John

[agfline]

I finally implemented that callback function to handle debug and logging. Everything should be up to date now, on both Ardour and libaaf side.

[johne53]

Thanks Adrien - with regard to your aaf branch in Ardour, I think you forgot to add the new debug.c file.

[agfline]

Thanks John. Don't know if you get notified when I commit. So just in case, it's fixed ;)

[johne53]

@Robin - back in the old ArdourXchange days I used to generate session names based on the name of the supplied AAF file. So if the filename was ABcde.aaf, the session would be called ABcde.ardour

But @Adrien finds the name by extracting it from the AAF file and I've come across one file where the session name is "Coros LO " (notice that the last character is a space character...) When generating a sub-folder for the interchange files, it then tries to use a name ending in a space character - which fails on Windows because directories aren't allowed to end in a space character. It then occurred to me that the internal name might conceivably contain other invalid characters, such as a colon.

I'm pretty sure that Ardour already contains a function for validating file names but I'm not sure if it knows about the fact that directories can't end in a space. Can you remind me what's the name of that function (so I can check) ?

[agfline]

I did actually made some quick and dirty cleaning of the AAF composition name, but only for the cache folder and not for Ardour session.

https://github.com/agfline/ardour_aaf_support/blob/dcd3ae34066fe071975a74ab29f020612682a3ee/session_utils/new_aaf_session.cc#L844-L860

I didn't know about this windows rule. If Ardour has a proper function for cleaning file names I'll go with it, otherwise I'll improve my code.

[johne53]

I discovered 'ARDOUR::legalize_for_path()' and also 'ARDOUR::legalize_for_universal_path()' which both look at the supplied string and replace any illegal characters with an underscore. But for Windows use, I'm wondering if 'ARDOUR::legalize_for_path()' should also be converting any colon characters, if found. And in both cases (again for Windows use) they should maybe replace the last character with an underscore if the last character happens to be a space? Needs some thinking about, I guess :-(

Maybe it'd be safer if Adrien could use the older system (as in ArdourXchange) which utilized the supplied filename, rather than interrogating the AAF file to find a name. At least that way, the name would be guaranteed to be valid for the given platform. In fact, I think I used that method because the internal name was often "Untitled".

[johne53]

I'm wondering if 'ARDOUR::legalize_for_path()' should also be converting any colon characters, if found.

By a strange coincidence I've just encountered that situation today! I've a small AAF file here which contains a short tone blip on the timeline - and the embedded clip's name begins with the text "TONE:" so it fails to create any audio for that clip.

Adrien - if you can let me know whereabouts an audio clip gets created I could try adding a call to 'ARDOUR::legalize_for_universal_path()' and see if that helps. I assumed it'd get handled in 'create_region()' somehow but I can't seem to figure out where :-(

[agfline]

I made a clean_filename() function which also replace last character in case it's a space or a dot.

Regarding AAF specification, the composition Mob::Name should identify the composition (that is, the timeline, project or session name, depending on software) and this was confirmed by all the files I've been testing so far.

The normal new_aaf_session behavior is to stick to that composition name and fallback to the AAF file name in case its empty. You also have an option to force session name to whatever you want using --session-name.

I just modified this option so you can force using the AAF file name : --session-name AAFFILE (AAFFILE is a keyword, do not replace it with your file name)

[johne53]

Thanks Adrien. I updated this morning and things are much better now. There's a small build issue in lines like this that you've added to 'utils.c':-

switch ( c ) {

  case L'à':
  case L'á':
  case L'â':
  case L'ã':
  case L'ä':

MSVC doesn't like the 'L' macro here (possibly because 'c' is already in wchar format?) but it compiles okay with Clang. And the really good news is that these changes have fixed quite a few AAF files here which previously weren't importing. In fact I've only one AAF file now which won't import. It generates the audio clips okay and then crashes while trying to create the first track:-

[i] new_aaf_session.cc : main() on line 1301 : Source file '865_2287_01_2' successfully imported to session.
[i] new_aaf_session.cc : main() on line 1301 : Source file '865_2287_01_1' successfully imported to session.
[i] new_aaf_session.cc : main() on line 1301 : Source file '865_2287_01' successfully imported to session.
[i] new_aaf_session.cc : prepare_audio_track() on line 585 : Track number 0 (Track 5) does not exist. Adding new track.
      ^ ^ ^ CRASHES AFTER PRINTING THE ABOVE LINE ^ ^ ^

It's quite a small AAF file (28MB) so possibly email-able? Should I try and email it to you?

@x42 - you and Paul will need to start considering how to integrate this for users. It's currently implemented as a console app with a large number of command-line options so my gut feeling is that it'll be too confusing for users. Maybe a dedicated Ardour dialog along the lines of Session->Import PT session?

[agfline]

Good news. Of course John, send me your file so I can look at it ;)

There's a small build issue in lines like this that you've added to 'utils.c'

I'll comment this out, there is finally no use for this function.

For my part, I still need some help to implement track levels / pan into Ardour. In the meantime, I'm doing code review and cleanup. A few major improvements to come with low level AAF parsing, those will require some testing on windows to ensure there is no drawback.

We're getting close !

[johne53]

I've just sent you a zipped up version of the problematic AAF file although it was still pretty big - so check your spam folder if you don't receive it.

[agfline]

Problem fixed. libAAF and Ardour are up to date.

[johne53]

Well done Adrien - all my AAF files here are importing correctly now!!

Let me know when you've completed your low-level changes and I'll be happy to re-test the AAF's here.

[agfline]

Let me know when you've completed your low-level changes and I'll be happy to re-test the AAF's here.

Well, you already have ! I pushed the low-level changes before the fix for your file ;) So everything is looking good.

Until we add support for track level / pan, I'll keep on doing code review and cleanup. I will also make more lightweight copyright-free test files for the test.sh script.

Thank you John for your time and efforts ;)

[johne53]

Yes I re-checked my AAF files here and they're all still importing correctly. So hopefully Paul and Robin can start thinking about the UI and integration features once they get some time. And it'd be nice to accommodate your progress log if poss.

[agfline]

Sure. Regarding integration, there will be a few things to consider like how user can select a folder containing external media files, maybe make a few things optional like import track names, track pan, track volume, clip gain, timecode etc.

Protools has one of the best UI for importing AAF :

I should also make new_aaf_session.cc more readable, it's still too messy. Maybe you guys have a .clang-format file ?

And it'd be nice to accommodate your progress log if poss.

What do you mean ? Are they too verbose ? Maybe they should go to a dedicated log file in session folder, so users can request support with it ?

[johne53]

Yea a log file for the last import could be helpful at times - but for normal use, it'd be good to display some simplified feedback during each import. It's very reassuring for users just to see the various clip names / tracks etc as they're each getting processed.

And we've now gone full circle (going back to Robin's first post...) as all this will eliminate the need to be printing stuff to stdout !!

[agfline]

So all the normal user logging (stderr/stdout) should be produced by Ardour itself (importing audio file, creating track, etc.). That is already the case in new_aaf_session.cc. And all the library-side log should be written to the file. I will make a callback for it in new_aaf_session.cc.

[x42]

@x42 - you and Paul will need to start considering how to integrate this for users.

The eventual goal is to integrate it like Pro Tools import. Menu > Session > Import AAF will add the aaf to the existing session.

[x42]

Maybe you guys have a .clang-format file ?

Yes, we do. copy or symlink tools/clang-format from ardour's source tree

[x42]

I consider this issue completed. Perhaps that discussion should continue elsewhere?

[johne53]

Before we close this thread...

The eventual goal is to integrate it like Pro Tools import. Menu > Session > Import AAF will add the aaf to the existing session.

Whenever you've time, is there some way you could work with @agfline to create (initially) a simplified import dialog - i.e. so we can at least make something available for beta testing?

I mentioned in https://github.com/agfline/LibAAF/issues/5#issuecomment-1657100629 that libaaf is working now in Mixbus as well as Ardour and I'm pretty sure there'd be Mixbus users willing to help test AAF.

[agfline]

The AAF import should work without any specific dialog, just by selecting the AAF file. However, we should consider to make a specific import dialog in the future with a few options, especially the ability for the user to select a directory that contains external audio files.

[johne53]

I think what Robin's saying is that an import window is needed, similar to what's already available when importing from Pro Tools (i.e. users are already familiar with that workflow). And he's recommending that the style of import should match the PT import style - where new tracks get added to an already loaded session, rather than creating a whole new session each time.

So in addition to create_new_session() we'd need an extra function called something like import_into_session()

Maybe one of us should create a new thread where we can start suggesting the steps needed so we can get to the stage of making this beta-testable?

[x42]

And he's recommending that the style of import should match the PT import style

Yes, I was really just thinking of a file-select dialog to get the ball rolling. Fancy import options dialog can come later.

[johne53]

New thread started here