Set the sequence name in MIDI using title information from \header block

Set the sequence name in MIDI using title information from \header block

[ht, 2015-08-04 22:35:22]

This patch adds support for setting the MIDI sequence name for MIDI output files
using the value of the "midititle" field from a relevant \header block, and
falling back to using the "title" field if "midititle"
has not been defined.  (This should be analogous to how "pdftitle" and "title"
work for customizing PDF metadata.)

The purpose of the change is to improve the previous behavior where the name of
every MIDI sequence created by LilyPond used to be shown by MIDI synthesizers as
"control track" (previously, this string was hard-coded as the name of every
initial track of MIDI files created by LilyPond by Control_track_performer).

The patch
  * extends every Performance instance with a reference to a \header block
associated with the performance, adds Scheme library routines for getting and
setting the associated \header (modeled after corresponding routines available
for the Score class), and updates the Book::process_score function to initialize
the header information of Performance objects attached to a score;
  * adds a "name" parameter to the Performance output routines, used for
updating the track name in the performance's first Audio_staff (assumed to
represent the control track) before outputting MIDI; and
  * changes the write-performances-midis function (in scm/midi.scm) to query the
MIDI sequence name for a performance from the performance's \header block
(adapted from the handle-metadata function in scm/framework-ps.scm).


For testing, I used the following LilyPond code:

----

\version "2.19.25"

\header {
  title = "Top-level title"
}

% Book A without a title defined in a \header block
\book {
  \bookOutputName "A"

  % A: score without a title in Book A -- will get the top-level title
  \score {
    \new Staff { c1 }
    \midi { }
  }

  % A-1: score with a title defined in a nested \header block (in Book A) --
  %      will get the title in this \header block
  \score {
    \new Staff { c1 }
    \header { title = "Score in Book A" }
    \midi { }
  }

  % A-2: score with a title and a separate MIDI title defined in a nested
  % \header block (in Book A) -- will get the MIDI title in this \header block
  \score {
    \new Staff { c1 }
    \header {
      title = "Another score in Book A"
      midititle = "MIDI title"
    }
    \midi { }
  }
}

% Book B with a title defined in a \header block
\book {
  \bookOutputName "B"
  \header { title = "Book B" }

  % B: score without a title in Book B -- will get the title of Book B
  \score {
    \new Staff { c1 }
    \midi { }
  }

  % B-1: score with a \header block (and title) of its own (in Book B) -- will
  %      get the title in this \header block
  \score {
    \new Staff { c1 }
    \header { title = "Score in Book B" }
    \midi { }
  }
}

% Book C: book with a title defined in a \header block with book parts
\book {
  \bookOutputName "C"
  \header { title = "Book C" }

  % Book part in Book C with a title of its own
  \bookpart {
    \header { title = "Part 1 in Book C" }

    % C: score without a title (in Part 1 of Book C) -- will get the title of
    %    its enclosing book part
    \score {
      \new Staff { c1 }
      \midi { }
    }

    % C-1: score with a \header (and a title) of its own (in Part 1 of Book C)
    %      -- will get this title
    \score {
      \new Staff { c1 }
      \header { title = "Score in Part 1 of Book C" }
      \midi { }
    }
  }

  % C-2: score without a title outside any book part of Book C -- will get the
  % title of Book C
  \score {
    \new Staff { c1 }
    \midi { }
  }

  % Book part in Book C with no \header block
  \bookpart {

    % C-3: score without a \header block (in Part 2 of Book C) -- will get the
    % title of Book C
    \score {
      \new Staff { c1 }
      \midi { }
    }

    % C-4: score with an explicit title (in Part 2 of Book C) -- will get this
    % title
    \score {
      \new Staff { c1 }
      \header { title = "Score in Part 2 of Book C" }
      \midi { }
    }
  }
}

----

[lemzwerg, 2015-08-05 03:58:25]

I don't have enough knowledge for a LGTM, but reading the source code I noticed
one detail...

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc
File lily/performance.cc (right):

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode92
lily/performance.cc:92: text->text_string_ = name;
This code allows overwriting `control track' only once.  I guess this is
intended, however, it looks limiting.  Wouldn't it be better to identify the
control track by another property, say Audio_text::CONTROL_TRACK_NAME, instead
of a comparison with a string that the user might modify?

[ht, 2015-08-05 06:55:18]

On 2015/08/05 03:58:25, lemzwerg wrote:
> I don't have enough knowledge for a LGTM, but reading the source code I
noticed
> one detail...
> 
> https://codereview.appspot.com/256230045/diff/1/lily/performance.cc
> File lily/performance.cc (right):
> 
> https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode92
> lily/performance.cc:92: text->text_string_ = name;
> This code allows overwriting `control track' only once.  I guess this is
> intended, however, it looks limiting.

Yes, this is actually intended: the first Audio_staff of a Performance is (very
strongly...) assumed here to be the track created by Control_track_performer,
and there should (assuming that the Control_track_performer is not added to or
removed from any context) be exactly one such track in the performance.

The Control_track_performer initially (in Control_track_performer::initialize)
fills this staff with three Audio_text elements (the first element of which –
and only that element – is the "track name" that is supposed to get overwritten
here).  During processing, only Audio_tempo or Audio_time_signature (but no
further Audio_text) elements may get added to this staff by the
Control_track_performer, so there shouldn't ever be a need to have to overwrite
the track name twice.

I don't think that it's possible to access or modify the track name in the
control track staff from within LilyPond input files since the Audio_text
element with the hard-coded name is added to the staff upon initialization.

> Wouldn't it be better to identify the
> control track by another property, say Audio_text::CONTROL_TRACK_NAME, instead
> of a comparison with a string that the user might modify?

I actually thought of this and agree that it (or any other mechanism which would
allow to identify the track name element of the control track staff without any
ambiguity) would probably be more robust solution, apart from these points:
 * Adding a special placeholder value to the Audio_text::Type enumeration will
make the enumeration a mix of "real" element types and "pseudo" values which
cannot really appear in a MIDI file.  This feels somewhat ugly, considering that
the values currently in the enumeration represent "proper" element types.
 * Without making absolutely sure that the Audio_text::Type enumeration values
are not ever accessed by their integral value anywhere in the code (this is bad
coding style, but how can one be sure that it's not been used?), it's safe to
add the new value only at the beginning or end of the enumeration (so, for
example, it may not be safe to just add the new value in a "logically" more
correct place, such as after the TRACK_NAME value).

I can of course change the patch to extend the enumeration if it's agreed that
it's a cleaner solution.  Another (similar) possibility would be to use a new
audio element type for the track name placeholder.

I think the absolute "best" solution would be to fill the track name from the
\header information upon creation of the control track staff instead of patching
the information to the staff afterwards before it's outputted.  With my very
limited understanding about the overall LilyPond architecture, however, I have
no idea on how to even begin to try to access the relevant \header properties on
the control track initialization since it involves passing data between the C++
and Scheme layers...  I managed to get the current solution only because I was
pointed to the PDF metadata handling code, which I could then adapt to add
similar "metadata" to MIDI files.

[pkx, 2015-08-05 07:50:08]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello Heikki

On 04/08/15 23:35, ht.lilypond.development@gmail.com wrote:
> Reviewers: ,
> 
> Message: This patch adds support for setting the MIDI sequence
> name for MIDI output files using the value of the "midititle" field
> from a relevant \header block, and falling back to using the
> "title" field if "midititle" has not been defined.  (This should
> be analogous to how "pdftitle" and "title" work for customizing
> PDF metadata.)

Does this patch have a google tracker? I couldn't find one. If not can
you create one (with the summary and the link to this Rietveld) so
that I can keep track of its progress while it is tested and reviewed?

Thanks

James

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJVwcAiAAoJEP8yVoKoS9i++rMP/3AyA5tI7OqmIT11kWPM+uOw
XCKAZ0bKy+1Inh+L1Eo9CuxcRP4hCnWcvjEEFFZ1AO4KnTwtPU4gpj496HJqEiNM
c3SnfKCyk73AIOf9JifVe1YNXB68GgD2Vl1bdKQvHu1byWn9Q69oDnH6q29ov9aN
WuH917ztTBUweW7M/sKdpXfE9vMOCLl1EPIsfU19ZWhSKqSwT33luynkQ9dwYVk0
Y40XwpsQAF4fOp6VyYAMfI1Ruts8PiIivA0JQtlHCeK0TqxKxJrkjpO8Zd3gqdqe
ztAG/QwpBjf0GPomLeZUcuoNrVXVq+eLLStuyq0cSj2lpBEZUZ80Ap3k3j958C4G
TopbB46WHsfYDtBJy0VezX/L587GEfThPBlR7ssYVdS5bBJnrlT/+PIv9gnderk8
CBIwugjqjlqUX01TRCH6Vf+wTeIREANTYRqrJ+xOEokw1AJersFtm2VcICQfReCn
LG7paq1Sv0ct5RlgunuWcuPBGgdmdY2tazcNTuO2OdnTfpjlumtUeaRNUXOCr7dk
l8Xll06vjC5GQAIpfSY3myGFgy189jUHgVDSTcWKMG7TTBH/kjulCiVVzjITKxF3
mo/M/qHO5aAG58tPLBsJ9vJsG2A4ud2+67GdwWttm00mLHm7xRQmNdhEvOB1/g1b
rO5Y70xweqHlwJcMRrqt
=Prhk
-----END PGP SIGNATURE-----

[ht, 2015-08-05 08:05:44]

On 2015/08/05 07:50:08, pkx_gnu.org wrote:
> 
> Does this patch have a google tracker? I couldn't find one. If not can
> you create one (with the summary and the link to this Rietveld) so
> that I can keep track of its progress while it is tested and reviewed?

I added a new issue manually:

https://code.google.com/p/lilypond/issues/detail?id=4539

(When initially running git-cl, I think that I mistyped my Google account
password, so the command just aborted after that - this is probably the reason
why no corresponding Google issue had been created.  Using git-cl so rarely, I
always get confused about in which state things get left if an error occurs
during the procedure... Sorry about this.)

[pkx166h, 2015-08-05 11:47:08]

On 2015/08/05 08:05:44, ht wrote:
> On 2015/08/05 07:50:08, http://pkx_gnu.org wrote:
> > 
> > Does this patch have a google tracker? I couldn't find one. If not can
> > you create one (with the summary and the link to this Rietveld) so
> > that I can keep track of its progress while it is tested and reviewed?
> 
> I added a new issue manually:
> 
> https://code.google.com/p/lilypond/issues/detail?id=4539
> 
> (When initially running git-cl, I think that I mistyped my Google account
> password, so the command just aborted after that - this is probably the reason
> why no corresponding Google issue had been created.  Using git-cl so rarely, I
> always get confused about in which state things get left if an error occurs
> during the procedure... Sorry about this.)

Actually there is a problem using git-cl to create issue trackers since they
changed their authentication method. So you can only use git-cl to upload to
Rietveld and all tracker issues have to be done manually I am afraid (creation
and updating).

Anyway, I have it now and am testing it.

[pkx166h, 2015-08-05 13:48:26]

Fails make

https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc
File lily/performance-scheme.cc (right):

https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc#ne...
lily/performance-scheme.cc:46: "Write @var{performance} to @var{filename} with
name @{name}.")
... with name @var{name}...

This breaks make.

[ht, 2015-08-05 15:15:22]

Fix doc build

[Dan Eble, 2015-08-06 02:30:37]

https://codereview.appspot.com/256230045/diff/1/lily/include/performance.hh
File lily/include/performance.hh (right):

https://codereview.appspot.com/256230045/diff/1/lily/include/performance.hh#n...
lily/include/performance.hh:44: void write_output (string filename, const string
&name) const;
It's not obvious what "name" is the name of.  A more descriptive name or a
comment would be helpful.

https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc
File lily/performance-scheme.cc (right):

https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc#ne...
lily/performance-scheme.cc:39: Performance *p = unsmob<Performance>
(performance);
LY_ASSERT_SMOB returns a pointer, doesn't it?  So this unsmob is redundant.

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc
File lily/performance.cc (right):

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode41
lily/performance.cc:41: header_ (SCM_EOL)
I don't work with Guile frequently enough to tell you whether or not you need to
do something to make sure that garbage collection works properly with this SCM
member.  David K. could probably tell you at a glance.

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode81
lily/performance.cc:81: if (i == 0 && !s->audio_items_. empty())
no space after the dot; space before ()

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode92
lily/performance.cc:92: text->text_string_ = name;
On 2015/08/05 03:58:25, lemzwerg wrote:
> This code allows overwriting `control track' only once.  I guess this is
> intended, however, it looks limiting.  Wouldn't it be better to identify the
> control track by another property, say Audio_text::CONTROL_TRACK_NAME, instead
> of a comparison with a string that the user might modify?

In addition to that, recognizing the control track with (i == 0) seems fragile. 
(Please forgive me for criticizing without offering an alternative.  I'm in a
hurry.)

[ht, 2015-08-06 11:05:56]

https://codereview.appspot.com/256230045/diff/1/lily/include/performance.hh
File lily/include/performance.hh (right):

https://codereview.appspot.com/256230045/diff/1/lily/include/performance.hh#n...
lily/include/performance.hh:44: void write_output (string filename, const string
&name) const;
On 2015/08/06 02:30:36, Dan Eble wrote:
> It's not obvious what "name" is the name of.  A more descriptive name or a
> comment would be helpful.

Changed name of argument to "performance_name".

https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc
File lily/performance-scheme.cc (right):

https://codereview.appspot.com/256230045/diff/1/lily/performance-scheme.cc#ne...
lily/performance-scheme.cc:39: Performance *p = unsmob<Performance>
(performance);
On 2015/08/06 02:30:37, Dan Eble wrote:
> LY_ASSERT_SMOB returns a pointer, doesn't it?  So this unsmob is redundant.

I just adapted this code from score-scheme.cc (which makes similar unsmob
calls), so maybe there's a need for a more general clean-up if this coding
pattern must be optimized.  Being not that experienced with how the smob classes
work internally on the C++ level, I'd rather be safe and follow the example than
take responsibility that a direct reinterpret_cast (which is what
"unchecked_unsmob" appears to do) will continue to work here.

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc
File lily/performance.cc (right):

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode41
lily/performance.cc:41: header_ (SCM_EOL)
On 2015/08/06 02:30:37, Dan Eble wrote:
> I don't work with Guile frequently enough to tell you whether or not you need
to
> do something to make sure that garbage collection works properly with this SCM
> member.  David K. could probably tell you at a glance.

Thanks for this observation, I added the Performance::mark_smob member function
that should handle the new SCM member variable.

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode81
lily/performance.cc:81: if (i == 0 && !s->audio_items_. empty())
On 2015/08/06 02:30:37, Dan Eble wrote:
> no space after the dot; space before ()

Done.

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode92
lily/performance.cc:92: text->text_string_ = name;
On 2015/08/06 02:30:37, Dan Eble wrote:
> On 2015/08/05 03:58:25, lemzwerg wrote:
> > This code allows overwriting `control track' only once.  I guess this is
> > intended, however, it looks limiting.  Wouldn't it be better to identify the
> > control track by another property, say Audio_text::CONTROL_TRACK_NAME,
instead
> > of a comparison with a string that the user might modify?
> 
> In addition to that, recognizing the control track with (i == 0) seems
fragile. 
> (Please forgive me for criticizing without offering an alternative.  I'm in a
> hurry.)

The original implementation rested on my interpretation of the guidelines
concerning the structure of a format 1 MIDI file (see my comment in
<http://lists.gnu.org/archive/html/lilypond-user/2015-08/msg00048.html> in the
thread that led to this patch), and I trusted the existing LilyPond
implementation to follow these guidelines.

I agree that while the assumptions regarding the staff structure probably rarely
break in practice (provided that no Control_track_performers are added to or
removed from any contexts in a LilyPond input file, which I think that no normal
user will do accidentally), the code just "looks" fragile...

I've now implemented the recognition of the control track staff here in a more
explicit way by defining a new Audio_staff subclass (Audio_control_track_staff),
and changing the Control_track_performer to create an instance of this subclass,
so the staff can then be any of the staves in a performance (although it really
should always be the first one). I also changed the tests to verify the audio
element structure of the staff into assertions since the control track is
expected to follow the structure that's defined in
Control_track_performer::initialize.

[ht, 2015-08-06 11:07:25]

Changes in response to comments by Dan Eble

[dak, 2015-08-06 11:22:27]

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc
File lily/performance.cc (right):

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode41
lily/performance.cc:41: header_ (SCM_EOL)
On 2015/08/06 11:05:56, ht wrote:
> On 2015/08/06 02:30:37, Dan Eble wrote:
> > I don't work with Guile frequently enough to tell you whether or not you
need
> to
> > do something to make sure that garbage collection works properly with this
SCM
> > member.  David K. could probably tell you at a glance.
> 
> Thanks for this observation, I added the Performance::mark_smob member
function
> that should handle the new SCM member variable.

That's wrong.  mark_smob is not a virtual member function of Music_output so
there is no reason Performance::mark_smob will ever be called.

Music_output provides the derived_mark virtual member function for the purpose
of adding your own marking to classes derived from Music_output.

[ht, 2015-08-06 11:57:01]

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc
File lily/performance.cc (right):

https://codereview.appspot.com/256230045/diff/1/lily/performance.cc#newcode41
lily/performance.cc:41: header_ (SCM_EOL)
On 2015/08/06 11:22:27, dak wrote:
> On 2015/08/06 11:05:56, ht wrote:
> > I added the Performance::mark_smob member function
> > that should handle the new SCM member variable.
> 
> That's wrong.  mark_smob is not a virtual member function of Music_output so
> there is no reason Performance::mark_smob will ever be called.

Ok, so I overlooked the significance of Music_output and used an incorrect smob
class (Score) as a model for extending Performance when I'd been better off with
using (for example) Paper_book instead.  Thanks for the correction, I hope to
have got it right this time.

[ht, 2015-08-06 11:59:09]

Use the appropriate Music_output interface in Performance for marking smobs for
GC

[pkx166h, 2015-08-07 10:49:56]

Oh one thing to add, I am seeing a lot of this in the reg test:

--snip--


input/regression/midi/staff-map-instrument.midi 	

@@ -1,4 +1,4 @@
-track 0  ev (0, (255, 3, 'control track'))
+track 0  ev (0, (255, 3, ''))
   ev (0, (255, 1, 'creator: '))
   ev (0, (255, 88, '\x04\x02\x12\x08'))
   ev (0, (255, 81, '\x0fB@'))

--snip--

I assume that is expected?

[ht, 2015-08-07 11:15:46]

On 2015/08/07 10:49:56, J_lowe wrote:
> Oh one thing to add, I am seeing a lot of this in the reg test:
> 
> --snip--
> 
> 
> input/regression/midi/staff-map-instrument.midi 	
> 
> @@ -1,4 +1,4 @@
> -track 0  ev (0, (255, 3, 'control track'))
> +track 0  ev (0, (255, 3, ''))
>    ev (0, (255, 1, 'creator: '))
>    ev (0, (255, 88, '\x04\x02\x12\x08'))
>    ev (0, (255, 81, '\x0fB@'))
> 
> --snip--
> 
> I assume that is expected?

Yes, such differences are exactly what's expected as a result of this change: in
place of the string "control track", there MIDI event should contain a string
from the title field of a \header block in the input file (or an empty string if
there's no \header block which could be associated with a \midi { } block in a
LilyPond input file).

I guess I need to still update the patch with the changes to the regression test
results – I'll try to do this today.

[ht, 2015-08-07 11:49:43]

On 2015/08/07 11:15:46, ht wrote:
> I guess I need to still update the patch with the changes to the regression
test
> results – I'll try to do this today.

Hmm... Reading through the "Regression tests" section of the Contributor's
guide, I'm starting to get the impression that the expected results of
regression tests are not actually part of the source tree, so they can't be
changed simply in a patchset.  Is this correct?

What I could do, however, is to add the input file (in my first comment to this
Rietveld issue) as a new regression test to the codebase.

[ht, 2015-08-08 20:29:16]

add regression tests; update changes.tely