-
-
Notifications
You must be signed in to change notification settings - Fork 24.2k
Android: Handle YUV_420_888 strides correctly in CameraFeed
#110720
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
Conversation
ec35fe3 to
b00c8ba
Compare
|
After applying the patch, I run into problems that acquiring image from image reader does not succeed. |
b00c8ba to
355c6ed
Compare
Same here Test Results for the PatchHello! I have tested the new export templates, and this is a very positive update. Godot version1. The Original Bug is FIXEDYour patch was successful! The original 2. A New, Different Error Has AppearedWhile the original error is fixed, the screen is still black. A new, different error appeared 8 times in the debugger: AnalysisBased on the C++ source file ( Summary:
This is fantastic progress, and it proves we are on the right track. Please let me know if you have another patch you would like me to test. Thank you again for your hard work on this @shiena |
The previous problem seems to be fixed in 355c6ed, so it now works as expected, thanks! |
modules/camera/camera_android.cpp
Outdated
| packed_y.resize(width * height); | ||
| if (y_row_stride == width) { | ||
| // No padding, direct copy | ||
| memcpy(packed_y.ptrw(), data_y.ptr(), width * height); |
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.
We are copying the same data twice here, is there any chances that this can be improved?
355c6ed to
4136769
Compare
|
@j20001970 |
|
@MuslimDev-sys |
|
I do not have Android devices that provide full planar YUV format to test, but 4136769 works well for the case of interleaved UV plane. |
Test Results for the Final Patch: Success!Hello @shiena, I have tested the new build with your different approach, and it is a complete success. Godot version
1. The Main Bug is FIXEDYour patch was successful! The camera feed is now working and visible on my device. The black screen is gone, and all the previous C++ errors ( 2. Next Steps and Final ObservationsNow that the main issue is solved, I can interact with the feed and have observed two new behaviors. I am not sure if these are new engine bugs or issues with your GDScript logic, so I am reporting them here for your review.
SummaryThe fix for the critical blocking bug is complete and successful. You have solved the core engine issue, and the camera is now functional on my device. Thank you again for all your hard work and persistence on this! @shiena |
|
@MuslimDev-sys |
ac850f0 to
2db8a67
Compare
619d046 to
7a00ad0
Compare
9a7e698 to
0b372b3
Compare
bf121b7 to
c1b5f17
Compare
|
On On |
19527c6 to
31351ba
Compare
|
@Alex2782 @The-Cyber-Captain |
|
@The-Cyber-Captain |
a93341c to
2b1618b
Compare
|
@Alex2782 @The-Cyber-Captain |
Ahhhhh. I'm more a User than a Dev. 🤣 Filtering logcats is about the edge of my skillset / setup. But I gave it a shot and updated the issue with them. Only to see you have a new fix; you absolute machine! 👍 I'll give that new build a test as soon as able, and report back. Like I say, will test it out and feed back how it goes. Thanks. |
|
@The-Cyber-Captain |
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.
It looks great on both of my devices now!
I can't say much about the code (I'm not very familiar with it)
Except that you should avoid auto.
https://contributing.godotengine.org/en/latest/engine/guidelines/cpp_usage_guidelines.html#auto-keyword
However, a line of code containing auto was already accepted 8 months ago. (CameraFeedAndroid::onDisconnected)
74d6390 to
dd93d31
Compare
|
Thank you for the review. Fixed by replacing |
m4gr3d
left a comment
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.
The code looks good. Can you squash the commits.
dd93d31 to
9d32ca7
Compare
|
Squashed. |
modules/camera/camera_android.h
Outdated
| std::atomic<bool> is_deactivating{ false }; | ||
| std::atomic<bool> session_close_pending{ false }; | ||
| std::atomic<bool> device_error_occurred{ false }; |
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.
We have our own SafeFlag wrapper for std::atomic that should probably be used here.
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.
Fixed. Replaced std::atomic<bool> with Godot's SafeFlag wrapper.
|
I finally tested the PR, using the MRP provided in #110711 Camera now works and I can switch between front and back camera but feed is too zoomed in and also seems like a bit tilted. |
- Handle stride-aligned YUV planes from camera driver - Fix callback context to support multiple camera feeds - Synchronize camera session closure before device close - Prevent image buffer stall when switching feeds - Deactivate camera feeds before removal - Replace auto with explicit type in callbacks Fixes godotengine#110711 Fixes godotengine#114468
9d32ca7 to
07a9f91
Compare
|
@syntaxerror247 |
|
@shiena I think I've figured out the issue and it is related to rotation. The camera feed isn't rotating with the device orientation. When I set the device to landscape, everything looks correct. However, when I switch to portrait mode, the camera feed doesn't rotate. So the issue seems to be caused by rotation handling somewhere. |
|
@syntaxerror247 |
Tested with this demo, issue is resolved now! I was looking at that demo project for the last 30 min trying to fix the rotation, for my understanding, could you share how you fixed the rotation bug? Maybe just drop the commit link? |
|
@syntaxerror247 |
|
Thanks! |
Stride-aligned plane handling:
Camera feed synchronization fixes:
Make callback structures instance members instead of static to ensure correct
thispointer is captured for each camera feed instanceFix resource leaks for session outputs and camera output target
Synchronize camera session closure before device close using semaphore to prevent "Device is closed but session is not notified" warning
Fix image buffer stall when switching camera feeds by acquiring and discarding pending images when feed is not active
Add atomic flags (
is_deactivating,session_close_pending,device_error_occurred) for proper synchronization during deactivationDeactivate feeds in
update_feeds()andremove_all_feeds()before removing them to prevent resource leaksImproved Android camera preview rotation so that feed transforms stay in sync with both sensor orientation and the current display rotation, ensuring the correct angle (and mirroring) is applied for front and back cameras without redundant updates.Fix race condition in camera feed deactivation by adding mutex protection and correcting resource cleanup orderAdd Android lifecycle management for camera rotation and pause/resume eventsImplement horizontal mirroring for front camera to provide mirror-like experienceAdd comprehensive safety checks for metadata, formats, and array boundsImplement metadata refresh mechanism to handle configuration changesBugsquad edit: