search

Davidy22 / SkunkBooth

Text based command line webcam photobooth app
https://Davidy22.github.io/SkunkBooth/
Other
50 stars 12 forks source link

Added autodetection of camera #89

Open brilam opened 2 years ago

brilam commented 2 years ago

Hi,

I've made the changes required to autodetect camera.

Closes:

84

Details: The default device will be device ID 0. Settings will list out all working device IDs. It is up to the user to select this if device ID 0 is incorrect.

Please let me know if you have any questions or have some feedback.

brilam commented 2 years ago

You need to run your code to make sure it works before submitting. This crashes immediately. Check your type hints, List is capitalized when used as a type hint.

Sorry. I am a little confused. It seems to pass the pre-commit check. How are you checking type hints?

Davidy22 commented 2 years ago

I just ran the code. Immediate subscript error trying to subscript list instead of List. Skimming through other type hints and all data structure type hints need to use square brackets, round brackets in type hints mean something different. See this cheat sheet for reference.

brilam commented 2 years ago

I just ran the code. Immediate subscript error trying to subscript list instead of List. Skimming through other type hints and all data structure type hints need to use square brackets, round brackets in type hints mean something different. See this cheat sheet for reference.

Weird. The code runs for me. But I will be sure to check that cheatsheet. Thanks.

brilam commented 2 years ago

Doesn't crash immediately anymore, so that's good. I'm testing this with a laptop camera and a USB webcam, and I'm seeing two issues right now.

For one, the camera currently in use is not displayed in the settings drop-down for camera selection, which in itself isn't a huge problem, but it does lead to a crash if I unplug the USB webcam because there's a crash "bug" in our current dropdown code if the number of options is 0. Either need to manually add the index of the camera in use, or find an alternate way to enumerate attached cameras. The current method looks like it fails if there are more than 10 cameras attached, which is probably unlikely but still an arbitrary limitation we'd like to avoid if possible.

Also, selecting the other camera does not actually switch cameras. Haven't dove into debugging this, but glancing over the code I do notice your code doesn't call close_camera() anywhere which might have something to do with it, haven't tested it though and you might find that the issue is elsewhere on investigation.

Thanks for having a looking at this PR. I can fix the issue where the camera being selected isn't being shown in settings. For the unplugging of a USB webcam, I am not really sure what you mean by manually add the index of the camera in use. Can you elaborate a bit more here? I am not really sure what you mean by manually adding the index here. As for why I added an arbitrary limit of 10 cameras, I've tried testing this locally and there seems to be very odd unexpected behavior. I only have webcam and it is a USB camera. If I don't set a pre-determined value, this method can run for quite a long time. Supposedly, I have a device ID of 701, which obviously doesn't make sense.

That's interesting. I thought it would work, but I wasn't entirely sure as I have only one camera to work with here. According to the code, I've two device IDs (0 and 1), 0 being the webcam, and 1 is just some non-working device. I tried toggling between the two. Switching to 1 shows no image, and switching to 0 works as fine. I suppose this needs more testing. Would you be able to assist in testing this fix after I've made the change?

Davidy22 commented 2 years ago

Linux has no blank virtual "camera," if I unplug my USB webcam, there will only be one camera listed. Or in this case, no cameras listed because of the current camera not being included bug. Since the list of cameras is empty because the current camera in use isn't detected, the crash bug happens. To stop this from happening, add the current camera to the list of cameras.

Is there no way to get just the list of cameras attached, instead of attempting to query a bunch of unknown device numbers? This approach seems inefficient, querying unspecified unknown devices seems like how security exploits happen as well.

If you don't have a second working webcam to test with, you can use OBS's virtual webcam to generate a second working camera to test with. I'll test with the OBS cam as well when you get your next draft in.

brilam commented 2 years ago

Linux has no blank virtual "camera," if I unplug my USB webcam, there will only be one camera listed. Or in this case, no cameras listed because of the current camera not being included bug. Since the list of cameras is empty because the current camera in use isn't detected, the crash bug happens. To stop this from happening, add the current camera to the list of cameras.

Is there no way to get just the list of cameras attached, instead of attempting to query a bunch of unknown device numbers? This approach seems inefficient, querying unspecified unknown devices seems like how security exploits happen as well.

If you don't have a second working webcam to test with, you can use OBS's virtual webcam to generate a second working camera to test with. I'll test with the OBS cam as well when you get your next draft in.

I definitely agree with you that testing these device numbers are seemingly bruteforce. From what I've searched, a lot of people mentioned similar complaints. And it still hasn't been addressed by OpenCV from what I can see in this StackOverflow post

Davidy22 commented 2 years ago

Hum, looks like opencv devs are advocating looking elsewhere for camera information on a quick google search so that's probably the way to go there, but more pressing right now is making camera switching actually work.