fix: SG-43388: RV leaks memory because of opened decoders

[deltag0]

fix: RV leaks memory because of opened decoders

Summarize your change.

• Re-enable old context map that manages the number of open threads to decode videos

Describe the reason for the change.

• Without the context map, the number of open decoder threads keeps increasing as we load more videos.
• RAM usage would increase a lot even by loading video clips without playing the video at all, and just staying on the first frame.

Describe what you have tested and on which operating system.

Tested on CY 2025 on Rocky Linux 9.8 and Mac

Opened over 1000 clips with 1GB of caching, peaking at 9GB, but staying bounded.
RAM no longer increases when loading clips without playback being on.

Add a list of changes, and note any that might need special attention during the review.

• After freeing the decoder with avcodec_free_context, nullify the tracked pointer to not point to freed memory
  No regressions by adding the context map

If possible, provide screenshots.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: deltag0 / name: deltag0 (686e739, 9c09807)

[deltag0]

I took a look at the code again to refresh myself, and I found that the m_subtitleMap looks like an unused field, so we could delete it.

[deltag0]

FYI @deltag0 : From our robot friend: Blocking issue

Use-after-free in ~IPGraph() — job dispatcher destroyed before the nodes that use it

IPGraph.cpp:325-330 deletes the JobDispatcher at the top of the destructor and does not null m_jobDispatcher. The new node-deletion loop at IPGraph.cpp:345-355 then runs afterward.

FileSourceIPNode::~FileSourceIPNode() calls cancelJobs() (FileSourceIPNode.cpp:322), which does:

int workItemID = m_workItemID.exchange(0); if (workItemID) { graph()->removeWorkItem(workItemID); // FileSourceIPNode.cpp:2405 graph()->waitWorkItem(workItemID); // FileSourceIPNode.cpp:2408 }

and both unconditionally dereference the freed dispatcher:

auto jobDispatcher = reinterpret_cast<JobDispatcher*>(m_jobDispatcher); jobDispatcher->removeJob(id); // IPGraph.cpp:3775-3776

Failure scenario: quit RV while any media-load work item is still outstanding (m_workItemID != 0) — e.g. quitting during or shortly after loading sources. ~IPGraph frees the dispatcher, then deletes the FileSourceIPNode, which calls removeJob()/waitJob() on freed memory. waitJob() blocking on a destroyed condition variable is the nastier of the two.

It's intermittent — m_workItemID is usually 0 by shutdown, which is why manual testing wouldn't catch it — but it's a genuine crash-on-exit regression, and this PR is the thing that made those destructors run at all.

Fix: move the node-deletion block before the dispatcher teardown, and defensively null the pointer:

IPGraph::~IPGraph() { setCachingMode(NeverCache, 1, 2, 1, 2, 1, 1, 24.0); finishCachingThread(); finishAudioThread();

//  Delete all nodes first: source-node destructors call back into the
//  graph (FileSourceIPNode::cancelJobs -> removeWorkItem/waitWorkItem),
//  so the job dispatcher must still be alive here.
for (auto& [name, node] : m_viewNodeMap)
{
    node->willDelete();
    node->disconnectInputs();
}

while (!m_viewNodeMap.empty())
{
    delete m_viewNodeMap.begin()->second;
}

m_nodeMap.clear();

if (m_jobDispatcher)
{
    auto jobDispatcher = reinterpret_cast<JobDispatcher*>(m_jobDispatcher);
    jobDispatcher->stop();
    delete jobDispatcher;
    m_jobDispatcher = nullptr;
}

pthread_mutex_destroy(&m_internalLock);
...

}

Separately, addWorkItem/removeWorkItem/waitWorkItem (IPGraph.cpp:3766-3792) should null-checnow prioritizeWorkItem is the only one that guards anything, and it guards id, not thedispatcher.

Fixed & double checked the use of the addition :D

[deltag0]

Other issues:

Other issues

1. std::unique_ptr for globalContextPool is a regression, not an improvement (MovieFFMpeg.cpp:449)

The old ContextPool* globalContextPool = 0; was an intentionally-leaked singleton — no destruction, no teardown ordering to reason about. Making it a unique_ptr adds a static destructor, and:

• ~unique_ptr calls delete ptr but does not null the stored pointer. So the if (!globalContextPool) return; guards in flushContext (:453), Reservation::Reservation (:481) and ~Reservation (:557) all still pass after the pool is destroyed, and every one of them then does LockGuard lock(gcp.m_mutex) on a destroyed boost::mutex.
• Static destruction order across the MovieFFMpeg plugin vs. any still-running decode thread or later-destroyed MovieFFMpegReader is unspecified.

Note that MovieFFMpegIO::~MovieFFMpegIO() (:5970) still carries the // XXX delete context pool ? comment — the ownership question is unresolved, and unique_ptr answers it with the riskiest option. I'd revert to the raw pointer, or keep the unique_ptr and explicitly globalContextPool.reset() at a controlled shutdown point so the guards actually work.

2. Re-enabled Reservation ctor can walk off an empty list (MovieFFMpeg.cpp:516-552)

Pre-existing code, but this PR is what makes it live again:

while (gcp.m_currentOpenThreads >= gcp.m_maxOpenThreads) { Context& closeContext = gcp.m_openContexts.back(); // UB if list is empty gcp.m_openContexts.pop_back(); ... if (closeContext.reserved) { cout << "ERROR: ..."; } // no decrement -> loop spins else if (closeContext.avContext) { / decrement, free */ } }

If enough contexts are simultaneously reserved that their combined thread_count reaches m_maxOpenThreads, the loop pops entries without ever decrementing the counter, drains m_openContexts, and then calls .back() on an empty list. With the default 500-thread budget and typical thread_count of 8-16 that needs ~30-60 concurrently-reserved contexts — reachable under heavy multi-source playback, which is precisely the workload this PR targets. The XXX Should never happen comment at :526 is the author of the original code flagging the same doubt.

Minimum fix: if (gcp.m_openContexts.empty()) break; at the top of the loop body.

3. Deleting the FFmpeg 6.0 rationale comment

The removed block (:5947 in the base) is institutional knowledge — it records why the pool was disabled and cites the FFmpeg docs. Since the PR resolves that concern rather than refuting it, replace it rather than delete it:

// The global context pool caps the number of simultaneously-open decoder // threads. It was disabled during the FFmpeg 6.0 migration because a context // cannot be reopened after being closed // (https://ffmpeg.org/doxygen/trunk/deprecated.html). That is now handled: // when the pool evicts a context it frees it and nulls the owning track's // avCodecContext, so openAVCodec() allocates a fresh context on next use.

4. Reservation::~Reservation writes through a potentially-freed pointer (MovieFFMpeg.cpp:564-574)

Context& context = *m_context; context.reserved = false; // write happens first ContextKey key(context.reader, context.streamIndex); // reads from the same object if (gcp.m_contextMap.find(key) == gcp.m_contextMap.end()) return; // ...check happens after

flushContext() erases the map node (:473), which destroys the Context. If that races with a live Reservation, the dtor writes to and reads from freed memory before the liveness check that was clearly meant to prevent exactly this. The check is unsalvageable as written since it derives the key from the dead object — the Reservation should cache reader/streamIndex at construction and look up by that.

I traced the two flushContext call sites in this PR and neither currently races: the Reservation at :2450 is scoped to its case block and destroyed before initializeAudio() runs at :2550, and close() shouldn't overlap a decode on the same reader. So this is latent, not active — but it's fragile enough to be worth a comment or a proper fix while the pool is being brought back.

5. Style — magic number (MovieFFMpeg.cpp:5956)

int poolSize = 500; — per the project's C++ conventions, non-obvious literals should be named:

constexpr int kDefaultContextPoolSize = 500;

Also worth documenting the units in a comment: it's a count of decoder threads, not contexts, which isn't obvious from the name.

Addressed 1,2,3,4

I think 5 is ok

[deltag0]

There's a comment saying something "should never happen" should we try to make sure it never happens, or leave it as is?

[bernie-laberge]

The tag is optional so we should not check its value here.
It is the m_jobDispatcher that we should check here to be safe.

[deltag0]

fixed

[bernie-laberge]

It is the m_jobDispatcher that we should check here to be extra safe, not the id

[deltag0]

fixed

[bernie-laberge]

It is the m_jobDispatcher that we should check here to be extra safe, not the id

[deltag0]

fixed

[bernie-laberge]

This should not compile anymore due to your recent change.

[deltag0]

fixed