Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ardour Support #5

Closed
agfline opened this issue Jul 2, 2023 · 141 comments
Closed

Ardour Support #5

agfline opened this issue Jul 2, 2023 · 141 comments

Comments

@agfline
Copy link
Owner

agfline commented Jul 2, 2023

This thread continues the discussion previously started here

@agfline
Copy link
Owner Author

agfline commented Jul 2, 2023

Thank you John for your help with git. I think I finally managed to have a local fork of the Ardour/aaf branch that is not detached, and which can be synced with Ardour/aaf. It's in ardour_aaf_support/aaf, note that the aaf branch is now set as default. I successfully did a bunch of commits last night.

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

I think it is due to my struggling with git and the ardour_aaf_support repos, I have also seen weird stuff at some point. Have you tried to start a brand new clone of the ardour_aaf_support/aaf branch ?

@johne53
Copy link

johne53 commented Jul 2, 2023

Thanks for the quick response Adrien. I guess I could ditch my existing repo and clone it all over again but it's taken me the best part of 2 days to make 'adour_aaf_support buildable with MSVC - so I'd prefer to leave that as a last resort. In the first instance I'll try switching to the AAF branch. In theory, that should carry on working here and hopefully it'll accept your future updates.

@johne53
Copy link

johne53 commented Jul 2, 2023

I'll try switching to the AAF branch. In theory, that should carry on working here

Yes, that's building fine here (though it'll be a few days before I get around to building the aaf branch that Robin's added...)

In the meantime @agfline, did you manage to reproduce those problems I reported relating to 100_BARS.aaf? It'd be interesting to know if they're reproducible with your (Linux?) code - or if they're only showing up in Windows.

@agfline
Copy link
Owner Author

agfline commented Jul 2, 2023

In the meantime @agfline, did you manage to reproduce those problems I reported relating to 100_BARS.aaf?

Indeed, I got the 0 Hz / 16398 bits issue on Linux too. This is the case for all AAF files containing embedded essences formatted in AIFC, I still have to investigate though.

@agfline
Copy link
Owner Author

agfline commented Jul 2, 2023

Got it...
There's a missing __attribute__((packed)) ;-)

agfline added a commit that referenced this issue Jul 2, 2023
missing pack attribute on riff chunk structures

Ardour/ardour#805 (comment)
#5 (comment)
@johne53
Copy link

johne53 commented Jul 3, 2023

Great news! I'll test it here later today and let you know.

@johne53
Copy link

johne53 commented Jul 3, 2023

Hmmm... I tried 100_BARS.aaf here and it no longer shows me the strange sample rate and bit depth. It even creates a .ardour session now - BUT - I'm seeing exceptions now at line 212 in aafi_release() (in AAFIFace.c ) :-

if ( (*aafi)->ctx.options.media_location ) {
	free( (*aafi)->ctx.options.media_location ); // <--- CRASHES HERE !!!!
}

and when I later try to load the generated .ardour session, it still contains no audio :-(

@agfline
Copy link
Owner Author

agfline commented Jul 3, 2023

Ok. No time to fix it before tonight, but meanwhile you can try to add --media-location /path/to/wherever/you/want/ to new_aaf_session

@johne53
Copy link

johne53 commented Jul 3, 2023

Hi Adrien - I added a --media-location path and just for the hell of it, I configured my debugger to skip the crashing line temporarily. But although it then didn't crash, the created session still doesn't contain any audio when loaded into Ardour. Basically, media files still aren't getting created at the moment.

@agfline
Copy link
Owner Author

agfline commented Jul 3, 2023

Did you set --media-cache ? Can you show the console output ?

@johne53
Copy link

johne53 commented Jul 3, 2023

No I didn't set --media-cache, only --media-location (I'd never needed to set either of them up to now...)

I'm just closing down here for the night but here's the output i see when the problem occurs:-

 Composition Name       : 100_BARS
 Composition Start      : 23814000
 Composition End        : 33521292
 Composition SampleRate : 44100 Hz
 Composition SampleSize : 16 bits

[i] F:\+GTK-SOURCES\ardour_aaf_support\session_utils\new_aaf_session.cc : main() on line 989 : Using AAF file sample rate : 44100 Hz
[i] F:\+GTK-SOURCES\ardour_aaf_support\session_utils\new_aaf_session.cc : main() on line 997 : Using AAF file bit depth : 16 bits
[i] F:\+GTK-SOURCES\ardour_aaf_support\session_utils\new_aaf_session.cc : main() on line 1021 : Using AAF composition name for Ardour session name : 100_BARS
ARDOUR_CONFIG_PATH not set in environment
ARDOUR_DATA_PATH not set in environment
*** WEAK-JACK: initializing
*** WEAK-JACK: OK. (0)
locate to 0 took 527 usecs for 1 tracks = 527 per track
[e] F:\+GTK-SOURCES\ardour_aaf_support\session_utils\new_aaf_session.cc : main() on line 1112 : Could not extract audio file from AAF : media cache was not set.
[e] F:\+GTK-SOURCES\ardour_aaf_support\session_utils\new_aaf_session.cc : main() on line 1112 : Could not extract audio file from AAF : media cache was not set.
: [ERROR]: Session start(23814000)/end(33521292))
locate to 0 took 204 usecs for 1 tracks = 204 per track
Butler drops pool trash
Graph::drop_threads() sema-counts: 0, 0, 1

@agfline
Copy link
Owner Author

agfline commented Jul 3, 2023

ARDOUR_CONFIG_PATH not set in environment
ARDOUR_DATA_PATH not set in environment

Those are Ardour messages I don't have with Linux. Don't know if they are important or not.

[e] F:+GTK-SOURCES\ardour_aaf_support\session_utils\new_aaf_session.cc : main() on line 1112 : Could not extract audio file from AAF : media cache was not set.

Explicitly says media cache was not set. Before I made the --media-cache parameter, the extract location of audio files was hard coded /tmp/ which doesn't work on windows. I updated --help accordingly, but I should have told you.

Actually when you are working with embedded files you have to set --media-cache and when you are working with external media files, you can set --media-location if the library can't locate the files on its own.

@johne53
Copy link

johne53 commented Jul 4, 2023

ARDOUR_CONFIG_PATH not set in environment
ARDOUR_DATA_PATH not set in environment

Those are Ardour messages I don't have with Linux. Don't know if they are important or not.

I'm pretty sure they're not.

Explicitly says media cache was not set. Before I made the --media-cache parameter, the extract location of audio files was hard coded /tmp/ which doesn't work on windows.

It seemed to work for me - but anyway... I've now set --media-cache to be F:/ here and it does produce an audio file called F:/100_BARS _1.wav - although it hasn't helped much. I see a problem now in the first two lines of import_sndfile_as_region(), namely:-

wstring ws(audioEssence->usable_file_path);     // <--- reports "Error reading characters of string."
string usable_file_path(ws.begin(), ws.end());  // <--- 'usable_file_path' is now set as "F/10BR 1wv¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦"

and needless to say, the code crashes almost immediately :-(

My guess would be a possible problem with character encoding. Maybe you're assuming that strings here will either be narrow character or wide character? But if you're obtaining those strings from the Ardour code, they'll almost certainly be getting returned in UTF-8 format.

@johne53
Copy link

johne53 commented Jul 5, 2023

ARDOUR_CONFIG_PATH not set in environment
I've now set --media-cache to be F:/ here and it does produce an audio file called F:/100_BARS _1.wav - although it hasn't helped much.

string usable_file_path(ws.begin(), ws.end());  // <--- 'usable_file_path' is now set as "**F/10BR 1wv*<etc>¦

It didn't occur to me yesterday but this morning I realised something:-

F/10BR 1wv is in fact every alternate letter from F:/100_BARS _1.wav (and with the NULL terminator missing)

So maybe not a problem with UTF-8 but surely this is some kinda problem with character encoding??

@johne53
Copy link

johne53 commented Jul 5, 2023

And something else just occurred to me...

the extract location of audio files was hard coded /tmp/ which doesn't work on windows

By a stroke of luck I'd chosen F:/ as my extract location and I just noticed a huge bunch of spurious .wav files from the past few weeks (which I didn't put there...)

So it's looking like the code wasn't set to the user's temp folder on Windows - but in fact it'd been trying to use the root folder of whichever drive the sessions were getting created on. And if that drive happened to be the user's C:\ drive, that'd definitely explain why the software wasn't being allowed to write its .wav files there.

@agfline
Copy link
Owner Author

agfline commented Jul 5, 2023

Indeed, that makes sense.

The --media-cache parameter will eventually be removed. A good practice will be to extract audio files to some system-dependent temp folder, and delete them once they are imported into Ardour session. Still, I think I'll make a dont-clear-cache parameter so one can investigate exported files if needed.

wstring ws(audioEssence->usable_file_path); // <--- reports "Error reading characters of string."
string usable_file_path(ws.begin(), ws.end()); // <--- 'usable_file_path' is now set as "F/10BR 1wv¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦¦"

I'm going to investigate this today.

@agfline
Copy link
Owner Author

agfline commented Jul 5, 2023

Since I don't know how to build Ardour on windows, I just made a small Cpp test program to reproduce your issue on windows, testing against the same 100_BARS.aaf file.
I can't reproduce, neither Error reading characters of string nor the weird name F/10BR 1wv.
Are you working with the latest libAAF / new_aaf_session.cc ?

@johne53
Copy link

johne53 commented Jul 5, 2023

Yes, in fact I updated it again this morning and I still see the problem.

Whereabouts does audioEssence->usable_file_path get assigned? I'll try putting a breakpoint there and see if the assignment looks okay.

@agfline
Copy link
Owner Author

agfline commented Jul 5, 2023

In your case (embedded audio file extraction) :

swprintf( audioEssence->usable_file_path, (strlen(filePath) * sizeof(wchar_t)), L"%s", filePath );

@johne53
Copy link

johne53 commented Jul 5, 2023

Okay, I can see a couple of problems. Firstly, swprintf() expects the number of characters to print, not the number of bytes. So you don't need to multiply by sizeof(wchar_t))

And this surprised me a bit but L"%s", filePath doesn't seem to produce a wide-character string here. You'd think it would but it didn't when I tested it. In the end, I made it work by using:-

mbstowcs( audioEssence->usable_file_path, filePath, strlen(filePath) );

That corrected this particular issue but I then hit a new set of errors:-

: [ERROR]: FFMPEGFileImportableSource: Failed to read file metadata
: [ERROR]: Import: cannot open input sound file "100_BARS _1.wav"
  [e] F:\+GTK-SOURCES\ardour_aaf_support\session_utils\new_aaf_session.cc : main() on line 1149 : Could not import '100 BARS _1' to session.

and Windows tells me that 100 BARS _1.wav is an invalid WAV file. I'm really quite surprised that any of this works for you !

@agfline
Copy link
Owner Author

agfline commented Jul 5, 2023

swprintf() expects the number of characters to print, not the number of bytes. So you don't need to multiply by sizeof(wchar_t))

Thanks for pointing that out.

L"%s", filePath doesn't seem to produce a wide-character string here

Well, it does actually work on my Linux and Windows VM. I didn't know about mbstowcs() and since it works for you, I'll switch to that function instead, no problem.

100 BARS _1.wav is an invalid WAV file

Can you send me that wav file so I take a look ?

I'm really quite surprised that any of this works for you !

And yet, it does work pretty well ! And I hope it will be soon working for you as well ;)

@johne53
Copy link

johne53 commented Jul 5, 2023

Yes, give me a few minutes and I'll send you 100 BARS _1.wav. In the meantime I found this article which explains why L"%s" doesn't work:-

https://learn.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=msvc-170

it seems that for the wprintf range of functions, L"%S" would need to use an upper-case 'S' to accommodate 'filePath' being a narrow character string.

We're slowly getting there :-)

@agfline
Copy link
Owner Author

agfline commented Jul 5, 2023

Just made an hash comparison between your file and mine and... they are exactly the same.

There are multiple ways to embed audio in AAF. In 100_BARS.aaf, each audio file is stored as a complete AIFC file. In such case, the extraction process is simply to take the file data stream, and to write it somewhere on your disk.

There is still an issue with file extension being .wav, but I highly doubt ffmpeg is yelling about that.

: [ERROR]: FFMPEGFileImportableSource: Failed to read file metadata
: [ERROR]: Import: cannot open input sound file "100_BARS _1.wav"

Wouldn't be some reading persmission/access issue ?
Maybe some Ardour Guru have an idea ?

@johne53
Copy link

johne53 commented Jul 5, 2023

I suspect it's yelling about the fact that it's not a WAV file. For every other WAV file here (including your older ones) the first 12 bytes invariably look like this:-

RIFF<4 bytes - possible length?>WAVE

But your newer ones don't have that section any more - so maybe there's some other header info missing too ?

@agfline
Copy link
Owner Author

agfline commented Jul 5, 2023

Like I said, it's not a WAV file, it's an AIFF/AIFC file with .wav file extension.
Even though file extension should be fixed to .aiff, the file should be well parsed by any player or ffmpeg. Not to mention the exact same file gets imported into Ardour on Linux.

@agfline
Copy link
Owner Author

agfline commented Jul 5, 2023

it seems that for the wprintf range of functions, L"%S" would need to use an upper-case 'S' to accommodate 'filePath' being a narrow character string.

I just made a change to test that out. I have a lot of other uses of swprintf() where mbstowcs() wont be an option, so when you have time can you confirm it's working ?

Also fixed the .aif file extension, although I doubt it fixes your issue.

@johne53
Copy link

johne53 commented Mar 12, 2024

I'm afraid it's not good news Adrien.

I built Ardour from last night's git and it compiled and linked fine. I then imported a very simple AAF here with only around 7 clips and that worked fine too. So I then tried a bigger import which used to import okay. The latest code does import a few clips but the majority are now producing errors which look like this:-

[Edit...] BTW it seems to be creating the right number of audio files and they're the right size but it's not creating their timeline clips for some reason. Any ideas..?

Capture-10

@agfline
Copy link
Owner Author

agfline commented Mar 12, 2024

Well, don't know if that's related but there is obviously an issue with the construction of unique file names (with all those weird spaces...).
Do you have the possibility to run aaftool.exe --aaf-summary --aaf-essences --aaf-clips --trace --no-color --verb 3 YOUR_FILE.AAF and post the output here ? You can download latest aaftool.exe here : https://github.com/agfline/LibAAF/releases/tag/v1.0
Otherwise you can send me your file and I'll take a look.

@johne53
Copy link

johne53 commented Mar 12, 2024

This one's an embedded AAF file and too large to upload anywhere (> 400MB) so I've run the latest aaftool.exe and I'll email you the result in a few minutes.

@agfline
Copy link
Owner Author

agfline commented Mar 12, 2024

Thanks John.

First, the log shows no error and that there is no problem with the building of unique essence name. Instead it shows that those white spaces in essence names are coming from the AAF file itself. Never seen such a thing, it seems that those white spaces fill a 31 bytes string length + 1 NULL termination in origination software. That is weird, but I doubt your final issue is coming from those spaces.

Can you tell if the non-importing files actually exist ? If they where successfully extracted or not ? If they are playing well with any audio player ?

@johne53
Copy link

johne53 commented Mar 12, 2024

I've got the same .ardour session here which got imported a few weeks ago, using Mixbus rather than Ardour. However, if I rename the newly created .ardour file from today and substitute the older one from Mixbus, all the expected regions will then appear on my timeline. And I do see the channel meters going up and down but unfortunately, I can't hear any actual audio. The Mixbus file references a Mixbus master channel but the Ardour / Mixbus master channels don't seem to be compatible any more (audio-wise)

But having said that, if I navigate to today's files just using Explorer, they're all conventional .wav files so if I double-click them, Windows does play the expected audio (i.e. the media files all got imported fine...)

@agfline
Copy link
Owner Author

agfline commented Mar 12, 2024

they're all conventional .wav files

Regarding Ardour logs and aaftool.exe, all files are embedded as AIFC so they should be extracted as .aif not .wav (eg. C:\Users\JohnE\AppData\Local\Temp\aaf-cache-DX9LK2\CAFE 4.wav .aif). Note that libaaf essence extraction is a raw byte-to-byte copy of internal essence data, without any data or audio processing.

By any chance, do you have encountered the same issue with a smaller AAF file you could send ?

@johne53
Copy link

johne53 commented Mar 12, 2024

I haven't encountered it yet in any smaller AAF's here but something just occurred to me...

Might this be a consequence of us stripping space characters from the end of directory names? Windows won't allow a directory name to end with a space character - although they're obviously legal in a file's name.

@agfline
Copy link
Owner Author

agfline commented Mar 12, 2024

I don't think so... Stripping spaces doesn't consider file extension, in your case spaces are in the middle of the name, before file ext. Plus if you say that file was working before, it means that the issue is coming from the recent update, either in libaaf or in ardour_ui_aaf.cc.

The AAF size does not matter here. I successfully import 52min/embedded documentary with no issue.

Can you try to import PR_AIFF_Internal.aaf from here : https://github.com/agfline/LibAAF/tree/master/test/aaf
So we can exclude a potential windows specific error with aiff files ?

@johne53
Copy link

johne53 commented Mar 12, 2024

I'll give it some thought overnight but I'm pretty sure this'll come down to the whitespace issue.

e.g. instead of looking for a .wav file with a name like "TITLES<followed by 25 spaces>.wav" (which would be the correct name) we might now be expecting the .wav file to be called "TITLES.wav" (with no spaces in the name)

and therefore the audio wouldn't get found, which might then prevent its region from getting created.. Hopefully Robin can offer his thoughts. It's a theory I can easily test but it'd need to wait until tomorrow.

@x42
Copy link
Contributor

x42 commented Mar 12, 2024

C:\Users\JohnE\AppData\Local\Temp\aaf-cache-DX9LK2\CAFE 4.wav .aif

Is it a RIFF (.wav) or AIFF (.aif) file? What does sndfile-info say?

@agfline
Copy link
Owner Author

agfline commented Mar 12, 2024

It's embedded into the AAF as an AIFF file (essences are described with AIFCDescriptor and FileDescriptor::Summary is a valid AIFC header). It's common when exporting an AAF and forcing essences to a specific format (here, AIFF), that essence names remain as the original source file (here the original wav filename). Note that AAF says AIFC but standard does not support compression...

Note that, although the AIFC standard is designed to support compressed audio data, the AIFC essence format
defined by this specification does not include any compressed audio formats. The only AIFC compression form
supported is NONE and the only AIFC data items that are necessary are the COMM and SSND data items.

That's why libaaf appends .aif to essence name.

@johne53
Copy link

johne53 commented Mar 13, 2024

What seems to happen during an import is that Session::import_files() gets called for files which have been created temporarily in a cache. But sometimes the cached file didn't get created. I've no idea why but I don't think it's connected with having spaces in the name.

Adrien - can you let me know whereabouts the cached file would get created (I assume your code must do this??) If I know where it's getting created, that might help me to figure out why some creations fail.

@agfline
Copy link
Owner Author

agfline commented Mar 13, 2024

Cached files are created in OS dependent temp directory. In your case cache path is printed in Ardour logs : C:\Users\JohnE\AppData\Local\Temp\aaf-cache-XXXXXX\

@johne53
Copy link

johne53 commented Mar 13, 2024

Sorry, I meant whereabouts in your code will the cached files be getting created?

Each audio file seems to get initially created in the cache folder, then converted to wav (assuming it got successfully created) and then the cached version gets deleted before moving onto the next one. So I assume this is either under your control or Ardour's control?

[Edit...] Could something like this be happening...

  1. A region needs to get created so its audio file gets cached (and subsequently converted)
  2. A 2nd region needs to get created which would normally use the same audio file
  3. But the file already exists now so it doesn't get cached a second time
  4. And because it didn't get cached, the 2nd region fails to get created

@agfline
Copy link
Owner Author

agfline commented Mar 13, 2024

I think you're on the right path.

Can you try replacing

https://github.com/Ardour/ardour/blob/9e9fd201b526afac2cc52c12c1d670f5cf49e1eb/gtk2_ardour/ardour_ui_aaf.cc#L592

with

if (audioEssenceFile->is_embedded) {

@johne53
Copy link

johne53 commented Mar 13, 2024

Thanks Adrien, that's fixed it. I temporarily went back to Mixbus (which I hadn't yet updated) and what used to happen was that a cache folder got created, then it got populated with lots of files, then at the end of the process, the whole folder got deleted.

But what's happening now is that the cache folder only ever contains one file. So each file must've been getting deleted when it was in fact still needed for creating further regions.

I haven't yet tried with a non-embedded AAF. Would they be likely to suffer from a similar issue? i.e. does this need a small rethink?

@agfline
Copy link
Owner Author

agfline commented Mar 13, 2024

There is no issue with non-embedded AAF, since we will never delete external existing files.

You're right, 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. IMHO there are two solutions :

  1. Keep it that way with the simple modification I proposed. It will maintain a low memory usage on linux, however it will reexport the same essence file(s) again and again, every time a clip uses it, dropping performance with some large AAF sharing the same essence across many clips.
  2. Track essence usage while parsing clips, so we can keep removing cache files only when we are sure they wont be used anymore (might be tricky to implement though...)

What do you think ?

@johne53
Copy link

johne53 commented Mar 13, 2024

How about something like this (if it doesn't need a major revamp...)

  • When creating a region, check to see if the audio already exists (i.e. in the session's interchange folder)
  • If it does exist, create the region by referencing it to the already existing audio
  • If it doesn't exist, extract the audio to the cache and reference it from there (just like at present)

That should guarantee that a region will always get created - but with only minimsal need to keep generating cache entries.

Or if that'd involve too much of a revamp, I think your first option would be my preference.

@agfline
Copy link
Owner Author

agfline commented Mar 13, 2024

That's actually how it used to work in the previous version. If essence file were already extracted we used that file. However for this to work, the cache folder needed to be cleared after the import process had finished.

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...

Maybe we can stick to option 1 for now, since it's guaranteed to work (with more or less performance drop depending on AAF file). Note that option 2 might also fail to preserve RAM in some very specific AAF compositions.

@x42, any thought about it ?

@x42
Copy link
Contributor

x42 commented Mar 13, 2024

Only a minority of GNU/Linux systems use a tmpfs for /tmp
It comes up on occasion since Ardour's installer also requires space there. We tell users to set $TMPDIR to a different location (or pass an option to the installer, or buy more RAM).

  • Option 1: directly import the files without intermediate save to $TMPDIR
  • Option 2: do not use $TMPDIR, and instead save it into the session-path perhaps in externals/ or create tmp/ there

@agfline
Copy link
Owner Author

agfline commented Mar 13, 2024

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

Of course if that's an option, it seems the most efficient.
What's the process with Ardour ? I guess I should provide a callback function to read embedded files ?

@johne53
Copy link

johne53 commented Mar 14, 2024

Can you try replacing

https://github.com/Ardour/ardour/blob/9e9fd201b526afac2cc52c12c1d670f5cf49e1eb/gtk2_ardour/ardour_ui_aaf.cc#L592

with

if (audioEssenceFile->is_embedded) {

I've tried some further imports this morning and all seems to be working again. So while you're both deciding on a suitable way forward, can one of you commit that change to Ardour's version of libaaf? Even if it ends up being temporary it'll at least give Ardour a working AAF importer for users to test.

@agfline
Copy link
Owner Author

agfline commented Mar 14, 2024

Since this thread is already too long, here's a new one #28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants