packages icon indicating copy to clipboard operation
packages copied to clipboard

[camerax] Re-land SurfaceProducer migration with fix for camera preview rotation

Open camsim99 opened this issue 1 year ago • 5 comments

Re-lands https://github.com/flutter/packages/pull/6462 with the following changes to buildPreview:

1: A fix for correctly rotating the camera preview

This fix is required for Surfaces not backed by a SurfaceTexture because they don't contain the transformation information needed to correctly rotate the camera preview. In that case, we use the logic described in https://developer.android.com/media/camera/camera2/camera-preview#orientation_calculation.

Fixes https://github.com/flutter/flutter/issues/149294.

2: A fix for correctly rotating the camera preview on naturally landscape-oriented devices

I believe this issue was caused because we assume that the natural orientation of the device is portrait up. We fix this here by adding an extra rotation for the camera preview based on the natural orientation of the device.

Fixes https://github.com/flutter/flutter/issues/149177.

Pre-launch Checklist

camsim99 avatar Jun 03 '24 17:06 camsim99

@chinmaygarde if you get the chance to test this out and/or bring the landscape device to SVL next week I would love to see how it behaves with this change!

Other than that fix (I left TODOs in the code), this PR can start going through code review :)

camsim99 avatar Jun 06 '24 21:06 camsim99

From triage: @chinmaygarde Waiting on you to bring the tablet for testing the fix.

yaakovschectman avatar Jun 13 '24 18:06 yaakovschectman

Oh, awesome. I can test it today and also bring the tablet down to SVL next week.

chinmaygarde avatar Jun 13 '24 18:06 chinmaygarde

Just an update: This works on all the devices I have access to. Including the the landscape natural ones. But we are going to gather in SVL for some more manual testing. That didn't happen last week as I was OOO sick.

chinmaygarde avatar Jun 24 '24 17:06 chinmaygarde

As discussed offline with @matanlurey, going to land the fixes to the camera preview rotation without Impeller support in https://github.com/flutter/packages/pull/7044 first and land this (which will essentially only include re-lading Impeller support) later on.

camsim99 avatar Jul 02 '24 19:07 camsim99

We need to add the lifecycle methods to correctly suspend/resume the camera surface: https://docs.flutter.dev/release/breaking-changes/android-surface-plugins#migration-guide

(Unless I missed something and that's being handled elsewhere?)

matanlurey avatar Aug 13 '24 20:08 matanlurey

We need to add the lifecycle methods to correctly suspend/resume the camera surface: https://docs.flutter.dev/release/breaking-changes/android-surface-plugins#migration-guide

(Unless I missed something and that's being handled elsewhere?)

I assume that the preview surface provider already handles that, but let me know if that doesn't cover everything!

camsim99 avatar Aug 14 '24 17:08 camsim99

Despite the names those are indeed different things

matanlurey avatar Aug 15 '24 18:08 matanlurey

I just meant that the method will be called when a new surface is needed which causes CameraX to detach the surface from the camera and return a new surface (see our impl), effectively doing the same thing that doc describes, so I don't think there's anything else to do here.

camsim99 avatar Aug 15 '24 18:08 camsim99

To double check that this method is called if a surface gets destroyed, though, I can follow up internally :)

camsim99 avatar Aug 15 '24 18:08 camsim99

I see, perhaps you're right - if it works it works!

matanlurey avatar Aug 15 '24 18:08 matanlurey

Followed up with the CameraX team and found out that we need to call SurfaceRequest.invalidate() (docs) in order for a new request for a surface to be called and thus, for the Preview.SurfaceProvider callback to handle creating a new surface.

~This means we either have to save the requests we receive to invalidate it in the SurfaceProducer callback or we create a new SurfaceProvider instance every time a request for a surface comes in. I assume the latter is discouraged, can you confirm that @matanlurey ?~ edit: JK I think I found a middle ground :)

camsim99 avatar Aug 15 '24 20:08 camsim99

auto label is removed for flutter/packages/6856, due to - The status or check suite Mac_arm64 custom_package_tests master has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit[bot] avatar Aug 19 '24 17:08 auto-submit[bot]