adding sample exporter #7627

szeli1 · 2024-12-19T20:14:26Z

This Pull request adds sample exporting to lmms that is independent from AudioEngine.

How to test:
Open lmms, load a sample into a sample clip. Open the context menu and click on the new option called "export sample buffer". A file dialog should open up. Select an audio file to export to, or create a new file by using a custom name in the file dialog. Click "open" inside the file dialog. The sample should be exported to the selected output file.

related pull request: #7538
this allows #7786 to be finished

src/gui/clips/SampleClipView.cpp

src/core/LmmsExporterSample.cpp

firewall1110 · 2025-01-03T20:19:00Z

It seems that #7627 only exports the content of sample clips into files, i.e. it will not replace the recorded data with references to the exported files.

Can this be fixed with new option to replace old reference or data with reference to exported file?

michaelgregorius · 2025-01-03T20:23:26Z

It seems that #7627 only exports the content of sample clips into files, i.e. it will not replace the recorded data with references to the exported files.

Can this be fixed with new option to replace old reference or data with reference to exported file?

We should not implement too many workflow crutches that we might never get rid of again. IMO recording to files should be implemented as fast as possible instead of some workarounds.

szeli1 · 2025-01-04T11:37:22Z

Can this be fixed with new option to replace old reference or data with reference to exported file?

Originally I couldn't do it because the exporting ran on a different thread and SampleClipView didn't wait for the exporting to be done before trying to import it.

firewall1110 · 2025-01-04T12:16:17Z

SampleClipView didn't wait for the exporting to be done

Can this exporting thread before quit trigger event connected to some SampleClip (OR ...) slot that do the same as happens when user load file to clip?

[Of course - only if user want this to happen]

szeli1 · 2025-01-04T18:13:19Z

I will need to move the FlacExporter into a new file, but it is ready for review.

szeli1 · 2025-01-05T14:32:38Z

I realized that I would rather not touch AudioFileFlac, because I don't fully understand how it works. For example the writeBuffer() function has "integer PCM encoding" built in, while FlacExporter doesn't and it doesn't need to have it. It can certainly be used to replace most libsndfile functions for AudioFileFlac, but I'm unsure if this should be done.

szeli1 · 2025-01-05T14:58:18Z

@michaelgregorius It seems like you introduced a crash by removing the code that ensures ".flac" file extensions are there. If the name (outputFilename) doesn't contain an extension in the file dialog, then lmms will crash. I believe this happens because after exporting, the exported sample will be reimported by the clip, but the clip will try to import outputFilename without the ".flac" extension that does not exist.

michaelgregorius · 2025-01-06T20:01:25Z

@michaelgregorius It seems like you introduced a crash by removing the code that ensures ".flac" file extensions are there. If the name (outputFilename) doesn't contain an extension in the file dialog, then lmms will crash. I believe this happens because after exporting, the exported sample will be reimported by the clip, but the clip will try to import outputFilename without the ".flac" extension that does not exist.

@szeli1, I have fixed this with commit 5fa60b3 by saving the file with the name that the users have selected and not appending the ".flac" extension.

include/LmmsMassExporter.h

szeli1 · 2025-01-06T21:44:13Z

@szeli1, I have fixed this with commit 5fa60b3 by saving the file with the name that the users have selected and not appending the ".flac" extension.

I don't know why do you prefer removing it, but sure, I approve if it works. I will remove the QFileInfo include.

szeli1 · 2025-01-08T06:07:19Z

@szeli1, I have fixed this with commit 5fa60b3 by saving the file with the name that the users have selected and not appending the ".flac" extension.

It now works, it can export files without file extension, I don't know why is this useful, because the result will be a file without an extension, so every time the user would like to open it, they will have to select a program that can open .flac files.

CorruptVoidSoul · 2025-01-09T20:53:13Z

So I tested this with a wav and an ogg file, both exported without problems and there doesn't seem to be any hearable difference.
In case it's wanted, system info is aarch64 Ubuntu running on top of Termux on a phone (samsung galaxy A25).

michaelgregorius · 2025-01-12T14:06:49Z

@szeli1, I have fixed this with commit 5fa60b3 by saving the file with the name that the users have selected and not appending the ".flac" extension.

It now works, it can export files without file extension, I don't know why is this useful, because the result will be a file without an extension, so every time the user would like to open it, they will have to select a program that can open .flac files.

The reason is that we should let users do what the want to do. The dialog already proposes the "flac" extension so this adds no extra work for the users. The default is that it is exported with the extension.

However, if the users for some reason decide that they want/need to have a file without extension then the previous implementation would make them jump through the hoops to export the file and then having to rename it manually to remove the extension. With the current implementation they simply save it without the extension and they are good.

The current implementation gives the best of both words. I guess we all have some experience with software that attempts to be clever only to do the wrong thing. So please let's not add such behavior to LMMS if it is not even necessary in some way.

src/core/SampleClip.cpp

firewall1110 · 2025-01-12T16:39:39Z

If users want to replace the sample with the exported one it should either be an option in the export dialog (initially disabled)

Something like this I mean when wrote:

Can this be fixed with new option to replace old reference or data with reference to exported file?

JohannesLorenz · 2025-02-16T13:08:47Z

This, again, should be merged into #5990, right?

szeli1 · 2025-02-16T13:15:42Z

This, again, should be merged into #5990, right?

This is the plan as far as I'm aware. (or merging it to master and rebasing #5990)

Dismissing my own review so that I am not blocking the process of this issue due to phasing out my involvement in the project.

Use `FileDialog::getSaveFileName` to retrieve the file name of the file into which the sample buffer is saved. This gives us lots of functionality automatically, e.g. the request if an existing file should be overwritten. Also let remove the appending of the `.flac` file extension and let the users decide using the system file dialog.

Save the exported file under the name that the users have selected and do not append ".flac". This fixes an exception that was thrown because the file is saved with the appended extension but then attempted to load without it.

szeli1 · 2025-05-08T09:06:04Z

I have moved the ThreadedExportManager to Engine.h because ideally one instance should handle all the exporting.
I also reworked ThreadedExportManager.h and removed unused code.
I replaced the clamping code in FlacExporter to std::clamp.

regulus79 · 2025-05-08T17:47:18Z

Testing this on Arch Linux + GNOME (I don't have the internet to download a build, so I pulled and compiled locally)

I tried opening a .wav sample and rightclicking and exporting, but even after entering a filename and pressing Save, it doesn't create a file. I tried multiple times with different filenames and different samples, but it didn't work.

For some reason when you click the "Export sample buffer" button, it opens the default gnome file explorer thing, not the lmms file dialog. But when double clicking on a sample to open a file, it opens the lmms file dialog.

szeli1 · 2025-05-10T09:09:06Z

I tried opening a .wav sample and rightclicking and exporting, but even after entering a filename and pressing Save, it doesn't create a file. I tried multiple times with different filenames and different samples, but it didn't work.

Thanks for noticing this, the issue was an if statement that checked when to abort. I have fixed it.

For some reason when you click the "Export sample buffer" button, it opens the default gnome file explorer thing, not the lmms file dialog. But when double clicking on a sample to open a file, it opens the lmms file dialog.

It was decided to use the default file browser to export.

Thanks for your testing!

AW1534 · 2025-05-11T13:21:20Z

It was decided to use the default file browser to export.

Could you link the discussion or explain why?

szeli1 · 2025-05-12T15:59:08Z

Could you link the discussion or explain why?

Usually custom file browsers are subpar compared to the file browsers provided by the os. This is the case with QT. Users can change what kind of file browser they use (on linux), but they can't replace QT's file browser.

AW1534 · 2025-05-12T16:24:40Z

Could you link the discussion or explain why?

Usually custom file browsers are subpar compared to the file browsers provided by the os. This is the case with QT. Users can change what kind of file browser they use (on linux), but they can't replace QT's file browser.

I agree with that, but switching to the native file browser is not as simple as it seems (as @regulus79, @tresf and I discovered in #7856). So if it's not strictly necessary for this PR, you should stick with the Qt browser for now, and #7856 will change it when it's ready.

Edit: I tested the AppImage and this PR seems to be affected by the same issues #7856 is. Exporting didn't work for me but I assume it's due to the broken file dialog

JohannesLorenz · 2025-06-14T20:22:38Z

src/core/ThreadedExportManager.cpp

+
+ThreadedExportManager::~ThreadedExportManager()
+{
+	stopExporting();


Let us assume I close LMMS while there are still buffers left to be rendered... Then this here will set m_isThreadRunning to false. Hence, threadedExportMethod will see shouldRun as false and will discard all pending requests - which means data loss. So, why not remove the m_isThreadRunning and just let stopExporting call m_thread->join(), which will wait until all pending requests are finished?

I would expect things to be lost when I start to export something and then close the application. However nothing indicates that the exporting was finished.

Maybe use the callback function? (To display a Text Float)

szeli1 force-pushed the sample_export branch from db25510 to 58f4e15 Compare December 19, 2024 20:36

qnebra mentioned this pull request Jan 2, 2025

Sample Track Recording Stage One #5990

Closed