agfline / LibAAF

Library for Advanced Authoring Format (AAF) file reading.
GNU General Public License v2.0
25 stars 5 forks source link

Ardour Support - LibAAF v1.0 #28

Open agfline opened 5 months ago

agfline commented 5 months ago

Continuation of thread #5

johne53 commented 5 months ago

x42 - Robin - I know that Ardour8.4 got released only recently but does Ardour tend to make interim releases with bug fixes etc?

x42 commented 5 months ago

does Ardour tend to make interim releases with bug fixes etc?

No. Only if a very severe issue (data-loss, crash at start etc) comes up we do a new release (e.g. 8.3 was very short lived).

There are however https://nightly.ardour.org builds available.

johne53 commented 5 months ago

I thought I might post something on the Mixbus forum to advise people that AAF's now available in Ardour - but can non-developers access those nightly builds?

x42 commented 5 months ago

can non-developers access those nightly builds?

Certainly. Everyone can download the demo version, and all subscribers, or users who paid more than $45 for a single build have access to the full versions.

johne53 commented 5 months ago

@agfline - this is mostly a heads-up as it's something that's been confusing me...

In further testing, I've noticed that imported sessions are often created now with markers on the Mins:Secs ruler or the Location Markers ruler. They're generally in pairs so I'm wondering if they're in fact range markers (and therefore on the wrong ruler bar??)

Capture-12

agfline commented 5 months ago

Regarding AAF markers :

If a marker has a duration, it is added to Ardour like this :

new Location (*s,  markerStart, markerEnd, marker->name, Location::Flags (Location::IsRangeMarker));

Therefore it should appear on the Range Markers ruler.

If a marker has no duration, it is added to Ardour like that :

new Location (*s,  markerStart, markerStart, marker->name, Location::Flags (Location::IsMark));

Regarding Session start/stop :

Those are not set as markers, but using the following :

s->maybe_update_session_range (start, end);

Is there anything we could improve ?

x42 commented 5 months ago

I've noticed that imported sessions are often created now with markers on the Mins:Secs ruler

Those red triangles show selection-range. They are not markers, and they are always on the top (like playhead triangle) regardless of which rulers are visible.

x42 commented 5 months ago

Is there anything we could improve ?

Nope. that is correct.

agfline commented 5 months ago

Option 1: directly import the files without intermediate save to $TMPDIR

Does Ardour already support importing files out of a buffer/callback ? From my understanding, it would require a custom Session::import_files (ImportStatus& status) function...

If nobody have time to work on it we can also deal with it later, since we already have a working import process. What do you think ?

x42 commented 5 months ago

Does Ardour already support importing files out of a buffer/callback

yes. e.g FFMPEGFileImportableSource which runs ffmpeg, and the raw output is read from a pipe (buffer). similarly Mp3FileImportableSource and other ImportableSources.

We could derive a new class AAFImportableSource is-aImportableSource`.

What API is available from libAAF? read, seek or a buffer and size?

agfline commented 5 months ago

I got a function which retrieves a complete stream out of CFB sectors, and allocates a buffer.

https://github.com/agfline/LibAAF/blob/336dda2c1eed72aa459260661a29aff31fd8335d/src/LibCFB/LibCFB.c#L729

... but it's beyond API. I could make something like :

aafi_getEmbeddedFile( aafiAudioEssenceFile *essenceFile, unsigned char *buf, size_t *bufsz )
x42 commented 5 months ago

...and what if an AAF contains a 3 hour long podcast that doesn't fit in RAM? or a festival live show where a track can be 14+ hours long?

Well, I suppose we can ask a user to get more memory :)

x42 commented 5 months ago

I'm curious about unsigned char* -- what data-format does AAF use for Audio?

It seems like cfb_getStream is close. Ideally with an opaque handle (libardour needs not know about libAAF internal types), and an API that mirrors standard read(3) API would be best.

Compare to libsndfile's virtual file I/O which does that.

agfline commented 5 months ago

I'm curious about unsigned char* -- what data-format does AAF use for Audio?

cfb_getStream() is not specifically intended for audio. It simply retrieves a data stream out of CFB sectors, hence the unsigned char* generic type. Basically, AAF can store 3 types of audio files : WAV/BWAV, AIFF, or raw PCM. When retrieving a WAV/BWAV or AIFF file, the buffer returned by cfb_getStream() holds the full file with "header".

It seems like cfb_getStream is close. Ideally with an opaque handle (libardour needs not know about libAAF internal types), and an API that mirrors standard read(3) API would be best.

Sounds good to me. But the question is how do we handle raw PCM ? All required data (sample rate/size, etc.) are available in aafiAudioEssenceFile structure. But to be really opaque, I guess libaaf should build and return a valid WAV "header" prior to raw PCM data, so Ardour receives a complete WAV file.

Does Ardour need to be aware of file type ?

x42 commented 5 months ago

Does Ardour need to be aware of file type ?

No, libsndfile handles this.

I was just curious, for some generic read function I'd expect the data-buffer to be void*, not unsigned char*.

Would the following API work for you?

ssize_t aaf_read_stream (void* aaf_handle, void* buf, size_t nbyte, off_t offset)

This follows POSIX pread(3): Read nbyes at byte-offset offset into buf and return the number of bytes read.

x42 commented 5 months ago

But the question is how do we handle raw PCM ?

hmm. We could special case this, perhaps another ImportableFileSource? How do you currently deal with this when using a file-cache?

All this is mess is why I don't want to deal with AAF, I'm amazed that you can put up with it :)

agfline commented 5 months ago

Would the following API work for you?

Yes it would. Note that AAF stores its content in a kind of FAT32 structure. So ideally, we should find a way to stick to a 512B/4096B sector size boundary for nbytes and offset...

How do you currently deal with this when using a file-cache ?

I'm writing fmt, bext and data chunks prior to raw PCM so the file becomes a readable WAV. I can easily reproduce it here, maybe with a dedicated function that returns a WAV header buffer only if essence is raw PCM ? This would maintain a sector-aligned read for raw PCM, with the actual read function...

All this is mess is why I don't want to deal with AAF, I'm amazed that you can put up with it :)

Challenge. That's what it's all about ;)

johne53 commented 5 months ago

Guys - please don't lose sight of the fact that AAF has only been fixed in the recent Ardour nightlies. For official releases (whether Ardour or Mixbus) there's still been nothing released with a working version of AAF import. That's (surely?) the most urgent problem to fix.

agfline commented 5 months ago

Maybe the next Ardour release could be based on current working code and we could work on this in a dedicated fork ?

johne53 commented 5 months ago

Hi Adrien - Did Dingo ever send you any ProTools AAF's? He sent me one called Johne_PT_AAF_test-A.aaf but it's crashing now in 'protools_post_processing()'

Just wondering if you could try it at your end sometime? (or any ProTools import)

[Edit...] It seems to be connected to your recent change to support multichannel clips.

agfline commented 5 months ago

Just wondering if you could try it at your end sometime? (or any ProTools import)

I don't have that file, nor any file from Dingo. But I bought ProTools a few weeks ago to make test files and I have had no issue with those. Can you send me that file ?

agfline commented 5 months ago

Thanks John. Just had a quick look at your file. It crashes with Ardour but aaftool reads it with no issue (with both --pt-remove-sae and --pt-true-fades set, which enable protools post processing) I'm not sure how I can debug it with Ardour...

x42 commented 5 months ago

I'm not sure how I can debug it with Ardour...

https://ardour.org/debugging_ardour :)

johne53 commented 5 months ago

I can let you know why the crash occurs although it doesn't make a lotta sense...

        while (audioItem != NULL) {
            aafiTimelineItem* audioItemNext = audioItem->next;

            if (audioItem->type != AAFI_AUDIO_CLIP) {
                audioItem = audioItem->next; // <--- THIS LINE !!!!
                continue;
            }

            aafiAudioClip* audioClip = (aafiAudioClip*)audioItem->data;

The indicated line gets reached on about the third iteration of the 'while' loop. And for whatever reason, audioItem->next; returns a garbage value.

Shortly before the crash (and further down in the while loop) there's a similar line (though not identical) saying:-

audioItem = audioItemNext;

and for whatever reason, that one doesn't crash,

johne53 commented 5 months ago

@x42 - I can see that libaaf has been updated in Ardour but for me at the moment, it'd be easier to test in Mixbus. So can you bring the changes across to Mixbus whenever you get a chance? In the meantime, I need to make the following point for Adrien's benefit:-

I'm shocked (yet again) to see that a simple fix has brought with it a whole plethora of unrelated changes. This'll require me to test absolutely everything here all over again and I'm simply not willing to keep doing this, week in and week out. I've been one of the strongest advocates for AAF but I never dreamed for a minute that it'd be undergoing such a constant stream of major changes. Until he regards it as stable, I doubt if Ben will want to include AAF in Mixbus - and until it gets released in Mixbus, we're unlikely to find any users who want it.

So please Adrien, you need to give us some idea of when you expect your codebase to stabilize. Or if there's some underlying reason why it can't be made stable, you need to help us understand why.

And sorry for the rant... haven't had my breakfast yet!

agfline commented 5 months ago

Hi John,

Before the release of libaaf v1.0, there were still many parts of code that needed to be somehow improved or rewritten. That's what I've been working on day and night for 3 months. The v1.0 also brings a solid test script with 63 AAF files to prevent regression in further updates. I run that test script for Linux and Windows before I push every update. Since libaaf v1.0, API and code base can be considered stable.

If you look closer at those changes you mention, there are a few bug fixes but most of those changes are about comment cleaning, code alignment, function/macro renaming and leftover unused code cleaning. No "active code" have been changed beyond bug fixes.

Let me remind you that this is the first time I work on a project of that size in collaboration and from that perspective, there is probably many things I still have to learn. I understand how confusing it is for you, when you see all those changes, even though they are simply "esthetic" changes... Would it be a better practice if I'd maintain a specific Ardour branch in libaaf ? So I could keep working in parallel without polluting Ardour's updates.

Sorry John, I hope we can improve our working process. And many thanks for your hard testing work !

johne53 commented 5 months ago

Many thanks Adrien and please let me clarify that I appreciate the efforts you're making with your test scripts and the move to UTF-8 and the extra work needed to improve fades etc. It's all great work but we need to find a way to fix occasional "show stoppers" without bringing in massive changes that no-one's tested.

Robin - do we have this problem with other 3rd-party contributions, such as lua, or have you figured out a solution in those cases?

x42 commented 5 months ago

Robin - do we have this problem with other 3rd-party contributions,

The only other 3rd party custom contribution that we have is libptfformat (pro-tools import). In this case damien is rather active and provides user support on IRC (he also curates Ardour's bundled plugins).

During initial development it was normal that various pro-tools sessions did not import correctly or were only imported partially. We just marked that feature as "experimental", or "beta" and collected bug reports. It took over a year until most kinks were sorted out (and fixes are still ongoing).

However, there was no dedicated person like you who did test with various pro-tools session, so bug-fixes were a lot slower and development was more incremental.

johne53 commented 5 months ago

In some ways it was just bad luck that our first release with AAF got made at a time when the code was buggy. So given that Ardour doesn't support interim releases, I'm keen to ensure we don't make the same mistake for release 2 (and hence why I'm getting in a strop about adding untested changes...)

In some ways, Adrien's suggestion about an AAF branch makes sense but I remember from last time that its header files grew increasingly out of sync - meaning that his code became unbuildable here against Mixbus. And of course it doesn't help with the main problem being that Ardour's aaf needs to get updated occasionally and "en masse" rather than git being able to keep things properly in sync with Adrien's libaaf. AFAICT Damien's code also receives "en masse" updates but in his case, ptformat is very much simpler so it's maybe not quite so risky.

I'm starting to think the real solution here would be to abandon libs/aaf and start using the AAF code like any other 3rd party library, such as glib / pango etc? It'd help if Adrien could harmonize his programming style to match the others but even that's less important in a 3rd party lib.

[Edit...] Or how about this..? Adrien creates a special branch for us - but in his own repo, rather than in Ardour - and we then build against that one. We could then pull in from master or cherry-pick individual commits, as required (or maybe that's what Adrien meant in the first place?)

x42 commented 5 months ago

@johne53 it is preferable to keep a copy in Ardour's source tree for various reasons: The AAF API/ABI keeps changing, and updating libAAF also requires updating Ardour's source. Furthermore upstream does not have a cross-platform build system that works on all systems we use Ardour on, and various GNU/Linux distros have not packaged it either. Eventually we could also statically link it. All that being said, the wscript allows to use external libAAF already, which Adrien uses with a local build during development.

I also think you overestimate the resources we have. I would not put additional workload on Adrien to maintain a separate branch. We also won't cherry-pck individual commits to Ardour for the same reason. The idea is to stay in sync with upstream when they make a release.

So given that Ardour doesn't support interim releases

There are daily builds available, and we ask users who rely on a certain new feature to use those.

johne53 commented 5 months ago

In which case, there's a risk that AAF might only ever work in the nightlies - i.e. if it works in a given Ardour release, that'll just be a matter of luck (at least in the early stages...) Presumably Damien's protools importer had the same limitation in its early days?

x42 commented 5 months ago

In which case, there's a risk that AAF might only ever work in the nightlies

I expect things to settle down over the next couple of months. We also should not pull in a new libAAF in the week before a release. That way we know what issues remain in a certain release.

The same is true for many other features in Ardour. At some point we have to make a decision that it is good enough for a majority of users and use-cases and release it. If we'd wait until AAF import is 100% reliable and robust there would likely never be a release.

Now more importantly, how can me we make testing easier for you? You have so far been the most prolific person testing and debugging, and it would be great if we can get libAAF into a very good state for the Ardour 8.5 release.

IMHO the goals for 8.5 would be: Do not crash regardless of what AAF file you throw at it. Import 90% of all AAF files correctly (Audio, ranges, meta-data).

agfline commented 5 months ago

Now more importantly, how can we make testing easier for you?

I think that's the point. Would testing script be an option with Ardour ?

Regarding "massive changes", I understand how confusing it is for John. What I can do for the current early stage is put all those style/comment changes aside in a dedicated branch and keep the master branch for bug fixes only.

Then once we have something stable enough (after the next Ardour release), I'll merge those two branches. Would it be OK for you John ?

In any case, I'm not changing any active code in the master branch since v1.0 (except for bug fixes)

johne53 commented 5 months ago

That sounds like a good way forward but I think Robin mentioned that 8.5 isn't due until the end of April or even May. So it might work better if I halt my own testing until maybe a few weeks before that release - and from that point until the release date, you only give us essential changes and bug fixes. In the meantime, if other users hit problems with the 8.4 release, they can hopefully let us have their error report - or even the troublesome AAF file.

agfline commented 5 months ago

Guys, can you tell me how I can define a macro in wscript ? I've searched an tried many things without success...

x42 commented 5 months ago

Can you elaborate? A macro to do what? What have you tried?

agfline commented 5 months ago

I'm trying to set a macro when compiling, like gcc -DSOME_FLAG

So far, I've tried the following without success :

conf.env.append_value ('CFLAGS', '-DSOME_FLAG')
conf.env.append_unique ('DEFINES', 'SOME_FLAG')
conf.env['SOME_FLAG'] = True
conf.define ('SOME_FLAG', 1)

obj.defines = [ 'SOME_FLAG=1' ]
obj.defines = [ 'SOME_FLAG' ]
obj.cflags = [ '-DSOME_FLAG' ]

[EDIT] I'm working on a dedicated branch, for the embedded file reading functions that we discussed. This involved me using libsndfile internally (and optionally), hence a macro to enable that feature.

x42 commented 5 months ago

obj.defines should work, like https://github.com/Ardour/ardour/blob/719debf292c9f68086ae0bf894c66174d2798d44/libs/aaf/wscript#L50-L54

-- Is this is to set defines for libraries (like HAVE_LIBSNDFILE)? Those are set by requiring a pkg-confing dependency. e.g. top-level wscript checks

autowaf.check_pkg(conf, 'sndfile', uselib_store='SNDFILE', atleast_version='1.0.18', mandatory=True)

and later libs/ardour/wscript has obj.uselib = ['SNDFILE' ....] which sets the CPProcessor define HAVE_LIBSNDFILE

agfline commented 5 months ago

obj.defines should work

Well, it does not seem to work when I try.

I replaced my custom macro with HAVE_LIBSNDFILE and added obj.uselib = ['SNDFILE'] to libs/aaf/wscript. Now the conditional code in .c file gets parsed by the compiler, but compilation fails with :

../libs/aaf/AAFIEssenceFile.c:390:5: warning: no previous prototype for ‘aafi_extract_audioEssenceFile’ [-Wmissing-prototypes]
  390 | int aafi_extract_audioEssenceFile(AAF_Iface *aafi,
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../libs/aaf/AAFIEssenceFile.c: In function ‘aafi_extract_audioEssenceFile’:
../libs/aaf/AAFIEssenceFile.c:401:3: error: unknown type name ‘SF_INFO’; did you mean ‘SA_SIGINFO’?
  401 |   SF_INFO sfinfo;
      |   ^~~~~~~
      |   SA_SIGINFO
../libs/aaf/AAFIEssenceFile.c:403:3: error: unknown type name ‘SNDFILE’; did you mean ‘NOFILE’?
  403 |   SNDFILE *infiles[1] = {NULL};
      |   ^~~~~~~
      |   NOFILE

...

Function aafi_extract_audioEssenceFile is declared in a header file, between #ifdef HAVE_LIBSNDFILE. So it's like HAVE_LIBSNDFILE was set for .c file and not for .h ?

All libsndfile typedefs and functions remain undefined, despite no error with <sndfile.h> inclusion.

agfline commented 5 months ago

OK... I just messed up writing HAVE_LIBSDNFILE instead of HAVE_LIBSNDFILE... BTW, working macro is HAVE_SNDFILE

agfline commented 5 months ago

@x42, here is an updated version of Ardour with libaaf cfb_stream_reader branch.

API now exposes seek / tell / read functions for embedded file reading.

https://github.com/agfline/LibAAF/blob/11fd79a50b7d68ca82878c85386ce29ddd120a40/include/libaaf/AAFIEssenceFile.h#L76-L79

API also exposes libsndfile-ready functions for reading virtual files. Note that aafi_sf_open_virtual_audioEssenceFile() abstracts file format type, especially when it is raw PCM.

https://github.com/agfline/LibAAF/blob/11fd79a50b7d68ca82878c85386ce29ddd120a40/include/libaaf/AAFIEssenceFile.h#L83-L92

I think merging the cfb_stream_reader branch will require a major version update of libaaf because of API change. Do you think Ardour could be updated with it for the 8.5 release ? Anyway, until you give me the green light @x42 and @johne53, I'll keep libaaf master branch for bug fixes as stated previously.

johne53 commented 5 months ago

Hi Adrien and thanks for all your hard work with this.

Truth is, I wasn't aware that Ardour doesn't make interim releases so my main concern is to ensure that its next official release doesn't go out with unstable or untested AAF code again. So I'd suggest you don't update Ardour until after that next official release. Outside of us (and a few users on IRC) I'm not aware of anyone who's had a chance to use the AAF import yet. So we've had little or no feedback, which is kinda disappointing.

Sorry to be so long-winded and I do appreciate all your efforts here (hugely!) but for me, the bottom line is that when Ardour next gets released, I'd prefer it to come with the AAF feature usable this time.

agfline commented 5 months ago

I'm not aware of anyone who's had a chance to use the AAF import yet. So we've had little or no feedback, which is kinda disappointing.

Me neither. Of course I'm also a bit disappointed... but it also let us time to fix and improve things. I guess it can take some time for people to know that AAF is ongoing with Ardour...? One competitive advantage is that Ardour AAF support might be the best out there. Of course libaaf isn't as powerful as the official SDK, but the end-user don't care about that. There is currently no implementation in commercial audio software which support as much AAF features/proprietary stuff as libaaf does. Eventually, I hope people will find out ;)

when Ardour next gets released, I'd prefer it to come with the AAF feature usable this time

No problem with that. In this case, I will stick to the second branch for all updates, so you can safely do testing with current Ardour version if you want ;)

johne53 commented 4 months ago

As we all know by now, an Ardour tester called Schmitty2005 discovered a serious AAF import problem when multiple clips need to be referenced to the same audio file (currently, the importer is generating multiple audio files, which can end up running out of disk space...)

https://discourse.ardour.org/t/aaf-beta-testing-suggestions-and-observations/110054/5?u=john_e

I'm wondering if there's any possibility of a 'quick fix' for this problem or is it something that'll need a big re-think?

agfline commented 4 months ago

Problem is that this issue is highly related to a previous one.

https://github.com/agfline/LibAAF/issues/5#issuecomment-1994296571 :

before we waited for the all extraction process to finish before removing cached files. However, this caused serious memory overload and system freeze when extracting large embedded AAF to /tmp/ on linux.

https://github.com/agfline/LibAAF/issues/5#issuecomment-1994504733 :

The downside is the memory issue on Linux (system temp directory writes to the RAM). Consider an embedded AAF file with more than 2 or 3 GB of audio being extracted directly to RAM on a 4 or even 8 GB RAM computer...

But now of course, we can't reuse an extracted audio file after it was deleted. So currently we must choose between the risk of saturating memory (and freezing computer) or saturating disk space.

In my opinion, the only solution is to read embedded files directly from AAF as Robin suggested (solution is ready, see https://github.com/agfline/LibAAF/issues/28#issuecomment-2028959661), this should solve the memory issue for good and also improve import speed. But we still have to address Schmitty2005 issue by finding a way to track essence files and reuse them with Ardour, I'm going to work on it.

EDIT: I might have answered too quickly. I'm going to try a few things with the current 8.5 Ardour version.

EDIT 2: Just found an old note I wrote... In fact, the absence of essence reuse was never implemented (since multichannel audio support). It's not related to the deletion of extracted cached files because what we want to reuse is the Ardour source file, not the temporary cached one that would import into ardour as a new source file. There is currently no way in libaaf to guess if an essence is being used by multiple clips, and that's what I need to work on.

johne53 commented 4 months ago

Is the problem that we're trying to generate audio files in tandem with creating the Editor tracks and clips? And therefore needing to keep track of audio clips while at the same time, generating our timeline objects? Here's some background...

In the very early days of Mixbus I wrote its first AAF import feature which was slightly complicated by the fact that Mixbus could use either mono or stereo audio tracks. This meant that I needed to examine the timeline objects at an early stage and decide if their audio clips would need to get imported as a stereo file or as twin mono files (and of course many were just a single mono file). And assuming that that stage completed successfully, the good thing was that I could later move on to generating the Mixbus timeline tracks and clips, safe in the knowledge that each clip's audio file would already exist and get found.

In other words, importing the actual audio was en entirely different step from importing the timeline info. There was no attempt to import audio "as we went along".

agfline commented 4 months ago

libaaf approach is to parse the entire AAF file first, retrieve and compile all useful data and expose those (tracks, clips, essences, etc.) through a custom interface, providing abstraction for all the complicated AAF stuff.

This makes implementation way simpler on the user side, instead of parsing Mobs, SourceClips and all this wonderful mess, each time you want to get something... So back to Ardour, when we reach the point of importing / creating tracks and regions, all we need to know is already known.

johne53 commented 4 months ago

Thanks Adrien, I'm just about to go out for the day but I'll be able to try your new commit tomorrow.

johne53 commented 4 months ago

One small problem Adrien - I'm a bit confused about what's going on here... I haven't had time to check the other sources but taking AAFIEssenceFile.h as an example...

Your commit shows edits around line 460 and some others around line 760 - but our copies of the file are quite small here and only have around 60 lines. Does that make any sense?

agfline commented 4 months ago

Yep sorry, I committed on the "new" libaaf branch... I'm going to commit back on the master branch and push to Ardour (update also requires a few changes on the Ardour side)