Make MainSoundConfiguration behave "correctly" with badly-formatted strings.

Add some documentation for a design "principle" when handling malformatted
data, and make MainSoundConfiguration adhere to that.

10 files changed, 114 insertions, 34 deletions

diff --git a/README.md b/README.md
index b8d53732..bcb781af 100644
--- a/README.md
+++ b/README.md
@@ -88,6 +88,38 @@ Run doxygen in the top-level directory and then see build/doc/html/index.html.
 There are some examples in the examples/ directory.
 
 
+# Approach to non-compliant DCPs
+
+In a number of places we read metadata from DCPs that may not be in the correct format.  A vague design principle is that we should be
+able to read such values, write them back out again the same, and find out that they are invalid.  However, it should be difficult to
+create new DCPs with badly-formatted metadata.
+
+For example, a `CPL` has a `MainSoundConfiguration` that is essentially a specially-formatted string: a comma-separated list of values,
+where the values come from a limited range.  A correct value might be "L,R,C,LFE,Ls,Rs,-,-" and an incorrect one "fish" or
+"01,02,03,04,05,06"
+
+The "happy" path is:
+
+```
+MainSoundConfiguration foo("L,R,C,LFE,Ls,Rs,-,-");
+std::cout << "Config has " << foo.channels() << ", first is mapped to " << dcp::channel_to_mca_name(foo.mapping(0).get(), foo.field()) << "\n";
+std::cout << "Value for XML: " << foo.to_string() << "\n";
+```
+
+In this case `foo.valid()` will be true and the details of the configuration can be accessed.
+
+In the "invalid" case we have:
+
+```
+MainSoundConfiguration foo("01,02,03,04,05,06");
+std::cout << "Value for XML: " << foo.to_string() << "\n";
+```
+
+Now `foo.valid()` will be `false` and calls to (for example) `foo.channels()` will throw `MainSoundConfigurationError`.  `foo.to_string()` is
+OK, however, and will return the same invalid string "01,02,03,04,05,06" as was passed to the constructor.
+
+
+
 # Coding style
 
 * Use C++11 but nothing higher, as we need the library to be usable on some quite old compilers.
diff --git a/src/cpl.cc b/src/cpl.cc
index a701f4ba..42319c07 100644
--- a/src/cpl.cc
+++ b/src/cpl.cc
@@ -52,6 +52,7 @@
 #include "reel_sound_asset.h"
 #include "reel_text_asset.h"
 #include "util.h"
+#include "verify.h"
 #include "version.h"
 #include "warnings.h"
 #include "xml.h"
@@ -175,7 +176,7 @@ CPL::CPL (boost::filesystem::path file, vector<dcp::VerificationNote>* notes)
 		auto asset_list = reels.front()->node_child("AssetList");
 		auto metadata = asset_list->optional_node_child("CompositionMetadataAsset");
 		if (metadata) {
-			read_composition_metadata_asset (metadata);
+			read_composition_metadata_asset(metadata, notes);
 			_read_composition_metadata = true;
 		}
 	}
@@ -266,7 +267,7 @@ CPL::write_xml(boost::filesystem::path file, shared_ptr<const CertificateChain>
 
 
 void
-CPL::read_composition_metadata_asset (cxml::ConstNodePtr node)
+CPL::read_composition_metadata_asset(cxml::ConstNodePtr node, vector<dcp::VerificationNote>* notes)
 {
 	_cpl_metadata_id = remove_urn_uuid(node->string_child("Id"));
 
@@ -310,13 +311,17 @@ CPL::read_composition_metadata_asset (cxml::ConstNodePtr node)
 	}
 
 	if (auto msc = node->optional_string_child("MainSoundConfiguration")) {
-		try {
-			_main_sound_configuration = MainSoundConfiguration(*msc);
-		} catch (MainSoundConfigurationError& e) {
+		_main_sound_configuration = MainSoundConfiguration(*msc);
+		if (!_main_sound_configuration->valid() && _standard == dcp::Standard::SMPTE && notes) {
 			/* With Interop DCPs this node may not make any sense, but that's OK */
-			if (_standard == dcp::Standard::SMPTE) {
-				throw e;
-			}
+			notes->push_back(
+				dcp::VerificationNote(
+					dcp::VerificationNote::Type::ERROR,
+					dcp::VerificationNote::Code::INVALID_MAIN_SOUND_CONFIGURATION,
+					fmt::format("{} could not be parsed", _main_sound_configuration->to_string()),
+					*_file
+					).set_cpl_id(_id)
+				);
 		}
 	}
 
diff --git a/src/cpl.h b/src/cpl.h
index 56c67345..949bcd1f 100644
--- a/src/cpl.h
+++ b/src/cpl.h
@@ -360,7 +360,7 @@ private:
 	friend struct ::verify_invalid_language3;
 
 	void maybe_write_composition_metadata_asset(xmlpp::Element* node, bool include_mca_subdescriptors) const;
-	void read_composition_metadata_asset (cxml::ConstNodePtr node);
+	void read_composition_metadata_asset(cxml::ConstNodePtr node, std::vector<dcp::VerificationNote>* notes);
 	void write_mca_subdescriptors(xmlpp::Element* parent, std::shared_ptr<const SoundAsset> asset) const;
 
 	std::string _issuer;
diff --git a/src/main_sound_configuration.cc b/src/main_sound_configuration.cc
index 1a081321..a48ff88d 100644
--- a/src/main_sound_configuration.cc
+++ b/src/main_sound_configuration.cc
@@ -47,11 +47,13 @@ using namespace dcp;
 
 
 MainSoundConfiguration::MainSoundConfiguration (string s)
+	: _configuration(s)
 {
 	vector<string> parts;
 	boost::split (parts, s, boost::is_any_of("/"));
 	if (parts.empty()) {
-		throw MainSoundConfigurationError(s);
+		_valid = false;
+		return;
 	}
 
 	if (parts[0] == "51") {
@@ -73,14 +75,19 @@ MainSoundConfiguration::MainSoundConfiguration (string s)
 	boost::split (channels, parts[1], boost::is_any_of(","));
 
 	if (channels.size() > 16) {
-		throw MainSoundConfigurationError (s);
+		_valid = false;
+		return;
 	}
 
 	for (auto i: channels) {
 		if (i == "-") {
 			_channels.push_back(optional<Channel>());
 		} else {
-			_channels.push_back(mca_id_to_channel(i));
+			try {
+				_channels.push_back(mca_id_to_channel(i));
+			} catch (UnknownChannelIdError&) {
+				_valid = false;
+			}
 		}
 	}
 }
@@ -90,19 +97,23 @@ MainSoundConfiguration::MainSoundConfiguration (MCASoundField field, int channel
 	: _field (field)
 {
 	_channels.resize (channels);
+	update_string();
 }
 
 
-string
-MainSoundConfiguration::to_string () const
+void
+MainSoundConfiguration::update_string()
 {
-	string c;
+	if (!_valid) {
+		return;
+	}
+
 	switch (_field) {
 	case MCASoundField::FIVE_POINT_ONE:
-		c = "51/";
+		_configuration = "51/";
 		break;
 	case MCASoundField::SEVEN_POINT_ONE:
-		c = "71/";
+		_configuration = "71/";
 		break;
 	default:
 		DCP_ASSERT(false);
@@ -110,23 +121,23 @@ MainSoundConfiguration::to_string () const
 
 	for (auto i: _channels) {
 		if (!i) {
-			c += "-,";
+			_configuration += "-,";
 		} else {
-			c += channel_to_mca_id(*i, _field) + ",";
+			_configuration += channel_to_mca_id(*i, _field) + ",";
 		}
 	}
 
-	if (c.length() > 0) {
-		c = c.substr(0, c.length() - 1);
+	if (!_configuration.empty() > 0) {
+		_configuration = _configuration.substr(0, _configuration.length() - 1);
 	}
-
-	return c;
 }
 
 
 optional<Channel>
 MainSoundConfiguration::mapping (int index) const
 {
+	throw_if_invalid();
+
 	DCP_ASSERT (static_cast<size_t>(index) < _channels.size());
 	return _channels[index];
 }
@@ -135,8 +146,12 @@ MainSoundConfiguration::mapping (int index) const
 void
 MainSoundConfiguration::set_mapping (int index, Channel c)
 {
+	throw_if_invalid();
+
 	DCP_ASSERT (static_cast<size_t>(index) < _channels.size());
 	_channels[index] = c;
+
+	update_string();
 }
 
 
diff --git a/src/main_sound_configuration.h b/src/main_sound_configuration.h
index d0ddd1f2..1f9c18e9 100644
--- a/src/main_sound_configuration.h
+++ b/src/main_sound_configuration.h
@@ -35,6 +35,8 @@
 #ifndef LIBDCP_MAIN_SOUND_CONFIGURATION_H
 #define LIBDCP_MAIN_SOUND_CONFIGURATION_H
 
+
+#include "exceptions.h"
 #include "types.h"
 #include <string>
 
@@ -61,25 +63,48 @@ extern ASDCP::UL channel_to_mca_universal_label (Channel c, MCASoundField field,
 class MainSoundConfiguration
 {
 public:
+	/** Set up a MainSoundConfiguration from a string.  If the string is valid, valid() will
+	 *  subsequently return true and all accessors can be called.  Otherwise, all accessors
+	 *  except to_string() will throw a MainSoundConfigurationError and to_string() will
+	 *  return the original invalid string.
+	 */
 	explicit MainSoundConfiguration(std::string);
 	MainSoundConfiguration (MCASoundField field_, int channels);
 
 	MCASoundField field () const {
+		throw_if_invalid();
 		return _field;
 	}
 
 	int channels () const {
+		throw_if_invalid();
 		return _channels.size();
 	}
 
 	boost::optional<Channel> mapping (int index) const;
 	void set_mapping (int index, Channel channel);
 
-	std::string to_string () const;
+	std::string to_string() const {
+		return _configuration;
+	}
+
+	bool valid() const {
+		return _valid;
+	}
 
 private:
-	MCASoundField _field;
-	std::vector<boost::optional<Channel>> _channels;
+	void update_string();
+
+	void throw_if_invalid() const {
+		if (!_valid) {
+			throw MainSoundConfigurationError(_configuration);
+		}
+	}
+
+	std::string _configuration;
+	mutable bool _valid = true;
+	mutable MCASoundField _field;
+	mutable std::vector<boost::optional<Channel>> _channels;
 };
 
 
diff --git a/src/verify.cc b/src/verify.cc
index 8775ab11..8d33bc0b 100644
--- a/src/verify.cc
+++ b/src/verify.cc
@@ -1641,7 +1641,7 @@ verify_cpl(Context& context, shared_ptr<const CPL> cpl)
 
 	if (context.dcp->standard() == Standard::SMPTE) {
 		if (auto msc = cpl->main_sound_configuration()) {
-			if (context.audio_channels && msc->channels() != *context.audio_channels) {
+			if (msc->valid() && context.audio_channels && msc->channels() != *context.audio_channels) {
 				context.error(
 					VerificationNote::Code::INVALID_MAIN_SOUND_CONFIGURATION,
 					String::compose("MainSoundConfiguration has %1 channels but sound assets have %2", msc->channels(), *context.audio_channels),
diff --git a/src/verify.h b/src/verify.h
index c333d86e..10a798d4 100644
--- a/src/verify.h
+++ b/src/verify.h
@@ -467,9 +467,10 @@ public:
 		 */
 		MISMATCHED_SOUND_CHANNEL_COUNTS,
 		/** The CPL contains a _<MainSoundConfiguration>_ tag which does not describe the number of
-		 *  channels in the audio assets.
+		 *  channels in the audio assets, or which is in some way badly formatted.
 		 *  note contains details of what is wrong
 		 *  file contains the CPL filename
+		 *  cpl_id contains the CPL ID
 		 */
 		INVALID_MAIN_SOUND_CONFIGURATION,
 		/** An interop subtitle file has a _<LoadFont>_ node which refers to a font file that is not found.
@@ -520,7 +521,7 @@ public:
 		 *  note contains the invalid namespace
 		 *  file contains the PKL filename
 		 */
-		INVALID_PKL_NAMESPACE
+		INVALID_PKL_NAMESPACE,
 	};
 
 	VerificationNote (Type type, Code code)
diff --git a/test/cpl_metadata_test.cc b/test/cpl_metadata_test.cc
index a24fd7e0..2fd7476a 100644
--- a/test/cpl_metadata_test.cc
+++ b/test/cpl_metadata_test.cc
@@ -184,7 +184,6 @@ BOOST_AUTO_TEST_CASE(main_sound_configuration_test6)
 BOOST_AUTO_TEST_CASE(main_sound_configuration_test_case_insensitive)
 {
 	dcp::MainSoundConfiguration msc("51/L,-,C,LFE,LS,RS,HI,VIN");
-	BOOST_CHECK_EQUAL(msc.to_string(), "51/L,-,C,LFE,Ls,Rs,HI,VIN");
 	BOOST_CHECK_EQUAL(msc.channels(), 8);
 	BOOST_CHECK_EQUAL(msc.field(), dcp::MCASoundField::FIVE_POINT_ONE);
 	BOOST_CHECK_EQUAL(msc.mapping(0).get(), dcp::Channel::LEFT);
diff --git a/test/verify_test.cc b/test/verify_test.cc
index 79db9f95..8a3d5676 100644
--- a/test/verify_test.cc
+++ b/test/verify_test.cc
@@ -5456,7 +5456,10 @@ BOOST_AUTO_TEST_CASE(verify_invalid_main_sound_configuration)
 				).set_cpl_id(cpl->id()),
 			ok(dcp::VerificationNote::Code::VALID_CONTENT_VERSION_LABEL_TEXT, cpl->content_version()->label_text, cpl),
 			dcp::VerificationNote(
-				dcp::VerificationNote::Type::ERROR, dcp::VerificationNote::Code::INVALID_MAIN_SOUND_CONFIGURATION, std::string{"MainSoundConfiguration has 6 channels but sound assets have 2"}, canonical(find_cpl(path))
+				dcp::VerificationNote::Type::ERROR,
+				dcp::VerificationNote::Code::INVALID_MAIN_SOUND_CONFIGURATION,
+				std::string{"MainSoundConfiguration has 6 channels but sound assets have 2"},
+				canonical(find_cpl(path))
 				).set_cpl_id(cpl->id())
 		});
 }
diff --git a/tools/dcpdiff.cc b/tools/dcpdiff.cc
index e4dee74a..bce87393 100644
--- a/tools/dcpdiff.cc
+++ b/tools/dcpdiff.cc
@@ -39,12 +39,12 @@
 #include "filesystem.h"
 #include "mxf.h"
 #include <getopt.h>
-#include <boost/bind.hpp>
-#include <boost/optional.hpp>
-#include <memory>
+#include <boost/bind/bind.hpp>
 #include <boost/filesystem.hpp>
+#include <boost/optional.hpp>
 #include <iostream>
 #include <list>
+#include <memory>
 
 
 using std::list;