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

[Camera] Improved resolution presets and matched quality between Android and iOS #1403

Closed
wants to merge 6 commits into from
Closed

[Camera] Improved resolution presets and matched quality between Android and iOS #1403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9a774a8
Added camcorderProfile to Android for better quality preset selection.
quentinleguennec Mar 26, 2019
1232d22
Improved best profile selection algorithm.
quentinleguennec Mar 26, 2019
39c2d0e
Camera: Now resolutionPreset also influence image capture quality. Fi…
quentinleguennec Mar 26, 2019
dcac87d
Use medium resolution preset for the example.
quentinleguennec Mar 27, 2019
018521d
Fixed merge conflict with version 0.4.3+2.
quentinleguennec Mar 29, 2019
945dc88
Fixed formating.
quentinleguennec Apr 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/camera/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
## 0.4.4

* Added 2 new quality presets (veryHigh and veryLow).
* Now quality presets match on Android and iOS
* Now quality presets can be used to control image capture quality.
** NOTE: ** Existing presets have been updated, this will affect the quality of pictures and videos in existing apps.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to keep the existing behavior the same, if possible, just to avoid having to create breaking changes. We can introduce breaking changes if needed but I don't think it's really worth it here.

It reads a little silly, but I think I'd prefer that we just add more variations of "high" to keep the existing presets the same.

  • "ultra high" = 2160 (or maybe QUALITY_HIGH? discussed below)
  • "very high" = 1080
  • "high" = 720
  • "medium" = 480
  • "low" = 240
  • "very low" = 176 (QCIF)

What do you think?


## 0.4.3+2

* Bump the minimum Flutter version to 1.2.0.
Expand Down
172 changes: 90 additions & 82 deletions packages/camera/android/src/main/java/io/flutter/plugins/camera/CameraPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import android.content.Context;
import android.content.pm.PackageManager;
import android.graphics.ImageFormat;
import android.graphics.Point;
import android.graphics.SurfaceTexture;
import android.hardware.camera2.CameraAccessException;
import android.hardware.camera2.CameraCaptureSession;
Expand All @@ -19,13 +18,13 @@
import android.hardware.camera2.CaptureFailure;
import android.hardware.camera2.CaptureRequest;
import android.hardware.camera2.params.StreamConfigurationMap;
import android.media.CamcorderProfile;
import android.media.Image;
import android.media.ImageReader;
import android.media.MediaRecorder;
import android.os.Build;
import android.os.Bundle;
import android.util.Size;
import android.view.Display;
import android.view.OrientationEventListener;
import android.view.Surface;
import androidx.annotation.NonNull;
Expand Down Expand Up @@ -97,9 +96,7 @@ public void onActivityStarted(Activity activity) {}
@Override
public void onActivityResumed(Activity activity) {
boolean wasRequestingPermission = requestingPermission;
if (requestingPermission) {
requestingPermission = false;
}
requestingPermission = false;
if (activity != CameraPlugin.this.activity) {
return;
}
Expand Down Expand Up @@ -295,9 +292,9 @@ private class Camera {
private Size captureSize;
private Size previewSize;
private CaptureRequest.Builder captureRequestBuilder;
private Size videoSize;
private MediaRecorder mediaRecorder;
private boolean recordingVideo;
private CamcorderProfile recordingProfile;

Camera(final String cameraName, final String resolutionPreset, @NonNull final Result result) {

Expand All @@ -307,32 +304,21 @@ private class Camera {
registerEventChannel();

try {
int minHeight;
switch (resolutionPreset) {
case "high":
minHeight = 720;
break;
case "medium":
minHeight = 480;
break;
case "low":
minHeight = 240;
break;
default:
throw new IllegalArgumentException("Unknown preset: " + resolutionPreset);
}

CameraCharacteristics characteristics = cameraManager.getCameraCharacteristics(cameraName);
StreamConfigurationMap streamConfigurationMap =
characteristics.get(CameraCharacteristics.SCALER_STREAM_CONFIGURATION_MAP);

//noinspection ConstantConditions
sensorOrientation = characteristics.get(CameraCharacteristics.SENSOR_ORIENTATION);
//noinspection ConstantConditions
isFrontFacing =
characteristics.get(CameraCharacteristics.LENS_FACING)
== CameraMetadata.LENS_FACING_FRONT;
computeBestCaptureSize(streamConfigurationMap);
computeBestPreviewAndRecordingSize(streamConfigurationMap, minHeight, captureSize);

recordingProfile = getBestAvailableCamcorderProfileForResolutionPreset(resolutionPreset);
computeBestPreviewSize(recordingProfile);
computeBestCaptureSize(streamConfigurationMap, recordingProfile, resolutionPreset);

if (cameraPermissionContinuation != null) {
result.error("cameraPermission", "Camera permission request ongoing", null);
Expand Down Expand Up @@ -404,62 +390,31 @@ private boolean hasAudioPermission() {
== PackageManager.PERMISSION_GRANTED;
}

private void computeBestPreviewAndRecordingSize(
StreamConfigurationMap streamConfigurationMap, int minHeight, Size captureSize) {
Size[] sizes = streamConfigurationMap.getOutputSizes(SurfaceTexture.class);

// Preview size and video size should not be greater than screen resolution or 1080.
Point screenResolution = new Point();
Display display = activity.getWindowManager().getDefaultDisplay();
display.getRealSize(screenResolution);

final boolean swapWH = getMediaOrientation() % 180 == 90;
int screenWidth = swapWH ? screenResolution.y : screenResolution.x;
int screenHeight = swapWH ? screenResolution.x : screenResolution.y;

List<Size> goodEnough = new ArrayList<>();
for (Size s : sizes) {
if (minHeight <= s.getHeight()
&& s.getWidth() <= screenWidth
&& s.getHeight() <= screenHeight
&& s.getHeight() <= 1080) {
goodEnough.add(s);
}
}

Collections.sort(goodEnough, new CompareSizesByArea());

if (goodEnough.isEmpty()) {
previewSize = sizes[0];
videoSize = sizes[0];
// The preview should never be bigger than 720p (1280 x 720) or it will mess up the recording.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know how this happens or why this is? I'm wondering if this constant is true for all devices, or if less powerful ones are sensitive to even smaller max preview sizes.

private void computeBestPreviewSize(CamcorderProfile profile) {
float ratio = (float) profile.videoFrameWidth / profile.videoFrameHeight;
if (profile.videoFrameWidth > 1280) {
Copy link
Contributor

Choose a reason for hiding this comment

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

readability nit: would be nice to have 1280 extracted out into a named static constant instead of used as a magic number throughout here.

previewSize = new Size(1280, Math.round(1280 / ratio));
} else if (profile.videoFrameHeight > 1280) {
previewSize = new Size(Math.round(1280 * ratio), 1280);
} else {
float captureSizeRatio = (float) captureSize.getWidth() / captureSize.getHeight();

previewSize = goodEnough.get(0);
for (Size s : goodEnough) {
if ((float) s.getWidth() / s.getHeight() == captureSizeRatio) {
previewSize = s;
break;
}
}

Collections.reverse(goodEnough);
videoSize = goodEnough.get(0);
for (Size s : goodEnough) {
if ((float) s.getWidth() / s.getHeight() == captureSizeRatio) {
videoSize = s;
break;
}
}
previewSize = new Size(profile.videoFrameWidth, profile.videoFrameHeight);
}
}

private void computeBestCaptureSize(StreamConfigurationMap streamConfigurationMap) {
// For still image captures, we use the largest available size.
captureSize =
Collections.max(
Arrays.asList(streamConfigurationMap.getOutputSizes(ImageFormat.JPEG)),
new CompareSizesByArea());
private void computeBestCaptureSize(
StreamConfigurationMap streamConfigurationMap,
CamcorderProfile profile,
String resolutionPreset) {
// For still image captures, use the largest image size if resolutionPreset is veryHigh
if ("veryHigh".equals(resolutionPreset)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mentioned below, but I think this would be another reason to have the "veryHigh" correspond with QUALITY_HIGH. That's already the highest resolution available, so we wouldn't need to try to parse the argument and get the capture size again here.

captureSize =
Collections.max(
Arrays.asList(streamConfigurationMap.getOutputSizes(ImageFormat.JPEG)),
new CompareSizesByArea());
} else {
captureSize = new Size(profile.videoFrameWidth, profile.videoFrameHeight);
}
}

private void prepareMediaRecorder(String outputFilePath) throws IOException {
Expand All @@ -469,19 +424,69 @@ private void prepareMediaRecorder(String outputFilePath) throws IOException {
mediaRecorder = new MediaRecorder();
mediaRecorder.setAudioSource(MediaRecorder.AudioSource.MIC);
mediaRecorder.setVideoSource(MediaRecorder.VideoSource.SURFACE);
mediaRecorder.setOutputFormat(MediaRecorder.OutputFormat.MPEG_4);
mediaRecorder.setAudioEncoder(MediaRecorder.AudioEncoder.AAC);
mediaRecorder.setVideoEncoder(MediaRecorder.VideoEncoder.H264);
mediaRecorder.setVideoEncodingBitRate(1024 * 1000);
mediaRecorder.setAudioSamplingRate(16000);
mediaRecorder.setVideoFrameRate(27);
mediaRecorder.setVideoSize(videoSize.getWidth(), videoSize.getHeight());
mediaRecorder.setProfile(recordingProfile);
mediaRecorder.setOutputFile(outputFilePath);
mediaRecorder.setOrientationHint(getMediaOrientation());

mediaRecorder.prepare();
}

private CamcorderProfile getBestAvailableCamcorderProfileForResolutionPreset(
String resolutionPreset) {
int camcorderProfileIndex;
switch (resolutionPreset) {
case "veryHigh":
camcorderProfileIndex = 0;
break;
case "high":
camcorderProfileIndex = 1;
break;
case "medium":
camcorderProfileIndex = 2;
break;
case "low":
camcorderProfileIndex = 3;
break;
case "veryLow":
camcorderProfileIndex = 4;
break;
default:
throw new IllegalArgumentException("Unknown resolution preset: " + resolutionPreset);
}

int cameraId = Integer.parseInt(cameraName);
switch (camcorderProfileIndex) {
case 0:
Copy link
Contributor

@jonasbark jonasbark Mar 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't it be better if the user selected veryHigh but QUALITY_2160P isn't available on the device that it uses the next best solution?
I think right now it would fall back to low quality if I read that correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what is actually happening. This bit above is just to convert the preset from a string to an int. having anything else than one of the preset value here in an error. It should never happen since on the dart side creating the CameraController require a ResolutionPreset object, so it's impossible for users to make a mistake here. But people updating/improving the plugin could so it's cleaner to take it into account.

The bit under is actually trying to use the requested preset and reducing if the preset is not available on the device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another precision, I did the same thing as what was already on the iOS side for the second switch (which is pretty smart BTW). The switch doesn't break or return unless the profile is available, which means it will try all the presets under the requested one if it's not available.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also took me a couple read throughs and this comment chain to understand the behavior of the two case statements one after the other. Just restating in my own words to make sure I'm understanding right.

  1. The first case on line 437 verifies that resolutionPreset is a valid argument, since this is just a random string from dart and could be something we're not expecting. It checks against the expected magic string names and throws if it doesn't match any of our magic constants. It translates the expected string names into ints ordered by preset quality to use in the second case statement below.
  2. The second case statement on line 458 deliberately uses the random ints from above and fallthroughs to get the highest available CamcorderProfile possible for a given string preset. It always falls through to QUALITY_LOW and will only throw if we're in some kind of actually exceptional state if there's no camera capture at all.

This logic is solid. But I think the combination of magic strings/numbers and silent case fall through behavior made this tricky to read. I have some subjective suggestions to try and make this easier to understand for anyone else going through the code later and not seeing this comment chain:

  • I think it would help to create a ResolutionPreset enum and extract the first case into a helper method that takes the String resolutionPreset and returns an enum, throwing if it doesn't work. That'll help in two ways. It'll make it immediately obvious what's happening where the first case is because it will just collapse the case into the line ResolutionPreset resolutionPreset = getResolutionPresetFromString(resolutionPreset), and the helper method itself will have obvious intent from the name. Then in the second switch below, the case statements will all be on named enum constants instead of random numbers. (Could also create an enum class with a factory constructor, but that's probably overkill here.) Also protects against any bugs from forgetting to cover all the string cases in the second switch.
  • I would add a comment to the second case explaining that it's deliberately falling through on all of these cases to try resolutions in order of descending quality, just to really point out that there's not a break here like normal.

if (CamcorderProfile.hasProfile(cameraId, CamcorderProfile.QUALITY_2160P)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to use QUALITY_HIGH here? That's the "highest available" resolution, like how the fallthrough is QUALITY_LOW.

return CamcorderProfile.get(CamcorderProfile.QUALITY_2160P);
}
case 1:
if (CamcorderProfile.hasProfile(cameraId, CamcorderProfile.QUALITY_1080P)) {
return CamcorderProfile.get(CamcorderProfile.QUALITY_1080P);
}
case 2:
if (CamcorderProfile.hasProfile(cameraId, CamcorderProfile.QUALITY_720P)) {
return CamcorderProfile.get(CamcorderProfile.QUALITY_720P);
}
case 3:
if (CamcorderProfile.hasProfile(cameraId, CamcorderProfile.QUALITY_480P)) {
return CamcorderProfile.get(CamcorderProfile.QUALITY_480P);
}
case 4:
if (CamcorderProfile.hasProfile(cameraId, CamcorderProfile.QUALITY_CIF)) {
return CamcorderProfile.get(CamcorderProfile.QUALITY_CIF);
}
default:
if (CamcorderProfile.hasProfile(
Integer.parseInt(cameraName), CamcorderProfile.QUALITY_LOW)) {
return CamcorderProfile.get(CamcorderProfile.QUALITY_LOW);
} else {
throw new IllegalArgumentException(
"No capture session available for current capture session.");
}
}
}

private void open(@Nullable final Result result) {
if (!hasCameraPermission()) {
if (result != null) result.error("cameraPermission", "Camera permission not granted", null);
Expand All @@ -494,7 +499,10 @@ private void open(@Nullable final Result result) {
// Used to steam image byte data to dart side.
imageStreamReader =
ImageReader.newInstance(
previewSize.getWidth(), previewSize.getHeight(), ImageFormat.YUV_420_888, 2);
recordingProfile.videoFrameWidth,
recordingProfile.videoFrameHeight,
ImageFormat.YUV_420_888,
2);

cameraManager.openCamera(
cameraName,
Expand Down
2 changes: 1 addition & 1 deletion packages/camera/example/lib/main.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ class _CameraExampleHomeState extends State<CameraExampleHome> {
if (controller != null) {
await controller.dispose();
}
controller = CameraController(cameraDescription, ResolutionPreset.high);
controller = CameraController(cameraDescription, ResolutionPreset.medium);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would like to revert this assuming the existing preset names are preserved like I mentioned above.


// If the controller is updated then update the UI.
controller.addListener(() {
Expand Down
57 changes: 41 additions & 16 deletions packages/camera/ios/Classes/CameraPlugin.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@

static FlutterError *getFlutterError(NSError *error) {
return [FlutterError errorWithCode:[NSString stringWithFormat:@"Error %d", (int)error.code]
message:error.domain
details:error.localizedDescription];
message:error.localizedDescription
details:error.domain];
}

@interface FLTSavePhotoDelegate : NSObject <AVCapturePhotoCaptureDelegate>
Expand Down Expand Up @@ -140,6 +140,7 @@ @interface FLTCam : NSObject <FlutterTexture,
@property(assign, nonatomic) BOOL isRecording;
@property(assign, nonatomic) BOOL isAudioSetup;
@property(assign, nonatomic) BOOL isStreamingImages;
@property(assign, nonatomic) NSString *resolutionPreset;
@property(nonatomic) CMMotionManager *motionManager;
- (instancetype)initWithCameraName:(NSString *)cameraName
resolutionPreset:(NSString *)resolutionPreset
Expand Down Expand Up @@ -167,6 +168,7 @@ - (instancetype)initWithCameraName:(NSString *)cameraName
error:(NSError **)error {
self = [super init];
NSAssert(self, @"super init cannot be nil");
_resolutionPreset = resolutionPreset;
_dispatchQueue = dispatchQueue;
_captureSession = [[AVCaptureSession alloc] init];

Expand Down Expand Up @@ -201,8 +203,11 @@ - (instancetype)initWithCameraName:(NSString *)cameraName
_motionManager = [[CMMotionManager alloc] init];
[_motionManager startAccelerometerUpdates];

[self setCaptureSessionPreset:resolutionPreset];

@try {
[self setCaptureSessionPreset:resolutionPreset];
} @catch (NSError *e) {
*error = e;
}
return self;
}

Expand All @@ -216,7 +221,9 @@ - (void)stop {

- (void)captureToFile:(NSString *)path result:(FlutterResult)result {
AVCapturePhotoSettings *settings = [AVCapturePhotoSettings photoSettings];
[settings setHighResolutionPhotoEnabled:YES];
if ([_resolutionPreset isEqualToString:@"veryHigh"]) {
[settings setHighResolutionPhotoEnabled:YES];
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this universally only needed for AVCaptureSessionPreset3840x2160? I'm having trouble finding what the objective cutoff is for this in the developer docs. They mention CMVideoDimensions highResolutionStillImageDimensions describing its dimensions when this flag is set, but I'm not seeing anywhere to see what the max dimensions without this flag set would be. Probably missing something obvious.

}
[_capturePhotoOutput
capturePhotoWithSettings:settings
delegate:[[FLTSavePhotoDelegate alloc] initWithPath:path
Expand All @@ -227,14 +234,25 @@ - (void)captureToFile:(NSString *)path result:(FlutterResult)result {

- (void)setCaptureSessionPreset:(NSString *)resolutionPreset {
Copy link
Contributor

Choose a reason for hiding this comment

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

(optional) Ah, okay. This is the exact same structure as the two new cases in Java. I think this would also be easier to read if it were split up like I mentioned above, but since this is existing code no pressure to refactor it.

int presetIndex;
if ([resolutionPreset isEqualToString:@"high"]) {
if ([resolutionPreset isEqualToString:@"veryHigh"]) {
presetIndex = 0;
} else if ([resolutionPreset isEqualToString:@"high"]) {
presetIndex = 1;
} else if ([resolutionPreset isEqualToString:@"medium"]) {
presetIndex = 2;
} else {
NSAssert([resolutionPreset isEqualToString:@"low"], @"Unknown resolution preset %@",
resolutionPreset);
} else if ([resolutionPreset isEqualToString:@"low"]) {
presetIndex = 3;
} else if ([resolutionPreset isEqualToString:@"veryLow"]) {
presetIndex = 4;
} else {
NSError *error = [NSError
errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey :
[NSString stringWithFormat:@"Unknown resolution preset %@", resolutionPreset]
}];
@throw error;
}

switch (presetIndex) {
Expand Down Expand Up @@ -268,13 +286,20 @@ - (void)setCaptureSessionPreset:(NSString *)resolutionPreset {
_previewSize = CGSizeMake(352, 288);
break;
}
default: {
NSException *exception = [NSException
exceptionWithName:@"NoAvailableCaptureSessionException"
reason:@"No capture session available for current capture session."
userInfo:nil];
@throw exception;
}
default:
if ([_captureSession canSetSessionPreset:AVCaptureSessionPresetLow]) {
_captureSession.sessionPreset = AVCaptureSessionPresetLow;
_previewSize = CGSizeMake(352, 288);
} else {
NSError *error =
[NSError errorWithDomain:NSCocoaErrorDomain
code:NSURLErrorUnknown
userInfo:@{
NSLocalizedDescriptionKey :
@"No capture session available for current capture session."
}];
@throw error;
}
}
}

Expand Down
Loading