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

Added autodetection of camera #89

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

brilam
Copy link
Contributor

@brilam brilam commented Oct 12, 2021

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.

* Default device will be device ID 0
* Settings will list out all working device IDs. It is up to the user to
  select this
@Trisanu-007 Trisanu-007 requested a review from Davidy22 October 12, 2021 05:04
Copy link
Owner

@Davidy22 Davidy22 left a comment

Choose a reason for hiding this comment

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

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.

@brilam
Copy link
Contributor Author

brilam commented Oct 12, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

brilam commented Oct 13, 2021

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 brilam requested a review from Davidy22 October 14, 2021 21:29
Copy link
Owner

@Davidy22 Davidy22 left a comment

Choose a reason for hiding this comment

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

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.

@brilam
Copy link
Contributor Author

brilam commented Oct 15, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

brilam commented Oct 16, 2021

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants