TIMOB-26599 Optionally disable video compression when importing video #10492
Conversation
Tests:
|
There was a problem hiding this comment.
Thanks for the PR! Please check the linting issues and document the new option.
cc @garymathews @jquick-axway Do we need parity with Android for this? Is there an equivalent option?
iphone/Classes/MediaModule.m
Outdated
| @@ -665,7 +665,7 @@ - (void)requestAudioRecorderPermissions:(id)args | |||
| TiThreadPerformOnMainThread(^() { | |||
| [[AVAudioSession sharedInstance] requestRecordPermission:^(BOOL granted) { | |||
| KrollEvent *invocationEvent = [[KrollEvent alloc] initWithCallback:callback | |||
| eventObject:[TiUtils dictionaryWithCode:(granted ? 0 : 1) message:nil] | |||
| eventObject:[TiUtils dictionaryWithCode:(granted ? 0 : 1)message:nil] | |||
There was a problem hiding this comment.
Please re-add the whitespace here and in the other places.
Tip: Run grunt format:ios to let clang-format fix linting issues for you!
There was a problem hiding this comment.
I was using clang-format. Somehow it seems to have done the deed with different settings and produced a different result.
iphone/Classes/MediaModule.m
Outdated
| @@ -1704,6 +1704,12 @@ - (void)showPicker:(NSDictionary *)args isCamera:(BOOL)isCamera | |||
| [self displayCamera:picker]; | |||
| } | |||
| } else { | |||
| if ([TiUtils isIOS11OrGreater]) { | |||
| BOOL disableVideoCompression = [TiUtils boolValue:@"disableVideoCompression" properties:args def:NO]; | |||
There was a problem hiding this comment.
Please document this option here: https://github.com/appcelerator/titanium_mobile/blob/53e39f5032eb42e3fac54b84d2f369af5c6ca95e/apidoc/Titanium/Media/Media.yml#L2017
|
I'm not sure if naming the property You're using the We currently offer a "videoQuality" property which can be set to low/medium/high. So, I'm guessing setting it to HIGH will still have iOS reformat the video from "h.264" to maybe a quicktime "mov" format? Is this the issue you're dealing with? Is it more of an issue with the returned video format? If you can tell me your use-case, then that would help. If you are after a "h.264" video format, then the issue here is we can't provide that on other platforms. The video format recorded will vary wildly on different Android devices. In any case, as it stands now, using the |
Hi Joshua. I was actually thinking on exposing the video quality property and constants. The code from the PR is a quick fix for my usage case. The current defaults cause bad UX when importing long videos from the gallery. It can take a long time to re-encode a 5 minutes video. On top of that, in the app I'm working on, I will later re-encode the video again using SDAVAssetExportSession libraries, to get a more fine tuned result. What shall we do, change the property name to useNativeFormat or directly expose the constants? @janvennemann @jquick-axway |
|
@rlustemberg , oh I see. This was a performance issue with it being re-encoded. Gotcha. (Sorry for not not fully understanding the issue.) I was just looking at this from a documentation and cross-platform perspective. Android doesn't re-encode videos selected from the gallery. So, this should never be an issue on Android. This sounds like an iOS only issue. How about we rename The word "transcoding" might better describe what's going on iOS. It's true by default. So in your case, you want to disable it and flip the code's logic if you don't mind. And the word "allow" fits with the other property settings too. @janvennemann, what do you think? |
|
Done! |
There was a problem hiding this comment.
@rlustemberg Thanks for PR! I have added few review comments.
iphone/Classes/MediaModule.m
Outdated
| @@ -1704,6 +1704,12 @@ - (void)showPicker:(NSDictionary *)args isCamera:(BOOL)isCamera | |||
| [self displayCamera:picker]; | |||
| } | |||
| } else { | |||
| if ([TiUtils isIOS11OrGreater]) { | |||
There was a problem hiding this comment.
Please use [TiUtils isIOSVersionOrGreater:@"11.0"] instead of [TiUtils isIOS11OrGreater] because later one is deprecated.
| type: Boolean | ||
| default: true | ||
| platforms: [iphone, ipad] | ||
| since: "8.1.0" | ||
|
|
There was a problem hiding this comment.
Please add minimum iOS version in document as well. Because this property is only available in iOS 11.0+.
e.g.
osver: {ios: {min: "11.0"}}
| @@ -1704,6 +1704,12 @@ - (void)showPicker:(NSDictionary *)args isCamera:(BOOL)isCamera | |||
| [self displayCamera:picker]; | |||
| } | |||
| } else { | |||
| if ([TiUtils isIOS11OrGreater]) { | |||
| BOOL allowTranscoding = [TiUtils boolValue:@"allowTranscoding" properties:args def:YES]; | |||
There was a problem hiding this comment.
Isn't it better if we expose it as a string/constant property and expose other constants as well which is available ? e.g Constants -
AVAssetExportPresetAppleM4A, AVAssetExportPresetHEVC3840x2160, AVAssetExportPresetHEVC1920x1080 etc.
There was a problem hiding this comment.
It is indeed. I've made a quick and dirty solution based on my use case, but I think it's better to provide full control. It's up to you guys, if you want that and the choice using strings or exporting the enum constants
There was a problem hiding this comment.
Those other constants tell iOS what to transcode the video formats to.
We can do this 1 of 2 ways.
(1) Keep the "allowTranscoding" property and add a "transcodeFormat" type later which tells the system which format to transcode to if "allowTranscoding" is true.
(2) Get rid of the "allowTranscoding" property and provide a "transcodeFormat" type instead which when set null (or to a NONE type?), will handle it from there.
I think the only things that worries me here is that format types available will vary between iOS versions... or perhaps worse, by device?
Hypothetically, if we were to support this on any other platform (Android and Windows), then you would need to query the system for what formats are available. You can't hard-code it to a format and expect it to magically work. For example, Android device manufacturers may not support some of Google's documented formats in order to lower the cost of the device since there is licensing/royalties involved (I've seen this with low-cost tablets in the past). And Windows N-edition and K-edition don't include any licensed formats by default.
Because of the above, I prefer to keep it simple for now.
Besides, wouldn't most devs want the original video format anyways?
There was a problem hiding this comment.
I agree with solution 1. Let's have it simple for now as it is. We can add "transcodeFormat" later.
@rlustemberg If you can resolve other two minor issues-
- add minimum iOS version in document
- Use [TiUtils isIOSVersionOrGreater:@"11.0"] instead of [TiUtils isIOS11OrGreater].
we will be good to go. Thanks!
There was a problem hiding this comment.
FR Passed: When 'allowTranscoding' is set to 'false' the newly formatted video is returned quicker than if 'allowTranscoding' was set to 'true'. Tested using the information found in: https://jira.appcelerator.org/browse/TIMOB-26599.
Ti.Media.openPhotoGallery({
allowTranscoding: false,
mediaTypes: [Ti.Media.MEDIA_TYPE_VIDEO],
success: function(e) {
// Got it.
},
});
Test Environment
iPhone 6S plus (12.1)
APPC CLI: 7.0.10-17
Operating System Name: Mac OS Mojave
Operating System Version: 10.14.2
Node.js Version: 8.9.1
Xcode 10.1
|
JIRA: https://jira.appcelerator.org/browse/TIMOB-26599
Optionally disable video compression when importing video