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

[ht . lilypond . development]

In short: LGTM.  The rest of this comment is just continuing discussion
on some of the points in your previous response.

On 2016/07/12 05:01:16, Dan Eble wrote:
> > What are the main user-level justifications for increasing the
> > complexity of the Dynamic_performer

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

Sorry about this, that example was not really meant as something that I
suggested as a problem that I'd wish to have fixed (the example was only
there to show how a previous patch of yours to "add \!'s" to all dynamic
span boundaries already improved the behavior from what it used to be) -
and the new behavior is even better since it avoids saturating the
volume altogether.

> The regression tests show no differences.

I wasn't thinking about regression tests here, but rather about the
existing input files that a user might have that could start behaving
differently from what they used to (for example, that the volume level
at the end of a sequence of crescendos followed by a sequence of
descrendos, all with unspecified volumes, will now always return to the
starting level regardless of the number of crescendo and descrendo
spans).  I'm not arguing for or against any particular heuristic of
handling unspecified (de)crescendos, only that any changes in behavior
might be seen differently by different users, and whatever heuristics
there are should preferably be documented in a detailed enough level so
that the intended behavior becomes clear.

I'm of course speaking strictly from the mindset of a (likely a very
small group of) users who have (due to the lack of reference
documentation) gone through the trouble of trying to learn the rules
about how LilyPond handles MIDI dynamics by studying the source code.
While such users might be annoyed about the changes in the rules if
there's nothing mentioned about the changes anywhere, they have only
themselves to blame for relying on undocumented behavior in the
first place...  So I believe you can safely ignore my concerns about
missing user-level documentation about the changes - I don't think
there will be any users that are going to complain about the changes.

> Well, the level of detail in the documentation should depend on the
use
> case for MIDI output.

True - however, I'm not certain whether there's any clear consensus
among the user/developer base about what the use case for LilyPond's
MIDI output actually is, besides the obvious thing that in practice the
MIDI backend is considered to be of secondary importance to the majority
of people (at least to the core developers - since v2.14, which
introduced the "velocity as MIDI volume" interpretation of volume, I
think that the fixes and enhancements to the MIDI code have mostly come
from outside developers).

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

I think this is just the personal opinion of one of the core developers.
I can only symphathize with your sentiments in that discussion.

In my opinion, if the above really were the only accepted use case for
MIDI output, then I'm baffled about why the developers of the MIDI
backend ever bothered with handling MIDI dynamics in the first place,
and why, for example, the articulate.ly script (even though it was made
by an outside developer) got accepted into the official releases.
Funnily enough, had LilyPond not had any support for MIDI dynamics when
I started using it, I'd probably have never got involved with LilyPond
development at all myself, to try to work around or fix bugs where the
MIDI output backend "tried too much (performance-wise)" and failed.

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

I think that removing the internal programming errors in MIDI dynamic
handling is a good thing since these errors could have been a source of
user confusion.  Maybe this would be still worth mentioning?


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

OK, this was just a thought (possibly raised since the equalizer
settings remaining the same was mentioned in the code comments, so I
began to wonder how strongly this is actually assumed throughout the
implementation - in the case where the implementation were known to
stop behaving as intended if equalization settings change, making the
interfaces too generic would bring little benefit and only make the
code more difficult to understand).

> > is it correct that the function just
> > overwrites the values of the queue's member variables

> Correct, and I think handling a change in equalizer settings at
> arbitrary moments during crescendi would greatly complicate this
> performer.

Don't worry, I'm didn't mean to suggest that you need to still work
on this.  The question was just due to my own confusion about the names
of the queue member variables (whether they were supposed to represent
the maximum and minimum over all elements of the queue or not).


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.
> [...]
> Would it help to put this example in the comments?

Definitely, thanks for the explanation!  With decisions like this,
it would be good to have comments that describe the idea - since from
just the code it could be difficult to judge whether the calculations
need to be that way because of code correctness or for some other
reasons.


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.

Isn't the last "if" block executed also on the first time since the
condition in the last "if" block and the "first time" case is the same?
(That is, wouldn't "volume" be assigned the default volume in the last
"if" block even without the "first time" case?)


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?

Neither :-), it was just about the "Idea" comment being inaccurate
(since the default volume level is not "mf").  However, it will become
correct if the default is changed to "mf" (on which we seem to agree in
the Doc issue about MIDI dynamics).

> If someone can point me to a good example of IM documentation, I will
> consider combining that with a future patch for another issue.

There don't seem to be many engravers or performers with that much
IM documentation (the longest examples that I could find which appear
to try to tell something about how an engraver works are
lily/bar-number-engraver.cc, lily/keep-alive-together-engraver.cc and
lily/paper-column-engraver.cc).  Yes, I admit that I've never written
such documentation myself...


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.

Yes, we're not in disagreement here.

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

Just do what you think is best, I'm happy with whatever decision you
make (and certainly didn't mean to upset you, and I'm sorry if I did).


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