-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[camera_android_camerax] Add support for NV21 image format #9644
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces support for the NV21 image format in the Android CameraX implementation. This is achieved by adding a new Java utility class to perform the YUV_420_888 to NV21 conversion, and exposing it to Dart via Pigeon. The changes in the Dart layer correctly use this new utility when the camera is initialized for NV21 image streaming.
My review focuses on several critical issues that will prevent the code from compiling, such as incorrect package names, missing imports, and typos in the new Java files. I've also pointed out an incorrect configuration in the Pigeon file. Addressing these points will be necessary to get the PR into a mergeable state.
// Note: the code in this file is taken directly from the official Google MLKit example: | ||
// https://github.com/googlesamples/mlkit | ||
|
||
package io.flutter.plugins.camera.media; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package declaration io.flutter.plugins.camera.media
does not match the file's directory structure. To ensure the class is correctly resolved and to maintain consistency, it should be io.flutter.plugins.camerax
.
package io.flutter.plugins.camera.media; | |
package io.flutter.plugins.camerax; |
import android.media.Image; | ||
import androidx.annotation.NonNull; | ||
import java.nio.ByteBuffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is missing imports for List
and PlaneProxy
, which will cause compilation errors.
import android.media.Image; | |
import androidx.annotation.NonNull; | |
import java.nio.ByteBuffer; | |
import android.media.Image; | |
import androidx.annotation.NonNull; | |
import androidx.camera.core.ImageProxy.PlaneProxy; | |
import java.nio.ByteBuffer; | |
import java.util.List; |
import androidx.camera.core.ImageProxy.PlaneProxy; | ||
import androidx.annotation.NonNull; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
@NonNull | ||
@Override | ||
public bytes[] getNv21Plane(@NonNull List<PlaneProxy> planeProxyList, long imageWidth, long imageHeight) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// TODO(camsim99) | ||
@ProxyApi( | ||
kotlinOptions: KotlinProxyApiOptions( | ||
fullClassName: 'androidx.camera.core.PlaneProxyUtils', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fullClassName
androidx.camera.core.PlaneProxyUtils
appears to be incorrect, as this class does not exist in the AndroidX library. This should point to the new native utility class you've added, which is io.flutter.plugins.camerax.PlaneProxyUtils
.
fullClassName: 'io.flutter.plugins.camerax.PlaneProxyUtils',
...wip :) might not even compile yet tbh
Fixes flutter/flutter#145961.
Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assist
bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3