Conversation
maerki
requested review from
NoahBussingerUbique,
bastianmorath,
Copilot,
matzf,
maurhofer-ubique and
zimmermannubique
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| std::lock_guard<std::recursive_mutex> lock(animationMutex); | ||
| long long animationDurationMs; | ||
| { | ||
| std::lock_guard<std::recursive_mutex> lock(paramMutex); | ||
| animationDurationMs = cameraZoomConfig.animationDurationMs; |
There was a problem hiding this comment.
The new animation-duration read in moveToCenterPositionZoom acquires animationMutex and then paramMutex, while other animated entry points such as setZoom (lines 293–300) and setRotation (lines 337–344) take paramMutex before animationMutex. If two threads call these functions concurrently, one can hold animationMutex while waiting for paramMutex and the other the reverse, producing a deadlock. Lock the mutexes in a consistent order or avoid the nested locking to keep concurrent calls safe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds thread safety protection for the cameraZoomConfig member by consistently using paramMutex locks when accessing it. This prevents data races that could occur when the camera configuration is updated concurrently with camera operations.
Key changes:
- Extracts
animationDurationMsfromcameraZoomConfiginto local variables under lock before creating animations - Adds locks to getter methods
getCameraConfig(),getCameraVerticalDisplacement(), andgetCameraPitch() - Protects an assert statement that reads interpolation values from the config
| std::lock_guard<std::recursive_mutex> lock(paramMutex); | ||
| return valueForZoom(cameraZoomConfig.verticalDisplacementInterpolationValues, zoom); | ||
| } | ||
|
|
||
| double MapCamera3d::getCameraPitch() { | ||
| std::lock_guard<std::recursive_mutex> lock(paramMutex); | ||
| return valueForZoom(cameraZoomConfig.pitchInterpolationValues, zoom); |
There was a problem hiding this comment.
The new lock added here creates unnecessary nested locking. The function updateZoom() already holds paramMutex (line 1818), but now getCameraVerticalDisplacement() and getCameraPitch() (called at lines 1820-1821) also try to acquire it.
While this is safe due to std::recursive_mutex, it's inefficient and indicates a design issue. Consider either:
- Making private helper versions of these functions that assume the lock is already held, OR
- Releasing the lock before calling these functions and re-acquiring it after
This would improve both performance and code clarity.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: maerki <6221466+maerki@users.noreply.github.com>
Co-authored-by: maerki <6221466+maerki@users.noreply.github.com>
[WIP] Address feedback on locking access to CameraZoomConfig
No description provided.