[image_picker]returned value should be of original video on iOS. #3457

jerryrt · 2021-01-25T12:19:42Z

[image_picker]Picked video should be of original video on iOS, i.e, no transcoding & resize.

The default pick-video behavior will get a transcoded video on iOS, which is small and of low quality, instead of the original video file. It is also not the same behavior on Android platform.

Two main problems for the video related code path:

The image picker behaves differently on different devices(iPhone 7, iPhone Xs, iPhone 12), even when they are of the same iOS version.(for my test, iOS 14.3). In my test, on iPhone 7, with "AVAssetExportPresetPassthrough" set on the picker controller, there is no value for "UIImagePickerControllerMediaURL" key; but on iPhone Xs/12, the key is valid.
Also, after reading the code, I found that in the picker delegate, the old way of detecting if the user is picking image or video is not that strict, only by checking if the "UIImagePickerControllerMediaURL" key is available. During the test, it will crash the app if the user is picking up video but can not find the value for key, because of the previous reason. There is better way of detecting it, by checking the media type key "UIImagePickerControllerMediaType".

Related issue and discussion:

flutter/flutter#55957

Pre-launch Checklist

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

stuartmorgan-g · 2021-02-09T21:21:57Z

Thanks for the submission! Could you clean up the PR (there's commented out code, which isn't something we would land) and add tests?

jerryrt · 2021-02-17T07:57:03Z

Thanks for the submission! Could you clean up the PR (there's commented out code, which isn't something we would land) and add tests?

Great, will follow your guide here.

Besides the removing of extra logging and commented code, I need to get familiar with the test setup in the plugins, will try my best to get it through.

stuartmorgan-g · 2021-02-23T21:11:44Z

I'm going to mark this as Draft for now to get it off our triage list; please mark it as ready once you've had a chance to update.

cyanglaz · 2021-03-25T18:43:16Z

@jerryrt I'm un-assigning myself from this PR. Please feel free to add me again when it's ready to be reviewed.

robertBadamshin · 2021-04-06T05:52:03Z

@jerryrt how are you? we need your pr :)

jerryrt · 2021-04-06T13:21:03Z

Oops, long delay here.

Our project moved to a new Flutter plugin for image selection because some extra feature is needed. However, I am still willing to pickup this pull request.

Since it's been days since the last check, and Flutter releases its 2.0 version, I will pull out the code to review it with current master branch, guess I could finish it in this week.

robertBadamshin · 2021-04-07T05:52:06Z

Which plugin do you use now?

jerryrt · 2021-04-07T06:05:30Z

Which plugin do you use now?

https://github.com/fluttercandies/flutter_wechat_assets_picker

Our project need the following feature which is hard to implement by current "image_picker"'s design, which is basically a Flutter wrapper over native system image picker APIs:

some degree of customization of the picker UI.
just one entrance to pickup image and/or video.

jerryrt · 2021-04-13T12:32:34Z

@jerryrt how are you? we need your pr :)

Hi.

I have merged my branch with official master branch, rechecked the code, and cleaned the code in this plugin.

Logging(NSLog) is all removed from the code. Only minimum comment is kept to document the different code path, but the format check in CI is still failing. Do I need to remove all the comment in the code?

For the test part, the sample project within this plugin works well.

Since this pull request does not change plugin API behavior, I also leave the XCTest code unmodified. But for running automated UI test, I can not find a way to successfully run the XCTest target, the console only popped out a few lines of complaint, and I have repeatedly uninstall/install several times already.

2021-04-13 20:10:42.013352+0800 RunnerUITests-Runner[1007:473959] Running tests...
2021-04-13 20:10:42.126271+0800 RunnerUITests-Runner[1007:473959] 未能载入软件包“RunnerUITests”，因为它已损坏或丢失必要的资源。请尝试重新安装软件包。
2021-04-13 20:10:42.126367+0800 RunnerUITests-Runner[1007:473959] (dlopen_preflight(/var/containers/Bundle/Application/A7FC8226-F9A1-422A-A0DC-2AE92E5EA04B/RunnerUITests-Runner.app/PlugIns/RunnerUITests.xctest/RunnerUITests): Library not loaded: @rpath/Flutter.framework/Flutter
Referenced from: /var/containers/Bundle/Application/A7FC8226-F9A1-422A-A0DC-2AE92E5EA04B/RunnerUITests-Runner.app/PlugIns/RunnerUITests.xctest/RunnerUITests
Reason: image not found)

Good day.

robertBadamshin · 2021-04-13T19:55:57Z

@jerryrt how are you? we need your pr :)

Hi.

I have merged my branch with official master branch, rechecked the code, and cleaned the code in this plugin.

Logging(NSLog) is all removed from the code. Only minimum comment is kept to document the different code path, but the format check in CI is still failing. Do I need to remove all the comment in the code?

For the test part, the sample project within this plugin works well.

Since this pull request does not change plugin API behavior, I also leave the XCTest code unmodified. But for running automated UI test, I can not find a way to successfully run the XCTest target, the console only popped out a few lines of complaint, and I have repeatedly uninstall/install several times already.

2021-04-13 20:10:42.013352+0800 RunnerUITests-Runner[1007:473959] Running tests...
2021-04-13 20:10:42.126271+0800 RunnerUITests-Runner[1007:473959] 未能载入软件包“RunnerUITests”，因为它已损坏或丢失必要的资源。请尝试重新安装软件包。
2021-04-13 20:10:42.126367+0800 RunnerUITests-Runner[1007:473959] (dlopen_preflight(/var/containers/Bundle/Application/A7FC8226-F9A1-422A-A0DC-2AE92E5EA04B/RunnerUITests-Runner.app/PlugIns/RunnerUITests.xctest/RunnerUITests): Library not loaded: @rpath/Flutter.framework/Flutter
Referenced from: /var/containers/Bundle/Application/A7FC8226-F9A1-422A-A0DC-2AE92E5EA04B/RunnerUITests-Runner.app/PlugIns/RunnerUITests.xctest/RunnerUITests
Reason: image not found)

Good day.

Hi.

I'm think you should ask @stuartmorgan or @cyanglaz about this :)

jerryrt · 2021-04-14T01:58:27Z

I'm going to mark this as Draft for now to get it off our triage list; please mark it as ready once you've had a chance to update.

Hi, @stuartmorgan , @cyanglaz the code is cleaned and reorganized, though the CI is still complaining?

Please check the last comment left on this thread for the details, and if further update is needed, I should be able to response timely for the rest of this week.

stuartmorgan-g · 2021-04-20T18:48:18Z

Only minimum comment is kept to document the different code path, but the format check in CI is still failing. Do I need to remove all the comment in the code?

You can click on the "Details" link for the format step to see what the formatting issues are; it's not related to comments.

And to clarify, I didn't say you should remove all comments, just the commented out code.

Since this pull request does not change plugin API behavior, I also leave the XCTest code unmodified.

It does change the plugin behavior though (the point of the PR is, after all, to change behavior), so needs testing—one or more tests that would fail without this PR, but path with it—per Flutter policy.

But for running automated UI test, I can not find a way to successfully run the XCTest target, the console only popped out a few lines of complaint, and I have repeatedly uninstall/install several times already.

Was this via https://github.com/flutter/plugins/blob/master/script/tool/README.md#run-xctests or some other method?

jerryrt · 2021-04-21T08:53:28Z

Only minimum comment is kept to document the different code path, but the format check in CI is still failing. Do I need to remove all the comment in the code?

You can click on the "Details" link for the format step to see what the formatting issues are; it's not related to comments.

And to clarify, I didn't say you should remove all comments, just the commented out code.

Thanks for the hint, I followed the README and run the formatter locally to solve the format issue and made another new commit of it.

flutter_plugins % dart run ./script/tool/lib/src/main.dart format --plugins image_picker

Looks like it works.

Since this pull request does not change plugin API behavior, I also leave the XCTest code unmodified.

It does change the plugin behavior though (the point of the PR is, after all, to change behavior), so needs testing—one or more tests that would fail without this PR, but path with it—per Flutter policy.

Yes, this PR does change the old behavior, but I had difficulty to update/implement the test coverage code.

First, I think there is no existing test coverage for video picking yet. I located the related test code here:

https://github.com/flutter/plugins/blob/b548ae18646d1725a8e1f004c26bf42e4d362aa8/packages/image_picker/image_picker/example/ios/RunnerUITests/ImagePickerFromGalleryUITests.m

As far as I can tell, it doesn't have a test case for video picking yet. For existing tests, the CI reported all tests passed.

Second, for image picking part, the current test only tests if the picked image exists or not, but doesn't care the image's actual content at all. For my test, ideally, I should implement a video picking test logic, and check if the picked one is the same file as I prepared beforehand , but I am puzzled how I can prepare a video sample for the emulator in CI env.

But for running automated UI test, I can not find a way to successfully run the XCTest target, the console only popped out a few lines of complaint, and I have repeatedly uninstall/install several times already.

Was this via https://github.com/flutter/plugins/blob/master/script/tool/README.md#run-xctests or some other method?

Yes.

For Dart test, all of the tests passed.

flutter_plugins % dart run ./script/tool/lib/src/main.dart test --plugins image_picker

For XCTest, it fails.

dart run ./script/tool/lib/src/main.dart xctest --target RunnerUITests --plugins image_picker

However, I thought I found the issue this time.

error: The linked library 'libPods-RunnerUITests.a' is missing one or more architectures required by this target: x86_64. (in target 'RunnerUITests' from project 'Runner')

I am developing and testing on a M1 mac, so this could be the reason. I am checking if I can enable the ARM64 architecture on the tests, or I will find a x86 machine to run it again locally.

robertBadamshin · 2021-05-12T07:04:32Z

@stuartmorgan should it get approve?

robertBadamshin · 2021-06-04T22:03:57Z

Oh my, why this MR still opened.....( @jerryrt need help?

jerryrt · 2021-06-08T01:53:21Z

Oh my, why this MR still opened.....( @jerryrt need help?

Yes, it is stuck somewhere.

In the early feedback, I failed to run the XCTest provided by original plugin developers, if this is the only issue, I do have a x86 dev machine at the moment, will re-run the XCTest this week and post the result here.

Issues from CI system are all fixed.

However, if full coverage of unit/integration test is required, some help is needed, quoted from early post.

Yes, this PR does change the old behavior, but I had difficulty to update/implement the test coverage code.

First, I think there is no existing test coverage for video picking yet. I located the related test code here:

https://github.com/flutter/plugins/blob/b548ae18646d1725a8e1f004c26bf42e4d362aa8/packages/image_picker/image_picker/example/ios/RunnerUITests/ImagePickerFromGalleryUITests.m

As far as I can tell, it doesn't have a test case for video picking yet. For existing tests, the CI reported all tests passed.

Second, for image picking part, the current test only tests if the picked image exists or not, but doesn't care the image's actual content at all. For my test, ideally, I should implement a video picking test logic, and check if the picked one is the same file as I prepared beforehand , but I am puzzled how I can prepare a video sample for the emulator in CI env.

stuartmorgan-g · 2021-06-08T11:35:27Z

Sorry, I missed your earlier comment.

However, if full coverage of unit/integration test is required

It does, as I said above: "It does change the plugin behavior though (the point of the PR is, after all, to change behavior), so needs testing—one or more tests that would fail without this PR, but path with it—per Flutter policy."

See also https://github.com/flutter/flutter/wiki/Tree-hygiene#tests if you want more details on the policy.

First, I think there is no existing test coverage for video picking yet.

Much of the code in this repository predates the strict testing policy, and our current test coverage is not where we'd like. The way test coverage improves is by applying stricter policies going forward than have been used in the past. Note that "doesn't have similar tests yet" is not one of the exemption reasons described above.

For existing tests, the CI reported all tests passed.

That was true before your PR as well. Again, this requires adding "one or more tests that would fail without this PR".

For my test, ideally, I should implement a video picking test logic, and check if the picked one is the same file as I prepared beforehand , but I am puzzled how I can prepare a video sample for the emulator in CI env.

UI tests run in the context of the example application; I would expect that you could add a small video file to the example app, have the UI test select it, and then validate its metadata (e.g., size).

Alternately, and perhaps more straightforwardly, you could extract all the logic around handling the path returned by UIImagePickerController into a well-encapsulated helper, and then write native unit tests specifically for that.

For XCTest, it fails.

dart run ./script/tool/lib/src/main.dart xctest --target RunnerUITests --plugins image_picker

However, I thought I found the issue this time.

error: The linked library 'libPods-RunnerUITests.a' is missing one or more architectures required by this target: x86_64. (in target 'RunnerUITests' from project 'Runner')

I am developing and testing on a M1 mac, so this could be the reason. I am checking if I can enable the ARM64 architecture on the tests, or I will find a x86 machine to run it again locally.

@jmagman Is this a known issue? I'm not up on what exactly is currently expected to work on M1.

jmagman · 2021-06-08T18:07:22Z

@jmagman Is this a known issue? I'm not up on what exactly is currently expected to work on M1.

I believe it should work, xctest assumes the example projects are already built. Maybe we should add a flag to it that builds the example project to make testing locally easier.

Also, --target was removed in #3667.

@jerryrt in the meantime, if you rebase on that, does this work?

$ cd packages/image_picker/image_picker/example
$ flutter build ios --config-only
$ cd -
$ dart run ./script/tool/lib/src/main.dart xctest --plugins image_picker

jerryrt · 2021-06-11T08:26:07Z

@jmagman Is this a known issue? I'm not up on what exactly is currently expected to work on M1.

I believe it should work, xctest assumes the example projects are already built. Maybe we should add a flag to it that builds the example project to make testing locally easier.

Also, --target was removed in #3667.

@jerryrt in the meantime, if you rebase on that, does this work?
$ cd packages/image_picker/image_picker/example
$ flutter build ios --config-only
$ cd -
$ dart run ./script/tool/lib/src/main.dart xctest --plugins image_picker

Just had time to follow this.

No, it failed. After rebase my branch to official master branch, here is the output from the cmd output.

jerry.tian@MacBook-Air-M1 flutter_plugins % cd packages/image_picker/image_picker/example
jerry.tian@MacBook-Air-M1 example % flutter build ios --config-only
Running "flutter pub get" in example...                          1,069ms
Building io.flutter.plugins.imagePickerExample for device (ios-release)...
Warning: Missing build name (CFBundleShortVersionString).
Warning: Missing build number (CFBundleVersion).
Action Required: You must set a build name and number in the pubspec.yaml file version field before submitting to the App Store.
Signing iOS app for device deployment using developer identity: "Apple Development: ******"
Running pod install...                                           1,231ms
jerry.tian@MacBook-Air-M1 example % cd -
~/Projects/Flutter_And_Dart/flutter_plugins
jerry.tian@MacBook-Air-M1 flutter_plugins % dart run ./script/tool/lib/src/main.dart xctest --plugins image_picker
script/tool/lib/src/main.dart: Warning: Interpreting this as package URI, 'package:flutter_plugin_tools/src/main.dart'.
script/tool/lib/src/common.dart:21:37: Error: Null safety features are disabled for this library.
Try removing the package language version or setting the language version to 2.12 or higher.
typedef Print = void Function(Object? object);
                                    ^
script/tool/lib/src/common.dart:87:21: Error: Null safety features are disabled for this library.
Try removing the package language version or setting the language version to 2.12 or higher.
    {PlatformSupport? requiredMode}) {
                    ^
script/tool/lib/src/common.dart:169:25: Error: The modifier 'required' is only available in null safe libraries.
void printErrorAndExit({required String errorMessage, int exitCode = 1}) {
                        ^^^^^^^^
script/tool/lib/src/common.dart:169:25: Error: Null safety features are disabled for this library.
Try removing the package language version or setting the language version to 2.12 or higher.
void printErrorAndExit({required String errorMessage, int exitCode = 1}) {
                        ^^^^^^^^
script/tool/lib/src/common.dart:249:15: Error: Null safety features are disabled for this library.
Try removing the package language version or setting the language version to 2.12 or higher.
  final GitDir? gitDir;
              ^
script/tool/lib/src/common.dart:251:6: Error: Null safety features are disabled for this library.
Try removing the package language version or setting the language version to 2.12 or higher.
  int? _shardIndex;
     ^
script/tool/lib/src/common.dart:252:6: Error: Null safety features are disabled for this library.
Try removing the package language version or setting the language version to 2.12 or higher.
  int? _shardCount;
     ^
script/tool/lib/src/common.dart:542:14: Error: Null safety features are disabled for this library.
Try removing the package language version or setting the language version to 2.12 or higher.
    Directory? workingDir,
             ^
script/tool/lib/src/common.dart:574:17: Error: Null safety features are disabled for this library.
Try removing the package language version or setting the language version to 2.12 or higher.
      {Directory? workingDir,
                ^
script/tool/lib/src/common.dart:602:20: Error: Null safety features are disabled for this library.
Try removing the package language version or setting the language version to 2.12 or higher.
  Future<io.Process?> start(String executable, List<String> args,
                   ^

jerryrt · 2021-06-11T08:32:20Z

Another follow up.

After reading the official master branch's commit history, I found this plugin did have some active development since the time I put my first line of code of change.

Thus , I suggest it would be better to close this PR here, and if time permits, I will create a new PR, based on the updated plugin code then, and put the new logic back.

stuartmorgan-g · 2021-06-23T17:26:10Z

After rebase my branch to official master branch, here is the output from the cmd output.

That looks like your tree was in a bad state, with at least part of the tools directory not synced to the same part as the rest. That script drives our CI, and there was never a state during the null safety migration where it wouldn't run.

I found this plugin did have some active development since the time I put my first line of code of change.

It doesn't look like there are any conflicts; do you see issues with your patch at master?

stuartmorgan-g · 2021-09-14T19:02:22Z

Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to submit a new PR if you have the time to address the review comments. Thanks!

jerryrt added 7 commits January 20, 2021 16:04

return original file instead of compressed one for video selection.

25276b0

fix typo

6857a06

use AVAssetExportPresetPassthrough to get original video.

3a53008

remove tmp&dirty hack.

3f4ca35

iOS Picker API behave differently on old/new devices, fix it.

1689eb9

Merge branch 'master' of https://github.com/flutter/plugins

cf202bb

clean comment.

8290916

jerryrt requested a review from cyanglaz as a code owner January 25, 2021 12:19

google-cla bot added the cla: yes label Jan 25, 2021

github-actions bot added the p: image_picker label Jan 25, 2021

Merge commit 'd2c011f80a0bbb2b40752c934566e5b8ad6c148f'

a619ad3

github-actions bot added the platform-ios label Feb 19, 2021

stuartmorgan-g marked this pull request as draft February 23, 2021 21:11

jerryrt added 2 commits March 3, 2021 16:26

Merge commit 'a6cbf671e3463953b7ade113537cbe043dec4a0e'

df742ce

Merge commit '47c380a9bbc78bae8b18a849183cab0e21a703f4'

f04f7cb

cyanglaz removed their request for review March 25, 2021 18:42

jerryrt added 2 commits April 12, 2021 18:39

Merge commit 'ee5d15e443b5d6de432a504a7f56e47aab8abda5'

8a39fca

remove NSLog for debuging, though some logging is kept as comment.

2aa3a5e

jerryrt force-pushed the master branch from 8d89f11 to 2aa3a5e Compare April 13, 2021 12:13

stuartmorgan-g marked this pull request as ready for review April 20, 2021 18:53

fix code format issue by calling clang-format(the one used in CI).

21feecb

jerryrt force-pushed the master branch from 45b782a to 21feecb Compare June 11, 2021 08:10

stuartmorgan-g closed this Sep 14, 2021

ramirezblindside mentioned this pull request Feb 22, 2023

Adding options to compress/original the picked videos in image_picker. flutter/flutter#24954

Open

[image_picker]returned value should be of original video on iOS. #3457

[image_picker]returned value should be of original video on iOS. #3457

Uh oh!

Conversation

jerryrt commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

stuartmorgan-g commented Feb 9, 2021

Uh oh!

jerryrt commented Feb 17, 2021

Uh oh!

stuartmorgan-g commented Feb 23, 2021

Uh oh!

cyanglaz commented Mar 25, 2021

Uh oh!

robertBadamshin commented Apr 6, 2021

Uh oh!

jerryrt commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertBadamshin commented Apr 7, 2021

Uh oh!

jerryrt commented Apr 7, 2021

Uh oh!

jerryrt commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertBadamshin commented Apr 13, 2021

Uh oh!

jerryrt commented Apr 14, 2021

Uh oh!

stuartmorgan-g commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerryrt commented Apr 21, 2021

Uh oh!

robertBadamshin commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertBadamshin commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerryrt commented Jun 8, 2021

Uh oh!

stuartmorgan-g commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmagman commented Jun 8, 2021

Uh oh!

jerryrt commented Jun 11, 2021

Uh oh!

jerryrt commented Jun 11, 2021

Uh oh!

stuartmorgan-g commented Jun 23, 2021

Uh oh!

stuartmorgan-g commented Sep 14, 2021

Uh oh!

Uh oh!

jerryrt commented Jan 25, 2021 •

edited

Loading

jerryrt commented Apr 6, 2021 •

edited

Loading

jerryrt commented Apr 13, 2021 •

edited

Loading

stuartmorgan-g commented Apr 20, 2021 •

edited

Loading

robertBadamshin commented May 12, 2021 •

edited

Loading

robertBadamshin commented Jun 4, 2021 •

edited

Loading

stuartmorgan-g commented Jun 8, 2021 •

edited

Loading