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

quentinleguennec · 2019-03-26T18:34:24Z

Description

This PR is here to address issues in the quality of video recording for the camera plugin on Android, as well as discrepancies in quality level between Android and iOS. It also allows to control the still picture quality (which was so far only maximum quality). The existing resolution presets have been updated to match between Android and iOS, and 2 new presets have been added (veryHigh and veryLow).

There are no breaking changes but the quality of the presets have been adjusted which will result in different picture/video quality on apps using this plugin.

We believe this change is necessary as, for the same resolution preset, Android video quality was very poor and the quality on iOS was to high, generating 4k files that are too heavy to upload.

Related Issues

#1186
#1107
flutter/flutter#30459
flutter/flutter#29500
flutter/flutter#26959

Checklist

Breaking Change

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.
But: Existing presets have been updated, this will affect the quality of pictures and videos in existing apps.

quentinleguennec · 2019-03-27T10:20:57Z

EDIT: I updated clang-format and now the formating is working fine.

The format test are failing but I ran flutter_plugin_tools (and it did the changes that are now failing).

pub global run flutter_plugin_tools format --plugins camera &&\
pub global run flutter_plugin_tools analyze --plugins camera &&\
pub global run flutter_plugin_tools test --plugins camera
Downloading Google Java Format...
Formatting all .dart files...
Unchanged /Users/mario/dev/flutter-plugins/packages/camera/example/lib/main.dart
Unchanged /Users/mario/dev/flutter-plugins/packages/camera/lib/camera_image.dart
Unchanged /Users/mario/dev/flutter-plugins/packages/camera/lib/camera.dart
Formatting all .java files...
Formatting all .m and .h files...
Activating tuneup package...
Package tuneup is currently active at version 0.3.6+1.
Resolving dependencies...
+ analysis_server_lib 0.1.4+2
+ args 1.5.1
+ charcode 1.1.2
+ cli_util 0.1.3+2
+ collection 1.14.11
+ intl 0.15.8
+ logging 0.11.3+2
+ meta 1.1.7
+ path 1.6.2
+ source_span 1.5.5
+ string_scanner 1.0.4
+ term_glyph 1.1.0
+ tuneup 0.3.6+1
+ yaml 2.1.15
Precompiling executables...
Precompiled tuneup:tuneup.
Installed executable tuneup.
Activated tuneup 0.3.6+1.
Running "flutter packages get" in camera...                         0.6s
Running "flutter packages get" in example...                        0.5s
Running "flutter packages get" in example...                        0.8s
Checking project camera...
No issues found; analyzed 5 source files in 7.6s.



No analyzer errors found!
SKIPPING camera - no test subdirectory
SKIPPING camera/example - no test subdirectory



All tests are passing!

jonasbark · 2019-03-27T15:25:37Z

packages/camera/android/src/main/java/io/flutter/plugins/camera/CameraPlugin.java

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


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.

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.

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.

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.

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.

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.

…xed uncaught exception.

quentinleguennec · 2019-03-29T12:23:02Z

Hi @mklim @bparrishMines,

The automated CI tests seems to be broken (the error message suggest a server issue). But the code passes all the tests when I run

pub global activate flutter_plugin_tools &&\
pub global run flutter_plugin_tools format --plugins camera &&\
pub global run flutter_plugin_tools analyze --plugins camera &&\
pub global run flutter_plugin_tools test --plugins camera

I think we can proceed with the review and I'll push a changeless version later to trigger the test again.
I have an other PR fixing video orientation metadata on iOS ready for when this one is merged.
Thanks.

quentinleguennec · 2019-04-17T10:50:09Z

Hi @mklim @bparrishMines ,
Can we get an ETA on when this PR could be reviewed?
Thanks

danieleformichelli · 2019-05-16T17:15:14Z

Is there any ETA for this to be merged? Video quality on Android is VERY bad

quentinleguennec · 2019-05-21T10:15:11Z

I'll drop this here while we wait for the PR review, it's a fix for the camera plugin and video plugin recording quality and rotation issues. For people interested in a fix have a look at the discussion on this issue: flutter/flutter#29951

Copy-paste, for more fixes (like issues with aspect ratio) check the previous link:
When I worked on solving those issue I did 4 fixes in the camera plugin and video_player plugin. And it is quite possible it will only work well only with the 4 fixes together. Here is the list of PR (only one of them has been merged so far and I have yet to get any answer from reviewers on the others):

flutter/plugins#1452
flutter/plugins#1403
flutter/plugins#1451
flutter/plugins#1470

I created a branch with those 4 fixes merged together, I suggest you try this branch first and see if it works on your side:
https://github.com/Tengio/plugins/tree/tested4you_video_player_camera_merge

You can use it by adding this in your pubspec.yaml:

 video_player:
    git:
      url: git://github.com/tengio/plugins.git
      ref: 310a8f06c4981121a5553b0f6f3a8cdf475b8af1
      path: packages/video_player
  camera:
    git:
      url: git://github.com/tengio/plugins.git
      ref: 310a8f06c4981121a5553b0f6f3a8cdf475b8af1
      path: packages/camera

danieleformichelli · 2019-06-03T05:45:00Z

@quentinleguennec using your branch fixes most of my problems on Android, in particular for the video quality.
I still have one problem: When I take a photo in portrait mode and I share it on telegram, it is sent rotated, if I share it through WhatsApp, it goes fine.
Using the master branch instead, I have the same problem but only when I take photo in landscape mode.
Are you aware of this? Is there any plan to fix it?

quentinleguennec · 2019-06-03T09:23:43Z

Hi @danyf90
I didn't intended to change the metadata of images with this PR, just videos, so that's a side effect.
Could you provide me with the following information please? :

Is this happening on Android or iOS or both?
How are you sharing it? By saving it to your device and then opening Whatsapp/Telegram and selecting it from there?
Does it happen if you take a picture with the default camera app of your device and share it extactly the same way?
Thanks

danieleformichelli · 2019-06-03T21:26:13Z

Ciao @quentinleguennec ,
Sorry for not being clear.
I have tested the app only on Android.
I got the problem only if I share the file using the file generated by the CameraController.takePicture using esys-flutter-share.
The problem could also be in the esys-flutter-share plug-in, but the fact that the behavior is changed is suspicious.

In case you want/need to test on your side, code is available here.

Thanks for your help

quentinleguennec · 2019-06-05T17:08:07Z

Hi @danyf90
I downloaded your project, I had a few issues to build it but I got it to run.
I'll try to have a look at the image rotation issue, but the timing is quite bad, I won't be available for a month starting this Saturday. I'll keep you updated if I manage to do it before Saturday.

khietlam · 2019-06-06T08:26:44Z

Thank you @quentinleguennec ,
I'm using your branch. It fixed all most my needs on iOS.

There have a issue when I take and upload a photo ( portrait mode) to the server. It will be rotated 90 degree counter-clockwise (landscape mode).

I tried to take photo with the default app in iPhone 6 plus and upload to server. Nothing changes abnormally.

Best.

bpillon · 2019-07-03T14:24:17Z

Please merge this ! Really useful :)

juliocbcotta · 2019-07-11T01:08:42Z

I don't know if this helps anyone, but I merged in my fork @quentinleguennec branch and master of this repo. Now I can have a better video quality and the fixes in here.
If anyone is interested https://github.com/BugsBunnyBR/plugins

1660293 · 2019-07-13T12:36:15Z

I don't know if this helps anyone, but I merged in my fork @quentinleguennec branch and master of this repo. Now I can have a better video quality and the fixes in here.
If anyone is interested https://github.com/BugsBunnyBR/plugins

This camera ratio is too bad!!!

bparrishMines · 2019-07-22T23:11:35Z

Thanks for the contribution!

This PR isn't trivial to review and we've begun refactoring the camera plugin as a whole, so I'm labeling it with "backlog". We will prioritize creating a temporary hotfix with this PR according to the issue's priority.

Relevant issues:
#1186
#1107
flutter/flutter#30459
flutter/flutter#29500
flutter/flutter#26959

mklim

Thanks for the solid contribution and the patience while we review this. This PR is fixing a high priority issue so I've moved it out of the backlog and reviewed it. The basic approach looks good, I have some feedback before merging. If you don't have time to keep working on this PR, let us know and one of us will be happy to shepherd it forward from here. Thanks again for the fix!

Nothing in this patch looks like it would effect orientation. I suspect that the rotation bug reported in earlier comments here have to do with pre-existing issues like flutter/flutter#35334.

In general it's unfortunate that we can't really test this. I'm manually testing this to verify for now. Testability will be great to have as part of the upcoming camera refactor.

nit: This needs to have merge conflicts fixed with the latest master, pubpec.yaml, etc.

mklim · 2019-07-30T18:19:00Z

packages/camera/CHANGELOG.md

+* 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.


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?

mklim · 2019-07-30T18:21:06Z

packages/camera/android/src/main/java/io/flutter/plugins/camera/CameraPlugin.java

-      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.


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.

mklim · 2019-07-30T18:25:09Z

packages/camera/android/src/main/java/io/flutter/plugins/camera/CameraPlugin.java

+    // The preview should never be bigger than 720p (1280 x 720) or it will mess up the recording.
+    private void computeBestPreviewSize(CamcorderProfile profile) {
+      float ratio = (float) profile.videoFrameWidth / profile.videoFrameHeight;
+      if (profile.videoFrameWidth > 1280) {


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

mklim · 2019-07-30T18:28:47Z

packages/camera/android/src/main/java/io/flutter/plugins/camera/CameraPlugin.java

+        CamcorderProfile profile,
+        String resolutionPreset) {
+      // For still image captures, use the largest image size if resolutionPreset is veryHigh
+      if ("veryHigh".equals(resolutionPreset)) {


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.

mklim · 2019-07-30T19:43:44Z

packages/camera/android/src/main/java/io/flutter/plugins/camera/CameraPlugin.java

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


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.

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.

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.

mklim · 2019-07-30T19:46:24Z

packages/camera/android/src/main/java/io/flutter/plugins/camera/CameraPlugin.java

+      int cameraId = Integer.parseInt(cameraName);
+      switch (camcorderProfileIndex) {
+        case 0:
+          if (CamcorderProfile.hasProfile(cameraId, CamcorderProfile.QUALITY_2160P)) {


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

mklim · 2019-07-30T19:57:08Z

packages/camera/example/lib/main.dart

@@ -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);


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

mklim · 2019-07-30T20:02:57Z

packages/camera/ios/Classes/CameraPlugin.m

@@ -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];


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.

mklim · 2019-07-30T20:17:59Z

packages/camera/ios/Classes/CameraPlugin.m

@@ -227,14 +234,25 @@ - (void)captureToFile:(NSString *)path result:(FlutterResult)result {

 - (void)setCaptureSessionPreset:(NSString *)resolutionPreset {


(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.

mklim · 2019-08-07T01:47:32Z

This has been sitting without response for awhile now and we would like to land it, so I've continued the work started here in #1952. I'm happy to continue working on landing my patches in this PR instead if that's easier, but someone with commit rights will need to apply the new changes to this branch. I'm going to close this in favor of the updated PR, let me know if you need anything changed. Thanks again for the contribution, this is extremely helpful.

quentinleguennec · 2019-08-08T15:46:14Z

Hi @mklim. I have reduced availability right now to work on this PR. Happy to see it moved and taken care of in #1952. Not sure I'll have time to check it out but if I do I'll try to land a hand where needed :)

quentinleguennec requested review from bparrishMines and mklim as code owners March 26, 2019 18:34

googlebot added the cla: yes label Mar 26, 2019

quentinleguennec mentioned this pull request Mar 27, 2019

[camera] change mediaRecorder.setVideoEncodingBitRate for better video quality #1107

Closed

jonasbark reviewed Mar 27, 2019

View reviewed changes

bparrishMines added the feature label Mar 27, 2019

quentinleguennec added 4 commits March 28, 2019 10:23

Added camcorderProfile to Android for better quality preset selection.

9a774a8

Improved best profile selection algorithm.

1232d22

Camera: Now resolutionPreset also influence image capture quality. Fi…

39c2d0e

…xed uncaught exception.

Use medium resolution preset for the example.

dcac87d

quentinleguennec force-pushed the tengio_improved_camera_plugin branch from f47e1de to dcac87d Compare March 28, 2019 10:30

Fixed merge conflict with version 0.4.3+2.

018521d

This was referenced Apr 5, 2019

Support output resolution and compression options in Camera plugin flutter/flutter#30459

Closed

Camera Package Issues flutter/flutter#29500

Closed

Flutter camera plugin record quality is very poor flutter/flutter#26959

Closed

Fixed formating.

945dc88

bparrishMines mentioned this pull request Apr 17, 2019

Future of the Camera Plugin (Refactor/Rework) flutter/flutter#31225

Closed

quentinleguennec mentioned this pull request May 3, 2019

Flutter Camera plugin captures video rotated 90 degrees when in Landscape on iPhone flutter/flutter#29951

Closed

bparrishMines added the backlog label Jul 22, 2019

mklim removed the backlog label Jul 30, 2019

mklim self-assigned this Jul 30, 2019

mklim reviewed Jul 30, 2019

View reviewed changes

mklim added in review labels Jul 30, 2019

mklim mentioned this pull request Aug 7, 2019

[Camera] Improved resolution presets #1952

Merged

13 tasks

mklim closed this Aug 7, 2019

mklim removed the WIP label Aug 7, 2019

		@@ -227,14 +234,25 @@ - (void)captureToFile:(NSString *)path result:(FlutterResult)result {

		- (void)setCaptureSessionPreset:(NSString *)resolutionPreset {

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

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

Uh oh!

Conversation

quentinleguennec commented Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

quentinleguennec commented Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasbark Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quentinleguennec commented Mar 29, 2019

Uh oh!

quentinleguennec commented Apr 17, 2019

Uh oh!

danieleformichelli commented May 16, 2019

Uh oh!

quentinleguennec commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danieleformichelli commented Jun 3, 2019

Uh oh!

quentinleguennec commented Jun 3, 2019

Uh oh!

danieleformichelli commented Jun 3, 2019

Uh oh!

quentinleguennec commented Jun 5, 2019

Uh oh!

khietlam commented Jun 6, 2019

Uh oh!

bpillon commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juliocbcotta commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1660293 commented Jul 13, 2019

Uh oh!

bparrishMines commented Jul 22, 2019

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mklim commented Aug 7, 2019

Uh oh!

quentinleguennec commented Aug 8, 2019

Uh oh!

Uh oh!

quentinleguennec commented Mar 26, 2019 •

edited

Loading

quentinleguennec commented Mar 27, 2019 •

edited

Loading

jonasbark Mar 27, 2019 •

edited

Loading

quentinleguennec commented May 21, 2019 •

edited

Loading

bpillon commented Jul 3, 2019 •

edited

Loading

juliocbcotta commented Jul 11, 2019 •

edited

Loading