Fix subtitle vertical position (#2367).

Previously we would not account for the differences in what vertical
position means between Interop and SMPTE.  For interop, vertical
position is the distance from the reference point to the text
baseline, whereas for SMPTE it is the distance from the reference
point to the top/middle/bottom of the subtitle (depending on the
reference).

This caused differences between the preview and the DCP for some
cases (notably, using SRT/SSA and making Interop DCPs, or converting
Interop DCP subs to SMPTE, or vice versa).

7 files changed, 303 insertions, 4 deletions

diff --git a/src/lib/reel_writer.cc b/src/lib/reel_writer.cc
index f9e29a16f..94b12ec7d 100644
--- a/src/lib/reel_writer.cc
+++ b/src/lib/reel_writer.cc
@@ -114,6 +114,7 @@ ReelWriter::ReelWriter (
 	, _content_summary (film()->content_summary(period))
 	, _job (job)
 	, _text_only (text_only)
+	, _font_metrics(film()->frame_size().height)
 {
 	/* Create or find our picture asset in a subdirectory, named
 	   according to those film's parameters which affect the video
@@ -889,6 +890,33 @@ ReelWriter::empty_text_asset (TextType type, optional<DCPTextTrack> track, bool
 }
 
 
+float
+ReelWriter::convert_vertical_position(StringText const& subtitle, dcp::Standard to) const
+{
+	if (subtitle.valign_standard == to) {
+		return subtitle.v_position();
+	}
+
+	auto const baseline_to_bottom = _font_metrics.baseline_to_bottom(subtitle);
+	auto const height = _font_metrics.height(subtitle);
+
+	float correction = 0;
+	switch (subtitle.v_align()) {
+	case dcp::VAlign::TOP:
+		correction = height - baseline_to_bottom;
+		break;
+	case dcp::VAlign::CENTER:
+		correction = (height / 2) - baseline_to_bottom;
+		break;
+	case dcp::VAlign::BOTTOM:
+		correction = baseline_to_bottom;
+		break;
+	}
+
+	return subtitle.v_position() + ((subtitle.valign_standard == dcp::Standard::SMPTE) ? correction : -correction);
+}
+
+
 void
 ReelWriter::write (PlayerText subs, TextType type, optional<DCPTextTrack> track, DCPTimePeriod period, FontIdMap const& fonts)
 {
@@ -928,6 +956,7 @@ ReelWriter::write (PlayerText subs, TextType type, optional<DCPTextTrack> track,
 	for (auto i: subs.string) {
 		i.set_in  (dcp::Time(period.from.seconds() - _period.from.seconds(), tcr));
 		i.set_out (dcp::Time(period.to.seconds() - _period.from.seconds(), tcr));
+		i.set_v_position(convert_vertical_position(i, film()->interop() ? dcp::Standard::INTEROP : dcp::Standard::SMPTE));
 		auto sub = make_shared<dcp::SubtitleString>(i);
 		if (type == TextType::OPEN_SUBTITLE) {
 			sub->set_font(fonts.get(i.font));
diff --git a/src/lib/reel_writer.h b/src/lib/reel_writer.h
index e2c713cc5..d5b531f41 100644
--- a/src/lib/reel_writer.h
+++ b/src/lib/reel_writer.h
@@ -26,6 +26,7 @@
 #include "font_id_map.h"
 #include "player_text.h"
 #include "referenced_reel_asset.h"
+#include "render_text.h"
 #include "types.h"
 #include "weak_film.h"
 #include <dcp/atmos_asset_writer.h>
@@ -121,6 +122,7 @@ private:
 		std::set<DCPTextTrack> ensure_closed_captions
 		) const;
 	void create_reel_markers (std::shared_ptr<dcp::Reel> reel) const;
+	float convert_vertical_position(StringText const& subtitle, dcp::Standard to) const;
 
 	dcpomatic::DCPTimePeriod _period;
 	/** the first picture frame index that does not already exist in our MXF */
@@ -147,5 +149,7 @@ private:
 	std::shared_ptr<dcp::AtmosAsset> _atmos_asset;
 	std::shared_ptr<dcp::AtmosAssetWriter> _atmos_asset_writer;
 
+	mutable FontMetrics _font_metrics;
+
 	static int const _info_size;
 };
diff --git a/src/lib/render_text.cc b/src/lib/render_text.cc
index 9c1c233c2..d3db4ac19 100644
--- a/src/lib/render_text.cc
+++ b/src/lib/render_text.cc
@@ -171,12 +171,12 @@ create_surface (shared_ptr<Image> image)
 
 
 static string
-setup_font (StringText const& subtitle)
+setup_font(shared_ptr<const dcpomatic::Font> font)
 {
 	auto font_file = default_font_file ();
 
-	if (subtitle.font && subtitle.font->file()) {
-		font_file = *subtitle.font->file();
+	if (font && font->file()) {
+		font_file = *font->file();
 	}
 
 	return FontConfig::instance()->make_font_available(font_file);
@@ -299,7 +299,7 @@ render_line (list<StringText> subtitles, dcp::Size target, DCPTime time, int fra
 	DCPOMATIC_ASSERT (!subtitles.empty ());
 	auto const& first = subtitles.front ();
 
-	auto const font_name = setup_font (first);
+	auto const font_name = setup_font(first.font);
 	auto const fade_factor = calculate_fade_factor (first, time, frame_rate);
 	auto const markup = marked_up (subtitles, target.height, fade_factor, font_name);
 	auto layout = create_layout ();
@@ -402,3 +402,63 @@ render_text (list<StringText> subtitles, dcp::Size target, DCPTime time, int fra
 
 	return images;
 }
+
+
+float
+FontMetrics::height(StringText const& subtitle)
+{
+	return get(subtitle)->second.second;
+}
+
+
+float
+FontMetrics::baseline_to_bottom(StringText const& subtitle)
+{
+	return get(subtitle)->second.first;
+}
+
+
+FontMetrics::Cache::iterator
+FontMetrics::get(StringText const& subtitle)
+{
+	auto id = Identifier(subtitle);
+
+	auto iter = _cache.find(id);
+	if (iter != _cache.end()) {
+		return iter;
+	}
+
+	auto const font_name = setup_font(subtitle.font);
+	auto layout = create_layout();
+	auto copy = subtitle;
+	copy.set_text("Qypjg");
+	setup_layout(layout, font_name, marked_up({copy}, _target_height, 1, font_name));
+	auto ink = layout->get_ink_extents();
+	auto const scale = float(_target_height * Pango::SCALE);
+	return _cache.insert({id, { ink.get_y() / scale, ink.get_height() / scale}}).first;
+}
+
+
+FontMetrics::Identifier::Identifier(StringText const& subtitle)
+	: font(subtitle.font)
+	, size(subtitle.size())
+	, aspect_adjust(subtitle.aspect_adjust())
+{
+
+}
+
+
+bool
+FontMetrics::Identifier::operator<(FontMetrics::Identifier const& other) const
+{
+	if (font != other.font) {
+		return font < other.font;
+	}
+
+	if (size != other.size) {
+	    return size < other.size;
+	}
+
+	return aspect_adjust < other.aspect_adjust;
+}
+
diff --git a/src/lib/render_text.h b/src/lib/render_text.h
index 6ff91dce1..762d79446 100644
--- a/src/lib/render_text.h
+++ b/src/lib/render_text.h
@@ -18,14 +18,55 @@
 
 */
 
+
 #include "position_image.h"
 #include "dcpomatic_time.h"
 #include "string_text.h"
 #include <dcp/util.h>
+#include <memory>
+
 
 namespace dcpomatic {
 	class Font;
 }
 
+
 std::string marked_up (std::list<StringText> subtitles, int target_height, float fade_factor, std::string font_name);
 std::list<PositionImage> render_text (std::list<StringText>, dcp::Size, dcpomatic::DCPTime, int);
+
+
+class FontMetrics
+{
+public:
+	FontMetrics(int target_height)
+		: _target_height(target_height)
+	{}
+
+	float baseline_to_bottom(StringText const& subtitle);
+	float height(StringText const& subtitle);
+
+private:
+	/** Class to collect the properties of a subtitle which affect the metrics we care about
+	 *  i.e. baseline position and height.
+	 */
+	class Identifier
+	{
+	public:
+		Identifier(StringText const& subtitle);
+
+		std::shared_ptr<dcpomatic::Font> font;
+		int size;
+		float aspect_adjust;
+
+		bool operator<(Identifier const& other) const;
+	};
+
+	using Cache = std::map<Identifier, std::pair<float, float>>;
+
+	Cache::iterator get(StringText const& subtitle);
+
+	Cache _cache;
+
+	int _target_height;
+};
+
diff --git a/test/data b/test/data
-Subproject 92d2787d602701d5faf53f0ab7430f62bec0916
+Subproject ef2374f03fbd715883da46bb3cb011befa11615
diff --git a/test/subtitle_position_test.cc b/test/subtitle_position_test.cc
new file mode 100644
index 000000000..0237a417c
--- /dev/null
+++ b/test/subtitle_position_test.cc
@@ -0,0 +1,164 @@
+/*
+    Copyright (C) 2022 Carl Hetherington <cth@carlh.net>
+
+    This file is part of DCP-o-matic.
+
+    DCP-o-matic 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.
+
+    DCP-o-matic 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 DCP-o-matic.  If not, see <http://www.gnu.org/licenses/>.
+
+*/
+
+
+#include "lib/content.h"
+#include "lib/content_factory.h"
+#include "lib/film.h"
+#include "lib/make_dcp.h"
+#include "lib/text_content.h"
+#include "test.h"
+#include <dcp/interop_subtitle_asset.h>
+#include <dcp/smpte_subtitle_asset.h>
+#include <dcp/language_tag.h>
+#include <boost/test/unit_test.hpp>
+#include <vector>
+
+
+using std::string;
+using std::vector;
+using std::shared_ptr;
+
+
+BOOST_AUTO_TEST_CASE(interop_correctly_placed_in_interop)
+{
+	string const name = "interop_in_interop_position_test";
+	auto fr = content_factory("test/data/dcp_sub.xml");
+	auto film = new_test_film2(name, fr);
+
+	film->set_interop(true);
+
+	make_and_verify_dcp (
+		film,
+		{
+			dcp::VerificationNote::Code::INVALID_STANDARD,
+			dcp::VerificationNote::Code::INVALID_SUBTITLE_SPACING,
+			dcp::VerificationNote::Code::INVALID_SUBTITLE_DURATION
+		});
+
+	auto output = subtitle_file(film);
+	dcp::InteropSubtitleAsset asset(output);
+	auto output_subs = asset.subtitles();
+	BOOST_REQUIRE_EQUAL(output_subs.size(), 1U);
+
+	/* The input subtitle should have been left alone */
+	BOOST_CHECK(output_subs[0]->v_align() == dcp::VAlign::BOTTOM);
+	BOOST_CHECK_CLOSE(output_subs[0]->v_position(), 0.08, 1e-3);
+}
+
+
+BOOST_AUTO_TEST_CASE(interop_correctly_placed_in_smpte)
+{
+	string const name = "interop_in_smpte_position_test";
+	auto fr = content_factory("test/data/dcp_sub.xml");
+	auto film = new_test_film2(name, fr);
+
+	film->set_interop(false);
+
+	make_and_verify_dcp (
+		film,
+		{
+			dcp::VerificationNote::Code::INVALID_SUBTITLE_SPACING,
+			dcp::VerificationNote::Code::MISSING_SUBTITLE_LANGUAGE,
+			dcp::VerificationNote::Code::MISSING_CPL_METADATA,
+			dcp::VerificationNote::Code::INVALID_SUBTITLE_FIRST_TEXT_TIME
+		});
+
+	auto output = subtitle_file(film);
+	dcp::SMPTESubtitleAsset asset(output);
+	auto output_subs = asset.subtitles();
+	BOOST_REQUIRE_EQUAL(output_subs.size(), 1U);
+
+	BOOST_CHECK(output_subs[0]->v_align() == dcp::VAlign::BOTTOM);
+	BOOST_CHECK_CLOSE(output_subs[0]->v_position(), 0.07074, 1e-3);
+}
+
+
+BOOST_AUTO_TEST_CASE(smpte_correctly_placed_in_interop)
+{
+	string const name = "smpte_in_interop_position_test";
+	auto fr = content_factory("test/data/short.srt");
+	auto film = new_test_film2(name, fr);
+
+	film->set_interop(true);
+
+	make_and_verify_dcp (
+		film,
+		{
+			dcp::VerificationNote::Code::INVALID_STANDARD,
+			dcp::VerificationNote::Code::INVALID_SUBTITLE_SPACING,
+			dcp::VerificationNote::Code::INVALID_SUBTITLE_FIRST_TEXT_TIME
+		});
+
+	auto output = subtitle_file(film);
+	dcp::InteropSubtitleAsset asset(output);
+	auto output_subs = asset.subtitles();
+	BOOST_REQUIRE_EQUAL(output_subs.size(), 1U);
+
+	BOOST_CHECK(output_subs[0]->v_align() == dcp::VAlign::TOP);
+	BOOST_CHECK_CLOSE(output_subs[0]->v_position(), 0.87079, 1e-3);
+}
+
+
+static
+void
+vpos_test(dcp::VAlign reference, float position, dcp::Standard from, dcp::Standard to)
+{
+	string standard = from == dcp::Standard::INTEROP ? "interop" : "smpte";
+	auto name = String::compose("vpos_test_%1_%2", standard, valign_to_string(reference));
+	auto in = content_factory(String::compose("test/data/%1.xml", name));
+	auto film = new_test_film2(name, in);
+
+	film->set_interop(to == dcp::Standard::INTEROP);
+
+	film->write_metadata();
+	make_dcp(film, TranscodeJob::ChangedBehaviour::IGNORE);
+	BOOST_REQUIRE(!wait_for_jobs());
+
+	auto out = subtitle_file(film);
+	vector<shared_ptr<const dcp::Subtitle>> subtitles;
+	if (to == dcp::Standard::INTEROP) {
+		dcp::InteropSubtitleAsset asset(out);
+		subtitles = asset.subtitles();
+	} else {
+		dcp::SMPTESubtitleAsset asset(out);
+		subtitles = asset.subtitles();
+	}
+
+	BOOST_REQUIRE_EQUAL(subtitles.size(), 1U);
+
+	BOOST_CHECK(subtitles[0]->v_align() == reference);
+	BOOST_CHECK_CLOSE(subtitles[0]->v_position(), position, 1e-3);
+}
+
+
+BOOST_AUTO_TEST_CASE(subtitles_correctly_placed_with_all_references)
+{
+	constexpr auto baseline_to_bottom = 0.00925926;
+	constexpr auto height = 0.0462963;
+
+	vpos_test(dcp::VAlign::TOP, 0.2 - height + baseline_to_bottom, dcp::Standard::INTEROP, dcp::Standard::SMPTE);
+	vpos_test(dcp::VAlign::CENTER, 0.11 - (height / 2) + baseline_to_bottom, dcp::Standard::INTEROP, dcp::Standard::SMPTE);
+	vpos_test(dcp::VAlign::BOTTOM, 0.08 - baseline_to_bottom, dcp::Standard::INTEROP, dcp::Standard::SMPTE);
+	vpos_test(dcp::VAlign::TOP, 0.1 + height - baseline_to_bottom, dcp::Standard::SMPTE, dcp::Standard::INTEROP);
+	vpos_test(dcp::VAlign::CENTER, 0.15 + (height / 2) - baseline_to_bottom, dcp::Standard::SMPTE, dcp::Standard::INTEROP);
+	vpos_test(dcp::VAlign::BOTTOM, 0.10 + baseline_to_bottom, dcp::Standard::SMPTE, dcp::Standard::INTEROP);
+}
+
diff --git a/test/wscript b/test/wscript
index fc63aac1e..e54d5c985 100644
--- a/test/wscript
+++ b/test/wscript
@@ -141,6 +141,7 @@ def build(bld):
                  subtitle_font_id_change_test.cc
                  subtitle_language_test.cc
                  subtitle_metadata_test.cc
+                 subtitle_position_test.cc
                  subtitle_reel_test.cc
                  subtitle_reel_number_test.cc
                  subtitle_timing_test.cc