Skip to content
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

Add Linux camera support #53666

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Add Linux camera support #53666

merged 1 commit into from
Sep 25, 2024

Conversation

pkowal1982
Copy link
Contributor

@pkowal1982 pkowal1982 commented Oct 11, 2021

I'm implementing camera support for Linux. Yes, I've seen this one.

It supports only cameras with V4L2_CAP_STREAMING capability as I had access only to such devices.

As I need some volunteers to review my PR or even better - test it - I've written a simple game which uses camera input for tracking head movement. Tracking is very simple and far from perfect but working.

Screenshot of a running game

You can download the Deep Space Immersion game from here.

Also there's simple Godot project for checking available cameras and video streams here.

I've extended CameraFeed interface a bit just to make the feed format changes from Godot possible.
Reviews, tests, comments really appreciated. :)

Short YouTube preview of gameplay:

Short game preview

@Schmetzler
Copy link

So you did it without libv4l2 which I suppose is good. But maybe it would be feasible to implement the possibility to use libv4l2 when it is installed to support more webcams. I did this by using an additional function structure (v4l2_funcs) which contains the standard v4l2 functions, which can be substituted with the libv4l2 ones.

I did it like this:

// camera_x11.h
struct v4l2_funcs {
	int (*open)(const char *file, int oflag, ...);
	int (*close)(int fd);
	int (*dup)(int fd);
	int (*ioctl)(int fd, unsigned long int request, ...);
	long int (*read)(int fd, void *buffer, size_t n);
	void *(*mmap)(void *start, size_t length, int prot, int flags, int fd, int64_t offset);
	int (*munmap)(void *_start, size_t length);
	bool libv4l2;
};

class V4l2_Device {
private:
	...
	// the v4l2 functions (either libv4l2 or normal v4l2)
	struct v4l2_funcs *funcs;
	...
};

class CameraX11 : public CameraServer {
private:
	struct v4l2_funcs funcs;
	...
};
// camera_x11.cpp
CameraX11::CameraX11() {
	// determine if libv4l2 is installed and
	// set functions appropriately
	libv4l2 = dlopen("libv4l2.so.0", RTLD_NOW);

	if (libv4l2 == NULL) {
		// the default v4l2 functions
		this->funcs.open = &open;
		this->funcs.close = &close;
		this->funcs.dup = &dup;
		this->funcs.ioctl = &ioctl;
		this->funcs.read = &read;
		this->funcs.mmap = &mmap;
		this->funcs.munmap = &munmap;
		this->funcs.libv4l2 = false;
#ifdef DEBUG_ENABLED
		print_line("libv4l2.so not found. Try standard v4l2 instead.");
#endif
	} else {
		// the libv4l2 functions
		this->funcs.open = (int (*)(const char *, int, ...))dlsym(libv4l2, "v4l2_open");
		this->funcs.close = (int (*)(int))dlsym(libv4l2, "v4l2_close");
		this->funcs.dup = (int (*)(int))dlsym(libv4l2, "v4l2_dup");
		this->funcs.ioctl = (int (*)(int, unsigned long int, ...))dlsym(libv4l2, "v4l2_ioctl");
		this->funcs.read = (long int (*)(int, void *, size_t))dlsym(libv4l2, "v4l2_read");
		this->funcs.mmap = (void *(*)(void *, size_t, int, int, int, int64_t))dlsym(libv4l2, "v4l2_mmap");
		this->funcs.munmap = (int (*)(void *, size_t))dlsym(libv4l2, "v4l2_munmap");
		this->funcs.libv4l2 = true;
#ifdef DEBUG_ENABLED
		print_line("libv4l2 found.");
#endif
	}

And after that I can pass the function structure to the V4l2_Device Object and use them as the normal v4l2 functions. In this way it would use libv4l2 if installed and if not the standard v4l2.

@pkowal1982
Copy link
Contributor Author

Thanks for feedback @Schmetzler.

I think the best way to go is to provide minimal working solution with complete interface (I've added methods for feed format manipulation, still some parameters should be discussed).

I'm not sure how many cameras work with standard v4l2 and how many more libv4l2 will add to this number. Maybe implementing libv4l2 is not worth at all? Less code is better and cameras are cheap, you can always get something working. Or maybe it should provide only libv4l2?

I've tested this code on bunch of different cameras, all of them supporting V4L2_CAP_STREAMING and single planar. I'm not sure if there's need for supporting other modes, I'm trying to get some volunteers to test the solution and report some feedback. Maybe I'll get someone with different setup. Or maybe nowadays all cameras will do it as mine? We'll see.

As to struct filled with function pointers, what do you think of refactoring it to class hierarchy, like V4l2Camera and Libv4l2Camera? Probably it would be more readable.

@Schmetzler
Copy link

Schmetzler commented Oct 12, 2021

I'm not sure how many cameras work with standard v4l2 and how many more libv4l2 will add to this number. Maybe implementing libv4l2 is not worth at all? Less code is better and cameras are cheap, you can always get something working. Or maybe it should provide only libv4l2?

I guess in the end you can make every camera work with only v4l2, but it would mean that every possible mode a camera may have should be implemented (I mean the color modes YUYV and so on). Libv4l2 does this transformation to RGB so you always can access at least an RGB image from the camera (as I am not firm with the other color modes I implemented the libv4l2 approach), but made it the way that it can theoretically function without that extra library. So in the end it could even safe some complexity.

I also only had a camera with STREAMING, but implemented the other modes either way, as I found a tutorial how to access the images in those modes.

Quick reminder I was the one that created the PR #47967

As to struct filled with function pointers, what do you think of refactoring it to class hierarchy, like V4l2Camera and Libv4l2Camera? Probably it would be more readable.

That maybe viable. But I am not sure if this would add more complexity than necessary.

@pkowal1982
Copy link
Contributor Author

v4l2 lists many pixel modes but I think vast majority of cameras support YUYV or JPEG. At least I did not stumble upon camera missing one of these. I'm not sure if it's worth to complicate code and include libv4l2 support for hard to estimate increase in device coverage. Also modes other than streaming which can't be even properly tested by us.

Probably decoding YUYV to RGB, grayscale, separated planes or just copying it plus decoding JPEG to RGB is subset which will cover most of real life situations.

@pkowal1982 pkowal1982 force-pushed the camera branch 2 times, most recently from e916f25 to e56d66d Compare October 15, 2021 19:21
@pkowal1982
Copy link
Contributor Author

Added exported game to itch.io.

@chookity-pokk
Copy link

Any update on this?

@pkowal1982
Copy link
Contributor Author

Nope, but it takes time. If you want to speed it up please build this branch yourself or use image from github and test it on your setup then leave a comment if it was working for you or you had any problems.

@Ansraer
Copy link
Contributor

Ansraer commented Dec 17, 2021

Couldn't get the head tracking to work properly, but can confirm that every usb cam I had access to worked fine out of the box.
EDIT: linked issue #46531

@BastiaanOlij
Copy link
Contributor

BastiaanOlij commented May 7, 2024

Given how platform specific the code in this module is, I think we might want to do away with the module approach and actually move each platform's code in their respective platform folders. This isn't something to do in this PR though, we should either do it after, or before this is merged (and then rebased). WDYT @BastiaanOlij @bruvzg?

I definitely struggled with this part of it all when I originally wrote the server. I think some of it originally started in the platform folders.
I think it makes the most sense to have the code there instead of modules. I guess it was moved there so it could be compiled out of the solution?

(and indeed, a discussion and action point for a separate PR, this PR looks good once the requested changes are done)

@pkowal1982 pkowal1982 force-pushed the camera branch 3 times, most recently from 22f2247 to 14d3bcd Compare May 7, 2024 19:18
@pkowal1982
Copy link
Contributor Author

Ok, implemented next bunch of suggestions.

I would like to ask for your opinion on set_format method.
What it does is setting one of predefined camera formats (resolution/framerate/compression) and in case of yuyv data also allows to set the output format - yuyv, separated, grayscale or rgb.
Setting output format is done by passing dictionary with key output having predefined value.
It's ugly and probably this dictionary should be replaced by an enum.
I've made it this way 'cause I was not sure if there will be any other parameters needed and adding parameters in future will probably result in braking api. Dictionary allows to pass different set of data without breaking api.
Now it's good time to decide what this method's signature should look like.

Also current implementation allows only changing yuyv data, jpg based compression always results in rgb.
I've made it this way because I think majority of users will want to use rgb and avoid writing shader to convert yuyv to rgb.
Is there any sense in adding convertion from rgb to other formats?

@BastiaanOlij
Copy link
Contributor

@pkowal1982 I think I get the zest of what you're doing with the format setting. I'm still a little worried that there is too much implementation in the base class without any of the other operating systems doing something with this. That makes it difficult to create functionality that works on multiple platforms. But I also have to admit that I lack hands on experience to really have a good opinion on how this works or should work.

Other than that issue, I think this looks really really good and I'm for merging it as is. We might want to make the format stuff experimental and get more feedback by users.

I agree that if we can base the work on RGB we should. I was always concerned about the overhead impact of converting yuyv to RGB on the CPU hence going for the shader solution, but it puts more pressure on the user. This may be a discussion for later and see if we can do this for any camera feed, especially if we make good use of threads and accept the overhead of the CPU based conversion.

@pkowal1982 pkowal1982 force-pushed the camera branch 2 times, most recently from 0dd0a13 to ccfd27d Compare July 2, 2024 07:04
@bezark

This comment was marked as off-topic.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

LGTM!

@akien-mga
Copy link
Member

I tried merging this locally, and I get this error when opening any project on Linux (Fedora 40 on Wayland):

ERROR: Cannot set format, error: 16.
   at: set_format (modules/camera/camera_feed_linux.cpp:328)

@pkowal1982 pkowal1982 force-pushed the camera branch 3 times, most recently from 108a6ee to e346bc8 Compare September 5, 2024 21:05
@@ -47,6 +47,10 @@ void CameraTexture::_bind_methods() {
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "camera_is_active"), "set_camera_active", "get_camera_active");
}

void CameraTexture::_on_format_changed() {
callable_mp((Resource *)this, &Resource::emit_changed).call_deferred();
Copy link
Member

@akien-mga akien-mga Sep 12, 2024

Choose a reason for hiding this comment

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

emit_changed alreaady handles deferring if it needs. Do you still need to explicitly defer it again here? This might lead to double deferring and two frames of delay.

call_deferred should only be used when really necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Skipping call_deferred results in some errors so I'm doing it wrong or this is the place where call_deferred is really necessary.
errors

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
callable_mp((Resource *)this, &Resource::emit_changed).call_deferred();
# FIXME: `emit_changed` is more appropriate, but causes errors for some reason.
callable_mp((Resource *)this, &Resource::emit_changed).call_deferred();

Copy link
Member

Choose a reason for hiding this comment

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

For the record, just reposting some comments from @RandomShaper and @smix8 after I asked maintainers about how to do this properly.

@RandomShaper
Resource::emit_changed() can only work from the main thread or a resource loading thread.
In the latter case, it employs some deferring magic to keep things fine on the main thread.
It won't handle being called from an arbitrarty thread, which seems to be the case here.
I'd like to know why it is being called from another thread in the first place.

@smix8
I think the problem in that PR is more that the CameraServer.add_feed() function, that should be called on the main thread, is called directly from the thread function that this PR adds. The base CameraServer also emits a changed signals with that function but without any deferred so that entire function is clearly designed to be only called on the main thread.
Using the call_deferred() is always a sloppy fix, because as the PR maker already noted, you sometimes need to do it twice because something else might flush the queue before while whatever uses your signal connected node or resource might still be locked. It is not running just once each frame.
imo the CameraServer should just sync its own updates with the main thread from its own queue, piggybacking on the main message queue with signals is always a valley of tears because that queue gets flushed at random points in the physics, scenetree, and main loop and maybe even in some other cursed places as well. It is totally overloaded with ugly hacks by now.

@RandomShaper
SceneTree's process_frame signal may be a suitable point to hook to.

@smix8
or do as e.g. the XRServer and just sync/process before all the script loop so things are updated before user scripts try to work with it.

This PR has been waiting long enough so I don't think this should be blocking now, but that's some food for thought and maybe further rework of the implementation to avoid future issues.

@LifeCANvs
Copy link

So does this mean we will have camera support for Godot 4.4 Linux projects? Very exciting!!! And then I can finally proceed with my ideas for a new game using webcam :)

How about Windows camera support? Will that also be available with Godot 4.4?

@Calinou
Copy link
Member

Calinou commented Sep 16, 2024

How about Windows camera support?

See #49763. There is a version of that PR rebased against master posted in the comments, but it didn't work when I tested it (and it uses a significantly different interface to access cameras).

Will that also be available with Godot 4.4?

We don't have an ETA for merging the Windows PR, since it's not in a mergeable state yet.

@LifeCANvs
Copy link

How about Windows camera support?

See #49763. There is a version of that PR rebased against master posted in the comments, but it didn't work when I tested it (and it uses a significantly different interface to access cameras).

Will that also be available with Godot 4.4?

We don't have an ETA for merging the Windows PR, since it's not in a mergeable state yet.

@Calinou thanks for taking the time to respond and explain to me 😊 and thanks to you and all other devs working on constantly improving Godot! I hope we can have fully working camera support for all Godot supported platforms soon... Wondering if this might help for the Windows support : https://github.com/microsoft/Windows-Camera

And how about support for USB camera devices using UVC?

(off topic: I'm also always looking out for your valuable contributions on Minetest! Thanks also for that)

@akien-mga akien-mga merged commit 26340e0 into godotengine:master Sep 25, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks! 🎉

Apologies for the long review process, but this is great work!

@akien-mga
Copy link
Member

akien-mga commented Sep 25, 2024

I tried merging this locally, and I get this error when opening any project on Linux (Fedora 40 on Wayland):

ERROR: Cannot set format, error: 16.
   at: set_format (modules/camera/camera_feed_linux.cpp:328)

I assumed this has been fixed, but it doesn't seem to be, I get this now any time I open a project.

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.