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 f74cb86f717161a9b6a9ffab8575594a5a8548b4..6d1d3977deb872ee3672f784d39d007c6c9549c9 100644 (file)
--- 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 8b29794abe2ff7aca21eff8267b8e6a76092c13c..a820411064234bef2d29aa9c7a95f91a1cd0771d 100644 (file)
--- 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 03f82d6fbb93d8ce96e12b1cdc5167108dec3971..1b3358735344adbc009ccccf2eb5ca710ba60f95 100644 (file)
--- 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;