Skip to content

Add CameraServer feeds_updated signal, and document async behavior#108165

Merged
Repiteo merged 1 commit intogodotengine:masterfrom
shiena:feature/improve-camera-server
Aug 8, 2025
Merged

Add CameraServer feeds_updated signal, and document async behavior#108165
Repiteo merged 1 commit intogodotengine:masterfrom
shiena:feature/improve-camera-server

Conversation

@shiena
Copy link
Copy Markdown
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
Comment thread doc/classes/CameraServer.xml Outdated
@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
Copy Markdown
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)

Comment thread doc/classes/CameraServer.xml Outdated
Comment thread doc/classes/CameraServer.xml Outdated
@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 2 times, most recently from 72aee69 to f905263 Compare July 3, 2025 13:01
Comment thread servers/camera_server.cpp Outdated
@shiena shiena force-pushed the feature/improve-camera-server branch 2 times, most recently from 733ff30 to 28431d0 Compare July 3, 2025 14:08
Comment thread modules/camera/camera_macos.mm Outdated
Comment thread servers/camera_server.h Outdated
Comment thread modules/camera/camera_linux.cpp Outdated
Copy link
Copy Markdown
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.

Comment thread modules/camera/camera_linux.cpp Outdated
Comment thread modules/camera/camera_linux.cpp Outdated
@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
Copy Markdown
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.

Comment thread doc/classes/CameraServer.xml Outdated
Comment thread doc/classes/CameraServer.xml Outdated
@shiena shiena force-pushed the feature/improve-camera-server branch from 62e4dfc to 3a3f3c9 Compare July 9, 2025 18:54
@shiena
Copy link
Copy Markdown
Contributor Author

shiena commented Jul 10, 2025

There are two issues with this camera application:

  • The CameraLinux::_update_devices function automatically updates the CameraFeed list, causing unnecessary background processing.
  • When CameraLinux::set_monitoring_feeds is called with true, false, true in sequence, the CameraFeed list is not updated from the second call onwards.
@onready var reload = $ReloadButton
var current_feed: CameraFeed

func _ready():
	reload.pressed.connect(_on_pressed_reload)

func _on_pressed_reload():
	if CameraServer.monitoring_feeds:
		CameraServer.monitoring_feeds = false
		await get_tree().process_frame
	if not CameraServer.feeds_updated.is_connected(_on_feeds_updated):
		CameraServer.feeds_updated.connect(_on_feeds_updated)
	CameraServer.monitoring_feeds = true

func _on_feeds_updated():
	var feeds = CameraServer.feeds()
	if not feeds.is_empty():
		var current_feed = feeds[0]
		current_feed.frame_changed.connect(_on_frame_changed, ConnectFlag.CONNECT_DEFERRED | ConnectFlag.CONNECT_ONE_SHOT)

func _on_frame_changed():
	// processing camera images
	pass

Therefore, we want to update the CameraFeed list only when CameraLinux::set_monitoring_feeds is set to true, and clear the CameraFeed list when set to false. Is this approach acceptable?

@Ivorforce
Copy link
Copy Markdown
Member

  • The CameraLinux::_update_devices function automatically updates the CameraFeed list, causing unnecessary background processing.

This was already the case before the PR. I think it's out of scope to fix this (unless there's an easy solution).

  • When CameraLinux::set_monitoring_feeds is called with true, false, true in sequence, the CameraFeed list is not updated from the second call onwards.

This is bad, that should definitely be fixed.

Therefore, we want to update the CameraFeed list only when CameraLinux::set_monitoring_feeds is set to true, and clear the CameraFeed list when set to false. Is this approach acceptable?

That would remove the Linux platform's ability to auto-update camera feeds altogether. I don't think this is an acceptable regression. The above bug with the true, false, true sequence should be fixed with the current background-updated implementation.

@shiena
Copy link
Copy Markdown
Contributor Author

shiena commented Jul 10, 2025

Understood. This PR will not address the true, false, true sequence issue.

@Ivorforce
Copy link
Copy Markdown
Member

Ivorforce commented Jul 10, 2025

Understood. This PR will not address the true, false, true sequence issue.

If the issue exists on current master, this PR does not have to address it (though it would be good to). If you don't intend to, could you open an issue for it?

@shiena
Copy link
Copy Markdown
Contributor Author

shiena commented Jul 14, 2025

I have created an issue and a pull request here.

#108582
#108584

@shiena shiena force-pushed the feature/improve-camera-server branch 2 times, most recently from 7051485 to 103f27a Compare July 17, 2025 06:01
Comment thread servers/camera_server.h Outdated
Comment thread modules/camera/camera_android.cpp Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the signal also be fired when all the feeds are removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

cameraIds is a temporary variable within this function to receive camera IDs from ACameraManager_getCameraIdList(). ACameraManager_deleteCameraIdList() releases that variable. Since we updated the camera feed before that point, we need to emit the feeds_updated signal.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure if this answered @m4gr3d's question

@shiena shiena force-pushed the feature/improve-camera-server branch 2 times, most recently from 0085b5d to 759bcaa Compare August 7, 2025 10:39
Comment thread servers/camera_server.h Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also meant to update the string value to camera_feeds_updated since in code a user would do:

CameraServer.camera_feed_added.connect(...)
CameraServer.camera_feed_removed.connect(...)
CameraServer.camera_feeds_updated.connect(...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed the signal name. Also reverted the variable name back to the original.

@shiena shiena force-pushed the feature/improve-camera-server branch 4 times, most recently from 274fa61 to f016980 Compare August 7, 2025 16:02
Copy link
Copy Markdown
Contributor

@m4gr3d m4gr3d 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!

@shiena shiena force-pushed the feature/improve-camera-server branch from f016980 to 2560ddb Compare August 7, 2025 18:54
Comment thread modules/camera/camera_android.cpp Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure if this answered @m4gr3d's question

@Ivorforce
Copy link
Copy Markdown
Member

Ivorforce commented Aug 7, 2025

I had another look, changes look sensible to me. Good work!

To release managers: While this is a new feature, its addition facilitates the new CameraServer workflow (where feed monitoring has to be enabled manually). So I'd argue this is 4.5 material since it's addressing an issue with a 4.5 change.

@Repiteo Repiteo merged commit 7f96fc5 into godotengine:master Aug 8, 2025
20 checks passed
@Repiteo
Copy link
Copy Markdown
Contributor

Repiteo commented Aug 8, 2025

Thanks!

@shiena shiena deleted the feature/improve-camera-server branch August 8, 2025 18:15
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.

6 participants