[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Issue 4048: Improve crescendo performance with unspecified dynamics

From: nine . fierce . ballads
Subject: Re: Issue 4048: Improve crescendo performance with unspecified dynamics (issue 302910043 by address@hidden)
Date: Mon, 11 Jul 2016 22:01:14 -0700

Heikki, thanks for taking the time to review this patch.

What are the main user-level justifications for increasing the
complexity of the
Dynamic_performer in this way (in terms of the amount of code)?

It addresses the original request in the ticket and other cases noted in

It also improves an example I saw elsewhere recently (I don't remember
where, but I think it was yours), something like mf < ... < ... < ... <
... f.  The volume no longer saturates early.

User acceptance of the changes could
suffer, especially if the new heuristics could change the MIDI output
for (and
in the worst case "break") existing scores.

The regression tests show no differences.

The acceptance of the new rules could possibly be helped by adding
also some
user-level documentation about the behavior of the new
Dynamic_performer.   (I'd
probably accept *any* documented Dynamic_performer implementation over
existing one.)

Well, the level of detail in the documentation should depend on the use
case for MIDI output.  I've been told very clearly that "It can serve as
a proofhearing aid or a practice aid.  It is not intended to serve as a
substitute for a player when recording.  For
that, LilyPond produces sheet music fit for running through a human"

For a proofhearing or practice aid, I don't see that any more needs to
be said than that Lilypond chooses the target dynamics of (de)crescendi
when they are not specified.  A user who cares for a specific dynamic
doesn't need any knowledge of the performer's heuristics; he needs only
to specify the dynamic.  A user who doesn't care, doesn't care.

Are there user-visible changes or improvements that should be
mentioned in the Changes document?

I don't think that these minor changes to MIDI output in cases where the
user didn't care enough to specify a dynamic are much to brag about.
File lily/ (right):
lily/ Real look_up_absolute_volume(SCM
On 2016/07/09 18:16:50, ht wrote:
missing space after open parenthesis

Thank you.  I've run on and
and amended my commits, though I first reverted some changes not related
to my work.
lily/ Real max_target_vol)
On 2016/07/09 18:16:50, ht wrote:
Assuming that equalizer settings do not change between dynamic spans
added to
the queue, this function appears to be called always with the same
values for
min_target_vol  and  max_target_vol.  Under this assumption, is it
necessary to
have these values passed every time as function parameters (removing
them could
simplify the interface)?

Assuming that context properties do not change in the course of the
performance does not seem very lilypondish.  It is true that I have not
tried to handle the possibility rigorously, but I thought it better to
write this interface this way.  It's small potatoes compared to the

is it correct that the function just
overwrites the values of the queue's member variables (that is, would
function actually need to keep track of the minimum and maximum volume
over all
UnfinishedSpans that are added to the queue)?

Correct, and I think handling a change in equalizer settings at
arbitrary moments during crescendi would greatly complicate this
performer.  I didn't think long about it before deciding it was not
worth my time.  Maybe it is worth someone's time, but I doubt it.  If it
matters to you, I suppose we could document it as a known issue.
lily/ depart_dir_ = next_grow_dir;
On 2016/07/09 18:16:50, ht wrote:
This looks like a redundant assignment due to the preceding condition.

Yes.  I appreciate your attention to detail.
lily/ // Return a volume which is reasonably
distant from the given volumes in the
On 2016/07/09 18:16:50, ht wrote:
I guess that the "given volumes" mean only  start_vol  and  end_vol,
min_vol  and  max_vol  as these just define the available volume
range.  If this
is correct, I think it would be useful to state this more explicitly

Yes.  I added max and min parameters after documenting the function.
I'll update the comment.

Also, aren't  min_vol  and  max_vol  again essentially fixed values
that equalization settings do not change)?

Yes, but I've made what I consider a reasonable effort to handle changes
to equalization settings: a solution that does not place them with the
highest possible precision, but is relatively simple.
lily/ // less than the farther one.
On 2016/07/09 18:16:50, ht wrote:
What is the purpose of computing two candidate values for  depart_vol
in this
function?  Does this improve the behavior in some corner cases (some
tests maybe), for example, make it less likely to hit the peak volume
depart_vol, or something else?  (Just curious to know.)

That was my intent.  Consider mf < ... > p.  The previous implementation
used 25% of the dynamic range for a (de)crescendo to an unspecified
target, and I've tried to preserve that, but is not possible to use a
25% change for both the crescendo and the decrescendo and meet the
constraints of this case.  The decrescendo is a greater change than the
crescendo.  Believing that 25% was already more than enough, I chose to
pad using 25% for the greater change and 7% for the lesser change.

Would it help to put this example in the comments?
lily/ // TODO: If this is impossible,
On 2016/07/09 18:16:50, ht wrote:
Is there still something left to be done here?

I've removed the comment.
lily/ Audio_span_dynamic::MINIMUM_VOLUME);
On 2016/07/09 18:16:50, ht wrote:
max  and  min  are usually used without namespace qualification

That's currently true in Lilypond, but someone has used max and min as
local variables in this function.
lily/ {
On 2016/07/09 18:16:50, ht wrote:
Could this case be combined with the code further below (possibly
the initialization of  volume) since the  if  statement directly
following this
block will be skipped anyway if the control flow passes through this

This block is executed the first time only, but the if
(!open_span_.dynamic_) below is executed whenever a (de)crescendo has
lily/ // Idea: look_up_absolute_volume
(ly_symbol2scm ("mf")).
On 2016/07/09 18:16:50, ht wrote:
Actually, the value 90.0/127.0 for Audio_span_dynamic::DEFAULT_VOLUME
is about
0.71, which lands between mf (0.68) and f (0.75) in the default

I know.  Did you mean to suggest that this idea is good or bad?
lily/ volume =
On 2016/07/09 18:16:50, ht wrote:
Should equalization be applied to this default volume here?

I believe it makes no difference because in the cases where this could
happen, a real volume should be interpolated later.  However, to
eliminate the question, and in case I am not correct, I will change it
and post a new patch.
lily/ "",
On 2016/07/09 18:16:50, ht wrote:
Now that the logic of Dynamic_performer is being redesigned, would
this be a
proper time to also add something about the specification of the
behavior to the Internals Manual, to avoid the current situation where
source code - which will increased in complexity after these changes -
is the
only resource for understanding the behavior of dynamics in MIDI?

If someone can point me to a good example of IM documentation, I will
consider combining that with a future patch for another issue.
File lily/ (right):
lily/ return new Midi_note (i);
On 2016/07/09 18:16:50, ht wrote:
Maybe there should be a comment (or possibly even assertion) here that

Audio_span_dynamic  events are never expected here, and their omission
as a case
in this if statement is intentional.

This conditional chain ends in "else assert (0);".
File lily/ (right):
lily/ }
On 2016/07/09 18:16:50, ht wrote:
When pushing the changes, maybe the removal of this flag and the
unnecessary classes could be done as a separate commit (since the
removal of the

It isn't the removal of this flag that makes the Audio_dynamic class
unnecessary.  The flag was unnecessary, period.  The Audio_dynamic class
is gone for an independent reason, which is that the entire performance
is now covered by a series of Audio_span_dynamics now.

If you'll excuse me, I've jumped through git's hoops enough in the past
few weeks that I'd rather not split out such a trivial change (just
removing the flag) into its own commit, but if someone felt strongly
enough about it to second your request, I'd do it.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]