[camera_platform_interface] [camera] [camera_android] Add NV21 as an image stream format #3277

acoutts · 2023-02-23T15:24:55Z

Note: this is a re-created PR from flutter/plugins#6985

This PR adds NV21 as an image stream format for android devices. This is the format required for things like MLKit. Unfortunately a lot of people tend to implement the yuv->nv21 incorrectly in dart, which results in countless issues of users saying the image stream doesn't work, or that mlkit doesn't work.

By allowing the plugin to send NV21 to dart, this will fix all of those yuv conversion issues.

Highlights:

Added an abstraction around ImageReader called ImageStreamReader and moved the image stream logic into this class to clean up the Camera class. This allows us to test the image stream handler in isolation.
Added tests for ImageStreamReader to make sure it only calls the removePlaneBufferPadding function for planes that contain padding.
Added a new utility class called ImageStreamReaderUtils which contains the logic for trimming the padding.
Added tests for ImageStreamReaderUtils to make sure it only removes padding when a given buffer contains padding to be removed.
There are tests to confirm both that removePlaneBufferPadding is only called when padding is present, and to confirm that if removePlaneBufferPadding does happen to be called with a buffer containing no padding, it still returns a valid buffer.

List which issues are fixed by this PR. You must list at least one issue.

Fixes [camera] Image stream data is corrupt on some Android devices (Vivo, Xiaomi, Motorola) flutter#118350

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

camsim99

Overall, this looks great! Thanks for this and especially for wrapping ImageReader :) just left a few comments.

camsim99 · 2023-02-27T17:10:33Z

.../camera_android/android/src/main/java/io/flutter/plugins/camera/media/ImageStreamReader.java

+
+    // We will convert the YUV data to NV21 which is a single-plane image
+    ByteBuffer bytes =
+        imageStreamReaderUtils.yuv420ThreePlanesToNV21(


Why not catch the illegal state exception here? Just wondering if this may not be the only cause.

I opted for doing it in the other way because I wanted to make sure we post an exception to dart and close the image after. If we catch it here then i'm not sure how we'd still return an error to dart because line 101 would presumedly return normally, which would then have the plugin try and post an image back to dart.

camsim99 · 2023-02-27T17:14:29Z

...ra_android/android/src/main/java/io/flutter/plugins/camera/media/ImageStreamReaderUtils.java

+    byte[] out = new byte[imageSize + 2 * (imageSize / 4)];
+
+    if (areUVPlanesNV21(yuv420888planes, width, height)) {
+      //      Log.i("flutter", "planes are NV21");


Nit: remember to remove the logs!

Fixed in e0b5716

camsim99 · 2023-02-27T17:17:23Z

...era_android/android/src/test/java/io/flutter/plugins/camera/media/ImageStreamReaderTest.java

+    imageStreamReader.onImageAvailable(mockImage, mockCaptureProps, mockEventSink);
+
+    // Make sure we processed the frame with parsePlanesForNv21
+    verify(mockImageStreamReaderUtils).yuv420ThreePlanesToNV21(any(), anyInt(), anyInt());


Can you verify the parameters for this call too?

Fixed in c170fe0

bparrishMines

This looks good to me, but the platform interface and android implementation are missing tests. You should be able to do something along the lines of: https://github.com/flutter/packages/blob/main/packages/camera/camera_platform_interface/test/method_channel/type_conversion_test.dart#L64

packages/camera/camera/CHANGELOG.md

bparrishMines · 2023-03-01T22:27:35Z

packages/camera/camera_android/CHANGELOG.md

+## 0.10.5
+
+* Adds support for NV21 as a new streaming format in Android which includes correct handling of image padding when present.
+


Remove extra space

Fixed in 000e783

bparrishMines · 2023-03-01T22:51:54Z

packages/camera/camera_platform_interface/CHANGELOG.md

@@ -1,3 +1,6 @@
+## 2.5.0
+* Adds NV21 as an image stream format (suitable for Android).


Needs a space above.

Fixed in 000e783

acoutts · 2023-03-09T15:26:16Z

This looks good to me, but the platform interface and android implementation are missing tests. You should be able to do something along the lines of: https://github.com/flutter/packages/blob/main/packages/camera/camera_platform_interface/test/method_channel/type_conversion_test.dart#L64

Tests added in cb8dc5e

bparrishMines

LGTM with one more comment

It also looks like this was made before we updated the .gitignore from the repo merge. You will need to remove all the added gradle related files in camera/example and pull from main.

packages/camera/camera/lib/src/camera_image.dart

acoutts · 2023-03-13T13:14:29Z

@bparrishMines i'm fixing one bug I've found today where sometimes imageStreamReader can be null when we try to subscribe a listener.

acoutts · 2023-03-13T13:59:26Z

Ok that's pushed.

camsim99

Android part looks good to me! Looks like you may need to fix some formatting. There was an Android test failure, too, but I think it just needed a re-run.

acoutts · 2023-03-13T20:45:32Z

Merged main, fixed formatting issue and ran the android tests locally to confirm they are all passing. The one that failed before seems to be a JVM out of memory error. I made the test image smaller to fix that issue in the test.

bparrishMines · 2023-03-15T21:28:39Z

@acoutts You can go ahead and create a separate PR that updates only the camera_platform_interface package.

acoutts · 2023-05-02T20:07:35Z

Whooohoo! Cleaned it up and bumped the pubspec versions. One thing i'm confused on now, it looks like the test case for bgra8888 in camera_image_test was overwritten / replaced with the nv21 one, so I fixed that as well.

…1 as an image stream format (flutter/packages#3277)

flutter/packages@407b7da...6bd59cd 2023-05-04 41930132+hellohuanlin@users.noreply.github.com [pigeon][reland]enable treat warnings as errors for swift code in unit test (flutter/packages#3901) 2023-05-04 stuartmorgan@google.com [video_player] Fix Android lints (flutter/packages#3886) 2023-05-04 stuartmorgan@google.com [quick_actions] Fix Android lint issues (flutter/packages#3885) 2023-05-03 andrewjohncoutts@gmail.com [camera_platform_interface] [camera] [camera_android] Add NV21 as an image stream format (flutter/packages#3277) 2023-05-03 stuartmorgan@google.com Revert "[pigeon]enable treat warning as errors for swift code in unit test" (flutter/packages#3898) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…tter#3639) This contains the changes for camera_android from flutter#3277

…image stream format (flutter#3277) ### Note: this is a re-created PR from flutter/plugins#6985 --- This PR adds NV21 as an image stream format for android devices. This is the format required for things like MLKit. Unfortunately a lot of people tend to implement the yuv->nv21 incorrectly in dart, which results in countless issues of users saying the image stream doesn't work, or that mlkit doesn't work. By allowing the plugin to send NV21 to dart, this will fix all of those yuv conversion issues. Highlights: - Added an abstraction aroundÂ `ImageReader`Â calledÂ `ImageStreamReader`Â and moved the image stream logic into this class to clean up theÂ `Camera`Â class. This allows us to test the image stream handler in isolation. - Added tests forÂ `ImageStreamReader`Â to make sure it only calls theÂ `removePlaneBufferPadding`Â function for planes that contain padding. - Added a new utility class calledÂ `ImageStreamReaderUtils`Â which contains the logic for trimming the padding. - Added tests forÂ `ImageStreamReaderUtils`Â to make sure it only removes padding when a given buffer contains padding to be removed. - There are tests to confirm both thatÂ `removePlaneBufferPadding`Â is only called when padding is present, and to confirm that ifÂ `removePlaneBufferPadding`Â does happen to be called with a buffer containing no padding, it still returns a valid buffer. *List which issues are fixed by this PR. You must list at least one issue.* - flutter/flutter#118350 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*

caiofreitas · 2023-07-31T19:18:56Z

Still facing this issue on these devices:
motorola edge 30 neo, poco x4 pro, poco x3 pro

ghost · 2023-07-31T19:20:41Z

It's been merged for a while now. Make sure you're using nv21 on android too. It uses yuv by default.

caiofreitas · 2023-07-31T19:46:32Z

It's been merged for a while now. Make sure you're using nv21 on android too. It uses yuv by default.

It was exactly that. My bad...

CameraController(frontCamera, ResolutionPreset.ultraHigh,
enableAudio: false, imageFormatGroup: ImageFormatGroup.nv21);

arrrrny · 2024-10-29T09:24:50Z

as of the latest version [0.11.0+2] this feature does not work. Only works at 0.10.6 and 0.10.5

Recreating PR from flutter/plugins

7e72f24

acoutts requested review from camsim99 and bparrishMines as code owners February 23, 2023 15:24

github-actions bot added p: camera platform-android labels Feb 23, 2023

camsim99 reviewed Feb 27, 2023

View reviewed changes

bparrishMines requested changes Mar 1, 2023

View reviewed changes

acoutts added 3 commits March 9, 2023 10:04

Log cleanup

e0b5716

Add test parameters

c170fe0

Tests for new image group type

cb8dc5e

acoutts added 3 commits March 9, 2023 10:27

Changelog spacing

093d9e7

Changelog spacing

c8ed510

Changelog spacing

000e783

acoutts mentioned this pull request Mar 9, 2023

[camera] Image stream data is corrupt on some Android devices (Vivo, Xiaomi, Motorola) flutter/flutter#118350

Closed

bparrishMines approved these changes Mar 10, 2023

View reviewed changes

packages/camera/camera/lib/src/camera_image.dart Show resolved Hide resolved

acoutts added 4 commits March 10, 2023 14:16

Add test to camera for nv21 image format

97787d5

Merge remote-tracking branch 'upstream/main' into fix-handle-padding

e19bfec

Merge remote-tracking branch 'upstream/main' into fix-handle-padding

fb6ec84

Remove tracked gradle files

14f2986

Handle cases where imageStreamReader is null to be ultra safe

1c03352

camsim99 approved these changes Mar 13, 2023

View reviewed changes

acoutts added 3 commits March 13, 2023 16:36

Merge remote-tracking branch 'upstream/main' into fix-handle-padding

b051e70

Formatters

7ae6b93

Fix out of memory error during tests because mock image was too big

c651052

acoutts mentioned this pull request Mar 15, 2023

[camera_platform_interface] Add NV21 as an image stream format #3469

Merged

acoutts added 5 commits May 2, 2023 16:01

Update pubspec.yaml

0c35521

Changelog spacing and version

cfabbad

Update pubspec.yaml

96fe939

Update pubspec.yaml

0e73f9a

Add back missing camera image test for bgra8888

8ff5d68

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label May 3, 2023

auto-submit bot merged commit 4c4bb46 into flutter:main May 3, 2023

engine-flutter-autoroll mentioned this pull request May 4, 2023

Roll Packages from 407b7da0079e to 6bd59cdb8fb5 (5 revisions) flutter/flutter#126070

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 4, 2023

4c4bb46 [camera_platform_interface] [camera] [camera_android] Add NV2…

02c7ea9

…1 as an image stream format (flutter/packages#3277)

engine-flutter-autoroll mentioned this pull request May 4, 2023

Roll Packages from 407b7da0079e to 6bd59cdb8fb5 (5 revisions) flutter/flutter#126078

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 4, 2023

4c4bb46 [camera_platform_interface] [camera] [camera_android] Add NV2…

dde213d

…1 as an image stream format (flutter/packages#3277)

reidbaker mentioned this pull request May 12, 2023

[url_launcher] Set broadcast reciever visability as required by target api 34 #3973

Merged

11 tasks

This was referenced May 13, 2023

Flutter. Face Detection. No face detected on Samsung Galaxy A23 Android 12: Access denied finding property "ro.mediatek.platform" flutter-ml/google_ml_kit_flutter#287

Closed

Not Working on Android 11 and 12 flutter-ml/google_ml_kit_flutter#285

Closed

This was referenced Jul 7, 2023

[Tool] New tool to download android dependencies #4408

Merged

[camera] tree fix format 2023 05 12 #4450

Closed

[tooling] mark infra steps as infra steps in yaml files #4473

Merged

nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023

[camera_android] Add NV21 as an image stream format flutter#3277 (flu…

8a6b9b5

…tter#3639) This contains the changes for camera_android from flutter#3277

reidbaker mentioned this pull request Jul 26, 2023

[Tooling] Add google owned cache for dependencies as an option in ci #4567

Merged

11 tasks

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[camera_platform_interface] [camera] [camera_android] Add NV21 as an image stream format #3277

[camera_platform_interface] [camera] [camera_android] Add NV21 as an image stream format #3277

acoutts commented Feb 23, 2023 •

edited by stuartmorgan

Loading

camsim99 left a comment

camsim99 Feb 27, 2023

acoutts Mar 9, 2023

camsim99 Feb 27, 2023

acoutts Mar 9, 2023

camsim99 Feb 27, 2023

acoutts Mar 9, 2023

bparrishMines left a comment

bparrishMines Mar 1, 2023

acoutts Mar 9, 2023

bparrishMines Mar 1, 2023

acoutts Mar 9, 2023

acoutts commented Mar 9, 2023

bparrishMines left a comment

acoutts commented Mar 13, 2023

acoutts commented Mar 13, 2023

camsim99 left a comment

acoutts commented Mar 13, 2023

bparrishMines commented Mar 15, 2023

acoutts commented May 2, 2023

caiofreitas commented Jul 31, 2023

ghost commented Jul 31, 2023

caiofreitas commented Jul 31, 2023

arrrrny commented Oct 29, 2024

		## 0.10.5

		* Adds support for NV21 as a new streaming format in Android which includes correct handling of image padding when present.

		@@ -1,3 +1,6 @@
		## 2.5.0
		* Adds NV21 as an image stream format (suitable for Android).

[camera_platform_interface] [camera] [camera_android] Add NV21 as an image stream format #3277

[camera_platform_interface] [camera] [camera_android] Add NV21 as an image stream format #3277

Conversation

acoutts commented Feb 23, 2023 • edited by stuartmorgan Loading

Note: this is a re-created PR from flutter/plugins#6985

Pre-launch Checklist

camsim99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparrishMines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acoutts commented Mar 9, 2023

bparrishMines left a comment

Choose a reason for hiding this comment

acoutts commented Mar 13, 2023

acoutts commented Mar 13, 2023

camsim99 left a comment

Choose a reason for hiding this comment

acoutts commented Mar 13, 2023

bparrishMines commented Mar 15, 2023

acoutts commented May 2, 2023

caiofreitas commented Jul 31, 2023

ghost commented Jul 31, 2023

caiofreitas commented Jul 31, 2023

arrrrny commented Oct 29, 2024

acoutts commented Feb 23, 2023 •

edited by stuartmorgan

Loading