Add FLAC export and related options #3731

irrenhaus3 · 2017-07-26T11:30:39Z

Implements #3679 by adding a FLAC rendering device to LMMS' Project renderer and related GUI configuration widgets in the export window.
The renderer is implemented in terms of libsndfile and hardens the dependency on it, requiring a minimal version of 1.0.18.
Additionally, since the FLAC compression level settings was not supported in libsndfile until version 1.0.26, a new CPP flag is added, called LMMS_HAVE_SF_COMPLEVEL. It is undefined if the sndfile version is less than 1.0.26. The "Compression Level" GUI element is only visible (and the setting received from it is only respected in the renderer) if LMMS_HAVE_SF_COMPLEVEL is defined.

Unrelated to this new feature, this PR also introduces a minor bug fix regarding the output file extension. If the output format doesn't match the file extension set by the user before the export dialog window opens, the extension will automatically be adjusted when the rendering process starts.

This symbol is defined if libsndfile is available in version >= 1.0.26 and thus has the command SF_SET_COMPRESSION_LEVEL.

The code related to compression is only generated if LMMS_HAVE_SF_COMPLEVEL is defined. Otherwise, the UI element for it will exist in the mockup, but stay hidden, and the underlying functionality is not available at all.

… types.

… condition in GUI and FLAC-renderer.

…alog

zonkmachine · 2017-07-26T18:52:32Z

src/core/audio/AudioFileFlac.cpp

+
+	m_sf = sf_open(
+#ifdef LMMS_BUILD_WIN32
+		outputFile().toLocal8bit().constData(),


toLocal8bit() -> toLocal8Bit()

Lol, and here I was wondering why travis exploded. Thanks!

irrenhaus3 · 2017-07-26T22:38:48Z

The QT5 builds are failing. When I tested it locally by running cmake with -DWANT_QT5=ON, compilation terminated on the file plugins/sf2_player/ui_patches_dialog.h. The offending line is
#include <QtGui/QAction>
I've googled around a bit and it seems that in Qt5, the QAction header is in QtWidgets, not in QtGui.
sf2_player is not affected by this PR, so I'm not really sure what to make of this. Sounds like something that may require some attention though.
Edit: Turns out I was just sloppy. See #3731 (comment)

zonkmachine · 2017-07-26T22:50:04Z

Edit: A blanket 'wrong' on my latest contribution, deleted...

PhysSong · 2017-07-27T01:59:06Z

src/gui/ExportProjectDialog.cpp

+		QString info="";
+		if (i==0){ info = tr("(fastest)", nullptr, QApplication::UnicodeUTF8); }
+		if (i==4){ info = tr("(default)", nullptr, QApplication::UnicodeUTF8); }
+		if (i==MAX_LEVEL){ info = tr("(smallest)", nullptr, QApplication::UnicodeUTF8); }


@irrenhaus3 I suggest using tr("(something)") without additional variable(s).

Here fails Qt5 build, too. As far as I know, QAppliction::UnicodeUTF8 is deprecated.

Good catch, thank you. The symbol was removed in the newest commit.

irrenhaus3 · 2017-07-27T09:45:57Z

My bad about that autogenerated SF2player GUI header. Turns out I didn't properly clean up my build directory and that particular header was left over from a previous QT4 build. Oops...

PhysSong · 2017-07-28T09:30:42Z

include/AudioFileFlac.h

@@ -0,0 +1,48 @@
+/*
+ * AudioFileFlac.h - Audio device which encodes a wave stream into a FLAC file.
+ */


You should put the copyright information here.

PhysSong · 2017-07-28T09:31:05Z

src/core/audio/AudioFileFlac.cpp

@@ -0,0 +1,95 @@
+#include <memory>


zonkmachine · 2017-07-31T02:13:20Z

You need text for lmms --help and man page ( doc/lmms.1). I can't test/review the LMMS_HAVE_SF_COMPLEVEL stuff. The rest i think is 'go!' 👍

PhysSong

One thing about release(): it releases pointer, not deletes it.
http://www.cplusplus.com/reference/memory/unique_ptr/release/

How about using reset() without any argument?

PhysSong · 2017-07-31T07:45:30Z

I tested it with both 1.0.25 and 1.0.28, the rest things work great.

PhysSong · 2017-07-31T07:52:50Z

I found one more bug just now: It breaks "Export tracks" path setting. If I choose directory '/foo/bar', it tries to export tracks to '/foo/bar.mp3/1_asdf.mp3'.

PhysSong · 2017-08-01T01:06:16Z

One more request: please add WANT_FLAC option to CMakeLists.txt and let the code be built with libsndfile 1.0.11~1.0.17 without flac export.

tresf · 2017-08-01T04:37:32Z

let the code be built with libsndfile 1.0.11~1.0.17 without flac export.

No, please do not recommend this. As stated in the PR, sndfile >= 1.0.18 will be the new minimum version on our master branch. This was discussed on Discord. Legacy systems that do not have sndfile >= 1.0.18 will have to use stable-1.2 or older. Why? Because Ubuntu 10.04 offered 1.0.21, and that distribution is now 7 years old and was end of life 2 years ago. :)

PhysSong · 2017-08-01T04:52:01Z

@tresf Quite true.
@irrenhaus3 However, WANT_FLAC and status message in CMakeLists.txt is still needed.

tresf · 2017-08-01T04:59:33Z

However, WANT_FLAC and status message in CMakeLists.txt is still needed.

Why?

PhysSong · 2017-08-01T05:18:18Z

Why?

@tresf Both OGG and MP3 have such things, so user can enable/disable such features manually.
It means users should be able to disable FLAC export feature if not needed, although it is supported.
https://github.com/LMMS/lmms/blob/stable-1.2/CMakeLists.txt#L55

tresf · 2017-08-01T05:20:42Z

Because they are (or were) separate libraries. Don't add a toggle for historical purposes please. Instead, remove OGG toggle.

PhysSong · 2017-08-01T05:53:27Z

Because they are (or were) separate libraries.

Yes.

Don't add a toggle for historical purposes please.

I don't agree. They were added because of historical purposes, but I think they are still useful.
I can disable some features if I don't want them, for build speed and some other purposes.

Instead, remove OGG toggle.

I agree that it should be, if FLAC toggle is not added, for consistency.

irrenhaus3 · 2017-08-01T12:57:54Z

@PhysSong: I agree with @tresf on this one. I really can't picture a use-case where a post-1.2 LMMS end user would want to disable FLAC, but keep WAV, especially since they're both implemented in terms of the same library. While I agree that it might make some sense in development and testing, this particular feature doesn't noticably increase build times or interfere with unrelated system components.
As such, I'd like to refrain from implementing a WANT_FLAC mode for now. If the need for it ever arises, it can easily be done anyway. But in this PR, I'll only do it if the majority of developers agrees on it.

PhysSong · 2017-08-01T13:06:55Z

@irrenhaus3 Then could you wait for other developers' opinion for CMakeLists.txt issue?
However, I think the code needs no more change(s).

zonkmachine · 2017-08-01T14:31:41Z

Removing the ogg toggle option is better than adding a flac toggle. Keep it simple.

tresf · 2017-08-01T14:46:10Z

src/core/ProjectRenderer.cpp

@@ -42,6 +43,11 @@ const ProjectRenderer::FileEncodeDevice ProjectRenderer::fileEncodeDevices[] =
 	{ ProjectRenderer::WaveFile,
 		QT_TRANSLATE_NOOP( "ProjectRenderer", "WAV-File (*.wav)" ),
 					".wav", &AudioFileWave::getInst },
+    { ProjectRenderer::FlacFile,


@irrenhaus3 this needs formatting, spaces versus tabs. :)

tresf · 2017-08-01T15:20:51Z

A follow-up task to merging this should be to update the required libsndfile version on our Compiling wiki page.

PhysSong · 2017-08-02T01:04:14Z

@irrenhaus3 Then could you remove WANT_OGG toggle and related flags?

irrenhaus3 · 2017-08-02T13:16:53Z

Update for the interested reader not on the LMMS Discord server:
In a discussion on the developers channel, we found that refactoring the OGG exporter to use sndfile and removing the WANT_OGGVORBIS flag are separate issues from FLAC export and will be handled in their own PR.

tresf · 2017-08-03T01:35:37Z

src/core/audio/AudioFileFlac.cpp

+		&m_sfinfo
+	);
+
+	sf_command(m_sf,SFC_SET_CLIPPING,nullptr,SF_TRUE);


It is conventional to put spaces between parameters.

Like foo(b, a, r);.

PhysSong · 2017-08-03T08:01:32Z

I recommend adding a status message for FLAC, like WAV one. It's not necessary, but I think having this will be better.

PhysSong · 2017-08-03T09:50:11Z

Great! Now everything looks fine.
I'll merge it soon, if there's no objection.

zthurman · 2018-11-02T15:10:06Z

I'm probably being a dumb but I'm not seeing this in the front end options on lmms 1.2.0-rc7 running on Arch or the lmms-git version 1.2.0-rc7.11.

Am I understanding correctly from the above that I have to build from source and tweak out a bit in order to use this enhancement?

zonkmachine · 2018-11-02T19:40:36Z

I'm probably being a dumb but I'm not seeing this in the front end options on lmms 1.2.0-rc7 running on Arch or the lmms-git version 1.2.0-rc7.11.

It's because this has been added to master and will be in lmms-1.3.

Am I understanding correctly from the above that I have to build from source and tweak out a bit in order to use this enhancement?

You either build master or you can cherry-pick commit e9a4063 to stable-1.2 .
I've seen unofficial builds around for master that you could try out but I don't know precisely where or if they are up to date. Check out our support channel at discord, https://discord.gg/3sc5su7 (#support), and ask there. You'll get help much sooner there I think. This is a closed pull request and late comments on those tend to get little attention.

* Add FLAC export, based on WAV renderer * Depend on sndfile>=1.0.18 (previously >=1.0.11) * Add compression option to FLAC export, if available. The code related to compression is only generated if LMMS_HAVE_SF_COMPLEVEL is defined(libsndfile>=1.0.26). * Save into the correct file extension upon single-export. * Use unique_ptr in FLAC renderer and be more expressive about involved types. * Use unique_ptr and remove manual memory management in ExportProjectDialog * Add 'flac' format info to --help and manpage

Levin and others added 11 commits July 26, 2017 13:14

Add FLAC export, based on WAV renderer

5f1c9dc

Indent with TAB character

b41bbab

Depend on sndfile>=1.0.18 (previously >=1.0.11)

ed7a4b6

Introduces new CPP definition LMMS_SF_HAVE_CMPLEVEL

d7bb49a

This symbol is defined if libsndfile is available in version >= 1.0.26 and thus has the command SF_SET_COMPRESSION_LEVEL.

Add compression option to FLAC export, if available.

36354a8

The code related to compression is only generated if LMMS_HAVE_SF_COMPLEVEL is defined. Otherwise, the UI element for it will exist in the mockup, but stay hidden, and the underlying functionality is not available at all.

Save into the correct file extension upon export.

8fe7994

Use unique_ptr in FLAC renderer and be more expressive about involved…

5ba265f

… types.

Enable compression setting unconditionally, only use the SF_COMPLEVEL…

caba25e

… condition in GUI and FLAC-renderer.

Use unique_ptr and remove manual memory management in ExportProjectDi…

f89475d

…alog

Populate compLevelCB unconditionally and make its strings translatable.

dfe6a69

Remove unused #include statement.

9d24ebc

zonkmachine reviewed Jul 26, 2017

View reviewed changes

Fixed a silly typo.

14b6ce3

PhysSong requested changes Jul 27, 2017

View reviewed changes

Remove use of deprecated symbol QApplication::UnicodeUTF8

20cbfbf

PhysSong reviewed Jul 28, 2017

View reviewed changes

Added copyright information

65443db

PhysSong requested changes Jul 31, 2017

View reviewed changes

Adapt file extension only in single-export mode.

d7ee49c

Add 'flac' format info to --help and manpage

18cda1d

tresf reviewed Aug 1, 2017

View reviewed changes

Code style

597d947

More code style

597bc8f

PhysSong approved these changes Aug 3, 2017

View reviewed changes

tresf reviewed Aug 3, 2017

View reviewed changes

irrenhaus3 added 2 commits August 3, 2017 11:05

CMakeLists says: FLAC's pretty OK.

2c3cb02

Yet more code style.

521c08c

PhysSong merged commit e9a4063 into LMMS:master Aug 3, 2017

tresf mentioned this pull request Aug 5, 2017

Add FLAC export support #3679

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FLAC export and related options #3731

Add FLAC export and related options #3731

irrenhaus3 commented Jul 26, 2017

zonkmachine Jul 26, 2017

irrenhaus3 Jul 26, 2017

irrenhaus3 commented Jul 26, 2017 •

edited

Loading

zonkmachine commented Jul 26, 2017 •

edited

Loading

PhysSong Jul 27, 2017

PhysSong Jul 27, 2017 •

edited

Loading

irrenhaus3 Jul 27, 2017

irrenhaus3 commented Jul 27, 2017

PhysSong Jul 28, 2017

PhysSong Jul 28, 2017

zonkmachine commented Jul 31, 2017

PhysSong left a comment •

edited

Loading

PhysSong commented Jul 31, 2017 •

edited

Loading

PhysSong commented Jul 31, 2017

PhysSong commented Aug 1, 2017

tresf commented Aug 1, 2017 •

edited

Loading

PhysSong commented Aug 1, 2017

tresf commented Aug 1, 2017

PhysSong commented Aug 1, 2017 •

edited

Loading

tresf commented Aug 1, 2017 •

edited

Loading

PhysSong commented Aug 1, 2017

irrenhaus3 commented Aug 1, 2017

PhysSong commented Aug 1, 2017 •

edited

Loading

zonkmachine commented Aug 1, 2017

tresf Aug 1, 2017

tresf commented Aug 1, 2017

PhysSong commented Aug 2, 2017

irrenhaus3 commented Aug 2, 2017

tresf Aug 3, 2017

PhysSong Aug 3, 2017

PhysSong commented Aug 3, 2017

PhysSong commented Aug 3, 2017

zthurman commented Nov 2, 2018

zonkmachine commented Nov 2, 2018

Add FLAC export and related options #3731

Add FLAC export and related options #3731

Conversation

irrenhaus3 commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irrenhaus3 commented Jul 26, 2017 • edited Loading

zonkmachine commented Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

PhysSong Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irrenhaus3 commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonkmachine commented Jul 31, 2017

PhysSong left a comment • edited Loading

Choose a reason for hiding this comment

PhysSong commented Jul 31, 2017 • edited Loading

PhysSong commented Jul 31, 2017

PhysSong commented Aug 1, 2017

tresf commented Aug 1, 2017 • edited Loading

PhysSong commented Aug 1, 2017

tresf commented Aug 1, 2017

PhysSong commented Aug 1, 2017 • edited Loading

tresf commented Aug 1, 2017 • edited Loading

PhysSong commented Aug 1, 2017

irrenhaus3 commented Aug 1, 2017

PhysSong commented Aug 1, 2017 • edited Loading

zonkmachine commented Aug 1, 2017

Choose a reason for hiding this comment

tresf commented Aug 1, 2017

PhysSong commented Aug 2, 2017

irrenhaus3 commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhysSong commented Aug 3, 2017

PhysSong commented Aug 3, 2017

zthurman commented Nov 2, 2018

zonkmachine commented Nov 2, 2018

irrenhaus3 commented Jul 26, 2017 •

edited

Loading

zonkmachine commented Jul 26, 2017 •

edited

Loading

PhysSong Jul 27, 2017 •

edited

Loading

PhysSong left a comment •

edited

Loading

PhysSong commented Jul 31, 2017 •

edited

Loading

tresf commented Aug 1, 2017 •

edited

Loading

PhysSong commented Aug 1, 2017 •

edited

Loading

tresf commented Aug 1, 2017 •

edited

Loading

PhysSong commented Aug 1, 2017 •

edited

Loading