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).

4 files changed, 138 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;
+};
+