From: Steinar H. Gunderson <sgunderson@bigfoot.com>
Date: Tue, 15 Jan 2019 17:39:10 +0000 (+0100)
Subject: When sending original frames, do the reading in the queueing thread.
X-Git-Tag: 1.8.2~19
X-Git-Url: https://git.sesse.net/?p=nageru;a=commitdiff_plain;h=8e88294902b4b6f43c8db83dbf04830b5b9d2bfd

When sending original frames, do the reading in the queueing thread.

Earlier, we'd only preload it and hope the encoder's second read would
come from the cache. But this is more consistent, and perhaps more
importantly, it fixes a race on the FrameReader, where the two threads
would try accessing it at the same time, causing the file descriptor
to be closed while being used, and a crash.

Now, the FrameReaders are consistently accessed from one thread only.
---

diff --git a/futatabi/frame_on_disk.h b/futatabi/frame_on_disk.h
index f74cb86..6d1d397 100644
--- a/futatabi/frame_on_disk.h
+++ b/futatabi/frame_on_disk.h
@@ -32,6 +32,8 @@ static bool inline operator==(const FrameOnDisk &a, const FrameOnDisk &b)
 // the sequential reads. (For this reason, each display has a private
 // FrameReader. Thus, we can easily keep multiple open file descriptors around
 // for a single .frames file.)
+//
+// Thread-compatible, but not thread-safe.
 class FrameReader {
 public:
 	FrameReader();
diff --git a/futatabi/video_stream.cpp b/futatabi/video_stream.cpp
index 8b29794..a820411 100644
--- a/futatabi/video_stream.cpp
+++ b/futatabi/video_stream.cpp
@@ -335,18 +335,14 @@ void VideoStream::schedule_original_frame(steady_clock::time_point local_pts,
 {
 	fprintf(stderr, "output_pts=%ld  original      input_pts=%ld\n", output_pts, frame.pts);
 
-	// Preload the file from disk, so that the encoder thread does not get stalled.
-	// TODO: Consider sending it through the queue instead.
-	(void)frame_reader.read_frame(frame);
-
 	QueuedFrame qf;
 	qf.local_pts = local_pts;
 	qf.type = QueuedFrame::ORIGINAL;
 	qf.output_pts = output_pts;
-	qf.frame1 = frame;
 	qf.display_func = move(display_func);
 	qf.queue_spot_holder = move(queue_spot_holder);
 	qf.subtitle = subtitle;
+	qf.encoded_jpeg.reset(new string(frame_reader.read_frame(frame)));
 
 	lock_guard<mutex> lock(queue_lock);
 	frame_queue.push_back(move(qf));
@@ -655,7 +651,7 @@ void VideoStream::encode_thread_func()
 
 		if (qf.type == QueuedFrame::ORIGINAL) {
 			// Send the JPEG frame on, unchanged.
-			string jpeg = frame_reader.read_frame(qf.frame1);
+			string jpeg = move(*qf.encoded_jpeg);
 			AVPacket pkt;
 			av_init_packet(&pkt);
 			pkt.stream_index = 0;
diff --git a/futatabi/video_stream.h b/futatabi/video_stream.h
index 03f82d6..1b33587 100644
--- a/futatabi/video_stream.h
+++ b/futatabi/video_stream.h
@@ -111,7 +111,15 @@ private:
 
 		int64_t output_pts;
 		enum Type { ORIGINAL, FADED, INTERPOLATED, FADED_INTERPOLATED, REFRESH } type;
-		FrameOnDisk frame1;  // The only frame for original frames.
+
+		// For original frames only. Made move-only so we know explicitly
+		// we don't copy these ~200 kB files around inadvertedly.
+		//
+		// TODO: Consider using vector<uint8_t> instead, so we save one copy.
+		std::unique_ptr<std::string> encoded_jpeg;
+
+		// For everything except original frames.
+		FrameOnDisk frame1;
 
 		// For fades only (including fades against interpolated frames).
 		FrameOnDisk secondary_frame;