Skip to content

Commit

Permalink
Improvement to video caching thread, to recover from the separation o…
Browse files Browse the repository at this point in the history
…f cache vs display frame #. Also found a mutex that was needed, to prevent crashing when the video thread calls timeline::GetFrame at certain times... colliding with another thread (and independent of OpenMP).
  • Loading branch information
jonoomph committed Oct 23, 2020
1 parent e556d0f commit 1723484
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 10 deletions.
22 changes: 13 additions & 9 deletions src/Qt/VideoCacheThread.cpp
Expand Up @@ -94,8 +94,10 @@ namespace openshot
while (!threadShouldExit() && is_playing) {

// Cache frames before the other threads need them
// Cache frames up to the max frames
while (speed == 1 && (position - current_display_frame) < max_frames)
// Cache frames up to the max frames. Reset to current position
// if cache gets too far away from display frame. Cache frames
// even when player is paused (i.e. speed 0).
while ((position - current_display_frame) < max_frames)
{
// Only cache up till the max_frames amount... then sleep
try
Expand All @@ -104,6 +106,14 @@ namespace openshot
ZmqLogger::Instance()->AppendDebugMethod("VideoCacheThread::run (cache frame)", "position", position, "current_display_frame", current_display_frame, "max_frames", max_frames, "needed_frames", (position - current_display_frame));

// Force the frame to be generated
if (reader->GetCache()->GetSmallestFrame()) {
int64_t smallest_cached_frame = reader->GetCache()->GetSmallestFrame()->number;
if (smallest_cached_frame > current_display_frame) {
// Cache position has gotten too far away from current display frame.
// Reset the position to the current display frame.
position = current_display_frame;
}
}
reader->GetFrame(position);
}

Expand All @@ -113,14 +123,8 @@ namespace openshot
// Ignore out of bounds frame exceptions
}

// Is cache position behind current display frame?
if (position < current_display_frame) {
// Jump ahead
position = current_display_frame;
}

// Increment frame number
position++;
position++;
}

// Sleep for 1 frame length
Expand Down
5 changes: 4 additions & 1 deletion src/Qt/VideoCacheThread.h
Expand Up @@ -45,10 +45,13 @@ namespace openshot
*/
class VideoCacheThread : Thread
{
private:
std::atomic_int position;

protected:
std::shared_ptr<Frame> frame;
int speed;
bool is_playing;
int64_t position;
int64_t current_display_frame;
ReaderBase *reader;
int max_frames;
Expand Down
10 changes: 10 additions & 0 deletions src/Timeline.cpp
Expand Up @@ -695,6 +695,7 @@ std::shared_ptr<Frame> Timeline::GetFrame(int64_t requested_frame)

// Check cache
std::shared_ptr<Frame> frame;
std::lock_guard<std::mutex> guard(get_frame_mutex);
#pragma omp critical (T_GetFrame)
frame = final_cache->GetFrame(requested_frame);
if (frame) {
Expand Down Expand Up @@ -724,6 +725,15 @@ std::shared_ptr<Frame> Timeline::GetFrame(int64_t requested_frame)
return frame;
}

// Check if previous frame was cached? (if not, assume we are seeking somewhere else on the Timeline, and need
// to clear all cache (for continuity sake). For example, jumping back to a previous spot can cause issues with audio
// data where the new jump location doesn't match up with the previously cached audio data.
std::shared_ptr<Frame> previous_frame = final_cache->GetFrame(requested_frame - 1);
if (!previous_frame) {
// Seeking to new place on timeline (destroy cache)
ClearAllCache();

This comment has been minimized.

Copy link
@ferdnyc

ferdnyc Oct 28, 2020

Contributor

@jonoomph

I have to be honest, this strikes me as kind of scorched-earth. I understand the intent, but... well, for starters, won't this mean that the entire cache gets cleared every time you start playing from the BEGINNING of the reader? GetFrame(0) will never be in the cache, right?

It seems to me that if merely skipping to a different point on the timeline requires invalidating the entire cache, then it's not really a cache at all, anymore. It's just a queue of immediately-pending frames — a readahead buffer. Right?

(Which, if that's all we need, then great! But that could be implemented much more simply using a std::deque of frame objects. A simple readahead buffer doesn't require a parent-child hierarchy of custom classes, incredible amounts of ancillary metadata and housekeeping code, and a monitoring and management logic spread across three languages in order to present a constantly-refreshing status display.)

This comment has been minimized.

Copy link
@ferdnyc

ferdnyc Oct 28, 2020

Contributor

Oh, nope! I take that back, about the first frame, since this is in the else block.

This comment has been minimized.

Copy link
@ferdnyc

ferdnyc Oct 28, 2020

Contributor

This still worries me, though. Like, what about reverse play? Won't that cause the cache to continually reset itself every frame?

}

// Minimum number of frames to process (for performance reasons)
int minimum_frames = OPEN_MP_NUM_PROCESSORS;

Expand Down
2 changes: 2 additions & 0 deletions src/Timeline.h
Expand Up @@ -33,6 +33,7 @@

#include <list>
#include <memory>
#include <mutex>
#include <set>
#include <QtGui/QImage>
#include <QtGui/QPainter>
Expand Down Expand Up @@ -175,6 +176,7 @@ namespace openshot {
std::set<openshot::FrameMapper*> allocated_frame_mappers; ///< all the frame mappers we allocated and must free
bool managed_cache; ///< Does this timeline instance manage the cache object
std::string path; ///< Optional path of loaded UTF-8 OpenShot JSON project file
std::mutex get_frame_mutex; ///< Mutex to protect GetFrame method from different threads calling it

/// Process a new layer of video or audio
void add_layer(std::shared_ptr<openshot::Frame> new_frame, openshot::Clip* source_clip, int64_t clip_frame_number, int64_t timeline_frame_number, bool is_top_clip, float max_volume);
Expand Down

0 comments on commit 1723484

Please sign in to comment.