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

Implement #2000 ("Add mp3 encoding and/or decoding support") #3615

Merged
merged 8 commits into from Jun 12, 2017

Conversation

@michaelgregorius
Copy link
Contributor

@michaelgregorius michaelgregorius commented Jun 5, 2017

Implement MP3 encoding for the song export when exporting via the GUI. The current implementation provides the following options to the user:

  • Setting the bit rate
  • Setting whether a variable bit rate should be used

Using MP3 encoding for command line exports is currently not supported and it's also not possible to import MP3s as samples or for the Audio File Processor plugin.

MP3 export is currently only enabled for Linux build, i.e. it is disabled for Windows and OS X builds.

Technical changes:

  • Add a new CMake module FindLame.cmake.
  • Add the option for MP3/Lame to the CMakeLists.txt and adjust all other related CMakeLists.txt accordingly (adding include paths and libraries to link).
  • Add a new class AudioFileMP3 which contains the Lame specific code. Register this new class in the list of export devices in ProjectRenderer.
  • Add a new enum to ProjectRenderer::ExportFileFormats.
  • Adjust ExportProjectDialog to show the appropriate widgets when MP3 is selected as the export format. To simplify this all widgets related to the sample rate have been put under a new parent widget which can then be hidden or shown (see changes in export_project.ui).

Other changes which are not directly related to the MP3 functionality:

  • Fix a bug where Ogg Vorbis was offered in the file selection dialog during export even if Ogg Vorbis has been disabled.
  • Add a method FileEncodeDevice::isAvailable which abstracts away the checking for the factory method.
Implement MP3 encoding for the song export when exporting via the GUI.
The current implementation provides the following options to the user:
* Setting the bit rate
* Setting whether a variable bit rate should be used

Using MP3 encoding for command line exports is currently not supported
and it's also not possible to import MP3s as samples or for the Audio
File Processor plugin.

MP3 export is currently only enabled for Linux build, i.e. it is
disabled for Windows and OS X builds.

Technical changes:

Add a new CMake module FindLame.cmake.

Add the option for MP3/Lame to the CMakeLists.txt and adjust all other
related CMakeLists.txt accordingly (adding include paths and libraries
to link).

Add a new class AudioFileMP3 which contains the Lame specific code.
Register this new class in the list of export devices in
ProjectRenderer.

Add a new enum to ProjectRenderer::ExportFileFormats.

Adjust ExportProjectDialog to show the appropriate widgets when MP3 is
selected as the export format. To simplify this all widgets related to
the sample rate have been put under a new parent widget which can then
be hidden or shown (see changes in export_project.ui).

Other changes which are not directly related to the MP3 functionality:

Fix a bug where Ogg Vorbis was offered in the file selection dialog
during export even if Ogg Vorbis has been disabled.

Add a method FileEncodeDevice::isAvailable which abstracts away the
checking for the factory method.
@michaelgregorius michaelgregorius mentioned this pull request Jun 5, 2017
2 of 3 tasks complete
Add libmp3lame-dev to the list of packages that are installed before the
build.
@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jun 5, 2017

I just successfully exported my first mp3 from within LMMS. ❤️

@tresf
Copy link
Member

@tresf tresf commented Jun 5, 2017

@michaelgregorius macOS works great, please add brew install lame in Travis and the tutorial and allow the build to use it. macdeployqt automatically picks up libmp3lame.0.dylib and properly bundles it with no additional code. 👍

Adjust the Travis script to install lame and remove the lines from the
CMakeLists.txt that disabled MP3 export for the OS X build.
@tresf

This comment has been minimized.

Copy link
Member

@tresf tresf commented on 969ade2 Jun 6, 2017

👍

@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 6, 2017

@tresf Thanks for testing this pull request on OS X and thanks for the instructions on how to get the OS X build going. I have updated the tutorial as proposed.

@tresf
Copy link
Member

@tresf tresf commented Jun 6, 2017

@michaelgregorius I've emailed Toby for the mingw repos. I'm not sure if he'll be able to get back to us in a timely fashion.

If the Windows build fails when we enable the WANT_MP3LAME, you probably just need a trick like this.

Also, this is against the "frozen" stable-1.2 branch. I understand why, but this will affect #3607 once merged.

@tresf
Copy link
Member

@tresf tresf commented Jun 6, 2017

@michaelgregorius just an FYI... We rather irresponsibly use src/CMakeLists.txt to copy the install target DLL files for Windows. We'll be adding a new item... if @tobydox keeps this consistent, it will be (on Ubuntu libmp3lame.so.0) he'll call libmp3lame-0.dll. It will need to be inserted here once available.

<offtopic> The reason I say this is irresponsible is that we should construct our library names based on which features we build with, not blindly based on a long list, so this can't make it in until mingw is ready. Long term, the project will build the DLLs based on what features are available. </offtopic>

<ontopic> I mention this because it's a follow-up task if this gets merged before Toby updates the PPA.

@tresf
Copy link
Member

@tresf tresf commented Jun 6, 2017

@michaelgregorius Toby's added mingw32-x-lame, mingw64-x-lame. They can be safely added to .travis/linux.win32.install.sh#L14 and .travis/linux.win64.install.sh#L19 respectively.

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Jun 6, 2017

I just did a quick test on export and LMMS hanged. I'm swamped with schoolwork, so idk when I'll be able to make a proper backtrace, I just wanted to make this comment if someone else could do extencive testing.

@tresf
Copy link
Member

@tresf tresf commented Jun 6, 2017

I just wanted to make this comment if someone else could do extencive testing.

I only did a single-track export using MacOS but thanks. We don't want to introduce any last-minute crashes to stable-1.2, so this is valuable information.

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Jun 6, 2017

Also the users on discord have been having clipping artifacts in the 24 bit export but I don't want to open an issue until I have some testing results and what causes them, which is going to be as soon as I get some stuff out of my plate 🙂

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jun 6, 2017

clipping artifacts in the 24 bit export but I don't want to open an issue until I have some testing results and what causes them, which is going to be as

It folds over at +/-1.0f .

Edit: I added a new issue for this here: #3616

@karmux
Copy link
Contributor

@karmux karmux commented Jun 6, 2017

I exported few tracks to mp3 too. Some simple projects and some complex. Didn't experience any problems 👍

Adjust the Travis scripts to install lame and remove the lines from the
CMakeLists.txt that disabled MP3 export for the Windows build.
@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 6, 2017

@tresf I fully understand that these changes are rather big and risky for stable-1.2. Shall I rebase them to master so that they will become available for 1.3? Otherwise 1.2 becomes a never ending game. 😉

@tobydox Thanks for providing the libraries so quickly! The Windows builds are already through on Travis so each platform should now have MP3 support. :)

@Umcaruje Thanks for the first quick test. This feature definitively should be tested extensively. If you have a file that leads to a hang please provide it for further insight.

@zonkmachine The fold over should be fixed with #3617.

@tresf
Copy link
Member

@tresf tresf commented Jun 6, 2017

@michaelgregorius, last step for Windows is copying the DLL in src/CMakeLists.txt

@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 6, 2017

@tresf Done! Sorry, I misunderstood this as something that was planned for the future.

@@ -139,6 +143,7 @@ SET(LMMS_REQUIRED_LIBS
${PULSEAUDIO_LIBRARIES}
${JACK_LIBRARIES}
${OGGVORBIS_LIBRARIES}
${LAME_LIBRARIES}

This comment has been minimized.

@tresf

tresf Jun 7, 2017
Member

So the way CMake works on all of these libraries, if can't find lame, the LAME_LIBRARIES variable will be a value of LAME_LIBRARIES-NOTFOUND which will abort the build. This is a problem with our build scripts and not necessarily a problem with this PR, but once merged, people without the proper lame libraries will get build errors, hence this comment about the "trick" mentioned here. #3615 (comment)

This comment has been minimized.

@michaelgregorius

michaelgregorius Jun 7, 2017
Author Contributor

So something like this, @tresf?

IF(WANT_MP3LAME)
	FIND_PACKAGE(Lame)
	IF(LAME_FOUND)
		SET(LMMS_HAVE_MP3LAME TRUE)
		SET(STATUS_MP3LAME "OK")
	ELSE(LAME_FOUND)
		SET(STATUS_MP3LAME "not found, please install libmp3lame-dev (or similar)")
		SET(LAME_LIBRARIES "")
		SET(LAME_INCLUDE_DIRS "")
	ENDIF(LAME_FOUND)
ELSE(WANT_MP3LAME)
	SET(STATUS_MP3LAME "Disabled for build")
ENDIF(WANT_MP3LAME)

On the other hand wouldn't it be cleaner to have a structure like the following in src/CMakeLists.txt?

IF(LMMS_HAVE_MP3LAME) # alternatively use LAME_FOUND
# Add include directory for lame here
# Add library for lame here
ENDIF()
@tresf
Copy link
Member

@tresf tresf commented Jun 7, 2017

This looks good to merge. I'd like a second set of eyes, specifically on the C++ code.

If we want this for 1.2.0, we'll need it part of RC4 so we have some solid testing on it prior to release.

@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 7, 2017

It would also be great if we had some more eyes on the variable bit rate code.

I first tried to use average bitrate (ABR) in the export code but for some reason this did not work and I am also not sure whether it really works with the current code which should be similar to what's done for variable bit rate Ogg exports.

When I encode an exported wav file with the lame command line front end using ABR this is also shown as something like "abr192" when played with mpg123. However when I played the files that LMMS should have encoded directly using ABR this was not shown.

Enable the user to switch between stereo, joint stereo and mono mode.
@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jun 8, 2017

If we want this for 1.2.0, we'll need it part of RC4 so we have some solid testing on it prior to release.

I think we do want this in 1.2.0 , and it's not only for the mp3 export. This PR also brings on a well needed cleanup to the export dialogue. I'm adding this to the RC4 project for now.

@zonkmachine zonkmachine added this to In Progress in Release 1.2.0 RC4 Jun 8, 2017
@Umcaruje Umcaruje added this to the 1.2.0 milestone Jun 10, 2017
@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Jun 10, 2017

@zonkmachine you can directly push to @michaelgregorius's branch, just an FYI

@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 10, 2017

@zonkmachine Thanks for taking the initiative to implement the command line export! However, I think that before we continue with the command line implementation we all should discuss what should go into the GUI for the 1.2 release so that we can then mirror that functionality for the command line.

Here is a screenshot of the current implementation:
2000-mp3-export-20170610

I propose that we discuss at least the following points:

  • I have added an option to explicitly set the stereo mode (stereo, joint stereo, mono). Shall we keep this? If we keep this a new option for that functionality will also need to be added to the command line.
  • As mentioned above the variable bit rate doesn't really seem to work right now. Shall we hide that checkbox in the case of MP3 export and only allow constant bit rate exports for now?

We also have to keep in mind to also update the man page with the new command line options.

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Jun 10, 2017

I vote we keep the stereo box, and remove the variable bitrate box. Also I think the default should be 320kbps bitrate. Another really neat thing would be that the export dialog remembers your previous settings, but that's off the scope of this PR.

Also I tested this today again, I couldn't get the hang with the same project file. It was either a random quirk, or some of the newer commits fixed it. Otherwise, great work, this is an fantastic addition to 1.2 👍

@tresf
Copy link
Member

@tresf tresf commented Jun 10, 2017

Ogg default now is 160, which is just fine for the average user and is a good compromise between file size and quality. I vote we keep mp3 and ogg to be the same default bitrate values.

@tresf
Copy link
Member

@tresf tresf commented Jun 10, 2017

the export dialog remembers your previous settings

Sounds like #678

Hide the variable bit rate check box in the export dialog when MP3
export is selected. Remove the code that handled variable bit rate as it
was not working correctly anyway.
@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 11, 2017

The latest commit removes the variable bit rate checkbox in the case of MP3 export and also removes the corresponding code in the export class. Here's the current state:
2000-mp3-export-20170611

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jun 11, 2017

When I encode an exported wav file with the lame command line front end using ABR this is also shown as something like "abr192" when played with mpg123. However when I played the files that LMMS should have encoded directly using ABR this was not shown.

Apart from the crash/hang on export @Umcaruje saw, this seem to be the only question mark. I have done a great number of exports now and have seen no issues but also no reference to abr in the output. I tried encoding with lame --abr 128 test.wav test.mp3 and there was no literal hint at abr128 in either mpg123 or mp3check.

zonkmachine@zonkmachine:~/builds/lmms/lmms$ mpg123 abrtest.mp3 
...
Playing MPEG stream from abrtest.mp3 ...
MPEG 1.0 layer III, 128 kbit/s, 44100 Hz joint-stereo
$ mp3check -l abrtest.mp3 
abrtest.mp3:
mpeg 1.0 layer 3 44.1kHz 128kbps joint stereo no emph --- orig ----     0:28.22

The selection of controls are good and I think this should be pretty much merged.

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Jun 12, 2017

Apart from the crash/hang on export @Umcaruje saw, this seem to be the only question mark.

Yeah you probably missed my comment: #3615 (comment)

Also I tested this today again, I couldn't get the hang with the same project file. It was either a random quirk, or some of the newer commits fixed it.

So, this is ready to merge, imo

@tresf
Copy link
Member

@tresf tresf commented Jun 12, 2017

Agreed.

@tresf
tresf approved these changes Jun 12, 2017
@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jun 12, 2017

Yeah you probably missed my comment: #3615 (comment)

Indeed.

@tresf tresf merged commit c2f26a7 into LMMS:stable-1.2 Jun 12, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tresf
Copy link
Member

@tresf tresf commented Jun 12, 2017

Cherry-picked and ready for master via ca5d2b6.

@zonkmachine zonkmachine moved this from In Progress to Done in Release 1.2.0 RC4 Jun 12, 2017
@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jun 15, 2017

@michaelgregorius Can you look into command line arguments for mp3?

@michaelgregorius michaelgregorius deleted the michaelgregorius:mp3-encoding branch Jun 15, 2017
@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 15, 2017

@zonkmachine I have implemented the command line export via #3641.

@@ -1316,7 +1316,8 @@ void Song::exportProject( bool multiExport )
efd.setFileMode( FileDialog::AnyFile );
int idx = 0;
QStringList types;
while( ProjectRenderer::fileEncodeDevices[idx].m_fileFormat != ProjectRenderer::NumFileFormats )
while( ProjectRenderer::fileEncodeDevices[idx].m_fileFormat != ProjectRenderer::NumFileFormats &&
ProjectRenderer::fileEncodeDevices[idx].isAvailable())

This comment has been minimized.

@PhysSong

PhysSong Jul 25, 2017
Member

@michaelgregorius This logic malfunctions if LMMS is built with WAV + MP3 support (no OGG). I found it while testing #3714.

This comment has been minimized.

@michaelgregorius

michaelgregorius Jul 25, 2017
Author Contributor

@PhysSong Good catch! For now I have asked @irrenhaus3 whether (s)he can fix it as part of #3713.

@PhysSong PhysSong moved this from Done to In Progress in Release 1.2.0 RC4 Sep 17, 2017
@PhysSong PhysSong moved this from In Progress to Done in Release 1.2.0 RC4 Sep 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.