Re: Issue 4048: Improve crescendo performance with unspecified dynamics (issue 302910043 by address@hidden)

[nine . fierce . ballads]

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
response.

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
the
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"
(http://lists.gnu.org/archive/html/lilypond-devel/2013-11/msg00194.html).

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.


https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc
File lily/dynamic-performer.cc (right):

https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode53
lily/dynamic-performer.cc:53: Real look_up_absolute_volume(SCM
dynamicString,
On 2016/07/09 18:16:50, ht wrote:
> missing space after open parenthesis

Thank you.  I've run fixcc.py on dynamic-performer.cc and audio-item.cc
and amended my commits, though I first reverted some changes not related
to my work.

https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode101
lily/dynamic-performer.cc:101: 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
rest.

> is it correct that the function just
> overwrites the values of the queue's member variables (that is, would
the
> 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.

https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode156
lily/dynamic-performer.cc:156: 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.

https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode217
lily/dynamic-performer.cc:217: // 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,
not
> 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
here.

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
(provided
> 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.

https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode234
lily/dynamic-performer.cc:234: // 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
regression
> tests maybe), for example, make it less likely to hit the peak volume
for
> 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?

https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode244
lily/dynamic-performer.cc:244: // TODO: If this is impossible,
programming_error.
On 2016/07/09 18:16:50, ht wrote:
> Is there still something left to be done here?

I've removed the comment.

https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode345
lily/dynamic-performer.cc:345: 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.

https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode381
lily/dynamic-performer.cc:381: {
On 2016/07/09 18:16:50, ht wrote:
> Could this case be combined with the code further below (possibly
simplifying
> the initialization of  volume) since the  if  statement directly
following this
> block will be skipped anyway if the control flow passes through this
block?

This block is executed the first time only, but the if
(!open_span_.dynamic_) below is executed whenever a (de)crescendo has
ended.

https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode382
lily/dynamic-performer.cc:382: // 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
> absolute-volume-alist.

I know.  Did you mean to suggest that this idea is good or bad?

https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode407
lily/dynamic-performer.cc:407: volume =
Audio_span_dynamic::DEFAULT_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.

https://codereview.appspot.com/302910043/diff/40001/lily/dynamic-performer.cc#newcode461
lily/dynamic-performer.cc:461: "",
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
intended
> behavior to the Internals Manual, to avoid the current situation where
the
> 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.

https://codereview.appspot.com/302910043/diff/40001/lily/midi-item.cc
File lily/midi-item.cc (right):

https://codereview.appspot.com/302910043/diff/40001/lily/midi-item.cc#newcode45
lily/midi-item.cc:45: 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);".

https://codereview.appspot.com/302910043/diff/40001/lily/staff-performer.cc
File lily/staff-performer.cc (right):

https://codereview.appspot.com/302910043/diff/40001/lily/staff-performer.cc#newcode352
lily/staff-performer.cc:352: }
On 2016/07/09 18:16:50, ht wrote:
> When pushing the changes, maybe the removal of this flag and the
resulting
> 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.

https://codereview.appspot.com/302910043/