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

[camera] flip/change camera while recording (split out PR for cam_avfoundation and cam_android) #7109

Conversation

BradenBagby
Copy link
Contributor

This is another split out PR for #6478. This PR only contains changes under Camera_AVFoundation and Camera_Android, while also pointing them to the merged platform interface changes version 2.4.0

Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bparrishMines
Copy link
Contributor

Secondary review request for @camsim99 @stuartmorgan or @hellohuanlin
I verified this is the same approved code from #6478

@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 13, 2023
@BradenBagby BradenBagby requested review from bparrishMines and removed request for hellohuanlin February 13, 2023 20:09
@BradenBagby
Copy link
Contributor Author

Did not mean to remove you @hellohuanlin and don't seem to be able to add you back

Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on iOS part - looks like it's the same code from #6478 which I have reviewed.

@BradenBagby
Copy link
Contributor Author

After looking at the logs Im still not sure whats failing here. I guess the build timed out but not sure why. Any ideas or suggestions @bparrishMines @stuartmorgan. Or can we just re-trigger these tests to run?

@stuartmorgan
Copy link
Contributor

After looking at the logs Im still not sure whats failing here. I guess the build timed out but not sure why. Any ideas or suggestions @bparrishMines @stuartmorgan. Or can we just re-trigger these tests to run?

There aren't any failing tests here; it will be landed automatically when the tree is green.

@auto-submit auto-submit bot merged commit 9c312d4 into flutter:main Feb 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 14, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 14, 2023
* f1a3fea7f Update GCLOUD_FIREBASE_TESTLAB_KEY (flutter/plugins#7176)

* 9c312d4d2 [camera] flip/change camera while recording (split out PR for cam_avfoundation and cam_android) (flutter/plugins#7109)
@BradenBagby
Copy link
Contributor Author

Looks like an android test on Testlab is failing for starqlteue_26. Probably due to this here #6478 (comment)

@BradenBagby
Copy link
Contributor Author

Looks like an android test on Testlab is failing for starqlteue_26. Probably due to this here #6478 (comment)

@stuartmorgan What solution do you think we should use: disable switching camera for devices below API 26, Or disable the plugin for older versions?

@BradenBagby
Copy link
Contributor Author

Looks like an android test on Testlab is failing for starqlteue_26. Probably due to this here #6478 (comment)

@stuartmorgan What solution do you think we should use: disable switching camera for devices below API 26, Or disable the plugin for older versions?

@bparrishMines

@stuartmorgan
Copy link
Contributor

Looks like an android test on Testlab is failing for starqlteue_26. Probably due to this here #6478 (comment)

Looks that way:

     FATAL EXCEPTION: Thread-7
Process: io.flutter.plugins.cameraexample, PID: 8633
java.lang.RuntimeException: eglSwapBuffers()EGL_BAD_SURFACE
	at io.flutter.plugins.camera.VideoRenderer.draw(VideoRenderer.java:362)
	at io.flutter.plugins.camera.VideoRenderer$2.run(VideoRenderer.java:295)

I'm confused how this passed the stable postsubmit test run given that. I'll make a revert PR.

@stuartmorgan
Copy link
Contributor

Looks like an android test on Testlab is failing for starqlteue_26. Probably due to this here #6478 (comment)

@stuartmorgan What solution do you think we should use: disable switching camera for devices below API 26, Or disable the plugin for older versions?

We should only disable the new feature, not the entire plugin.

@BradenBagby
Copy link
Contributor Author

Looks like an android test on Testlab is failing for starqlteue_26. Probably due to this here #6478 (comment)

@stuartmorgan What solution do you think we should use: disable switching camera for devices below API 26, Or disable the plugin for older versions?

We should only disable the new feature, not the entire plugin.

Sorry about that. I don't have access to my Android device right now so will do this tomorrow.

But to be sure. Im thinking the new feature for devices < 26 should throw some sort of unsupported error when user tries to flip camera mid recording and not mess with the recording state (camera keeps recording unflipped). Is that what you're thinking?

stuartmorgan added a commit that referenced this pull request Feb 14, 2023
… cam_avfoundation and cam_android) (#7109)" (#7181)

This reverts commit 9c312d4.
@stuartmorgan
Copy link
Contributor

That sounds reasonable to me.

So the next step here would be to make a new PR that starts with a revert of the revert (with no changes) as the first commit, then makes changes in follow-up commits, to make it easy to review just the changes. We'll make sure to have a team member push a commit to the PR once it's reviewed so that we can get full presubmit tests.

BradenBagby added a commit to BradenBagby/plugins that referenced this pull request Feb 15, 2023
…t PR for cam_avfoundation and cam_android) (flutter#7109)" (flutter#7181)"

This reverts commit 66d5724.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: camera platform-android platform-ios
Projects
None yet
7 participants