-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[camera] Add front camera mirroring update on iOS #4420
Conversation
@egemenzeytinci Please do not approve PRs. As our contribution documentation says, that is reserved for committers. You're welcome to leave review feedback, but using the approval flag only creates confusion for others following the progress of PRs. |
@stuartmorgan I would like to solve conflicts. may I ask, if there will be a review process? |
Can you explain what you are intending to accomplish here? I'm not sure what you mean by "updating mirroring". |
I have tried to revert the mirroring effect (currently videos are horizontally mirrored) on iOS when front camera is active. The video mirror enabled in CameraPlugin.m#L394-L396. The native iOS camera application fixes video mirroring when video recorded by front camera. For example, when I look to the left while video recording with front camera, the output also look to the left but iOS and also Android native application looks to the right. I believe this is the expected behavior but the camera plugin does not handle this so I have tried to the handle this. Could you please check example video files? example.zip |
What is the purpose of having one part of the plugin code enable mirroring only to manually undo that mirroring elsewhere in the same plugin? |
When we remove mirroring from CameraPlugin.m#L394-L396 the camera preview does not show the video correctly (which is video flipped horizontally in the preview and this is not expected behavior from camera) but output is quite right. I believe the maintainers had 2 options, showing the correct video in the preview ( |
I see, thanks for the explanation. Is there a compelling reason to do this in native code, rather than eliminating the mirroring on the native side and then flipping the preview in Dart? It seems like that would be far less code, and fix all platforms at once. (/cc @bparrishMines do you have any insight into the initial decision to flip in native code?) |
for the initial work, I didn't want to change the main logic so I only applied flip operation on native side. I have tried to do flip operation on Dart side in here camera_preview.dart#L47 by using Should I update the Dart code or keep it on native side @stuartmorgan? /cc @bparrishMines Tested on iPhone 11, iPhone 7 and Redmi 8A with camera plugin example application. The test code added in camera_preview.dart#L47, return Transform(
alignment: Alignment.center,
transform: Matrix4.rotationY(
controller.description.lensDirection == CameraLensDirection.front ? pi : 0,
),
child: child,
); |
@bparrishMines Any input on the question above? It seems like it would make much more sense to record unflipped and only flip the preview display itself, but I'm not sure if there's context for the original design that I'm missing. |
@trK54Ylmz You may also need to handle the fact that Transform rotates prior to painting and not prior to layout. Which could have some consequences in how the Widget is laid out in a full app. @stuartmorgan I believe the Dart solution provided by @trK54Ylmz would definitely be the quickest and easiest solution to this problem. But, I'll provide some context why that is below. The TLDR is that preview and video recording share the same capture output because it didn't require preview to use a The longer explanation is that preview, video Recording, and byte streaming all use the same AVCaptureVideoDataOutput because:
So assuming this is all still true and I didn't forget anything, we could swap out the preview frames to render to a |
We have the same issue on Android though, as I understand it, which is part of why I was suggesting a Dart-side mirroring solution. |
I only really know the context of the native side of the API. Most of the Dart code is relatively new to me. But it makes sense from my understanding of how the frames are obtained. I don't think Android had the same issue since mirroring is handled automatically by @mvanbeusekom Do you have thoughts on this solution since you worked on rotation issues for camera before? |
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
@bparrishMines @mvanbeusekom What's the status on deciding how we want to move forward here? |
I think @bparrishMines is correct and this issue only occurs on iOS. I have been studying the if ([_captureDevice position] == AVCaptureDevicePositionFront && connection.supportsVideoMirroring) {
connection.automaticallyAdjustsVideoMirroring = NO;
connection.videoMirrored = YES;
} I didn't have the time to fully test this, but it seems the suggested solution on the Apple developer forums. |
Why are people in flutter/flutter#27650 saying it's happening on Android as well though? |
There's also a mirroring issue on windows. Camera preview is mirrored (wrong), actual picture taken is not mirrored (correct). Tested both with an integrated laptop webcam and an external usb webcam |
That sounds like correct behavior to me; please file a new issue for discussion. |
@trK54Ylmz are you planning on creating an issue for further discussion? Implementation wise @mvanbeusekom's approach sounds very promising IMO. |
Thanks for your contribution! Since I haven't seen a reply, I'm going to reluctantly close this pull request for now, but if you get time to address the comments, we'd be more than happy to take a look at a new patch. If you do send a new patch, it would be very useful to reference this one in the description in order to help reviewers see the previous context. |
Any update on this? This is happening on Android too @jmagman @stuartmorgan. Flutter doctor
|
This PR is closed, the issue is tracked at flutter/flutter#27650. We had a big backlog of PRs, which is why this one took so long to be reviewed. We've cleared out most of that backlog and would happily review a PR similar to this one with the feedback addressed. |
This PR intents to update mirroring on front camera on iOS devices
flutter/flutter#91491
The current and updated videos, example.zip
Pre-launch Checklist
dart format
.)[shared_preferences]
///
).