Fix snapping behavior of current video position marker

Lots of text incoming, since timestamp wrangling is hell and I want to
write down my reasoning before I inevitably forget it again in two
weeks.

Before commit 5d4973a, snapping an
audio line's end time to the "current video position" indicator line
would reliably make the line end on the frame before the current video
frame (i.e. make the current video frame be the first frame where the
selected line is *not* visible any more).

Commit 5d4973a broke this, making the
behavior of snapping audio times to the "current video position"
inconsistent.

The commit in question is definitely not fully correct, ultimately just
because the entire concept of implicitly converting millisecond
timestamps to centisecond timestamps is flawed in and of itself, and
bound to always fail in sufficiently crazy edge cases. Fixing the
timestamp conversion properly would entail either working with
centisecond timestamps from the beginning, or somehow making the process
of converting timestamps aware of the context the timestamps are from
(e.g. "coming from some frame timestamp").

However, this specific issue was only *exposed* by this commit, and not
solely caused by it. Its root cause was just that the "current video
position" marker on the audio display would mark the *exact* time of the
current video frame, rather than the ideal start/end time for a line to
start/end at that video frame. This is why only snapping to the "current
video position" broke while snapping to keyframes worked fine. It's
quite possible that snapping to the "current video frame" marker was
just never thought of as a use case.

So, for now, TypesettingTools#421 can just be fixed by
making a line snapped to the "current video position" marker snap to the
middle of the frame rather than at the frame's exact start, and
5d4973a can be untangled at some later
time.

However, we would still like to *draw* the "current video position"
marker at the exact start time of the current video frame, so that one
can tell from the audio display which lines are visible at the current
video frame and which aren't.

Hence, we use the following middle-ground solution:
- Keep drawing the "current video position" marker at the exact start
  time of video frames
- Do not allow snapping to the exact time of the "current video
  position" marker. Instead, allow either snapping to the middle of the
  previous frame, or the middle of the current frame.
- To make this behavior understandable for the user, draw a
  semi-transparent rectangle behind the "current video position" marker
  indicating the duration of the previous and current video frame.

Fixes TypesettingTools#421.

Author: arch1t3cht

src/audio_display.cpp

@@ -50,6 +50,7 @@
 #include <algorithm>
 
 #include <wx/dcbuffer.h>
+#include <wx/dcgraph.h>
 #include <wx/mousestate.h>
 
 /// @class AudioDisplayInteractionObject
@@ -848,12 +849,13 @@ void AudioDisplay::OnPaint(wxPaintEvent&)
 
 		if (audio_bounds.Intersects(updrect))
 		{
+			wxGCDC gcdc(dc);	// Markers may use alpha, which needs a GCDC
 			TimeRange updtime(
 				std::max(0, TimeFromRelativeX(updrect.x - foot_size)),
 				std::max(0, TimeFromRelativeX(updrect.x + updrect.width + foot_size)));
 
 			PaintAudio(dc, updtime, updrect);
-			PaintMarkers(dc, updtime);
+			PaintMarkers(gcdc, updtime);
 			PaintLabels(dc, updtime);
 		}
 	}
@@ -898,8 +900,17 @@ void AudioDisplay::PaintMarkers(wxDC &dc, TimeRange updtime)
 	{
 		int marker_x = RelativeXFromTime(marker->GetPosition());
 
-		dc.SetPen(marker->GetStyle());
-		dc.DrawLine(marker_x, audio_top, marker_x, audio_top+audio_height);
+		if (marker->GetWidth() == 0) {
+			dc.SetPen(marker->GetStyle());
+			dc.DrawLine(marker_x, audio_top, marker_x, audio_top+audio_height);
+		} else {
+			int w = RelativeXFromTime(marker->GetPosition() + marker->GetWidth() - 1) - marker_x + 1;
+
+			dc.SetBrush(wxBrush(marker->GetStyle().GetColour()));
+			dc.SetPen(*wxTRANSPARENT_PEN);
+
+			dc.DrawRectangle(marker_x, audio_top, w, audio_height);
+		}
 
 		if (marker->GetFeet() == AudioMarker::Feet_None) continue;
 

src/audio_marker.cpp

@@ -82,6 +82,36 @@ void AudioMarkerProviderKeyframes::GetMarkers(TimeRange const& range, AudioMarke
 		out.push_back(&*a);
 }
 
+class VideoPositionSnapPoint final : public AudioMarker {
+	int position = -1;
+
+public:
+	void SetPosition(int new_pos) { position = new_pos; }
+
+	int GetPosition() const override { return position; }
+	FeetStyle GetFeet() const override { return Feet_None; }
+	wxPen GetStyle() const override { return wxPen{}; }
+};
+
+class VideoPositionRange final : public AudioMarker {
+	Pen style;
+	int position = -1;
+	int width = 0;
+
+public:
+	VideoPositionRange(bool previous) : style{previous ? "Colour/Audio Display/Previous Frame Range" : "Colour/Audio Display/Current Frame Range"} { }
+
+	void SetPosition(int frame, int nextframe) {
+		position = frame + 1;
+		width = std::max(1, nextframe - frame);
+	}
+
+	int GetPosition() const override { return position; }
+	int GetWidth() const override { return width; }
+	FeetStyle GetFeet() const override { return Feet_None; }
+	wxPen GetStyle() const override { return style; }
+};
+
 class VideoPositionMarker final : public AudioMarker {
 	Pen style{"Colour/Audio Display/Play Cursor"};
 	int position = -1;
@@ -92,7 +122,6 @@ class VideoPositionMarker final : public AudioMarker {
 	int GetPosition() const override { return position; }
 	FeetStyle GetFeet() const override { return Feet_None; }
 	wxPen GetStyle() const override { return style; }
-	operator int() const { return position; }
 };
 
 VideoPositionMarkerProvider::VideoPositionMarkerProvider(agi::Context *c)
@@ -105,33 +134,61 @@ VideoPositionMarkerProvider::VideoPositionMarkerProvider(agi::Context *c)
 
 VideoPositionMarkerProvider::~VideoPositionMarkerProvider() { }
 
-void VideoPositionMarkerProvider::SetPosition(int frame_number) {
-	marker->SetPosition(c->videoController->TimeAtFrame(frame_number));
+void VideoPositionMarkerProvider::SetPositions(int frame_number) {
+	auto vc = c->videoController.get();
+
+	range1->SetPosition(vc->TimeAtFrame(frame_number - 1), vc->TimeAtFrame(frame_number));
+	range2->SetPosition(vc->TimeAtFrame(frame_number), vc->TimeAtFrame(frame_number + 1));
+	marker->SetPosition(vc->TimeAtFrame(frame_number));
+	snap1->SetPosition(vc->TimeAtFrame(frame_number, agi::vfr::START));
+	snap2->SetPosition(vc->TimeAtFrame(frame_number, agi::vfr::END));
 }
 
 void VideoPositionMarkerProvider::Update(int frame_number) {
-	SetPosition(frame_number);
+	SetPositions(frame_number);
 	AnnounceMarkerMoved();
 }
 
 void VideoPositionMarkerProvider::OptChanged(agi::OptionValue const& opt) {
 	if (opt.GetBool()) {
 		video_seek_slot.Unblock();
+		range1 = std::make_unique<VideoPositionRange>(true);
+		range2 = std::make_unique<VideoPositionRange>(false);
 		marker = std::make_unique<VideoPositionMarker>();
-		SetPosition(c->videoController->GetFrameN());
+		snap1 = std::make_unique<VideoPositionSnapPoint>();
+		snap2 = std::make_unique<VideoPositionSnapPoint>();
+		SetPositions(c->videoController->GetFrameN());
 	}
 	else {
 		video_seek_slot.Block();
+		range1.reset();
+		range2.reset();
 		marker.reset();
+		snap1.reset();
+		snap2.reset();
 	}
 }
 
-void VideoPositionMarkerProvider::GetMarkers(const TimeRange &range, AudioMarkerVector &out) const {
+void VideoPositionMarkerProvider::GetMarkers(const TimeRange &timerange, AudioMarkerVector &out) const {
 	if (!c->project->VideoProvider())
 		return;
 
-	if (marker && range.contains(*marker))
+	for (auto range : {range1.get(), range2.get()})
+		if (range && timerange.overlaps({range->GetPosition(), range->GetPosition() + range->GetWidth() - 1}))
+			out.push_back(range);
+
+	if (marker && timerange.contains(marker->GetPosition()))
 		out.push_back(marker.get());
+
+}
+
+void VideoPositionMarkerProvider::GetSnapMarkers(const TimeRange &timerange, AudioMarkerVector &out) const {
+	if (!c->project->VideoProvider())
+		return;
+
+	for (auto snap : {snap1.get(), snap2.get()})
+		if (snap && timerange.contains(snap->GetPosition()))
+			out.push_back(snap);
 }
 
 SecondsMarkerProvider::SecondsMarkerProvider()

src/audio_marker.h

@@ -28,6 +28,8 @@ class AudioMarkerKeyframe;
 class Pen;
 class Project;
 class VideoPositionMarker;
+class VideoPositionRange;
+class VideoPositionSnapPoint;
 class wxPen;
 
 namespace agi {
@@ -53,6 +55,10 @@ class AudioMarker {
 	/// @return The marker's position in milliseconds
 	virtual int GetPosition() const = 0;
 
+	/// @brief Get the marker's width
+	/// @return The marker's width if the marker should be a rectangle, or 0 if the marker should be drawn as a line (with the thickness specified by the style)
+	virtual int GetWidth() const { return 0; }
+
 	/// @brief Get the marker's drawing style
 	/// @return A pen object describing the marker's drawing style
 	virtual wxPen GetStyle() const = 0;
@@ -72,9 +78,12 @@ class AudioMarkerProvider {
 
 	~AudioMarkerProvider() = default;
 public:
-	/// @brief Return markers in a time range
+	/// @brief Return markers in a time range to render in audio display
 	virtual void GetMarkers(const TimeRange &range, AudioMarkerVector &out) const = 0;
 
+	/// @brief Return markers in a time range to snap timed lines to
+	virtual void GetSnapMarkers(const TimeRange &range, AudioMarkerVector &out) const { GetMarkers(range, out); };
+
 	DEFINE_SIGNAL_ADDERS(AnnounceMarkerMoved, AddMarkerMovedListener)
 };
 
@@ -141,12 +150,16 @@ class AudioMarkerProviderKeyframes final : public AudioMarkerProvider {
 class VideoPositionMarkerProvider final : public AudioMarkerProvider {
 	agi::Context *c;
 
+	std::unique_ptr<VideoPositionRange> range1;
+	std::unique_ptr<VideoPositionRange> range2;
 	std::unique_ptr<VideoPositionMarker> marker;
+	std::unique_ptr<VideoPositionSnapPoint> snap1;
+	std::unique_ptr<VideoPositionSnapPoint> snap2;
 
 	agi::signal::Connection video_seek_slot;
 	agi::signal::Connection enable_opt_changed_slot;
 
-	void SetPosition(int frame_number);
+	void SetPositions(int frame_number);
 	void Update(int frame_number);
 	void OptChanged(agi::OptionValue const& opt);
 
@@ -155,6 +168,7 @@ class VideoPositionMarkerProvider final : public AudioMarkerProvider {
 	~VideoPositionMarkerProvider();
 
 	void GetMarkers(const TimeRange &range, AudioMarkerVector &out) const override;
+	void GetSnapMarkers(const TimeRange &range, AudioMarkerVector &out) const override;
 };
 
 /// Marker provider for lines every second

src/audio_timing_dialogue.cpp

@@ -445,8 +445,8 @@ void AudioTimingControllerDialogue::GetMarkers(const TimeRange &range, AudioMark
 		boost::upper_bound(markers, range.end(), marker_ptr_cmp()),
 		back_inserter(out_markers));
 
-	keyframes_provider.GetMarkers(range, out_markers);
 	video_position_provider.GetMarkers(range, out_markers);
+	keyframes_provider.GetMarkers(range, out_markers);
 }
 
 void AudioTimingControllerDialogue::OnSelectedSetChanged()
@@ -897,8 +897,8 @@ int AudioTimingControllerDialogue::SnapMarkers(int snap_range, std::vector<Audio
 
 		snap_markers.clear();
 		TimeRange range(pos - snap_range, pos + snap_range);
-		keyframes_provider.GetMarkers(range, snap_markers);
-		video_position_provider.GetMarkers(range, snap_markers);
+		keyframes_provider.GetSnapMarkers(range, snap_markers);
+		video_position_provider.GetSnapMarkers(range, snap_markers);
 
 		for (const auto marker : snap_markers)
 		{

src/libresrc/default_config.json

@@ -107,6 +107,8 @@
 			"Line boundary End" : "rgb(0, 0, 216)",
 			"Line boundary Start" : "rgb(216, 0, 0)",
 			"Play Cursor" : "rgb(255,255,255)",
+			"Current Frame Range" : "rgba(255,255,255,160)",
+			"Previous Frame Range" : "rgba(255,255,255,200)",
 			"Seconds Line" : "rgb(0,100,255)",
 			"Spectrum" : "Icy Blue",
 			"Syllable Boundaries" : "rgb(255,255,0)",

src/preferences.cpp

@@ -283,6 +283,8 @@ void Interface_Colours(wxTreebook *book, Preferences *parent) {
 
 	auto audio = p->PageSizer(_("Audio Display"));
 	p->OptionAdd(audio, _("Play cursor"), "Colour/Audio Display/Play Cursor");
+	p->OptionAdd(audio, _("Current frame range"), "Colour/Audio Display/Current Frame Range", {.alpha = true});
+	p->OptionAdd(audio, _("Previous frame range"), "Colour/Audio Display/Previous Frame Range", {.alpha = true});
 	p->OptionAdd(audio, _("Line boundary start"), "Colour/Audio Display/Line boundary Start");
 	p->OptionAdd(audio, _("Line boundary end"), "Colour/Audio Display/Line boundary End");
 	p->OptionAdd(audio, _("Line boundary inactive line"), "Colour/Audio Display/Line Boundary Inactive Line");

src/preferences_base.cpp

@@ -161,7 +161,7 @@ wxControl *OptionPage::OptionAdd(PageSection section, const wxString &name, cons
 		}
 
 		case agi::OptionType::Color: {
-			auto cb = new ColourButton(section.box, wxSize(40,10), false, opt->GetColor());
+			auto cb = new ColourButton(section.box, wxSize(40,10), kwargs.alpha, opt->GetColor());
 			cb->Bind(EVT_COLOR, ColourUpdater(opt_name, parent));
 			Add(section, name, cb);
 			return cb;

src/preferences_base.h

@@ -37,6 +37,7 @@ struct OptionAddArgs {
 	double min = 0;
 	double max = INT_MAX;
 	double inc = 1;
+	bool alpha = false;
 };
 
 class OptionPage : public wxScrolled<wxPanel> {