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

diff --git a/src/lib/reel_writer.cc b/src/lib/reel_writer.cc
index f9e29a16f4c15537358f68398b09c8f11b8fdea7..94b12ec7d38506147541c09d2b99634f82945a6d 100644 (file)
--- 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 e2c713cc5fccc56e115ff2ee8427887f8c5ceec1..d5b531f410ed9b6793237305296fbf43c83e66d9 100644 (file)
--- 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 9c1c233c26c12c1bf0202e4f2c9683c2eb3f00f9..d3db4ac1940c245987d16c4f055e632d82f2cec7 100644 (file)
--- 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 6ff91dce1f98346fe09521a36833bb4de8020dfe..762d794466e9747984dbf3be5d6e11ea153c9b15 100644 (file)
--- 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
index 92d2787d602701d5faf53f0ab7430f62bec09166..ef2374f03fbd715883da46bb3cb011befa116154 160000 (submodule)
--- a/test/data
+++ b/test/data
@@ -1 +1 @@
-Subproject commit 92d2787d602701d5faf53f0ab7430f62bec09166
+Subproject commit ef2374f03fbd715883da46bb3cb011befa116154

diff --git a/test/subtitle_position_test.cc b/test/subtitle_position_test.cc
new file mode 100644 (file)
index 0000000..0237a41
--- /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 fc63aac1eba2ce6d29580ba3a8656e7d2e56dcf0..e54d5c9852da268687ab52837ace35bdd909ffd8 100644 (file)
--- 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