Extract and fix closed text display logic (#2857).

This was buggy, but here it is extracted, fixed and gains a basic test.

7 files changed, 289 insertions, 108 deletions

diff --git a/src/lib/closed_text_display.cc b/src/lib/closed_text_display.cc
new file mode 100644
index 000000000..7ec8d5d8a
--- /dev/null
+++ b/src/lib/closed_text_display.cc
@@ -0,0 +1,128 @@
+/*
+    Copyright (C) 2024 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 "butler.h"
+#include "closed_text_display.h"
+#include "constants.h"
+#include "dcp_text_track.h"
+#include "dcpomatic_time.h"
+#include "string_text.h"
+#include "text_ring_buffers.h"
+
+
+using std::weak_ptr;
+using boost::optional;
+
+
+void
+ClosedTextDisplay::set_butler(weak_ptr<Butler> butler)
+{
+	_butler = butler;
+}
+
+
+class ClosedTextSorter
+{
+public:
+	bool operator() (StringText const & a, StringText const & b)
+	{
+		return from_top(a) < from_top(b);
+	}
+
+private:
+	float from_top (StringText const & c) const
+	{
+		switch (c.v_align()) {
+		case dcp::VAlign::TOP:
+			return c.v_position();
+		case dcp::VAlign::CENTER:
+			return c.v_position() + 0.5;
+		case dcp::VAlign::BOTTOM:
+			return 1.0 - c.v_position();
+		}
+		DCPOMATIC_ASSERT (false);
+		return 0;
+	}
+};
+
+
+bool
+ClosedTextDisplay::update(dcpomatic::DCPTime time, optional<DCPTextTrack> track)
+{
+	bool changed = false;
+
+	/* Clear _current if it should not be visible */
+	if (!_current.empty() && _current_period && !_current_period->contains(time)) {
+		_current = {};
+		_current_period = {};
+		changed = true;
+	}
+
+	/* Fill next if necessary and possible */
+	if (_next.empty() && track) {
+		if (auto butler = _butler.lock()) {
+			optional<TextRingBuffers::Data> data;
+			while (auto d = butler->get_closed_caption()) {
+				if (d->track == *track && d->period.to > time) {
+					data = d;
+					break;
+				}
+			}
+
+			if (data) {
+				auto to_show = data->text.string;
+				std::sort(to_show.begin(), to_show.end(), ClosedTextSorter());
+
+				auto j = to_show.begin();
+				int k = 0;
+				while (j != to_show.end() && k < MAX_CLOSED_CAPTION_LINES) {
+					_next.push_back(j->text());
+					++j;
+					++k;
+				}
+
+				_next_period = data->period;
+			}
+		}
+	}
+
+	/* Swap next in if it's time */
+	if (_next_period && _next_period->contains(time)) {
+		_current = _next;
+		_current_period = _next_period;
+		_next = {};
+		_next_period = {};
+		changed = true;
+	}
+
+	return changed;
+}
+
+
+void
+ClosedTextDisplay::clear()
+{
+	_current = {};
+	_current_period = {};
+	_next = {};
+	_next_period = {};
+}
+
diff --git a/src/lib/closed_text_display.h b/src/lib/closed_text_display.h
new file mode 100644
index 000000000..f2cf7dd11
--- /dev/null
+++ b/src/lib/closed_text_display.h
@@ -0,0 +1,60 @@
+/*
+    Copyright (C) 2024 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 "dcpomatic_time.h"
+#include <memory>
+#include <string>
+#include <vector>
+
+
+class Butler;
+class DCPTextTrack;
+class Film;
+
+
+/** @class ClosedTextDisplay
+ *  @brief Helper for displaying closed text from a butler.
+ *
+ *  This class extracts the logic used by the closed text dialogue box,
+ *  mostly to make it more easily testable.
+ */
+class ClosedTextDisplay
+{
+public:
+	void clear();
+	void set_butler(std::weak_ptr<Butler> butler);
+
+	/** Set up the display for a particular time and text track */
+	bool update(dcpomatic::DCPTime time, boost::optional<DCPTextTrack> track);
+
+	/** @return the current closed text lines that should be shown */
+	std::vector<std::string> const& current() const {
+		return _current;
+	}
+
+private:
+	std::weak_ptr<Butler> _butler;
+	std::vector<std::string> _current;
+	boost::optional<dcpomatic::DCPTimePeriod> _current_period;
+	std::vector<std::string> _next;
+	boost::optional<dcpomatic::DCPTimePeriod> _next_period;
+};
+
diff --git a/src/lib/wscript b/src/lib/wscript
index 87a1ca787..7b00468e8 100644
--- a/src/lib/wscript
+++ b/src/lib/wscript
@@ -52,6 +52,7 @@ sources = """
           cinema.cc
           cinema_sound_processor.cc
           change_signaller.cc
+          closed_text_display.cc
           collator.cc
           colour_conversion.cc
           config.cc
diff --git a/src/wx/closed_captions_dialog.cc b/src/wx/closed_captions_dialog.cc
index 5cd8c73ef..16c9977bd 100644
--- a/src/wx/closed_captions_dialog.cc
+++ b/src/wx/closed_captions_dialog.cc
@@ -58,13 +58,10 @@ ClosedCaptionsDialog::ClosedCaptionsDialog (wxWindow* parent, FilmViewer* viewer
 		)
 	, _viewer (viewer)
 	  /* XXX: empirical and probably unhelpful default size here; needs to be related to font metrics */
-        , _display (new wxPanel(this, wxID_ANY, wxDefaultPosition, wxSize(640, (640 / 10) + 64)))
+        , _display_panel(new wxPanel(this, wxID_ANY, wxDefaultPosition, wxSize(640, (640 / 10) + 64)))
 	, _track (new wxChoice(this, wxID_ANY))
-	, _current_in_lines (false)
 	, _timer (this)
 {
-	_lines.resize (MAX_CLOSED_CAPTION_LINES);
-
 	wxBoxSizer* sizer = new wxBoxSizer (wxVERTICAL);
 
 	wxBoxSizer* track_sizer = new wxBoxSizer (wxHORIZONTAL);
@@ -72,11 +69,11 @@ ClosedCaptionsDialog::ClosedCaptionsDialog (wxWindow* parent, FilmViewer* viewer
 	track_sizer->Add (_track, 0, wxEXPAND | wxLEFT, DCPOMATIC_SIZER_X_GAP);
 
 	sizer->Add (track_sizer, 0, wxALL, DCPOMATIC_SIZER_GAP);
-	sizer->Add (_display, 1, wxEXPAND);
+	sizer->Add(_display_panel, 1, wxEXPAND);
 
 	Bind (wxEVT_SHOW, boost::bind(&ClosedCaptionsDialog::shown, this, _1));
 	Bind (wxEVT_TIMER, boost::bind(&ClosedCaptionsDialog::update, this));
-	_display->Bind (wxEVT_PAINT, boost::bind(&ClosedCaptionsDialog::paint, this));
+	_display_panel->Bind (wxEVT_PAINT, boost::bind(&ClosedCaptionsDialog::paint, this));
 	_track->Bind (wxEVT_CHOICE, boost::bind(&ClosedCaptionsDialog::track_selected, this));
 
 	SetSizerAndFit (sizer);
@@ -95,7 +92,7 @@ ClosedCaptionsDialog::shown (wxShowEvent ev)
 void
 ClosedCaptionsDialog::track_selected ()
 {
-	_current = {};
+	_display.clear();
 	_viewer->slow_refresh ();
 	update ();
 }
@@ -103,7 +100,7 @@ ClosedCaptionsDialog::track_selected ()
 void
 ClosedCaptionsDialog::paint ()
 {
-	wxPaintDC dc (_display);
+	wxPaintDC dc(_display_panel);
 	dc.SetBackground (*wxBLACK_BRUSH);
 	dc.Clear ();
 	dc.SetTextForeground (*wxWHITE);
@@ -114,11 +111,14 @@ ClosedCaptionsDialog::paint ()
 	font.SetPixelSize (wxSize (0, line_height * 0.8));
 	dc.SetFont (font);
 
-	for (int i = 0; i < MAX_CLOSED_CAPTION_LINES; ++i) {
-		wxString const good = _lines[i].Left (MAX_CLOSED_CAPTION_LENGTH);
+	auto current = _display.current();
+
+	for (size_t i = 0; i < current.size(); ++i) {
+		auto current_wx = std_to_wx(current[i]);
+		auto const good = current_wx.Left(MAX_CLOSED_CAPTION_LENGTH);
 		dc.DrawText (good, 8, line_height * i);
-		if (_lines[i].Length() > MAX_CLOSED_CAPTION_LENGTH) {
-			auto const bad = _lines[i].Right(_lines[i].Length() - MAX_CLOSED_CAPTION_LENGTH);
+		if (current_wx.Length() > MAX_CLOSED_CAPTION_LENGTH) {
+			auto const bad = current_wx.Right(current_wx.Length() - MAX_CLOSED_CAPTION_LENGTH);
 			wxSize size = dc.GetTextExtent (good);
 			dc.SetTextForeground (*wxRED);
 			dc.DrawText (bad, 8 + size.GetWidth(), line_height * i);
@@ -127,106 +127,20 @@ ClosedCaptionsDialog::paint ()
 	}
 }
 
-class ClosedCaptionSorter
-{
-public:
-	bool operator() (StringText const & a, StringText const & b)
-	{
-		return from_top(a) < from_top(b);
-	}
-
-private:
-	float from_top (StringText const & c) const
-	{
-		switch (c.v_align()) {
-		case dcp::VAlign::TOP:
-			return c.v_position();
-		case dcp::VAlign::CENTER:
-			return c.v_position() + 0.5;
-		case dcp::VAlign::BOTTOM:
-			return 1.0 - c.v_position();
-		}
-		DCPOMATIC_ASSERT (false);
-		return 0;
-	}
-};
-
 void
 ClosedCaptionsDialog::update ()
 {
-	auto const time = _viewer->time ();
-
-	if (_current_in_lines && _current && _current->period.to > time) {
-		/* Current one is fine */
-		return;
-	}
-
-	if (_current && _current->period.to < time) {
-		/* Current one has finished; clear out */
-		for (int j = 0; j < MAX_CLOSED_CAPTION_LINES; ++j) {
-			_lines[j] = "";
-		}
-		Refresh ();
-		_current = {};
-	}
-
-	if (!_current && !_tracks.empty()) {
-		/* We have no current one: get another */
-		auto butler = _butler.lock ();
-		DCPOMATIC_ASSERT (_track->GetSelection() >= 0);
-		DCPOMATIC_ASSERT (_track->GetSelection() < int(_tracks.size()));
-		auto track = _tracks[_track->GetSelection()];
-		if (butler) {
-			while (true) {
-				auto d = butler->get_closed_caption();
-				if (!d) {
-					break;
-				}
-				if (d->track == track) {
-					_current = d;
-					break;
-				}
-			}
-
-			_current_in_lines = false;
-		}
-	}
-
-	if (_current && _current->period.contains(time)) {
-		/* We need to set this new one up */
-
-		auto to_show = _current->text.string;
-
-		for (int j = 0; j < MAX_CLOSED_CAPTION_LINES; ++j) {
-			_lines[j] = "";
-		}
-
-		std::sort(to_show.begin(), to_show.end(), ClosedCaptionSorter());
-
-		auto j = to_show.begin();
-		int k = 0;
-		while (j != to_show.end() && k < MAX_CLOSED_CAPTION_LINES) {
-			_lines[k] = std_to_wx (j->text());
-			++j;
-			++k;
-		}
-
-		Refresh ();
-		_current_in_lines = true;
-	}
-
-	if (!_current && _tracks.empty()) {
-		for (int i = 0; i < MAX_CLOSED_CAPTION_LINES; ++i) {
-			_lines[i] = wxString();
-		}
+	DCPOMATIC_ASSERT(_track->GetSelection() >= 0);
+	DCPOMATIC_ASSERT(_track->GetSelection() < int(_tracks.size()));
+	if (_display.update(_viewer->time(), _tracks.empty() ? boost::optional<DCPTextTrack>() : _tracks[_track->GetSelection()])) {
+		Refresh();
 	}
 }
 
 void
 ClosedCaptionsDialog::clear ()
 {
-	_current = {};
-	_current_in_lines = false;
+	_display.clear();
 	Refresh ();
 }
 
@@ -234,7 +148,7 @@ ClosedCaptionsDialog::clear ()
 void
 ClosedCaptionsDialog::set_butler (weak_ptr<Butler> butler)
 {
-	_butler = butler;
+	_display.set_butler(butler);
 }
 
 void
diff --git a/src/wx/closed_captions_dialog.h b/src/wx/closed_captions_dialog.h
index dbe4b6aa5..345b5fb5e 100644
--- a/src/wx/closed_captions_dialog.h
+++ b/src/wx/closed_captions_dialog.h
@@ -19,6 +19,7 @@
 */
 
 
+#include "lib/closed_text_display.h"
 #include "lib/dcpomatic_time.h"
 #include "lib/text_ring_buffers.h"
 #include <dcp/warnings.h>
@@ -26,6 +27,7 @@ LIBDCP_DISABLE_WARNINGS
 #include <wx/wx.h>
 LIBDCP_ENABLE_WARNINGS
 
+
 class Butler;
 class FilmViewer;
 
@@ -46,12 +48,10 @@ private:
 	void track_selected ();
 
 	FilmViewer* _viewer;
-	wxPanel* _display;
+	wxPanel* _display_panel;
 	wxChoice* _track;
-	boost::optional<TextRingBuffers::Data> _current;
-	bool _current_in_lines;
-	std::vector<wxString> _lines;
 	std::vector<DCPTextTrack> _tracks;
 	std::weak_ptr<Butler> _butler;
 	wxTimer _timer;
+	ClosedTextDisplay _display;
 };
diff --git a/test/closed_text_display_test.cc b/test/closed_text_display_test.cc
new file mode 100644
index 000000000..e3494471c
--- /dev/null
+++ b/test/closed_text_display_test.cc
@@ -0,0 +1,77 @@
+/*
+    Copyright (C) 2024 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/butler.h"
+#include "lib/closed_text_display.h"
+#include "lib/content_factory.h"
+#include "lib/cross.h"
+#include "lib/dcpomatic_time.h"
+#include "lib/image.h"
+#include "lib/film.h"
+#include "lib/player.h"
+#include "lib/text_content.h"
+#include "test.h"
+#include <boost/test/unit_test.hpp>
+#include <memory>
+
+
+BOOST_AUTO_TEST_CASE(closed_text_display_test1)
+{
+	auto text = content_factory("test/data/frames.srt");
+	auto film = new_test_film("closed_text_display_test1", text);
+
+	DCPTextTrack track("test", dcp::LanguageTag("de-DE"));
+	text[0]->text[0]->set_type(TextType::CLOSED_CAPTION);
+	text[0]->text[0]->set_dcp_track(track);
+	text[0]->text[0]->set_use(true);
+
+	Player player(film, Image::Alignment::COMPACT);
+	auto butler = std::make_shared<Butler>(
+		film, player, AudioMapping(), 2, boost::bind(PlayerVideo::force, AV_PIX_FMT_RGB24), VideoRange::FULL, Image::Alignment::PADDED, true, false, Butler::Audio::DISABLED
+		);
+
+	auto await_butler = [butler]() {
+		while (butler->video_frames_in_buffer() < 4) {
+			dcpomatic_sleep_seconds(1);
+		}
+	};
+
+	await_butler();
+
+	ClosedTextDisplay display;
+	display.set_butler(butler);
+
+	display.update(dcpomatic::DCPTime::from_frames(2, 24), track);
+	BOOST_CHECK(display.current() == std::vector<std::string>{ "2" });
+
+	butler->seek({}, true);
+	await_butler();
+	display.update(dcpomatic::DCPTime::from_frames(0, 24), track);
+	BOOST_CHECK(display.current() == std::vector<std::string>{ "0" });
+
+	display.clear();
+	BOOST_CHECK(display.current().empty());
+
+	butler->seek(dcpomatic::DCPTime::from_frames(100, 24), true);
+	display.update(dcpomatic::DCPTime::from_frames(100, 24), track);
+	BOOST_CHECK(display.current().empty());
+}
+
diff --git a/test/wscript b/test/wscript
index cd23badc0..a13b9fa89 100644
--- a/test/wscript
+++ b/test/wscript
@@ -63,6 +63,7 @@ def build(bld):
                  cinema_sound_processor_test.cc
                  client_server_test.cc
                  closed_caption_test.cc
+                 closed_text_display_test.cc
                  collator_test.cc
                  colour_conversion_test.cc
                  config_test.cc