GstEnginePipeline: Add fader mutex#2129
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds explicit synchronization around the GstEnginePipeline fader (QTimeLine) pointer to avoid concurrent access/destruction issues when the pipeline is manipulated across threads.
Changes:
- Introduces
mutex_fader_to guard reads/writes of thefader_shared pointer. - Refactors multiple call sites to copy
fader_under lock and operate on the local copy outside the lock. - Adds an explanatory comment about queued
SetStateAsyncSlotinvocation safety on object destruction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/engine/gstenginepipeline.h | Adds mutex_fader_ to synchronize access to the fader timeline pointer. |
| src/engine/gstenginepipeline.cpp | Uses mutex_fader_ to guard fader_ access across disconnect/init/state-change/fader lifecycle paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change synchronizes ChangesFader thread-safety refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/engine/gstenginepipeline.cpp (1)
2460-2465: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCustom deleter calls
stop()directly — potential cross-thread control.The deleter runs on whatever thread drops the last
SharedPtrreference. Callingtimeline->state()/timeline->stop()there can touch theQTimeLineoff its owning thread. SincedeleteLater()already defers destruction to the owning thread and~QTimeLinestops the timeline itself, the explicitstop()is redundant and is the only cross-thread-unsafe part; consider dropping it and relying ondeleteLater()alone.♻️ Proposed simplification
SharedPtr<QTimeLine> fader(new QTimeLine(static_cast<int>(duration_msec)), [](QTimeLine *timeline) { - if (timeline->state() != QTimeLine::State::NotRunning) { - timeline->stop(); - } timeline->deleteLater(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/gstenginepipeline.cpp` around lines 2460 - 2465, The custom deleter for QTimeLine in the fader setup is doing thread-unsafe control by calling state() and stop() from whichever thread releases the last SharedPtr. Update the deleter in the QTimeLine creation logic to remove the explicit stop() check entirely and rely on deleteLater() for cleanup, since QTimeLine will stop during destruction on its owning thread. Keep the fix localized to the fader SharedPtr/QTimeLine deleter so the timeline is only deferred for deletion without direct cross-thread manipulation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/engine/gstenginepipeline.cpp`:
- Around line 2460-2465: The custom deleter for QTimeLine in the fader setup is
doing thread-unsafe control by calling state() and stop() from whichever thread
releases the last SharedPtr. Update the deleter in the QTimeLine creation logic
to remove the explicit stop() check entirely and rely on deleteLater() for
cleanup, since QTimeLine will stop during destruction on its owning thread. Keep
the fix localized to the fader SharedPtr/QTimeLine deleter so the timeline is
only deferred for deletion without direct cross-thread manipulation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 184dfcca-695d-4c55-adcb-9229bd8d22cf
📒 Files selected for processing (2)
src/engine/gstenginepipeline.cppsrc/engine/gstenginepipeline.h
36a88c2 to
c98fce1
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/engine/gstenginepipeline.cpp:2589
- FaderTimelineStateChanged() drops mutex_fader_ before updating fader_running_. Another thread can replace fader_ in the gap, allowing a stale timeline signal to update fader_running_ for the new fade. Keep the update inside the same critical section used to validate sender().
{
QMutexLocker l(&mutex_fader_);
// ResumeFaderAsync() keeps a superseded QTimeLine alive with a SharedPtr until its queued resume() runs, so it can still emit stateChanged after StartFader() replaced fader_ with a new one; ignore it in that case.
if (!fader_ || sender() != &*fader_) {
return;
}
}
qLog(Debug) << "Pipeline" << id() << "fader state changed to" << (state == QTimeLine::State::Running ? "running" : state == QTimeLine::State::Paused ? "paused" : "not running");
fader_running_ = state == QTimeLine::State::Running;
| SharedPtr<QTimeLine> fader; | ||
| { | ||
| QMutexLocker l(&mutex_fader_); | ||
| fader = fader_; | ||
| } | ||
| if (volume_fading_ && fader) { | ||
| qLog(Debug) << "Pipeline" << id() << "setting volume" << (fader->direction() == QTimeLine::Direction::Forward ? 1.0 : 0.0); | ||
| g_object_set(G_OBJECT(volume_fading_), "volume", fader->direction() == QTimeLine::Direction::Forward ? 1.0 : 0.0, nullptr); | ||
| } | ||
|
|
||
| { | ||
| QMutexLocker l(&mutex_fader_); | ||
| // Only clear fader_ if it still refers to the fader captured above: StartFader() may have already replaced it with a newer one in the meantime. | ||
| if (fader_ == fader) { | ||
| fader_.reset(); | ||
| } | ||
| } | ||
|
|
||
| FaderTimelineFinished(); | ||
| qLog(Debug) << "Pipeline" << id() << "finished fading"; | ||
|
|
||
| ClearFaderState(); | ||
|
|
||
| } |
| // Stop these unconditionally: a fader timeout may already be pending in the event queue even if there's no live fader_ above (e.g. it fired concurrently and is about to run FaderTimelineTimeout()/ClearFaderState()), which would otherwise restart the fudge timer and emit FaderFinished for a pipeline that is being torn down. | ||
| timer_fader_timeout_->stop(); | ||
| timer_fader_fudge_->stop(); |
Summary by CodeRabbit