Add CLI equivalent to GUI's "export tracks" #2310

rcorre · 2015-09-04T02:03:22Z

The GUI has an "export tracks" option that exports each track to a separate file.
This adds a --render-tracks CLI option to enable the same behavior from the command line.

In order to do this, I moved much of the rendering logic out of the gui code in ExportProjectDialog and into its own class called RenderManager.

Due to #588, --render-tracks will segfault just as --render does, but the output seems intact.

Resolves #2254.

tresf · 2015-09-04T02:09:23Z

Thanks. FYI, #2258

-Tres

rcorre · 2015-09-04T03:04:56Z

--rendertracks, then? Unless someone can come up with a nice, one-word description of what this does.

tresf · 2015-09-04T03:38:32Z

Render is assumed, right? Perhaps just --tracks

Wallacoloo · 2015-09-04T03:55:18Z

@tresf the usage for the flag is --render-tracks <project> which makes it appear that you can use it without the --render flag, so I don't see how render can be assumed.

Wallacoloo · 2015-09-04T03:58:21Z

src/core/ProjectRenderer.cpp

@@ -124,6 +124,15 @@ ProjectRenderer::ExportFileFormats ProjectRenderer::getFileFormatFromExtension(



+QString ProjectRenderer::getFileExtensionFromFormat(
+		ExportFileFormats _fmt )


In all future code added to lmms, arguments should not be prefixed with _ - as I understand, that is an old convention that is being phased out.

tresf · 2015-09-04T04:01:32Z

[...] you can use it without the --render flag [...]

Then perhaps we change it. What makes this different from --loop or any other modifier?

rcorre · 2015-09-04T11:39:35Z

In the GUI, loop and other modifiers are options within the export menu, while 'export tracks' is a totally different menu.

However that doesn't have to define the CLI interface, I'd be fine with

lmms --render --tracks proj.mmpz -f ogg -o output_dir

Thoughts?

Wallacoloo · 2015-09-04T16:17:17Z

@rcorre I believe your suggestion would require a more intelligent argument parser than what we have currently. The current system expects --render <filename> and would thus attempt to open the file --tracks if given a command like above.

If it could behave like --render [options] <filename>, that would be an improvement, since then options like --tracks or --loop are valid only when used within the --render command. It may be more clear to require the commands to be passed like --render --tracks,--loop,proj.mmpz, for example, such that everything after --render is still contained in one single argv[i] argument.

If anyone wants to do the work to support either the format you used above or the one where all render arguments are given as one argv[i], go ahead. Otherwise, I'll leave the discussion to you and @tresf - I personally have no problem with the implementation you originally proposed in this PR, w/ the hyphen removed (--rendertracks <filename>).

tresf · 2015-09-06T05:44:57Z

No objections here either. 👍

Wallacoloo · 2015-09-09T02:19:41Z

I checked out your branch, did a rebase master and then ran ./lmms --rendertracks ~/projects/example.mmpz -o ./example-tracks after creating an example-tracks directory. It left the directory empty, and segfaulted.

The segfault was misleading - it looks like it's due to our rather poor separation between program core and gui - AudioFileDevice is attempting to show an error message about not being able to open the output file, but it tries to do so through the gui and crashes because the gui doesn't actually exist (one of the other open PRs sort-of resolves this).

It turned out the actual issue was just that the -o argument relies on the path having a trailing "/" when used with --rendertracks. How do you feel about making the trailing slash optional?

Also, lmms --render <project>.mmpz defaults to placing the output at <project>.wav when no -o flag is specified. Do you think something similar could be done for the --rendertracks option for consistency, rather than have it error out when no -o is given? Maybe place the tracks in <project> or <project>.tracks?

Wallacoloo · 2015-09-09T02:19:56Z

Oops, forgot to tag you @rcorre.

rcorre · 2015-09-09T02:43:35Z

@Wallacoloo, the trailing slash should definitely be optional, glad you caught that.
I'm not as sold on the default output directory. It worries me a bit that you could overwrite files without explicitly specifying a path (though, admittedly the chance of having a random directory named .tracks that contains anything other than previously exported tracks is low).

On a related note, should -o create the output directory if it doesn't exist? It would be convenient and I can't think of a significant risk.

Wallacoloo · 2015-09-09T03:42:52Z

@rcorre fair points. Maybe just present a syntax error if no -o option is given then?

As far as implicitly creating the directory goes, I think it would be a good idea to create the deepest node if its parent directory exists. I'm not sure about creating the entire tree (i.e. mkdir -p style) though, as that might be undesirable if the user makes a typo in a long path. But it could certainly be argued either way - I'd be happy with either implementation.

softrabbit · 2015-09-09T07:09:20Z

If it could behave like --render [options] , that would be an improvement, since then options like --tracks or --loop are valid only when used within the --render command.

I think any order of options should be OK, as long as there is one non-option argument that refers to a project file. I.e. drop the parameter from the --render option and complain if there's no file to load. --render --output foo.wav project.mmpz --interpolation whatever. Or you could at the end the render command put.

It may be more clear to require the commands to be passed like --render --tracks,--loop,proj.mmpz, for example, such that everything after --render is still contained in one single argv[i] argument.

Hell no. Maybe after dropping the -- part, i.e. --render tracks,loop,proj.mmpz``` (and that's pretty ugly to me, too).

Maybe just present a syntax error if no -o option is given then? As far as implicitly creating the directory goes, I think it would be a good idea to create the deepest node if its parent directory exists.

👍

For the exact wording of options my preference is --render --tracks ahead of any --render-tracks variation. Preferably with the input file allowed at any position.

rcorre · 2015-09-10T01:33:00Z

@Wallacoloo: I haven't been able to repro the trailing slash issue. I'm using QDir.filePath, which I think should take care of adding a / if needed: https://github.com/rcorre/lmms/blob/master/src/core/RenderManager.cpp#L197

maybe something happened during the rebase? I'll try to get this branch back up to date

Wallacoloo · 2015-09-11T04:43:44Z

@rcorre Interesting - the problem seems to occur only when the output directory contains a literal ".". For example:

mkdir ./out.tracks
./lmms --rendertracks ~/lmms/projects/example.mmpz -o ./out.tracks

fails with a segfault.
However,

mkdir ./out
./lmms --rendertracks ~/lmms/projects/example.mmpz -o ./out

succeeds.

The error could very well be in a part of the code not modified by you. My guess would be that the -o option tries to determine the render format by extracting (and removing) the argument's extension, but I haven't looked at the code so that's almost baseless.

rcorre · 2015-09-11T12:24:47Z

# bad
./lmms --rendertracks ~/lmms/projects/example.mmpz -o ./out.tracks
# good
./lmms --rendertracks ~/lmms/projects/example.mmpz -o ./out.tracks/

The plot thickens...

I guess this is the first lmms option to work with a directory argument from the CLI, so I'll have to make sure all the cases are handled -- all the right stuff is probably in QDir/QFile -- I just need to find the right way of applying it.

EDIT:
oh, I see. If you leave off the trailing '/' and have a '.', it isn't recognized as a directory.

Currently ProjectRenderer has a helper getFileFormatFromExtension, this adds a similar helper getFileExtensionFromFormat. This will, for example, return "ogg" for OggFile.

Much of the multi-track rendering logic was intermixed with GUI code in ExportProjectDialog. This creates the RenderManager class to provide rendering logic that could be shared between the CLI and GUI interfaces.

Remove the rendering logic from the gui code in ExportProjectDialog and let RenderManger handle it instead. This is part of an effort to allow the CLI and the GUI to share the same rendering logic, setting the state for a --render-tracks CLI option similiar to the "Export Tracks" GUI option.

This command allows rendering each track of a song to a different file. It should provide the same functionality as the "Export Tracks" GUI option. Usage could look like: lmms --render-tracks project.mmpz -f ogg -o output/

Remove the _ prefix from the parameters to the newly added ProjectRenderer::getFileExtensionFromFormat. This naming convention is being phased out.

Follow convention of avoiding '-' in command names.

Do not call baseName on the path passed to -o when using the --rendertracks option. This was mangling directories that contained a literal '.' if a '/' was not explicitly specified at the end. Still call baseName for --render as the argument to -o is a file and we need to set the extension (ogg/wav).

rcorre · 2015-09-13T17:01:46Z

Turns out we were calling baseName on the arg to -o as it was previously always a file.
If you passed a directory with no trailing '/' and a '.' it was misinterpreted as a file.

I've fixed this and rebased to master.

As a side note, the current behavior of --render always strips the extension:

lmms --render file.mmpz -f ogg -o file.myext

will produce a file named file.ogg. I'm just preserving this behavior for now, but maybe it is a separate issue.

Wallacoloo · 2015-09-13T22:20:43Z

src/core/RenderManager.cpp

+	// find all currently unnmuted tracks -- we want to render these.
+	for( auto it = tl.begin(); it != tl.end(); ++it )
+	{
+		Track* tk = (*it);


In the future, feel free to use range-based iterators, i.e. for (Track * tk : tl) { ... }. Saves a bit of redundancy.

Oh, cool. I didn't even know those existed. I'm not normally a c++ programmer :)

Wallacoloo · 2015-09-13T22:35:55Z

the current behavior of --render always strips the extension [...] I'm just preserving this behavior for now, but maybe it is a separate issue.

Good call - that'd be best handled as a separate issue.

I tested this pretty thoroughly & everything is working as expected on my machine. The code looks fine, so we're good to merge :)

Nice work!

Add CLI equivalent to GUI's "export tracks"

tresf · 2015-09-14T03:56:01Z

FYI - Next time, we should squash these into a single commit before merging to simplify our commit history.

rcorre · 2015-09-14T11:33:33Z

duly noted

Wallacoloo reviewed Sep 4, 2015
View reviewed changes

rcorre added 8 commits September 13, 2015 07:28

Add ProjectRenderer::getFileExtensionFromFormat.

e863df7

Currently ProjectRenderer has a helper getFileFormatFromExtension, this adds a similar helper getFileExtensionFromFormat. This will, for example, return "ogg" for OggFile.

Initial implementation of RenderManager.

1a22635

Much of the multi-track rendering logic was intermixed with GUI code in ExportProjectDialog. This creates the RenderManager class to provide rendering logic that could be shared between the CLI and GUI interfaces.

Add --render-tracks CLI command.

795d429

This command allows rendering each track of a song to a different file. It should provide the same functionality as the "Export Tracks" GUI option. Usage could look like: lmms --render-tracks project.mmpz -f ogg -o output/

Document --render-tracks in --help and the manpage.

6b84062

Don't use old naming convention in new methods.

ea838c4

Remove the _ prefix from the parameters to the newly added ProjectRenderer::getFileExtensionFromFormat. This naming convention is being phased out.

Rename --render-tracks to --rendertracks.

d8ce830

Follow convention of avoiding '-' in command names.

rcorre force-pushed the master branch from 01ee3ea to fdb2787 Compare September 13, 2015 16:54

Wallacoloo reviewed Sep 13, 2015
View reviewed changes

Wallacoloo added a commit that referenced this pull request Sep 13, 2015

Merge pull request #2310 from rcorre/master

2fb0bab

Add CLI equivalent to GUI's "export tracks"

Wallacoloo merged commit 2fb0bab into LMMS:master Sep 13, 2015

Wallacoloo mentioned this pull request Sep 15, 2015

PianoRoll: Draw note names on all white keys, highlight white keys #2285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CLI equivalent to GUI's "export tracks" #2310

Add CLI equivalent to GUI's "export tracks" #2310

rcorre commented Sep 4, 2015

tresf commented Sep 4, 2015

rcorre commented Sep 4, 2015

tresf commented Sep 4, 2015

Wallacoloo commented Sep 4, 2015

Wallacoloo Sep 4, 2015

tresf commented Sep 4, 2015

rcorre commented Sep 4, 2015

Wallacoloo commented Sep 4, 2015

tresf commented Sep 6, 2015

Wallacoloo commented Sep 9, 2015

Wallacoloo commented Sep 9, 2015

rcorre commented Sep 9, 2015

Wallacoloo commented Sep 9, 2015

softrabbit commented Sep 9, 2015

rcorre commented Sep 10, 2015

Wallacoloo commented Sep 11, 2015

rcorre commented Sep 11, 2015

rcorre commented Sep 13, 2015

Wallacoloo Sep 13, 2015

rcorre Sep 13, 2015

Wallacoloo commented Sep 13, 2015

tresf commented Sep 14, 2015

rcorre commented Sep 14, 2015

		@@ -124,6 +124,15 @@ ProjectRenderer::ExportFileFormats ProjectRenderer::getFileFormatFromExtension(



		QString ProjectRenderer::getFileExtensionFromFormat(
		ExportFileFormats _fmt )

Add CLI equivalent to GUI's "export tracks" #2310

Add CLI equivalent to GUI's "export tracks" #2310

Conversation

rcorre commented Sep 4, 2015

tresf commented Sep 4, 2015

rcorre commented Sep 4, 2015

tresf commented Sep 4, 2015

Wallacoloo commented Sep 4, 2015

Wallacoloo Sep 4, 2015

Choose a reason for hiding this comment

tresf commented Sep 4, 2015

rcorre commented Sep 4, 2015

Wallacoloo commented Sep 4, 2015

tresf commented Sep 6, 2015

Wallacoloo commented Sep 9, 2015

Wallacoloo commented Sep 9, 2015

rcorre commented Sep 9, 2015

Wallacoloo commented Sep 9, 2015

softrabbit commented Sep 9, 2015

rcorre commented Sep 10, 2015

Wallacoloo commented Sep 11, 2015

rcorre commented Sep 11, 2015

rcorre commented Sep 13, 2015

Wallacoloo Sep 13, 2015

Choose a reason for hiding this comment

rcorre Sep 13, 2015

Choose a reason for hiding this comment

Wallacoloo commented Sep 13, 2015

tresf commented Sep 14, 2015

rcorre commented Sep 14, 2015