Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[camera] Add front camera mirroring update on iOS #4420

Closed
wants to merge 6 commits into from

Conversation

trK54Ylmz
Copy link

@trK54Ylmz trK54Ylmz commented Oct 9, 2021

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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

@google-cla google-cla bot added the cla: yes label Oct 9, 2021
@trK54Ylmz trK54Ylmz changed the title Add front camera mirroring update on iOS [camera] Add front camera mirroring update on iOS Oct 9, 2021
@stuartmorgan
Copy link
Contributor

@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.

@trK54Ylmz
Copy link
Author

@stuartmorgan I would like to solve conflicts. may I ask, if there will be a review process?

@stuartmorgan
Copy link
Contributor

Can you explain what you are intending to accomplish here? I'm not sure what you mean by "updating mirroring".

@trK54Ylmz
Copy link
Author

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
The archive file contains 2 video files, the first one is recorded with current implementation and the other one reverts mirroring effect means flips every scene horizontally.

@stuartmorgan
Copy link
Contributor

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.

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?

@trK54Ylmz
Copy link
Author

trK54Ylmz commented Oct 20, 2021

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.

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 (videoMirrored = YES) or saving the correct video for the output (videoMirrored = NO or removing the lines 394:396), I think camera preview has higher priority in this case. These changes are aims to provide correct behavior for the camera preview and the output.

@stuartmorgan
Copy link
Contributor

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?)

@trK54Ylmz
Copy link
Author

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 Matrix4 transformation, seems working properly.

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,
);

@stuartmorgan
Copy link
Contributor

@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.

@bparrishMines
Copy link
Contributor

bparrishMines commented Nov 16, 2021

@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 PlatformView (we currently render it to a Texture). And this implementation was written before PlatformViews existed.

The longer explanation is that preview, video Recording, and byte streaming all use the same AVCaptureVideoDataOutput because:

  1. An AVCaptureSession can only have one AVCaptureVideoDataOutput at a time. And since all three required this output, they had to share the output frames.
  2. Previews can be streamed to an AVCaptureVideoPreviewLayer. However, as far as I have been able to research, this would require using PlatformViews to show the frames. (e.g. with a UIView). And PlatformViews didn't exist during the implementation.
  3. Video recording could be done with AVCaptureMovieFileOutput. However, AVCaptureMovieFileOutput and AVCaptureVideoDataOutput can't be used with the same AVCaptureSession.
  4. Video recording also couldn't use AVCaptureMovieFileOutput because there is no support for pausing and resuming recording on iOS, only MacOS. And this feature was required by a major customer, so we can't really remove it either. (Side note: support for this on iOS required a complex solution I implemented with AVCaptureVideoDataOutput.

So assuming this is all still true and I didn't forget anything, we could swap out the preview frames to render to a UIView instead. Then the mirroring could be handled separately on the native side.

@stuartmorgan
Copy link
Contributor

So assuming this is all still true and I didn't forget anything, we could swap out the preview frames to render to a UIView instead. Then the mirroring could be handled separately on the native side.

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.

@bparrishMines
Copy link
Contributor

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 Camera2 and there is no option to change it. I think the issue with Android had to do with a rotation on the z-axis: flutter/flutter#76210

@mvanbeusekom Do you have thoughts on this solution since you worked on rotation issues for camera before?

@godofredoc godofredoc changed the base branch from master to main January 6, 2022 22:45
@flutter-dashboard
Copy link

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.

@stuartmorgan
Copy link
Contributor

@bparrishMines @mvanbeusekom What's the status on deciding how we want to move forward here?

@mvanbeusekom
Copy link
Contributor

I think @bparrishMines is correct and this issue only occurs on iOS.

I have been studying the AVFoundation API a bit and I had the impression we could resolve the mirroring problem using the AVCaptureConnection.videoMirrored() method. Something along the lines of (which is a small extension of the current code at CameraPlugin.m#L414-416):

  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.

@stuartmorgan
Copy link
Contributor

I think @bparrishMines is correct and this issue only occurs on iOS.

Why are people in flutter/flutter#27650 saying it's happening on Android as well though?

@jgoyvaerts
Copy link

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

@stuartmorgan
Copy link
Contributor

There's also a mirroring issue on windows. Camera preview is mirrored (wrong), actual picture taken is not mirrored (correct).

That sounds like correct behavior to me; please file a new issue for discussion.

@hellohuanlin
Copy link
Contributor

@trK54Ylmz are you planning on creating an issue for further discussion? Implementation wise @mvanbeusekom's approach sounds very promising IMO.

@jmagman
Copy link
Member

jmagman commented Apr 19, 2022

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.

@jmagman jmagman closed this Apr 19, 2022
@reiko-dev
Copy link

reiko-dev commented Jul 17, 2022

Any update on this? This is happening on Android too @jmagman @stuartmorgan.
Reversing the mirror is easy for images, but for videos we can't do much without making the user wait, wait and then Hate our app, before the reversed mirrored video comes in.

Flutter doctor

[√] Flutter (Channel stable, 3.0.4, on Microsoft Windows [versÆo 10.0.19044.1766], locale pt-BR)
• Flutter version 3.0.4 at C:\src\flutter
• Upstream repository https://github.com/flutter/flutter.git
• Framework revision 85684f9300 (2 weeks ago), 2022-06-30 13:22:47 -0700
• Engine revision 6ba2af10bb
• Dart version 2.17.5
• DevTools version 2.12.2

[√] Android toolchain - develop for Android devices (Android SDK version 31.0.0-rc1)
• Android SDK at C:\Users\lukas\AppData\Local\Android\sdk
• Platform android-33, build-tools 31.0.0-rc1
• Java binary at: C:\Program Files\Android\Android Studio1\jre\bin\java
• Java version OpenJDK Runtime Environment (build 11.0.12+7-b1504.28-7817840)
• All Android licenses accepted.

[√] Chrome - develop for the web
• Chrome at C:\Program Files\Google\Chrome\Application\chrome.exe

[√] Visual Studio - develop for Windows (Visual Studio Community 2019 16.10.1)
• Visual Studio at C:\Program Files (x86)\Microsoft Visual Studio\2019\Community
• Visual Studio Community 2019 version 16.10.31402.337
• Windows 10 SDK version 10.0.19041.0

[!] Android Studio (version 4.0)
• Android Studio at C:\Program Files\Android\Android Studio
• Flutter plugin can be installed from:
https://plugins.jetbrains.com/plugin/9212-flutter
• Dart plugin can be installed from:
https://plugins.jetbrains.com/plugin/6351-dart
X Unable to determine bundled Java version.
• Try updating or re-installing Android Studio.

[√] Android Studio (version 2021.2)
• Android Studio at C:\Program Files\Android\Android Studio1
• Flutter plugin can be installed from:
https://plugins.jetbrains.com/plugin/9212-flutter
• Dart plugin can be installed from:
https://plugins.jetbrains.com/plugin/6351-dart
• Java version OpenJDK Runtime Environment (build 11.0.12+7-b1504.28-7817840)

[√] VS Code (version 1.69.1)
• VS Code at C:\Users\lukas\AppData\Local\Programs\Microsoft VS Code
• Flutter extension version 3.44.0

[√] Connected device (4 available)
• SM G950F (mobile) • ce12171c638f281003 • android-arm64 • Android 9 (API 28)
• Windows (desktop) • windows • windows-x64 • Microsoft Windows [versÆo 10.0.19044.1766]
• Chrome (web) • chrome • web-javascript • Google Chrome 103.0.5060.114
• Edge (web) • edge • web-javascript • Microsoft Edge 103.0.1264.49

[√] HTTP Host Availability
• All required HTTP hosts are available

! Doctor found issues in 1 category.

@jmagman
Copy link
Member

jmagman commented Jul 27, 2022

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.

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

Successfully merging this pull request may close these issues.

9 participants