Menu
▾
▴
#2707 Patch: Midi volume fixes
Originally created by: *anonymous
Originally created by: k-ohara5...@oco.net
Originally owned by: k-ohara5...@oco.net
A tracker for the last two of four MIDI patches at
http://lists.gnu.org/archive/html/lilypond-devel/2012-07/msg00441.html
Originally posted by: k-ohara5...@oco.net
The last two of Heikki's group of four MIDI patches are posted at
<http://codereview.appspot.com/6428075>
Labels: -Type-Enhancement Type-Defect Patch-new
Originally posted by: pkx1...@gmail.com
Patchy the autobot says: passes tests. I do get +warning: (De)crescendo with unspecified starting volume in MIDI. in output if that matters
Labels: -Patch-new Patch-review
Originally posted by: gra...@percival-music.ca
I think we can be pickier with such things. Let's reject patches which add warnings.
Labels: -Patch-review Patch-needs_work
Originally posted by: dak@gnu.org
Is it possible to get a pointer on what causes the warning? After all, the warning _might_ newly be flagging a problem that was already present before.
Originally posted by: k-ohara5...@oco.net
Well, that warning text is new with Heikki's patch.
It is intended to indicate that a voice has a (de)crescendo without any preceding dynamic indication, so the midi output just guessed the initial volume to be mid-range,
*and* there is a \midi {] block so presumably the user might care to know that LilyPond set an arbitrary initial volume.
I don't which regression test caused that message, but if I find that it fits the intent of the warning, I'll put this back on patch-review.
Originally posted by: k-ohara5...@oco.net
Sorry I missed that warning. It flagged a point in Sängers Morgenleid where a temporary voice has a decrescendo without any preceding \p or \f, etc. Since LilyPond gives each Voice independent midi dynamics, it is ambiguous how loud that decrescendo should be at its start. The warning is helpful.
I made the warning also print the specific input with the (de)crescendo, and added expect-warning lines to the reg-tests that triggest the warning. The reg-tests are now clean. <http://codereview.appspot.com/6428075>
Originally posted by: pkx1...@gmail.com
Patchy the autobot says: passes tests.
Labels: -Patch-new Patch-review
Originally posted by: dak@gnu.org
If the semantics of our Midi support has changed in a manner that makes the Morgenlied regtest produce indeterminate output, I see three approaches:
a) implement fallback semantics that emulate previous behavior in this case
b) implement fallback semantics that fit better with LilyPond overall
c) adapt the regtest to the new semantics
Suppressing the warnings using "expect-warning" does not make sense in a real-world example: the expectation for a real-world example it that it should reflect correct usage.
If the warning mechanism is supposed to be tested, this requires a separate non-real-world example.
Originally posted by: k-ohara5...@oco.net
The semantics have not changed in any manner that produces indeterminate output
For at least two years (maybe forever) Lilypond has set dynamics independently for each Voice, and if the user does not start a Voice with an absolute dynamic (\p or \f, etc.) Lilypond arbitrarily starts the Voice at a mid-range dynamic level. With Heikki's patch, the 'mid-range' level takes equalization settings into account, and you get a warning if Lilypond makes this arbitrary setting of midi dynamics.
Originally posted by: dak@gnu.org
"At least two years (maybe forever)" would be
commit [re5380f29f23e204a603f8398368d2a7dc0260aa0]
Author: Jan Nieuwenhuizen <janneke@gnu.org>
Date: Thu Mar 3 16:40:06 2011 +0100
Map voices to channels in MIDI output.
This fixes a long standing weirdness in MIDI output, with voices being
all merged into one channel with channel number equalling track
number.
With voices mapped to channels, midi2ly can neatly recreate voices
without needing to do tricky guessing.
released as 2.13.53. input/regression/morgenlied.ly has for the duration of its easily traceable history since
commit [r6e9f6d75e2b1ffa9b69fe1ea0f8f034afa8a0598]
Author: Graham Percival <graham@percival-music.ca>
Date: Sun Jan 10 13:54:13 2010 +0000
Reinstate delete files and put them in regression.
not has had any change related to that change in semantics. It is good that Heikki's patch now delivers a warning that tells us we have overlooked fixing this file for over a year, by adding appropriate per-voice dynamics that would have been redundant previous to Jan's change.
If we show an example using Midi, it does not make sense to have the Midi blare out at \mf suddenly when it is supposed to decrescendo from \p.
Originally posted by: dak@gnu.org
Presumably the most straightforward fix is to put the Midi-channel mapping back to staff or so?
Originally posted by: k-ohara5...@oco.net
Except for a brief period in 2011, by default Lilypond puts all the Voices on a Staff onto the same MIDI channel. LilyPond for a longer time has used a separate Dynamic_performer for each Voice, keeping track of audio volume separately for each Voice. This means the Voices used to fight over channel volume, so the new implementation of dynamics in the note-on velocity is better.
Yes, a decrescendo in a temporary voice does disturb the existing dynamics on the same Staff, even in version 2.12
I did put midiChannelMapping back to Staff, as default, before version 2.14.
'morgenlied.ly' would have needed several tweaks, at least moving Dynamic_performer to the PianoStaff, to get proper dynamics in the midi output, even with version 2.12.
Originally posted by: dak@gnu.org
Ok, what you are saying is that getting correct Midi dynamics for a straightforward little piece like this one is so hard that there is no point in even trying.
I consider it likely that we can agree that this situation is not satisfactory. Since you are (at the very least between the two of us and likely more) more acquainted with the Midi code/mappings and its principles, could you try coming up with a plan for the kind of input/output relation that would be desired in the context of temporary and/or multiple voices?
This will, of course, constitute a separate issue, and I don't think that we currently have one spelling out the requirements to deal with this kind of situation in a manner that will be both convenient and understandable to most users.
Originally posted by: dak@gnu.org
As a stopgap measure, how about
hide =
#(define-music-function (parser location m) (ly:music?)
#{ \tweak #'stencil ##f #m #})
And using
-\hide\p
in the respective implicit voices?
Sure, ugly, but easy to understand and use. Assuming it works: I have not actually tried it. We'd put the definition of hide in the normal music functions rather than in this example.
Originally posted by: dak@gnu.org
Actually, somewhat nicer
hide =
#(define-music-function (parser location grob m) ((string? *unspecified*) ly:music?)
#{ \tweak $grob #'stencil ##f #m #})
since one can then also optionally specify the grob to disappear.
I'll make a separate proposal/issue.
Originally posted by: dak@gnu.org
Now the obvious fix, and one which the source code has apparently already been preppred for, is
diff --git a/input/regression/morgenlied.ly b/input/regression/morgenlied.ly
index 182cc64..49fd4b1 100644
--- a/input/regression/morgenlied.ly
+++ b/input/regression/morgenlied.ly
@@ -112,7 +112,7 @@ pianoRH = \relative c''' \repeat volta 2 {
<g e>8( <es fis a> <d f b> <c e c'>) [r8] r |
[r8] c'( e,) f r a |
\once \override DynamicLineSpanner #'padding =#3
- [r8] << { s8 s8 } << { fis(\> g)\!
+ [r8] << { s8\> s8\! } << { fis( g)
} \\ { c,4 } >> >> [r8] <e c g> <e c g> |
<d c a>4. [r8] \clef bass <d b f> <d b f> |
e,16_" "_\markup { \bold\italic cresc. } g c g e g d gis b gis d gis |
That should get rid of the warning. Of course, with the sparse distribution of dynamic commands it is unlikely anyway that the Midi reflects the intent of the composer throughout the voices and staffs.
Originally posted by: k-ohara5...@oco.net
The obvious fix with << { s8\> s8\! } << {...} \\ {...} >> >>
avoids Heikki's warning (about a decrescendo starting a brand new voice) but the problem remains, or is maybe a bit worse. << \\ >> voices still receive the default dynamic level.
Getting proof-readable MIDI dynamics output from morgenlied.ly is not terribly difficult, but it involves problems beyond those solved in Heikki's patches.
I mentioned above that I move Dynamic_performer to the PianoStaff context, so the dynamics affect both staves, but this no longer works in general. Jan's new code does not support the "hack" of moving performers to different contexts. The Dynamic_performer keeps track of the current dynamic level in its assigned domain, but now the Staff_performer matches notes to dynamics based on the id-string of the Voice from which they came (using dynamic_map_ in the code). If I name any voices explicitly { \new Voice = "my-id-string" } or implicitly << \\ >> the notes in these voices are not connected to the dynamics collected at PianoStaff level, so they come out mezzo-forte-ish.
In practice I give up and just \remove Dynamic_performer. Heikki tries harder, and shares with the rest of us.
Originally posted by: k-ohara5...@oco.net
Modified morgenlied.ly for MIDI output.
Originally posted by: dak@gnu.org
Jan's Midi changes were supposed to get better dynamics in more complex cases. If they make it impossible to get reasonable dynamics in simple cases, they are doing more harm than good. Moving performers to different levels is a rather typical way of creating per-Staff/per-Voice semantics, and short of prohibiting <<\\>> altogether, that would seem rather appropriate for use of implicit parallel voicing.
Perhaps Jan has some other suggestion how to make this piece actually work under the new system?
Cc: jan.nieuwenhuizen
Originally posted by: dak@gnu.org
Put on countdown to Thursday 2012-08-09 20:00 GMT+2. I would strongly suggest changing morgenlied according to comment #17: that confines the dynamics problem strictly to the short split-voice passage, where we have the best chance that future improvements in split-voice handling will iron it out. Suppressing the warning or putting in extensive workarounds for the currently broken Midi, in contrast, appear like more of a distraction. This particular change should not change the countdown status.
Originally posted by: dak@gnu.org
(No comment was entered for this change.)
Labels: -Patch-review Patch-countdown
Originally posted by: dak@gnu.org
Countdown completed. Please consider heeding the suggestion in comment #21 before pushing.
Labels: -Patch-countdown Patch-push
Originally posted by: k-ohara5...@oco.net
Pushed in commits
[r3b8c237179a4dc6b3ff43d3c200e9ea972e6aee1]
[r76444b472a32fd24d9cca1e25efcb1a86d0f7f76]
We have the option to verify with the examples Heikki provided in his email, attached here. It intentionally gives warnings, but produces midi that sounds reasonably like the printed score.
Labels: -Patch-push fixed_2_15_43
Status: Fixed
Originally posted by: Elu...@gmail.com
(No comment was entered for this change.)
Status: Verified