fix: reset colorFrameThread_ on stream disable so re-enable can recreate it#95
Open
mk37972 wants to merge 1 commit into
Open
fix: reset colorFrameThread_ on stream disable so re-enable can recreate it#95mk37972 wants to merge 1 commit into
mk37972 wants to merge 1 commit into
Conversation
…ate it
colorFrameThread_, leftColorFrameThread_, and rightColorFrameThread_ are
std::shared_ptr<std::thread> members guarded at creation time by:
if (!colorFrameThread_ && enable_stream_[COLOR]) {
colorFrameThread_ = std::make_shared<std::thread>(
[this]() { onNewColorFrameCallback(); });
}
Their worker functions loop on enable_stream_[COLOR] / COLOR_LEFT /
COLOR_RIGHT and exit when the flag goes false. However, the shared_ptr
is never reset after the thread exits, so on the next startStreams()
the "!colorFrameThread_" guard is permanently false (pointer is
non-null, just holds a finished thread) and the thread is never
recreated. After one toggle_<color_stream> false -> true cycle, color
frame processing is permanently stopped.
This patch fixes the lifecycle in two parts:
1. Wait predicate in onNewColorFrameCallback (and Left / Right siblings)
now wakes on enable_stream_[stream] going false, so a worker thread
parked in colorFrameCV_.wait() can exit promptly when toggle disables
its stream. Without this, notify_all() alone cannot free the thread
because the existing predicate only watches the queue and
is_running_; the worker would simply go back to wait.
2. toggleSensor(), after flipping enable_stream_[stream_index] to false,
notify_all()s the matching CV, join()s the worker, and reset()s the
shared_ptr. The next startStreams() guard then evaluates true and
recreates the worker normally.
The shutdown path through clean() / dtor is unaffected: is_running_
goes false there, both the existing and new predicate disjuncts wake
the threads, and clean() joins them as before.
The same structural bug exists in OrbbecSDK_ROS2's color frame thread.
The same shape of fix would apply there.
2aa0c42 to
e20cbf8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
colorFrameThread_,leftColorFrameThread_, andrightColorFrameThread_arestd::shared_ptr<std::thread>members. They are created instartStreams()(andstartStream()) under the guard:Their worker functions loop on
enable_stream_[COLOR] / COLOR_LEFT / COLOR_RIGHTand exit when the flag goes false. However, theshared_ptris never reset after the thread exits, so on the nextstartStreams()the!colorFrameThread_guard is permanently false (pointer is non-null, just holds a finished thread) and the worker is never recreated. After one/{camera}/toggle_color false -> truecycle (or similarly for left/right color), color frame processing is permanently stopped for that camera until the node is restarted.The same lifecycle exists in v2.7.6 through v2.8.7 unchanged. The structural bug is present in
OrbbecSDK_ROS2's color frame thread as well.Reproduction
The pipeline reports as restarted and
enable_stream_[COLOR] == true, butonNewFrameSetCallback()pushes color frames intocolorFrameQueue_that no worker is draining, because the original worker exited and a replacement was never created.Fix
Two-part patch:
1.
src/ob_camera_node.cpp: let the worker wake on stream disableThe existing wait predicate watches only the queue and
is_running_:colorFrameCV_.wait(lock, [this]() { return !colorFrameQueue_.empty() || !(is_running_.load()); });A
notify_all()call alone cannot wake the worker into exiting during a toggle: the predicate evaluates false (queue empty,is_running_still true) and the worker goes straight back to wait. Add the stream'senable_stream_[stream_index]flag to the predicate so the worker can observe the disable and break out of the loop. Same change applied toonNewLeftColorFrameCallbackandonNewRightColorFrameCallback.2.
src/ros_service.cpp:toggleSensorjoins + resets the shared_ptr on disableAfter
enable_stream_[stream_index] = enabled;and beforestartStreams();, when disabling a color stream:notify_all()the matching CV,join()the worker, thenreset()theshared_ptr. The subsequentstartStreams()guard then evaluates true and recreates the worker normally on re-enable.Total: 2 files, +44 / -9 lines. No public API or behavioral change for callers that never disable streams.
Shutdown path (unchanged)
The
clean()/ destructor path usesis_running_.store(false)to terminate the workers, which the existing predicate disjunct already handles. The patch adds a new disjunct alongside it, not replacing it, so shutdown ordering remains identical.Notes
enable_stream_[stream_index]while holdingcolorFrameMtx_, but the write happens underdevice_lock_intoggleSensor. A data race on a non-atomic bool, benign on x86 (byte read/write atomic). For full correctness,enable_stream_could be changed tostd::array<std::atomic_bool, ...>. That is a separate type change worth doing but not strictly necessary for this fix.OrbbecSDK_ROS2if you'd like a follow-up PR there.Happy to iterate on style or split into smaller commits if useful.