-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Fix: Safe camera detection for macOS (M1/M2) to prevent OpenCV AVX/th… #1236
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
base: main
Are you sure you want to change the base?
Fix: Safe camera detection for macOS (M1/M2) to prevent OpenCV AVX/th… #1236
Conversation
Reviewer's GuideThis pull request modifies the File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @brynwhyman - I've reviewed your changes - here's some feedback:
- Camera detection on Linux is now limited to index 0; consider retaining multi-index probing for Linux if the original crash was macOS-specific.
- The OpenCV fallback for Windows camera detection (when
pygrabber
finds no devices) appears to have been removed; please clarify if this was intentional.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
def get_available_cameras(): | ||
"""Returns a list of available camera names and indices.""" | ||
""" | ||
Safe camera detection for macOS and Unix-like systems that avoids threading and AVX crashes. | ||
Returns a tuple of (camera_indices, camera_names). | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Clarify behavior differences across platforms in the docstring.
Also note that on Windows the function enumerates devices via DirectShow; add this to the docstring to distinguish platform strategies.
def get_available_cameras(): | |
"""Returns a list of available camera names and indices.""" | |
""" | |
Safe camera detection for macOS and Unix-like systems that avoids threading and AVX crashes. | |
Returns a tuple of (camera_indices, camera_names). | |
""" | |
def get_available_cameras(): | |
""" | |
Safe camera detection that avoids threading and AVX crashes. | |
On macOS and Unix-like systems, it utilizes a safe detection mechanism to list available cameras. | |
On Windows, devices are enumerated via DirectShow. | |
Returns a tuple of (camera_indices, camera_names). | |
""" |
return camera_indices, camera_names | ||
# macOS or Linux | ||
try: | ||
print("[Info] Safely checking for available cameras...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Use a logging framework instead of print statements.
A logging framework provides configurable levels and outputs, improving maintainability and configurability of debug and error messages in production.
Suggested implementation:
import logging
# (other imports below)
logging.error(f"[Camera Detection Error - Windows]: {e}")
logging.info("Safely checking for available cameras...")
logging.warning("Default camera (index 0) not available.")
logging.error(f"[Camera Detection Error - Unix]: {e}")
Please ensure that the logging configuration (level, formatting, etc.) is set up in your application so that these logging calls behave as expected. If there is an existing logging setup in your code base, this configuration might already be in place.
Likely solves this issue too: #1166 |
Should I make the changes suggested by the bot? |
tryed on M4 and seem that didn't work. Seems the camera is loading but the UI doesn't show anything Log: Version apple: 15.5 Solved: Just use python 3.10.16 Now I have the fps problems |
@arturo-lr-dev thanks for testing.
Is this something that exists when trying to install from the main branch too? Wondering if it's an issue with this PR or not |
@brynwhyman it happens with both branches, i think is not a problem of this branch |
This PR addresses a crash I had to work around when launching on Apple Silicon (M1/M2/M3). The issue stemmed from OpenCV's threaded camera enumeration causing a fatal error due to AVX instruction mismatches or GIL violations.
get_available_cameras()
logic with a safe fallback for macOS that avoids multithreaded access and AVX instruction usage.pygrabber
) for full cross-platform compatibility.Tested on:
Let me know if you'd like help extending this further. Thanks!
Summary by Sourcery
Implement a safe camera detection mechanism for macOS and Unix-like systems to prevent crashes related to OpenCV threading and AVX instructions
New Features:
Bug Fixes:
Enhancements: