aaf session import #805
Closed
aaf session import #805
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks this looks promising. I think we should include libAAF in Ardour source-tree (similar to libs/ptformat, libs/libltc, libs/zita-resampler, etc). This will make things significantly easier with cross-platform builds. Might even build it as static library. I suggest to also remove all commented code, and excessive whitespace. For the latter we can also just pass it through Meanwhile I will investigate the crash in |
x42
reviewed
Jun 15, 2023
session_utils/common.cc
Outdated
Comment on lines
252
to
273
| ARDOUR::cleanup (); | ||
| /* | ||
| ==151729== Process terminating with default action of signal 11 (SIGSEGV) | ||
| ==151729== Access not within mapped region at address 0x210 | ||
| ==151729== at 0x5E5934B: RCUWriter<std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::shared_ptr<ARDOUR::Port>, ARDOUR::PortManager::SortByPortName, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::shared_ptr<ARDOUR::Port> > > > >::RCUWriter(RCUManager<std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::shared_ptr<ARDOUR::Port>, ARDOUR::PortManager::SortByPortName, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::shared_ptr<ARDOUR::Port> > > > >&) (rcu.h:288) | ||
| ==151729== by 0x5E4EF11: ARDOUR::PortManager::unregister_port(std::shared_ptr<ARDOUR::Port>) (port_manager.cc:675) | ||
| ==151729== by 0x60BB829: ARDOUR::TransportMaster::unregister_port() (transport_master.cc:463) | ||
| ==151729== by 0x60B935A: ARDOUR::TransportMaster::~TransportMaster() (transport_master.cc:89) | ||
| ==151729== by 0x5CA0D6D: ARDOUR::TimecodeTransportMaster::~TimecodeTransportMaster() (transport_master.h:482) | ||
| ==151729== by 0x5D7FCC2: ARDOUR::MTC_TransportMaster::~MTC_TransportMaster() (mtc_slave.cc:73) | ||
| ==151729== by 0x5D7FCE9: ARDOUR::MTC_TransportMaster::~MTC_TransportMaster() (mtc_slave.cc:76) | ||
| ==151729== by 0x60BF853: std::_Sp_counted_ptr<ARDOUR::MTC_TransportMaster*, (__gnu_cxx::_Lock_policy)2>::_M_dispose() (shared_ptr_base.h:380) | ||
| ==151729== by 0x1336EE: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (shared_ptr_base.h:158) | ||
| ==151729== by 0x133498: std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() (shared_ptr_base.h:733) | ||
| ==151729== by 0x5A59217: std::__shared_ptr<ARDOUR::TransportMaster, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() (shared_ptr_base.h:1183) | ||
| ==151729== by 0x5A59259: std::shared_ptr<ARDOUR::TransportMaster>::~shared_ptr() (shared_ptr.h:121) | ||
| ==151729== If you believe this happened as a result of a stack | ||
| ==151729== overflow in your program's main thread (unlikely but | ||
| ==151729== possible), you can try to increase the size of the | ||
| ==151729== main thread stack using the --main-stacksize= flag. | ||
| ==151729== The main thread stack size used in this run was 8388608. | ||
| */ | ||
| // ARDOUR::cleanup (); |
|
No problem for adding libAAF sources to Ardour source-tree. However a few things to consider :
Since there is still some work to do, what's the better approach in your opinion ? Wait a little ? Make a specific branch ? Merge with master so people can test ? I'm quite new to large project contributions on github... ;) Regarding code style, I obviously should configure tab size in my editor... But regarding commented code, are you talking about libAAF sources or |
Do please check with Paul. He's already had a bad experience with OMF and he's stated multiple times that he doesn't want AAF in Ardour.. If Paul's still reluctant I'd suggest creating a Mixbus branch first and then merging into Master once it's proved itself. And hopefully by then, Paul might be happy to backport it? |
|
Well... I don't know the history about Ardour and OMF, however the lack of AAF support keeps Ardour (thus the only GPL solution) away from the entire world of audiovisual post-production, broadcast and film. Plus, the way the integration is proposed here does not impact Ardour itself, since it remains as a side-program for now. I don't know much about Mixbus. I understand it shares a large part of its sources with Ardour, however is it also open source / GPL ? Is there a repos somewhere ? Otherwise, libAAF is GPL and anyone willing to implement it is free to do so. |
|
I am certainly not prepared to maintain support for the actual parsing and handling of AAF, due to its monstrous design. But I don't have any objection to a 3rd party library and then support being built around that. My only concern is that there will likely be issues inside libaaf that show up as it gets wider use, and I don't want people involved more directly with Ardour to have to deal with them. Otherwise, this is great! |
|
I would assume that the relationship will be similar to https://github.com/zamaudio/ptformat, where we keep issues specific to ProTools session format separate. Changes happen upstream first, and we regularly pull in stable versions of libptformat into Ardour's source tree for convenience (same for libltc etc). Anyway, if the library works on GNU/Linux, it should be relatively straight forward to also make it run on *BSD, and macOS. If you only use POSIX API then no change is needed. I will prepare Ardour's GUI, e.g. separate |
|
PS. I really hate cmake. It has the worst syntax of every build system, and does not heed any standard env variables or established parameters. meson FTW. That being said: With the old Makefile I was able to compile libAAF on macOS. I cannot test it but on ARM (AppleSilicon) systems I'd expect (this is for agfline/LibAAF@bc4d094) |
|
Thank you for your encouraging feedback, I think we're on the right path.
Of course I will handle all libAAF related issues. I won't inflict to learn the AAF specs on anyone.
I switched to cmake recently, mostly because the Makefiles had become way too messy in the project and a pain to maintain. I chose cmake because it automates a lot of tasks inside a compact script, it's also very popular and documented. For now it fits my needs and the size of the project, but maybe I could change for something else in the future.
Does it still compile with cmake or do you have an issue ?
Those warnings are related to some low level FAT structures that require the I will fix the rest of the warnings, thanks. |
|
This entire adventure makes me curious if writing a library for dealing with AAF is the best way to go. Ardour teams has had OpenTimelineIO on the radar for a few years now, and OTIO supports AAF to an extent: https://opentimelineio.readthedocs.io/en/stable/tutorials/feature-matrix.html Thoughts? |
@adrien - as someone who comes from an MSVC environment I'm not familiar with the attribute keyword but could this be a simple typo maybe? I've noticed that in some places it's written like this:- whereas in other places it's like this:- Or are both versions valid? |
|
I've been looking for some official CFB microsoft code. Well, they don't pack their CFB structures. |
|
Looking at the source-code, this specific issue is fine. There is an 8 bit offset, and the pointer to And from ISO/IEC 9899:2017, Section 6.3.2.3
Since memcmp uses unsigned chars, this specific case will work, but since memcmp is declared as |
|
Thank you for your help Robin. |
|
In case it's useful, some background might help.... back in the early days, Ardour made extensive use of a 3rd party library called Jack which had similar issues with array sizes. IIRC it was mostly needed when a client and server could be from different compilers. Let's say that a given struct had 2 x elements. The first element was 12 bytes long and the second element, 4 bytes long. With the default compiler packing (usually 8-bytes) gcc made each element start on an 8-byte boundary. So there'd be the 1st element - then 4 x empty bytes - then the 2nd element, giving a total size of 20 bytes (i.e. each element starting on an 8-byte boundary). But MSVC used a different interpretation. It only added empty space if it couldn't fit the 2nd element within the remainder of 8-bytes. So MSVC used 12 bytes for the 1st element but then realised it had 4 x unused bytes, so it allocated them to the 2nd element - giving a different total structure size of 16 bytes. The only packing they both agreed on was 1-byte packing but of course, each compiler implemented it differently :-( In the end. we solved it by introducing the concept or PRE_PACKED_STRUCTURE and POST_PACKED STRUCTURE. If you download the Jack source code ([https://jackaudio.org/downloads) you'll find a header file (jack/systemdeps.h) which shows you how this got implemented for the different compilers (and for ARM if you look a bit lower down...) And there's another header file (jack/types.h) with a couple of structs showing how to implement them. Thankfully, i was all a lot easier than we thought!! |
Good call. The warning does indeed go away with It's probably prudent to also cast |
agfline
added a commit
to agfline/LibAAF
that referenced
this pull request
Jun 17, 2023
Restore `__attribute__((packed))` on `aafIndirect_t` and remove previous warning fix in [3acdb0f](3acdb0f). Use a simple cast instead to remove the warning (see [here](Ardour/ardour#805 (comment)) and [here](Ardour/ardour#805 (comment)))
agfline
added a commit
to agfline/LibAAF
that referenced
this pull request
Jun 17, 2023
|
Compilation should be warning-free now, but let me know if you still have some. |
|
@agfline - now that you've removed the attribute stuff I've been able to build new_aaf_session.cc here (using MSVC) and link it against libAAF.lib (albeit that you'd missed out quite a few of the attribute statements...) At the moment though, it's crashing at runtime - I think because I haven't specified a template path. And it looks like I'd need to provide some more command line arguments such as the sample rate and master channels. Given that my aaf file is called 100_BARS.aaf, could you give me an example of the command line I'd need to use here? |
|
Well, the problem was not coming from the packed statement itself, but from the usage of pointers against packed structures. There are still some structures that absolutely require packing (thanks to AAF standard and its unaligned structures...) Here is how I use new_aaf_session on Linux (note the use of ardour's session-utils All other parameters are optional. |
|
I guess john may be confused by the old Once installed, This branch/pull-request has not been updated, are there more commits elsewhere? What are you testing with? |
|
I've just used a simple MSVC project here and changed its source file to be new_aaf_session.cc and all the source files here are from git - BUT - it just occurred to me that my header files now are the ones with packing removed, whereas my copy of libAAF.dll is from a couple of days ago (when packing was still applicable). So that difference is probably the reason for the crash. Adrien - whenever you get some time, can you send me an updated version of libAAF.dll together with its corresponding link lib? Thanks. |
|
An update... if I revert those packed attributes that I'd needed to comment out and then I build here using Clang, I don't see the crash any more. However, the generated .ardour session ends up empty. Is there a small aaf file somewhere that's known to work? And if so, could someone maybe post a link here (or email a link to me) so I can give it a try? |
|
Another update :
This is a lot of work besides my regular daily job... please give me few days to finalize, I'll commit all of this and compile the windows port for you @johne53. I think it should be done this weekend. @x42 did you face any more warnings during compilation ? |
|
Meanwhile @johne53, just sent you a small AAF to play with. |
|
Hi Adrien, I got your new AAF file here but I'm seeing the same result as yesterday... i.e. the importer does generate a .ardour session file - and Ardour will open the session okay - but there's no audio present and no regions or timeline tracks. I've a hunch it'll just be a mismatch between your older libAAF.dll (with its packed structures) and the newer header files where they're no longer packed. So I'll wait awhile and try again at the weekend or next week, whenever you can build a new libAAF.dll and link lib. Thanks again. |
|
@agfline Hi Adrien = early start here but I've hit a few more problems with your latest commits and library build. Firstly, in new_aaf_session.cc the calls to locate_external_essence_file() seem to have a parameter missing. And there are various places where you've used original_file or media_location when you probably meant original_file_path or media_location_path But the big problem is that I need a link lib that corresponds to libaaf.dll. On Windows they come as a pair and it'd usually be called libaaf.lib. For your original DLL I needed to make a link lib locally but it was a lengthy process and I could easily have got it wrong (in fact, that might be why your importer isn't working properly here...) I'm not 100% certain how mingw would name the link lib but it might be called libaaf.a - ask Robin if you're not sure.. There's no rush here - just fit it in whenever you can, thanks, John |
|
Hi John, Sorry I was surely not clear enough in my email, but there were changes in new_aaf_session.cc to match those in libaaf. Just sent you the static lib version by email. |
|
Thanks Adrien, I'll try the new link lib when I get a chance. But regarding new_aaf_session.cc - I understand that you needed to make changes but what I'm saying is that the changed code won't compile any more - for example here, at line 941:- aafi->ctx.options.media_location = strdup( media_location.c_str() ); I'm guessing you probably meant:- aafi->ctx.options.media_location = strdup( media_location_path.c_str() ); |
|
@adrien - I just built here using the CMake/MSVC project and I've tested it with 2 x AAF files. For when you get some time, the crash doesn't happen any more but in both cases, the import failed on the grounds that the sample size and bit depth were both deemed to be zero. Normally I'd attach a screenshot except that the relevant text is dark grey on a black background (so not easily readable...) |
|
Great ! |
|
Here's the output text where the error occurs (it seems as if it's looking for external audio when the audio is all internal in this AAF):- 04493AAFClassID_TimelineMobSlot [3] (DataDef : AAFDataDef_Sound) 02987 AAFClassID_SourceClip 01600 AAFClassID_EssenceData : Could not retrieve Data stream node /Root Entry/Header-2/Content-3b03/EssenceData-1902{1}/. 02021 AAFClassID_Filler __ERROR ......\src\AAFIface\AAFIAudioFiles.c:1681 in parse_audio_summary() : Could not locate external audio essence file : (null) CompanyName : Creative Post Production Composition Name : 100_BARS [i] F:+GTK-SOURCES\ardour_aaf_support\session_utils\new_aaf_session.cc : main() on line 978 : Using AAF file sample rate : 0 Hz |
|
Ok, the actual problem is here : |
|
PS. When configuraing Ardour with |
agfline
added a commit
to agfline/LibAAF
that referenced
this pull request
Jun 26, 2023
|
@agfline - that seems to have fixed the bit depth but the sample rate is still getting detected as 0Hz :-( [Edit...] Actually... I'm saying it's fixed the bit depth but it's showing up here as 16398 bits. Shouldn't be something like 16 or 32 ?? |
pauldavisthefirst
pushed a commit
that referenced
this pull request
Jun 27, 2023
|
I have pending commits for Ardour/aaf branch. Should I do a fork ? How do we proceed ? |
Do whatever is most convenient to you. Once the dust has settled we can do a rebase or merge. |
@agfline - as a starting point, maybe re-configure your personal repo (ardour_aaf_support ?) so that it synchronises with Robin's new aaf branch, rather than syncing to Ardour master. That might help me too, as i'm still unable to build here from your repo :-( |
agfline
force-pushed
the
master
branch
2 times, most recently
from
c7ff1d1
to
e756786
Compare
|
I have synced ardour_aaf_support with Ardour, however I don't know how to make ardour_aaf_support point to Ardour's aaf branch... Any idea ? [EDIT] It's done, sorry for the noob question... |
|
I'll commit a bunch of updates to ardour_aaf_support tomorrow. |
|
Sure, just import a >1 channel audio file, and select "1 track per file" Or create a track with >1 input, and record something. |
|
Thank you Paul, |
|
@agfline - is there some way you could hold back on this for a while? Back when I wrote my own AAF importer (ArdourXchange) I implemented a feature whereby both channels of a stereo clip could occupy just 1 x Ardour timeline track - but there were complications to take into account. i.e. no matter how many channels a clip contains, there's no requirement (in AAF) for them all to get used somewhere. And what happens if an AAF track contains a combination of audio clips, all with different numbers of channels? I think you're maybe getting ahead of yourself here. At the moment, your imported sessions won't even play any audio so you need to get them back to working (and being testable) again. We can sort out the more complicated features later. Regarding git, I'm by no means an expert but if you can update from Ardour master, you've presumably set an upstream repo? So if you're currently typing something like this:- you'd presumably just need to change it to this:- |
not quite, combining multiple mono files into a stereo region would be "Merge files". It is only available if both files have the exact same duration. Editor::import_sndfiles (paths, Editing::ImportMergeFiles, ImportAsTrack, ...)
Yes, but that will just create the Sources, after that you need a "whole file region" using RegionFactory::create, adding all the sources to it: ardour/gtk2_ardour/editor_audio_import.cc Lines 811 to 831 in 45b2791 |
Thank you Robin I'll try this. Regarding my fork of Ardour, It seems I'm stuck with the master branch. I think I did the fork of Ardour with "Copy the master branch only" ticked and now, I can't find a way to switch to the aaf branch locally (without being "detached" or something...) So I think I'm going to remove ardour_aaf_support repos, so I can do another clean fork of Ardour without "Copy the master branch only" ticked. What do you think, will this action remove the current conversation ? Sorry to bug you with this, but it's driving me crazy... ! |
|
Apparently git remote offers something called set-branches so that you can switch to tracking some other branch. I often need to push things to origin but in my case (except for adding and removing upstream repos) I've never needed to make changes to them - so it's not something I've ever used but it's documented here:- |
|
Sorry to keep adding to a closed thread but I'm not quite sure where to start a new one!! Anyway... a couple of weeks ago I made a git clone from the ardour_aaf_support repo and here's a screenshot of how the master branch looked at that time (notice the 4 x most recent entries are all from agfline):-
Since then... each time I do another 'fetch' here, git rebase keeps placing those same entries at the top of the list - which suggests that they no longer exist in the official repo:-
Does this make sense to anyone? It's what I see while working from 'master' - but the last I heard, 'aaf' wasn't syncing yet with upstream Ardour :-( Or do I need to switch to the 'aaf' branch anyway? |
|
Sorry, that discussion was automatically closed when I was struggling with git and the ardour_aaf_support repos... Discussion moved here |
agfline
added a commit
to agfline/LibAAF
that referenced
this pull request
Jul 2, 2023
missing pack attribute on riff chunk structures Ardour/ardour#805 (comment) #5 (comment)
pauldavisthefirst
pushed a commit
that referenced
this pull request
Aug 9, 2023
pauldavisthefirst
pushed a commit
that referenced
this pull request
Nov 13, 2023
pauldavisthefirst
pushed a commit
that referenced
this pull request
Nov 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds new_aaf_session.cc in session_utils. This should be considered as a POC.
Requires libaaf