Skip to content

Add CameraServer feeds_updated signal, and document async behavior #108165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

shiena
Copy link
Contributor

@shiena shiena commented Jul 1, 2025

fixed: #108136

  • Add SafeFlag to prevent concurrent camera thread initialization
  • Set monitoring_feeds to true only after device enumeration completes
  • Document that monitoring_feeds requires waiting a few frames
  • Add code examples for GDScript and C# showing proper usage pattern

This ensures camera feeds are properly initialized before being accessed and prevents potential crashes from concurrent thread creation.

@shiena shiena requested review from a team as code owners July 1, 2025 13:56
@shiena shiena force-pushed the feature/improve-camera-server branch from eba8332 to 36446f6 Compare July 1, 2025 14:02
@AThousandShips AThousandShips added this to the 4.5 milestone Jul 1, 2025
@shiena shiena force-pushed the feature/improve-camera-server branch 2 times, most recently from a5aa0f3 to 3792213 Compare July 1, 2025 17:26
Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rework the API instead of documenting workarounds. (again, not directed to you @shiena)

@adamscott adamscott changed the title CameraServer: Fix monitoring_feeds race condition and document async behavior Fix CameraServer.monitoring_feeds race condition and document async behavior Jul 1, 2025
@shiena shiena force-pushed the feature/improve-camera-server branch from 3792213 to 72aee69 Compare July 1, 2025 19:05
shiena added 2 commits July 3, 2025 21:10
…behavior

- Add SafeFlag to prevent concurrent camera thread initialization
- Set monitoring_feeds to true only after device enumeration completes
- Document that monitoring_feeds requires waiting a few frames
- Add code examples for GDScript and C# showing proper usage pattern

This ensures camera feeds are properly initialized before being accessed
and prevents potential crashes from concurrent thread creation.
@shiena shiena force-pushed the feature/improve-camera-server branch from 72aee69 to f905263 Compare July 3, 2025 13:01
@shiena shiena force-pushed the feature/improve-camera-server branch from f120205 to 5a6d1ea Compare July 3, 2025 13:36
@shiena shiena force-pushed the feature/improve-camera-server branch from 733ff30 to 28431d0 Compare July 3, 2025 14:08
@@ -169,12 +171,16 @@ inline void CameraLinux::set_monitoring_feeds(bool p_monitoring_feeds) {

CameraServer::set_monitoring_feeds(p_monitoring_feeds);
if (p_monitoring_feeds) {
camera_thread.start(CameraLinux::camera_thread_func, this);
if (!activating.is_set()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't fully thread safe. But I'm noticing on reviewing SafeFlag that it's improperly implemented, and cannot be made truly thread safe. For the moment, this is probably fine. I'll address my SafeFlag concerns some other time.

@Ivorforce Ivorforce changed the title Fix CameraServer.monitoring_feeds race condition and document async behavior Add CameraServer feeds_updated signal, and document async behavior Jul 3, 2025
Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my perspective now.
Thanks for doing all the adjustments!

This still needs a review from @godotengine/xr though.

@shiena shiena force-pushed the feature/improve-camera-server branch from 62e4dfc to 3a3f3c9 Compare July 9, 2025 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CameraServer on Linux in 4.5-beta1 fails to detect cameras.
4 participants