From: Carl Hetherington Date: Fri, 18 Nov 2022 09:56:42 +0000 (+0100) Subject: Fix subtitle vertical position (#2367). X-Git-Tag: v2.16.34~31 X-Git-Url: https://git.carlh.net/gitweb/?p=dcpomatic.git;a=commitdiff_plain;h=323b8cbb0b95297fbd027ffdc4ea5003b59ef25f 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 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 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 track, DCPTimePeriod period, FontIdMap const& fonts) { @@ -928,6 +956,7 @@ ReelWriter::write (PlayerText subs, TextType type, optional 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(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 @@ -121,6 +122,7 @@ private: std::set ensure_closed_captions ) const; void create_reel_markers (std::shared_ptr 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 _atmos_asset; std::shared_ptr _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) static string -setup_font (StringText const& subtitle) +setup_font(shared_ptr 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 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 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 +#include + namespace dcpomatic { class Font; } + std::string marked_up (std::list subtitles, int target_height, float fade_factor, std::string font_name); std::list render_text (std::list, 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 font; + int size; + float aspect_adjust; + + bool operator<(Identifier const& other) const; + }; + + using Cache = std::map>; + + Cache::iterator get(StringText const& subtitle); + + Cache _cache; + + int _target_height; +}; + diff --git a/test/data b/test/data index 92d2787d6..ef2374f03 160000 --- 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 index 000000000..0237a417c --- /dev/null +++ b/test/subtitle_position_test.cc @@ -0,0 +1,164 @@ +/* + Copyright (C) 2022 Carl Hetherington + + 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 . + +*/ + + +#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 +#include +#include +#include +#include + + +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> 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