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.

6 files changed, 75 insertions, 29 deletions

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)