Issue 4048: Improve crescendo performance with unspecified dynamics

lily/dynamic-performer.cc

 /*
   This file is part of LilyPond, the GNU music typesetter.
 
   Copyright (C) 2000--2015 Jan Nieuwenhuizen <janneke@gnu.org>
 
   LilyPond is free software: you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation, either version 3 of the License, or
   (at your option) any later version.
 
@@ skipping 32 matching lines @@
   void close_and_enqueue_span ();
   Real compute_departure_volume (Direction depart_dir,
                                  Real start_vol,
                                  Real end_vol,
                                  Real min_vol,
                                  Real max_vol);
   bool drive_state_machine (Direction next_grow_dir);
   // next_vol < 0 means select a target dynamic based on growth direction.
   // return actual next volume (computed if not provided)
   Real finish_queued_spans (Real next_vol = -1.0);
-  Real look_up_absolute_volume(SCM dynamicString,

[ht, 2016/07/09 18:16:50]

missing space after open parenthesis

[Dan Eble, 2016/07/12 05:01:15]

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.

-                               Real defaultValue);
+  Real look_up_absolute_volume (SCM dynamicString,
+                                Real defaultValue);
 
 private:
   // This performer queues a number of dynamic spans waiting for the following
   // pattern before computing their volume levels.
   //
   //  1. the first (de)crescendo, followed by ...
   //  2. zero or more spans that either change in the same direction as the
   //     first or do not change, followed by ...
   //  3. zero or more spans that either change in the opposite direction as the
   //     first or do not change
   //
   // The search may be cut short by an absolute dynamic or the end of the
   // context.
   enum State
-    {
-      STATE_INITIAL = 0, // waiting for a (de)crescendo
-      STATE_DEPART, // enqueued the first span, gathering same-direction spans
-      STATE_RETURN // gathering opposite-direction spans
-    };
+  {
+    STATE_INITIAL = 0, // waiting for a (de)crescendo
+    STATE_DEPART, // enqueued the first span, gathering same-direction spans
+    STATE_RETURN // gathering opposite-direction spans
+  };
 
   struct UnfinishedSpan
   {
     Audio_span_dynamic *dynamic_;
     Direction grow_dir_;
 
     UnfinishedSpan () : dynamic_ (0), grow_dir_ (CENTER) {}
   };
 
   struct DynamicQueue
   {
     vector<UnfinishedSpan> spans_;
     // total duration of (de)crescendi (i.e. excluding fixed-volume spans)
     Real change_duration_;
     Real min_target_vol_;
     Real max_target_vol_;
 
     DynamicQueue () : change_duration_ (0) {}
 
     void clear ()
     {
       spans_.clear ();
       change_duration_ = 0;
     }
 
-    void push_back (const UnfinishedSpan& span,
+    void push_back (const UnfinishedSpan &span,
                     Real min_target_vol,
                     Real max_target_vol)

[ht, 2016/07/09 18:16:50]

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

Also, if this function were to get called with different values for 
min_target_vol  and  max_target_vol,  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)?

[Dan Eble, 2016/07/12 05:01:15]

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

     {
       if (span.grow_dir_ != CENTER)
         change_duration_ += span.dynamic_->get_duration ();
       min_target_vol_ = min_target_vol;
       max_target_vol_ = max_target_vol;
       spans_.push_back (span);
     }
 
     void set_volume (Real start_vol, Real target_vol);
   };
 
 private:
   Stream_event *script_event_;
   Drul_array<Stream_event *> span_events_;
   Direction next_grow_dir_;
   Direction depart_dir_;
   UnfinishedSpan open_span_;
   DynamicQueue depart_queue_;
   DynamicQueue return_queue_;
   State state_;
 };
 
 Dynamic_performer::Dynamic_performer ()
   : script_event_ (0),
     next_grow_dir_ (CENTER),
     depart_dir_ (CENTER),
     state_ (STATE_INITIAL)
 {
   span_events_[LEFT]
-    = span_events_[RIGHT] = 0;
+  = span_events_[RIGHT] = 0;
 }
 
 bool
 Dynamic_performer::drive_state_machine (Direction next_grow_dir)
 {
   switch (state_)
     {
     case STATE_INITIAL:
       if (next_grow_dir != CENTER)
         {
           state_ = STATE_DEPART;
           depart_dir_ = next_grow_dir;
         }
       break;
 
     case STATE_DEPART:
       if (next_grow_dir == -depart_dir_)
         state_ = STATE_RETURN;
       break;
 
     case STATE_RETURN:
       if (next_grow_dir == depart_dir_)
         {
           state_ = STATE_DEPART;
-          depart_dir_ = next_grow_dir;

[ht, 2016/07/09 18:16:50]

This looks like a redundant assignment due to the preceding condition.

[Dan Eble, 2016/07/12 05:01:15]

Yes.  I appreciate your attention to detail.

           return true;
         }
       break;
     }
 
   return false;
 }
 
 void
 Dynamic_performer::close_and_enqueue_span ()
 {
   if (!open_span_.dynamic_)
-    programming_error("no open dynamic span");
+    programming_error ("no open dynamic span");
   else
     {
-      DynamicQueue& dq =
-        (state_ == STATE_RETURN) ? return_queue_ : depart_queue_;
+      DynamicQueue &dq
+      = (state_ == STATE_RETURN) ? return_queue_ : depart_queue_;
 
       // Changing equalizer settings in the course of the performance does not
       // seem very likely.  This is a fig leaf: Equalize these limit volumes
       // now as the required context properties are current.  Note that only
       // the limits at the end of the last span in the queue are kept.
 
       // Resist diminishing to silence.  (Idea: Look up "ppppp"
       // with dynamicAbsoluteVolumeFunction, however that would yield 0.25.)
       const Real min_target = equalize_volume (0.1);
-      const Real max_target =
-        equalize_volume (Audio_span_dynamic::MAXIMUM_VOLUME);
+      const Real max_target
+      = equalize_volume (Audio_span_dynamic::MAXIMUM_VOLUME);
 
       open_span_.dynamic_->set_end_moment (now_mom ());
       dq.push_back (open_span_, min_target, max_target);
     }
 
   open_span_ = UnfinishedSpan ();
 }
 
 // Set the starting and target volume for each span in the queue.  The gain
 // (loss) of any (de)crescendo is proportional to its share of the total time
@@ skipping 12 matching lines @@
       if (it->grow_dir_ != CENTER)
         {
           // grant this (de)crescendo its portion of the gain
           dur += it->dynamic_->get_duration ();
           vol = start_vol + gain * (dur / change_duration_);
         }
       it->dynamic_->set_volume (prev_vol, vol);
     }
 }
 
-// Return a volume which is reasonably distant from the given volumes in the

[ht, 2016/07/09 18:16:50]

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.

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

[Dan Eble, 2016/07/12 05:01:15]

Yes.  I added max and min parameters after documenting the function.  I'll
update the comment.
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.

-// given direction, for use as a peak volume in a passage with a crescendo
-// followed by a decrescendo (or vice versa).  If the given volumes are equal,
-// the returned volume is a also reasonable target volume for a single
-// (de)crescendo.
+// Return a volume which is reasonably distant from the given start and end
+// volumes in the given direction, for use as a peak volume in a passage with a
+// crescendo followed by a decrescendo (or vice versa).  If the given volumes
+// are equal, the returned volume is a also reasonable target volume for a
+// single (de)crescendo.
+//
+// The given minimum and maximum volumes are the allowable dynamic range.
 Real
 Dynamic_performer::compute_departure_volume (Direction depart_dir,
                                              Real start_vol,
                                              Real end_vol,
                                              Real min_vol,
                                              Real max_vol)
 {
   if (depart_dir == CENTER)
     return start_vol;
 
   // Try to find a volume that is a minimum distance from the starting and
   // ending volumes.  If the endpoint volumes differ, the nearer one is padded
   // less than the farther one.

[ht, 2016/07/09 18:16:50]

I believe I eventually understood what this function returns by following the
calculations, but the terms "nearer", "farther" (to/from what?), "one" (volume
or dynamic span endpoint in time?) and "padded" didn't help my intuitive
understanding about the behavior.

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

[Dan Eble, 2016/07/12 05:01:15]

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?

 
   // Idea: Use a context property or callback, e.g. the difference between two
   // dynamics in dynamicAbsoluteVolumeFunction.  0.25 is the default difference
   // between "p" and "ff". (Isn't that rather wide for this purpose?)

[ht, 2016/07/09 18:16:50]

I think using "0.25 * the available volume range" as the gain for volume changes
without an explicit dynamic level at the endpoint in the previous implementation
was an attempt to make the gain dependent on equalization settings.  If I
remember correctly, the value 0.25 was originally used as the fixed gain
regardless of any equalization settings.

   const Real far_padding = 0.25;
   const Real near_padding = 0.07;
 
   // If for some reason one of the endpoints is already below the supposed
   // minimum or maximum, just accept it.
-  // TODO: If this is impossible, programming_error.

[ht, 2016/07/09 18:16:50]

Is there still something left to be done here?

[Dan Eble, 2016/07/12 05:01:15]

I've removed the comment.

   min_vol = min (min (min_vol, start_vol), end_vol);
   max_vol = max (max (max_vol, start_vol), end_vol);
 
   const Real vol_range = max_vol - min_vol;
 
   const Real near = minmax (depart_dir, start_vol, end_vol)
-    + depart_dir * near_padding * vol_range;
+                    + depart_dir * near_padding * vol_range;
   const Real far = minmax (-depart_dir, start_vol, end_vol)
-    + depart_dir * far_padding * vol_range;
+                   + depart_dir * far_padding * vol_range;
   const Real depart_vol = minmax (depart_dir, near, far);
   return max (min (depart_vol, max_vol), min_vol);
 }
 
 Real
 Dynamic_performer::finish_queued_spans (Real next_vol)
 {
   if (depart_queue_.spans_.empty ())
     {
-      programming_error("no dynamic span to finish");

[ht, 2016/07/09 18:16:50]

missing whitespace before open parenthesis

+      programming_error ("no dynamic span to finish");
       return next_vol;
     }
 
   const Real start_vol = depart_queue_.spans_.front ().dynamic_->get_start_volume ();
 
   if (return_queue_.spans_.empty ())
     {
       Real depart_vol = next_vol;
 
       // If the next dynamic is not specified or is inconsistent with the
       // direction of growth, choose a reasonable target.
       if ((next_vol < 0) || (depart_dir_ != sign (next_vol - start_vol)))
         {
           depart_vol = compute_departure_volume (depart_dir_,
                                                  start_vol, start_vol,
                                                  depart_queue_.min_target_vol_,
                                                  depart_queue_.max_target_vol_);
         }
 
-      depart_queue_.set_volume(start_vol, depart_vol);

[ht, 2016/07/09 18:16:50]

missing whitespace before open parenthesis

+      depart_queue_.set_volume (start_vol, depart_vol);
       depart_queue_.clear ();
       return (next_vol >= 0) ? next_vol : depart_vol;
     }
   else
     {
       // If the next dynamic is not specified, return to the starting volume.
       const Real return_vol = (next_vol >= 0) ? next_vol : start_vol;
-      Real depart_vol = compute_departure_volume(depart_dir_,

[ht, 2016/07/09 18:16:50]

missing whitespace before open parenthesis

-                                                 start_vol, return_vol,
-                                                 depart_queue_.min_target_vol_,
-                                                 depart_queue_.max_target_vol_);
+      Real depart_vol = compute_departure_volume (depart_dir_,
+                                                  start_vol, return_vol,
+                                                  depart_queue_.min_target_vol_,
+                                                  depart_queue_.max_target_vol_);
       depart_queue_.set_volume (start_vol, depart_vol);
       depart_queue_.clear ();
       return_queue_.set_volume (depart_vol, return_vol);
       return_queue_.clear ();
       return return_vol;
     }
 }
 
 Real
 Dynamic_performer::equalize_volume (Real volume)
@@ skipping 30 matching lines @@
       if (ly_is_procedure (eq))
         s = scm_call_1 (eq, s);
 
       if (is_number_pair (s))
         {
           Interval iv = ly_scm2interval (s);
           volume = iv[MIN] + iv.length () * volume;
         }
     }
   return std::max (std::min (volume, Audio_span_dynamic::MAXIMUM_VOLUME),
                    Audio_span_dynamic::MINIMUM_VOLUME);

[ht, 2016/07/09 18:16:50]

max  and  min  are usually used without namespace qualification

[Dan Eble, 2016/07/12 05:01:15]

That's currently true in Lilypond, but someone has used max and min as local
variables in this function.

 }
 
 void
 Dynamic_performer::finalize ()
 {
   if (open_span_.dynamic_)
     close_and_enqueue_span ();
   finish_queued_spans ();
 }
 
 Real
-Dynamic_performer::look_up_absolute_volume(SCM dynamicString,
-                                           Real defaultValue)
+Dynamic_performer::look_up_absolute_volume (SCM dynamicString,
+                                            Real defaultValue)
 {
   SCM proc = get_property ("dynamicAbsoluteVolumeFunction");
 
   SCM svolume = SCM_EOL;
   if (ly_is_procedure (proc))
     svolume = scm_call_1 (proc, dynamicString);
 
   return robust_scm2double (svolume, defaultValue);
 }
 
 void
 Dynamic_performer::process_music ()
 {
   Real volume = -1;
 
   if (script_event_) // explicit dynamic
     {
       volume = look_up_absolute_volume (script_event_->get_property ("text"),
                                         Audio_span_dynamic::DEFAULT_VOLUME);
       volume = equalize_volume (volume);
     }
   else if (!open_span_.dynamic_) // first time only
     {

[ht, 2016/07/09 18:16:50]

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?

[Dan Eble, 2016/07/12 05:01:16]

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

       // Idea: look_up_absolute_volume (ly_symbol2scm ("mf")).

[ht, 2016/07/09 18:16:50]

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.

[Dan Eble, 2016/07/12 05:01:16]

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

       // It is likely to change regtests.
       volume = equalize_volume (Audio_span_dynamic::DEFAULT_VOLUME);
     }
 
   // end the current span at relevant points
   if (open_span_.dynamic_
       && (span_events_[START] || span_events_[STOP] || script_event_))
     {
       close_and_enqueue_span ();
       if (script_event_)
         {
           state_ = STATE_INITIAL;
           volume = finish_queued_spans (volume);
         }
     }
 
   // start a new span so that some dynamic is always in effect
   if (!open_span_.dynamic_)
     {
       if (drive_state_machine (next_grow_dir_))
         volume = finish_queued_spans (volume);
 
       // if not known by now, use a default volume for robustness
       if (volume < 0)
-        volume = Audio_span_dynamic::DEFAULT_VOLUME;

[ht, 2016/07/09 18:16:50]

Should equalization be applied to this default volume here?

[Dan Eble, 2016/07/12 05:01:16]

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.

-
-      Stream_event *cause =
-        span_events_[START] ? span_events_[START] :
-        script_event_ ? script_event_ :
-        span_events_[STOP];
+        volume = equalize_volume (Audio_span_dynamic::DEFAULT_VOLUME);
+
+      Stream_event *cause
+      = span_events_[START] ? span_events_[START]
+        : script_event_ ? script_event_
+        : span_events_[STOP];
 
       open_span_.dynamic_ = new Audio_span_dynamic (now_mom (), volume);
       open_span_.grow_dir_ = next_grow_dir_;
       announce_element (Audio_element_info (open_span_.dynamic_, cause));
     }
 }
 
 void
 Dynamic_performer::stop_translation_timestep ()
 {
   script_event_ = 0;
   span_events_[LEFT]
-    = span_events_[RIGHT] = 0;
+  = span_events_[RIGHT] = 0;
   next_grow_dir_ = CENTER;
 }
 
 void
 Dynamic_performer::listen_decrescendo (Stream_event *r)
 {
   Direction d = to_dir (r->get_property ("span-direction"));
   if (ASSIGN_EVENT_ONCE (span_events_[d], r) && (d == START))
     next_grow_dir_ = SMALLER;
 }
@@ skipping 15 matching lines @@
 void
 Dynamic_performer::boot ()
 {
   ADD_LISTENER (Dynamic_performer, decrescendo);
   ADD_LISTENER (Dynamic_performer, crescendo);
   ADD_LISTENER (Dynamic_performer, absolute_dynamic);
 }
 
 ADD_TRANSLATOR (Dynamic_performer,
                 /* doc */
                 "",

[ht, 2016/07/09 18:16:50]

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?

[Dan Eble, 2016/07/12 05:01:15]

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

 
                 /* create */
                 "",
 
                 /* read */
                 "dynamicAbsoluteVolumeFunction "
                 "instrumentEqualizer "
                 "midiMaximumVolume "
                 "midiMinimumVolume "
                 "midiInstrument ",
 
                 /* write */
                 ""
                );