Try to remove the PKL object, and make it so that we don't keepdcp-editor2

the PKL assets around after reading them.

But verification needs to see those assets, to check the hashes.

12 files changed, 189 insertions, 244 deletions

diff --git a/src/asset.cc b/src/asset.cc
index 6e200b0d..ba43063d 100644
--- a/src/asset.cc
+++ b/src/asset.cc
@@ -41,7 +41,7 @@
 #include "compose.hpp"
 #include "dcp_assert.h"
 #include "exceptions.h"
-#include "pkl.h"
+#include "pkl_internal.h"
 #include "raw_convert.h"
 #include "util.h"
 #include "warnings.h"
@@ -51,9 +51,10 @@ LIBDCP_ENABLE_WARNINGS
 #include <boost/algorithm/string.hpp>
 
 
+using std::shared_ptr;
 using std::string;
+using std::vector;
 using boost::function;
-using std::shared_ptr;
 using boost::optional;
 using namespace boost::filesystem;
 using namespace dcp;
@@ -81,7 +82,7 @@ Asset::Asset (string id, path file)
 
 
 void
-Asset::add_to_pkl (shared_ptr<PKL> pkl, path root) const
+Asset::add_to_pkl (vector<PKLAsset>& assets, path root, dcp::Standard standard) const
 {
 	DCP_ASSERT (_file);
 
@@ -97,7 +98,7 @@ Asset::add_to_pkl (shared_ptr<PKL> pkl, path root) const
 		return;
 	}
 
-	pkl->add_asset (_id, _id, hash(), file_size(_file.get()), pkl_type(pkl->standard()));
+	assets.push_back(PKLAsset(_id, {}, hash(), file_size(_file.get()), pkl_type(standard)));
 }
 
 
diff --git a/src/asset.h b/src/asset.h
index 88c7d1d1..0f840b00 100644
--- a/src/asset.h
+++ b/src/asset.h
@@ -43,7 +43,6 @@
 
 #include "object.h"
 #include "types.h"
-#include "pkl.h"
 #include <boost/filesystem.hpp>
 #include <boost/function.hpp>
 #include <boost/optional.hpp>
@@ -60,6 +59,9 @@ struct asset_test;
 namespace dcp {
 
 
+class PKLAsset;
+
+
 /** @class Asset
  *  @brief Parent class for DCP assets, i.e. picture, sound, subtitles, closed captions, CPLs, fonts
  *
@@ -91,7 +93,7 @@ public:
 
 	virtual void write_to_assetmap (xmlpp::Node* node, boost::filesystem::path root) const;
 
-	virtual void add_to_pkl (std::shared_ptr<PKL> pkl, boost::filesystem::path root) const;
+	virtual void add_to_pkl (std::vector<PKLAsset>& assets, boost::filesystem::path root, dcp::Standard standard) const;
 
 	/** @return the most recent disk file used to read or write this asset, if there is one */
 	boost::optional<boost::filesystem::path> file () const {
diff --git a/src/atmos_asset.cc b/src/atmos_asset.cc
index ae381737..d1fb5718 100644
--- a/src/atmos_asset.cc
+++ b/src/atmos_asset.cc
@@ -41,6 +41,7 @@
 #include "atmos_asset_reader.h"
 #include "atmos_asset_writer.h"
 #include "exceptions.h"
+#include "util.h"
 #include <asdcp/AS_DCP.h>
 
 
diff --git a/src/dcp.cc b/src/dcp.cc
index 803701f5..28a00665 100644
--- a/src/dcp.cc
+++ b/src/dcp.cc
@@ -52,7 +52,7 @@
 #include "metadata.h"
 #include "mono_picture_asset.h"
 #include "picture_asset.h"
-#include "pkl.h"
+#include "pkl_internal.h"
 #include "raw_convert.h"
 #include "reel_asset.h"
 #include "reel_subtitle_asset.h"
@@ -168,13 +168,20 @@ DCP::read (vector<dcp::VerificationNote>* notes, bool ignore_incorrect_picture_m
 		boost::throw_exception (XMLError ("No packing lists found in asset map"));
 	}
 
+	auto pkl_assets = vector<PKLAsset>();
+
+	bool first = true;
 	for (auto i: pkl_paths) {
-		_pkls.push_back (make_shared<PKL>(_directory / i));
+		auto metadata = read_pkl(_directory / i, pkl_assets);
+		if (first) {
+			_metadata = metadata;
+			first = false;
+		}
 	}
 
 	/* Now we have:
 	     paths - map of files in the DCP that are not PKLs; key is ID, value is path.
-	     _pkls - PKL objects for each PKL.
+	     pkl_assets - all the assets from all the PKLs that we found.
 
 	   Read all the assets from the asset map.
 	 */
@@ -184,10 +191,10 @@ DCP::read (vector<dcp::VerificationNote>* notes, bool ignore_incorrect_picture_m
 	*/
 	vector<shared_ptr<Asset>> other_assets;
 
-	for (auto i: paths) {
-		auto path = _directory / i.second;
+	for (auto const& id_and_path: paths) {
+		auto path = _directory / id_and_path.second;
 
-		if (i.second.empty()) {
+		if (id_and_path.second.empty()) {
 			/* I can't see how this is valid, but it's
 			   been seen in the wild with a DCP that
 			   claims to come from ClipsterDCI 5.10.0.5.
@@ -206,21 +213,16 @@ DCP::read (vector<dcp::VerificationNote>* notes, bool ignore_incorrect_picture_m
 		}
 
 		/* Find the <Type> for this asset from the PKL that contains the asset */
-		optional<string> pkl_type;
-		for (auto j: _pkls) {
-			pkl_type = j->type(i.first);
-			if (pkl_type) {
-				break;
-			}
-		}
-
-		if (!pkl_type) {
+		auto pkl_asset_with_id = std::find_if(pkl_assets.begin(), pkl_assets.end(), [](PKLAsset const& a) { return a.id() == id_and_path.first; });
+		if (pkl_asset_with_id != pkl_assets.end() {
 			/* This asset is in the ASSETMAP but not mentioned in any PKL so we don't
 			 * need to worry about it.
 			 */
 			continue;
 		}
 
+		auto pkl_type = pkl_asset_with_id->type();
+
 		auto remove_parameters = [](string const& n) {
 			return n.substr(0, n.find(";"));
 		};
diff --git a/src/dcp.h b/src/dcp.h
index a70ff8d1..0d533284 100644
--- a/src/dcp.h
+++ b/src/dcp.h
@@ -170,13 +170,6 @@ public:
 		return _directory;
 	}
 
-	/** @return PKLs if this DCP was read from an existing one, or if write_xml() has been called on it.
-	 *  If neither is true, this method returns an empty vector.
-	 */
-	std::vector<std::shared_ptr<PKL>> pkls () const {
-		return _pkls;
-	}
-
 	boost::optional<boost::filesystem::path> asset_map_path () {
 		return _asset_map;
 	}
@@ -200,8 +193,7 @@ private:
 	boost::filesystem::path _directory;
 	/** The CPLs that make up this DCP */
 	std::vector<std::shared_ptr<CPL>> _cpls;
-	/** The PKLs that make up this DCP */
-	std::vector<std::shared_ptr<PKL>> _pkls;
+	PKLMetadata _pkl_metadata;
 	/** File that the ASSETMAP was read from or last written to */
 	mutable boost::optional<boost::filesystem::path> _asset_map;
 
diff --git a/src/interop_subtitle_asset.cc b/src/interop_subtitle_asset.cc
index 798f1cda..993be0ae 100644
--- a/src/interop_subtitle_asset.cc
+++ b/src/interop_subtitle_asset.cc
@@ -43,6 +43,7 @@
 #include "file.h"
 #include "interop_load_font_node.h"
 #include "interop_subtitle_asset.h"
+#include "pkl_internal.h"
 #include "raw_convert.h"
 #include "subtitle_asset_internal.h"
 #include "subtitle_image.h"
@@ -282,15 +283,15 @@ InteropSubtitleAsset::write_to_assetmap (xmlpp::Node* node, boost::filesystem::p
 
 
 void
-InteropSubtitleAsset::add_to_pkl (shared_ptr<PKL> pkl, boost::filesystem::path root) const
+InteropSubtitleAsset::add_to_pkl (vector<PKLAsset>& assets, boost::filesystem::path root, dcp::Standard standard) const
 {
-	Asset::add_to_pkl (pkl, root);
+	Asset::add_to_pkl (assets, root, standard);
 
 	for (auto i: _subtitles) {
 		auto im = dynamic_pointer_cast<dcp::SubtitleImage> (i);
 		if (im) {
 			auto png_image = im->png_image ();
-			pkl->add_asset (im->id(), optional<string>(), make_digest(png_image), png_image.size(), "image/png");
+			assets.push_back(PKLAsset(im->id(), optional<string>(), make_digest(png_image), png_image.size(), "image/png"));
 		}
 	}
 }
diff --git a/src/interop_subtitle_asset.h b/src/interop_subtitle_asset.h
index b953b303..95427f01 100644
--- a/src/interop_subtitle_asset.h
+++ b/src/interop_subtitle_asset.h
@@ -69,7 +69,7 @@ public:
 		) const override;
 
 	void write_to_assetmap (xmlpp::Node* node, boost::filesystem::path root) const override;
-	void add_to_pkl (std::shared_ptr<PKL> pkl, boost::filesystem::path root) const override;
+	void add_to_pkl (std::vector<PKLAsset>& assets, boost::filesystem::path root, dcp::Standard standard) const override;
 
 	std::vector<std::shared_ptr<LoadFontNode>> load_font_nodes () const override;
 
diff --git a/src/metadata.h b/src/metadata.h
index 364cb388..7b490ab6 100644
--- a/src/metadata.h
+++ b/src/metadata.h
@@ -41,6 +41,8 @@
 #define LIBDCP_METADATA_H
 
 
+#include "types.h"
+#include <boost/optional.hpp>
 #include <string>
 
 
@@ -77,7 +79,20 @@ public:
 };
 
 
+class PKLMetadata
+{
+public:
+	std::string id;
+	boost::optional<dcp::Standard> standard;
+	boost::optional<std::string> annotation_text;
+	std::string issue_date;
+	std::string issuer;
+	std::string creator;
+};
+
+
 }
 
 
 #endif
+
diff --git a/src/pkl.h b/src/pkl.h
deleted file mode 100644
index 01988e70..00000000
--- a/src/pkl.h
+++ /dev/null
@@ -1,149 +0,0 @@
-/*
-    Copyright (C) 2018-2021 Carl Hetherington <cth@carlh.net>
-
-    This file is part of libdcp.
-
-    libdcp 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 2 of the License, or
-    (at your option) any later version.
-
-    libdcp is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License
-    along with libdcp.  If not, see <http://www.gnu.org/licenses/>.
-
-    In addition, as a special exception, the copyright holders give
-    permission to link the code of portions of this program with the
-    OpenSSL library under certain conditions as described in each
-    individual source file, and distribute linked combinations
-    including the two.
-
-    You must obey the GNU General Public License in all respects
-    for all of the code used other than OpenSSL.  If you modify
-    file(s) with this exception, you may extend this exception to your
-    version of the file(s), but you are not obligated to do so.  If you
-    do not wish to do so, delete this exception statement from your
-    version.  If you delete this exception statement from all source
-    files in the program, then also delete it here.
-*/
-
-
-/** @file  src/pkl.cc
- *  @brief PKL class
- */
-
-
-#ifndef LIBDCP_PKL_H
-#define LIBDCP_PKL_H
-
-
-#include "object.h"
-#include "types.h"
-#include "util.h"
-#include "certificate_chain.h"
-#include <libcxml/cxml.h>
-#include <boost/filesystem.hpp>
-
-
-namespace dcp {
-
-
-class PKL : public Object
-{
-public:
-	PKL (Standard standard, boost::optional<std::string> annotation_text, std::string issue_date, std::string issuer, std::string creator)
-		: _standard (standard)
-		, _annotation_text (annotation_text)
-		, _issue_date (issue_date)
-		, _issuer (issuer)
-		, _creator (creator)
-	{}
-
-	explicit PKL (boost::filesystem::path file);
-
-	Standard standard () const {
-		return _standard;
-	}
-
-	boost::optional<std::string> annotation_text () const {
-		return _annotation_text;
-	}
-
-	boost::optional<std::string> hash (std::string id) const;
-	boost::optional<std::string> type (std::string id) const;
-
-	void add_asset (std::string id, boost::optional<std::string> annotation_text, std::string hash, int64_t size, std::string type);
-	void write (boost::filesystem::path file, std::shared_ptr<const CertificateChain> signer) const;
-
-	/** @return the most recent disk file used to read or write this PKL, if there is one */
-	boost::optional<boost::filesystem::path> file () const {
-		return _file;
-	}
-
-	class Asset : public Object
-	{
-	public:
-		Asset (cxml::ConstNodePtr node)
-			: Object (remove_urn_uuid(node->string_child("Id")))
-			, _annotation_text (node->optional_string_child("AnnotationText"))
-			, _hash (node->string_child("Hash"))
-			, _size (node->number_child<int64_t>("Size"))
-			, _type (node->string_child("Type"))
-		{}
-
-		Asset (std::string id, boost::optional<std::string> annotation_text, std::string hash, int64_t size, std::string type)
-			: Object (id)
-			, _annotation_text (annotation_text)
-			, _hash (hash)
-			, _size (size)
-			, _type (type)
-		{}
-
-		boost::optional<std::string> annotation_text () const {
-			return _annotation_text;
-		}
-
-		std::string hash () const {
-			return _hash;
-		}
-
-		int64_t size () const {
-			return _size;
-		}
-
-		std::string type () const {
-			return _type;
-		}
-
-	private:
-		boost::optional<std::string> _annotation_text;
-		std::string _hash;
-		int64_t _size = 0;
-		std::string _type;
-	};
-
-	std::vector<std::shared_ptr<Asset>> asset_list () const {
-		return _asset_list;
-	}
-
-private:
-
-	Standard _standard = dcp::Standard::SMPTE;
-	boost::optional<std::string> _annotation_text;
-	std::string _issue_date;
-	std::string _issuer;
-	std::string _creator;
-	std::vector<std::shared_ptr<Asset>> _asset_list;
-	/** The most recent disk file used to read or write this PKL */
-	mutable boost::optional<boost::filesystem::path> _file;
-};
-
-
-}
-
-
-#endif
diff --git a/src/pkl.cc b/src/pkl_internal.cc
index 22589b4d..eee6f8dc 100644
--- a/src/pkl.cc
+++ b/src/pkl_internal.cc
@@ -39,7 +39,7 @@
 
 #include "dcp_assert.h"
 #include "exceptions.h"
-#include "pkl.h"
+#include "pkl_internal.h"
 #include "raw_convert.h"
 #include "util.h"
 #include "warnings.h"
@@ -52,6 +52,7 @@ LIBDCP_ENABLE_WARNINGS
 using std::string;
 using std::shared_ptr;
 using std::make_shared;
+using std::vector;
 using boost::optional;
 using namespace dcp;
 
@@ -60,102 +61,76 @@ static string const pkl_interop_ns = "http://www.digicine.com/PROTO-ASDCP-PKL-20
 static string const pkl_smpte_ns   = "http://www.smpte-ra.org/schemas/429-8/2007/PKL";
 
 
-PKL::PKL (boost::filesystem::path file)
-	: _file (file)
+PKLMetadata
+read_pkl (boost::filesystem::path file, vector<PKLAsset>& assets)
 {
 	cxml::Document pkl ("PackingList");
 	pkl.read_file (file);
 
+	PKLMetadata metadata;
+
 	if (pkl.namespace_uri() == pkl_interop_ns) {
-		_standard = Standard::INTEROP;
+		metadata.standard = Standard::INTEROP;
 	} else if (pkl.namespace_uri() == pkl_smpte_ns) {
-		_standard = Standard::SMPTE;
+		metadata.standard = Standard::SMPTE;
 	} else {
-		boost::throw_exception (XMLError ("Unrecognised packing list namesapce " + pkl.namespace_uri()));
+		boost::throw_exception(XMLError("Unrecognised packing list namesapce " + pkl.namespace_uri()));
 	}
 
-	_id = remove_urn_uuid (pkl.string_child ("Id"));
-	_annotation_text = pkl.optional_string_child ("AnnotationText");
-	_issue_date = pkl.string_child ("IssueDate");
-	_issuer = pkl.string_child ("Issuer");
-	_creator = pkl.string_child ("Creator");
+	metadata.id = remove_urn_uuid(pkl.string_child("Id"));
+	metadata.annotation_text = pkl.optional_string_child("AnnotationText");
+	metadata.issue_date = pkl.string_child("IssueDate");
+	metadata.issuer = pkl.string_child("Issuer");
+	metadata.creator = pkl.string_child("Creator");
 
 	for (auto i: pkl.node_child("AssetList")->node_children("Asset")) {
-		_asset_list.push_back (make_shared<Asset>(i));
+		assets.push_back(PKLAsset(i));
 	}
-}
-
 
-void
-PKL::add_asset (std::string id, boost::optional<std::string> annotation_text, std::string hash, int64_t size, std::string type)
-{
-	_asset_list.push_back (make_shared<Asset>(id, annotation_text, hash, size, type));
+	return metadata;
 }
 
 
 void
-PKL::write (boost::filesystem::path file, shared_ptr<const CertificateChain> signer) const
+write_pkl (boost::filesystem::path file, PKLMetadata const& metadata, vector<PKLAsset> const& assets, shared_ptr<const CertificateChain> signer)
 {
+	DCP_ASSERT(metadata.standard);
+
 	xmlpp::Document doc;
 	xmlpp::Element* pkl;
-	if (_standard == Standard::INTEROP) {
+	if (metadata.standard == Standard::INTEROP) {
 		pkl = doc.create_root_node("PackingList", pkl_interop_ns);
 	} else {
 		pkl = doc.create_root_node("PackingList", pkl_smpte_ns);
 	}
 
-	pkl->add_child("Id")->add_child_text ("urn:uuid:" + _id);
-	if (_annotation_text) {
-		pkl->add_child("AnnotationText")->add_child_text (*_annotation_text);
+	pkl->add_child("Id")->add_child_text("urn:uuid:" + metadata.id);
+	if (metadata.annotation_text) {
+		pkl->add_child("AnnotationText")->add_child_text(*metadata.annotation_text);
 	}
-	pkl->add_child("IssueDate")->add_child_text (_issue_date);
-	pkl->add_child("Issuer")->add_child_text (_issuer);
-	pkl->add_child("Creator")->add_child_text (_creator);
+	pkl->add_child("IssueDate")->add_child_text(metadata.issue_date);
+	pkl->add_child("Issuer")->add_child_text(metadata.issuer);
+	pkl->add_child("Creator")->add_child_text(metadata.creator);
 
 	auto asset_list = pkl->add_child("AssetList");
-	for (auto i: _asset_list) {
+	for (auto const& i: assets) {
 		auto asset = asset_list->add_child("Asset");
-		asset->add_child("Id")->add_child_text ("urn:uuid:" + i->id());
-		if (i->annotation_text()) {
-			asset->add_child("AnnotationText")->add_child_text (*i->annotation_text());
+		asset->add_child("Id")->add_child_text ("urn:uuid:" + i.id());
+		if (i.annotation_text()) {
+			asset->add_child("AnnotationText")->add_child_text (*i.annotation_text());
 		}
-		asset->add_child("Hash")->add_child_text (i->hash());
-		asset->add_child("Size")->add_child_text (raw_convert<string>(i->size()));
-		asset->add_child("Type")->add_child_text (i->type());
+		asset->add_child("Hash")->add_child_text(i.hash());
+		asset->add_child("Size")->add_child_text(raw_convert<string>(i.size()));
+		asset->add_child("Type")->add_child_text(i.type());
 	}
 
 	indent (pkl, 0);
 
 	if (signer) {
-		signer->sign (pkl, _standard);
+		signer->sign (pkl, *metadata.standard);
 	}
 
 	doc.write_to_file_formatted (file.string(), "UTF-8");
-	_file = file;
-}
-
-
-optional<string>
-PKL::hash (string id) const
-{
-	for (auto i: _asset_list) {
-		if (i->id() == id) {
-			return i->hash();
-		}
-	}
-
-	return {};
 }
 
 
-optional<string>
-PKL::type (string id) const
-{
-	for (auto i: _asset_list) {
-		if (i->id() == id) {
-			return i->type();
-		}
-	}
-
-	return {};
-}
diff --git a/src/pkl_internal.h b/src/pkl_internal.h
new file mode 100644
index 00000000..3bbaca47
--- /dev/null
+++ b/src/pkl_internal.h
@@ -0,0 +1,106 @@
+/*
+    Copyright (C) 2018-2021 Carl Hetherington <cth@carlh.net>
+
+    This file is part of libdcp.
+
+    libdcp 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 2 of the License, or
+    (at your option) any later version.
+
+    libdcp is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with libdcp.  If not, see <http://www.gnu.org/licenses/>.
+
+    In addition, as a special exception, the copyright holders give
+    permission to link the code of portions of this program with the
+    OpenSSL library under certain conditions as described in each
+    individual source file, and distribute linked combinations
+    including the two.
+
+    You must obey the GNU General Public License in all respects
+    for all of the code used other than OpenSSL.  If you modify
+    file(s) with this exception, you may extend this exception to your
+    version of the file(s), but you are not obligated to do so.  If you
+    do not wish to do so, delete this exception statement from your
+    version.  If you delete this exception statement from all source
+    files in the program, then also delete it here.
+*/
+
+
+/** @file  src/pkl_internal.cc
+ *  @brief PKL handling.
+ */
+
+
+#ifndef LIBDCP_PKL_INTERNAL_H
+#define LIBDCP_PKL_INTERNAL_H
+
+
+#include "certificate_chain.h"
+#include "metadata.h"
+#include "object.h"
+#include "types.h"
+#include "util.h"
+#include <libcxml/cxml.h>
+#include <boost/filesystem.hpp>
+
+
+namespace dcp {
+
+
+class PKLAsset : public Object
+{
+public:
+	PKLAsset (cxml::ConstNodePtr node)
+		: Object (remove_urn_uuid(node->string_child("Id")))
+		, _annotation_text (node->optional_string_child("AnnotationText"))
+		, _hash (node->string_child("Hash"))
+		, _size (node->number_child<int64_t>("Size"))
+		, _type (node->string_child("Type"))
+	{}
+
+	PKLAsset (std::string id, boost::optional<std::string> annotation_text, std::string hash, int64_t size, std::string type)
+		: Object (id)
+		, _annotation_text (annotation_text)
+		, _hash (hash)
+		, _size (size)
+		, _type (type)
+	{}
+
+	boost::optional<std::string> annotation_text () const {
+		return _annotation_text;
+	}
+
+	std::string hash () const {
+		return _hash;
+	}
+
+	int64_t size () const {
+		return _size;
+	}
+
+	std::string type () const {
+		return _type;
+	}
+
+private:
+	boost::optional<std::string> _annotation_text;
+	std::string _hash;
+	int64_t _size = 0;
+	std::string _type;
+};
+
+
+PKLMetadata read_pkl (boost::filesystem::path file, std::vector<PKLAsset>& assets);
+void write_pkl (boost::filesystem::path file, PKLMetadata const& metadata, std::vector<PKLAsset> const& assets, std::shared_ptr<const CertificateChain> signer);
+
+
+}
+
+
+#endif
diff --git a/src/wscript b/src/wscript
index da4d0d1e..977c1fc2 100644
--- a/src/wscript
+++ b/src/wscript
@@ -77,7 +77,7 @@ def build(bld):
              openjpeg_image.cc
              picture_asset.cc
              picture_asset_writer.cc
-             pkl.cc
+             pkl_internal.cc
              rating.cc
              raw_convert.cc
              reel.cc
@@ -171,7 +171,6 @@ def build(bld):
               openjpeg_image.h
               picture_asset.h
               picture_asset_writer.h
-              pkl.h
               rating.h
               raw_convert.h
               rgb_xyz.h