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

Draft: [camera_android] Add NV21 as an image stream format #6985

Closed
wants to merge 37 commits into from

Conversation

acoutts
Copy link
Contributor

@acoutts acoutts commented Jan 17, 2023

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.

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

  • 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. (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, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

@acoutts
Copy link
Contributor Author

acoutts commented Jan 17, 2023

I'm not sure how to fix that formatting issue being flagged by repo_checks. I tried google-java-format and dart format.

@beroso
Copy link
Contributor

beroso commented Jan 18, 2023

I'm not sure how to fix that formatting issue being flagged by repo_checks. I tried google-java-format and dart format.

The contributing guid has the necessary info about it.
https://github.com/flutter/plugins/blob/main/CONTRIBUTING.md

There is a link to specific documentation about the tools for testing, formatting, etc.
https://github.com/flutter/plugins/blob/main/script/tool/README.md

@acoutts
Copy link
Contributor Author

acoutts commented Jan 18, 2023

Fixed the formatting and licenses but I can't clearly see what submit-queue -- Cirrus CI failure on main means.

@acoutts acoutts changed the title [camera_android] Fix YUV image stream on android devices which add padding to the image buffers at certain resolutions Draft: [camera_android] Fix YUV image stream on android devices which add padding to the image buffers at certain resolutions Jan 19, 2023
@gordinmitya
Copy link

@acoutts you also may find these discussions about performance of this approach useful:
android/camera-samples#405
android/camera-samples#315
android/camera-samples#399

@acoutts
Copy link
Contributor Author

acoutts commented Jan 20, 2023

Thanks @gordinmitya - i actually found another example which works even better and i'm redoing my approach for this PR.

The official google MLKit demo app has a good bmp conversion class which contains some functions to convert YUV420 into NV21: https://github.com/googlesamples/mlkit/blob/master/android/vision-quickstart/app/src/main/java/com/google/mlkit/vision/demo/BitmapUtils.java#L177-L220

Instead of making this PR try and process the YUV data to remove padding, we shouldn't process YUV at all. If a user wants YUV, they should get it exactly as it comes from the camera device - padding and all. It should be up to them to handle padding or do whatever they want to do with it.

The problem really is a bad implementation everywhere on how to convert the YUV to NV21 in dart. The most common use case for this is for MLKit which needs NV21 as an input image. All of the dart examples I've seen simply concatenate all 3 planes into one big Uint8List and pass it into MLKit. For devices with padding (like the vivo), this of course doesn't work as the image is invalid. This bad implementation of YUV->NV21 conversion is what my issue here is really about.

Instead I am going to add NV21 as a new image format group option for the flutter camera, using the bmp util functions from google's MLKit demo to convert the frames in native before sending it over to dart. This will handle all padding situations and make sure a valid NV21 image is delivered to the dart image stream.

This way there's no change to the existing API, and instead we offer users the ability to get NV21 from the device if they want.

@acoutts
Copy link
Contributor Author

acoutts commented Jan 20, 2023

I've updated my approach to convert to nv21 on the android side before sending back to dart.

@gordinmitya
Copy link

@acoutts after benchmarking, I realized that MLKit solution is more efficient than mine from yuv2buf + it always outputs in NV21 format. So I think you implemented it in the best way.

The path with per-pixel manipulations is quite rare (when a phone returns yuv_420_888 image, meaning u and v planes both have pixelStride == 1). However, even in this case, performance is okay.

Shame to me. I should have benchmarked my solution more extensively earlier. 🤦 I'll update the code in my repo soon.

@acoutts acoutts changed the title Draft: [camera_android] Fix YUV image stream on android devices which add padding to the image buffers at certain resolutions Draft: [camera_android] Add NV21 as an image stream format Jan 24, 2023
@acoutts acoutts closed this Jan 25, 2023
@acoutts
Copy link
Contributor Author

acoutts commented Jan 25, 2023

Part 1 (camera_platform_interface): #7021
Part 2 (camera): #7022
Part 3 (camera_android): #7023

@camsim99 @bparrishMines i've closed this PR in favor of the 3 split ones above which organizes the changes by each package modified. Please perform reviews on those. We need part 1 merged and published first before 2 and 3 can utilize the new ImageFormatGroup option for nv21.

@acoutts
Copy link
Contributor Author

acoutts commented Feb 17, 2023

Alright I just finished fixing up the tests with the latest approach. This is now ready for review @camsim99 @bparrishMines @stuartmorgan

@stuartmorgan-g
Copy link
Contributor

We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages.

Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord.

Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state!

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.

5 participants