Fixes issue #2129 ("can't export as loop with CLI")

[michaelgregorius]

Adds a new command line option to render a song as a loop ("-l", "--
loop").

Also cleaned up the code which parses the command line options by
pulling out methods that print the version and the help.

[Umcaruje]

Could you please update the man page and include this feature in it? I think this should be documented 🍷

[michaelgregorius]

Done. I have also added lmms.io and the github page as URLs and have updated the date of the man page.

[tresf]

Also, if you don't mind adding another change, I never put this feature into the man pages either: #1784

[Umcaruje]

These old sf links are invalid and should be removed.

I changed the sf links in #2124

[michaelgregorius]

@tresf I'd prefer if you document #1784 because you know better what that feature is about. Thanks!

[tresf]

Fair enough, although it can be copied directly from the help if you change your mind. :)

https://github.com/LMMS/lmms/blob/master/src/core/main.cpp#L198

[tresf]

I changed the sf links in #2124

NP, we'll just hit a merge conflict on one of the PR's then and we can fix it then.

[Umcaruje]

NP, we'll just hit a merge conflict on one of the PR's then and we can fix it then.

👍

[michaelgregorius]

I have changed my mind. :)

The allowroot option is now also documented in the man page.

[Wallacoloo]

What does it mean to export as a loop? Is this just exporting only the portion of the song enclosed by the song editor's loop points, but otherwise producing a "normal" wav/ogg file? If so, I would suggest the long option "--use-loop-points" instead. Or does it export the whole song, but write different options into the wav/ogg headers in order to signal to a media player that the file should be repeated indefinitely after being played fully? Sorry if these questions are clear from reading the code. I'm on mobile, but I'll read through the changes later.

[michaelgregorius]

@Wallacoloo Exporting as a loop means that the full song is rendered but only till the end of the song. The intention of this feature is to be able to loop the exported file seamlessly. This means that for example reverb tails that you normally want to have included in your rendering will not be rendered.

[Wallacoloo]

@michaelgregorius gotcha. In GUIs, I recall seeing this option presented as a drop-down menu labeled "loop mode" with options "wrap remainder" (which takes any nonzero output after the end of the song and wraps it around back to the start by adding it into the output at the beginning of the song), "truncate remainder" (I.e. your implementation) and "render remaining".

Given that those are 3 mutually-exclusive options, I, personally, think it makes more sense to implement the CLI option as "--loop-mode=X", even if we only implement the truncate and render modes for now.

What are your thoughts on that?

[michaelgregorius]

@Wallacoloo I assume that implementing the wrapping option that you have proposed would need quite some effort. I don't have that much time (and also my own sideprojects) and therefore would like to propose to merge the changes so far and open a separate issue for your proposal. 😃

I have also tried to provide a command line option for the "Export between loop markers" option but unfortunately doing so in a non-GUI context makes the application crash with the following stack:

0   MidiTime::operator int  MidiTime.h  114 0x4d6ba8    
1   TimeLineWidget::loopBegin   TimeLineWidget.h    91  0x545603    
2   Song::startExport   Song.cpp    650 0x5415da    
3   ProjectRenderer::run    ProjectRenderer.cpp 162 0x53510c    
4   ??          0x7ffff681192c
5   start_thread            0x7ffff7bc5354  
6   clone           0x7ffff434ebfd  

The problem is that Song::startExport accesses the TimeLineWidget of the PlayPos instance which is 0 because we are in a non-GUI context. Yet another example that LMMS really needs to have its layers separated (which is no small feat I know).

[tresf]

@Wallacoloo I assume that implementing the wrapping option that you have proposed would need quite some effort.

From what I read, his proposition was to make the command line option configurable now by taking a parameter, so that when we do add this wrap-around feature, we'll have less changes to the CLI.

[michaelgregorius]

[...] even if we only implement the truncate and render modes for now.

Sorry, I somehow missed that part. :)

So the only option for now would be something like --loop-mode=truncate? By the way, currently there is no other option that uses =. So to be consistent with the other options I assume it would rather be -l truncate and --loop-mode truncate?

[Wallacoloo]

Any reason to keep http://lmms.sf.net around even though it redirects to https://lmms.io, which is also listed?

[Wallacoloo]

@michaelgregorius the proposal would be for --loop-mode=truncate and --loop-mode=render (the implied default when no --loop-mode flag is given). You are right that no options currently use =, so yes, --loop-mode truncate, --loop-mode render would likely be best.

However, --loop-mode render [...] is disappointingly close to --loop-mode --render [...], which could create confusion. On the other hand --loop-mode render-remainder is a bit verbose. So, I don't know. Maybe there's a better phrase than "render remainder". Maybe just --loop-mode none or --loop-mode disabled would do.

It does seem like there isn't much use of this --loop-mode <x> switch until (and if) we add support for more looping modes, like wrap-around though. So it should be perfectly fine to just go with -l and --loop-mode for now, and in the future, we can expand the command to -l [mode] and --loop-mode [mode] if we ever need to, in which mode defaults to truncate, thus preserving full backwards-compatibility :-)

So yeah, I think you can pretty much disregard most of what I said then. The code looks good. Thanks for separating out the help/version printing into their own functions! I left an inline comment in the only place that I had a suggestion.

[tresf]

@Wallacoloo the sf links will be remove by @Umcaruje's PR per #2131 (comment), #2131 (comment)

[Wallacoloo]

@tresf oops. Sorry for not reading through this thread all the way :/

[michaelgregorius]

I have pushed another commit that changes the long option from --loop to --loop-mode and have also adjusted the documentation accordingly.

Concerning the options for the loop mode: do I understand correctly that --loop-mode render would just render the song without any loops? If that's the case I'd propose to drop this case altogether and to render the complete song if the there is no -l or --loop-mode option given. The loop option would then only describe cases where you really want to render loops.

[Wallacoloo]

@michael sounds good to me.

[8tab]

s/2014/2015/

[michaelgregorius]

The second year of the copyright is now always calculated dynamically so there will be no more need to adjust it manually in the future.

[tresf]

@michaelgregorius @Wallacoloo is there a reason we can't rebase, squash and commit this?

[Wallacoloo]

@tresf This looks fine - green light from me.

[tresf]

@Wallacoloo 👍

@michaelgregorius once this is squashed into a single commit and rebased against master, we'll merge this in. Thanks for the hard work. :)

[michaelgregorius]

@tresf Done! 😃

[tresf]

Travis is happy... merging. 👍