[video_player_avfoundation] Applies the standardized transform for videos with different orientations by AlexV525 · Pull Request #5069 · flutter/plugins

AlexV525 · 2022-03-18T03:30:35Z

Changes:

Split a standalone category (AVAssetTrack+Utils).
Add the FakeAVAssetTrack in the test.
Tested with all values of UIImageOrientation.

Pre-launch Checklist

AlexV525 · 2022-03-18T05:09:39Z

/cc @jmagman Would a test like: Test if transform.tx equals to naturalSize.height effective? Also, we might need a vertical taken video for this fix. The current fix only works like magic. 🪄

AlexV525 · 2022-03-20T08:39:58Z

The current test only tests with the existing video's transform in the new case. If we want to cover more cases, we might need another video for this test.

packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m

jmagman · 2022-03-22T22:49:29Z

Can you confirm that the reproduction case in flutter/flutter#17606 is not regressed?

AlexV525 · 2022-03-23T02:14:39Z

Can you confirm that the reproduction case in flutter/flutter#17606 is not regressed?

This fix does not change the orientation of videos, since it does not manipulate with a, b, c, d of the CGAffineTransform. Though I'll post more screenshots with more cases later.

AlexV525 · 2022-03-23T17:42:48Z

I didn't get the time to make screenshots, but based on the comment at fluttercandies/flutter_photo_manager#685 (comment), all cases are well addressed.

hellohuanlin · 2022-03-24T00:43:56Z

@AlexV525 I checked out your code and replaced the example remote url to https://squiredev.com/golf.mp4, but still got the same issue

AlexV525 · 2022-03-24T02:36:45Z

@AlexV525 I checked out your code and replaced the example remote url to https://squiredev.com/golf.mp4, but still got the same issue

@hellohuanlin Did you override the avfoundation to the local path in the example? With the video you provided, I got the correct display on my device.

D997FCCE6F3C744B4D0C156E09188EFE.mp4

hellohuanlin · 2022-03-24T18:41:08Z

@AlexV525 I checked out your code and replaced the example remote url to https://squiredev.com/golf.mp4, but still got the same issue

@hellohuanlin Did you override the avfoundation to the local path in the example? With the video you provided, I got the correct display on my device.

D997FCCE6F3C744B4D0C156E09188EFE.mp4

Not sure what you mean by "override the avfoundation to the local path. can you share the code?

packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m

packages/video_player/video_player_avfoundation/CHANGELOG.md

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

AlexV525 · 2022-03-25T06:28:17Z

Not sure what you mean by "override the avfoundation to the local path. can you share the code?

In video_player/example/pubspec.yaml, add the following code and run flutter pub get again:

dependency_overrides:
  video_player_avfoundation:
    path: ../../video_player_avfoundation

AlexV525 · 2022-03-25T13:11:06Z

I've done a further investigation about what is the preferredTransform represents in a video track. It turns out it's corresponding to UIImage.Orientation, each case mentioned in the answer correspond to an orientation case. So I've made four videos and manipulated them with rotations, splitting all cases out with comments for better understanding.

stuartmorgan-g

Sorry, I hit the wrong button with my last comments; they were intended to be "request changes" not "approve", as I don't think this has sufficient test coverage.

packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m

…ion`

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

stuartmorgan-g · 2022-04-07T19:13:08Z

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

+ * on iOS 14 and above. This method provide a standardized transform
+ * according to the orientation of the track.
+ */
+static CGAffineTransform FLTGetStandardizedTransformForTrack(AVAssetTrack *track) {


A static function is internal to the compilation unit (this file), so can't be access from the test. You need a public declaration of this in a header, just as you do for ObjC classes you want to use in other files.

You should have a separate AVAssetTrackUtils.h and AVAssetTrackUtils.m where you declare and implement this, just as you did when you had it as a category, just using a (non-static) function like this instead of a category.

Thanks! That helps a lot

packages/video_player/video_player_avfoundation/CHANGELOG.md

hellohuanlin · 2022-04-08T19:49:39Z

packages/video_player/video_player_avfoundation/ios/Classes/AVAssetTrackUtils.h

+/**
+ * Note: https://stackoverflow.com/questions/64161544
+ * `AVAssetTrack.preferredTransform` can have wrong `tx` and `ty`
+ * on iOS 14 and above. This function provides a standardized transform


oh just double check - have you tested both iOS 14+ and earlier OS version?

Sorry I can only and have tested this on iOS14+...

let me try it on earlier OS. could you please hold this PR for now?

let me try it on earlier OS. could you please hold this PR for now?

Sure. Thanks!

Tested on iOS 13.7 and it worked. I am curious where is this "iOS 14 and above" coming from?

this one: "used to be broken and now is working"

let me try iOS 12 too

it shows a blank video on iOS 12.4. Same result with or without this fix.

Let me update the comment then.

it shows a blank video on iOS 12.4. Same result with or without this fix.

Maybe we can file another issue to track this?

sounds good if you think it's unrelated.

stuartmorgan-g

LGTM with nits.

packages/video_player/video_player_avfoundation/CHANGELOG.md

packages/video_player/video_player_avfoundation/ios/Classes/AVAssetTrackUtils.h

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

…orm for videos with different orientations (flutter/plugins#5069)

* master: (153 commits) Roll Flutter from fd360c4 to ef5a6da (1 revision) (flutter#5298) [video_player_avfoundation] Applies the standardized transform for videos with different orientations (flutter#5069) Roll Flutter from 3752fb7 to fd360c4 (2 revisions) (flutter#5295) Roll Flutter from 3c4d7a1 to 3752fb7 (11 revisions) (flutter#5294) Add owners for Android implementations (flutter#5293) Roll Flutter from f4875ae to 3c4d7a1 (2 revisions) (flutter#5292) [video_player_web] Stop buffering when browser canPlayThrough. (flutter#5068) Roll Flutter from aa5d7b6 to f4875ae (1 revision) (flutter#5289) Roll Flutter from 44be0b8 to aa5d7b6 (12 revisions) (flutter#5286) Roll Flutter from ec8289c to 44be0b8 (2 revisions) (flutter#5284) Roll Flutter from 329ceae to ec8289c (26 revisions) (flutter#5283) [flutter_plugin_tools] Preserve Dart SDK version in all-plugins-app (flutter#5281) [webview_flutter_wkwebview] Implements the `HostApis` and methods for the `CookieManager`. (flutter#5244) [webview_flutter_wkwebview] Implement `WKNavigationDelegate.didFinishNavigation` as a proof of concept for callback methods (flutter#5199) Roll Flutter from 08e467d to 2b83332 (5 revisions) (flutter#5270) Roll Flutter from e2d1206 to 08e467d (1 revision) (flutter#5268) Roll Flutter from ff9d6e5 to e2d1206 (2 revisions) (flutter#5266) Roll Flutter from 0575932 to ff9d6e5 (1 revision) (flutter#5265) [local_auth] Refactor package to make use of new platform interface and native implementations (flutter#4701) Roll Flutter from cb968c5 to 0575932 (1 revision) (flutter#5263) ...

…orm for videos with different orientations (flutter/plugins#5069)

…deos with different orientations (flutter#5069)

github-actions bot added p: video_player platform-ios labels Mar 18, 2022

AlexV525 changed the title ~~[video_player] Always apply preferred transform on iOS~~ [video_player] Apply natural size transform with 90/270 degrees orientated videos on iOS Mar 18, 2022

AlexV525 force-pushed the fix-ios-video-transform branch from e8cf3ac to a80dd47 Compare March 18, 2022 05:06

AlexV525 force-pushed the fix-ios-video-transform branch 2 times, most recently from a80dd47 to 7c1211e Compare March 18, 2022 08:27

AlexV525 changed the title ~~[video_player] Apply natural size transform with 90/270 degrees orientated videos on iOS~~ [video_player] Apply transform accordingly for videos Mar 18, 2022

AlexV525 force-pushed the fix-ios-video-transform branch 2 times, most recently from 75aa03b to f984752 Compare March 20, 2022 08:38

AlexV525 marked this pull request as ready for review March 20, 2022 08:38

AlexV525 requested a review from jmagman March 20, 2022 08:40

AlexV525 commented Mar 20, 2022

View reviewed changes

jmagman requested a review from hellohuanlin March 23, 2022 21:41

stuartmorgan-g approved these changes Mar 24, 2022

View reviewed changes

hellohuanlin suggested changes Mar 24, 2022

View reviewed changes

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m Outdated Show resolved Hide resolved

This comment was marked as duplicate.

Sign in to view

AlexV525 force-pushed the fix-ios-video-transform branch from 5483df7 to a91fa2c Compare March 25, 2022 13:05

AlexV525 requested a review from hellohuanlin March 25, 2022 13:11

stuartmorgan-g suggested changes Mar 25, 2022

View reviewed changes

packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m Outdated Show resolved Hide resolved

packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m Show resolved Hide resolved

AlexV525 force-pushed the fix-ios-video-transform branch from a91fa2c to d45e721 Compare March 26, 2022 13:37

AlexV525 added 4 commits April 7, 2022 23:24

✅ testTransformFixByOrientation -> `validateTransformFixForOrientat…

85da558

…ion`

🎨 Extra blank lines

bc4b06d

💡 Rewrite and move around comments

7a74c2a

♻️ Rewrite the category as a C function

2ae271b

AlexV525 commented Apr 7, 2022

View reviewed changes

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m Outdated Show resolved Hide resolved

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m Outdated Show resolved Hide resolved

AlexV525 marked this pull request as draft April 7, 2022 16:26

Apply suggestions from code review

7c91a3f

stuartmorgan-g reviewed Apr 7, 2022

View reviewed changes

AlexV525 added 2 commits April 8, 2022 10:35

♻️ Rewrite the category as a C function x2

1c7bc9a

🎨 Format fix

2bb2f41

AlexV525 marked this pull request as ready for review April 8, 2022 02:48

AlexV525 commented Apr 8, 2022

View reviewed changes

packages/video_player/video_player_avfoundation/CHANGELOG.md Outdated Show resolved Hide resolved

Apply suggestions from code review

e452c89

hellohuanlin reviewed Apr 8, 2022

View reviewed changes

AlexV525 requested a review from stuartmorgan-g April 12, 2022 02:15

💡 Remove the comment about iOS 14+

07cb30e

stuartmorgan-g approved these changes Apr 18, 2022

View reviewed changes

Apply suggestions from code review

b801da1

AlexV525 force-pushed the fix-ios-video-transform branch from eb0fb1d to b801da1 Compare April 19, 2022 03:30

AlexV525 changed the title ~~[video_player] Apply transform accordingly for videos~~ [video_player_avfoundation] Applies the standardized transform for videos with different orientations Apr 19, 2022

AlexV525 added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Apr 19, 2022

fluttergithubbot merged commit 06257dd into flutter:main Apr 19, 2022

engine-flutter-autoroll mentioned this pull request Apr 19, 2022

Roll Plugins from c9addb447354 to 8084ac1f363a (7 revisions) flutter/flutter#102147

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 19, 2022

06257ddbf [video_player_avfoundation] Applies the standardized transf…

a61aa7e

…orm for videos with different orientations (flutter/plugins#5069)

engine-flutter-autoroll mentioned this pull request Apr 19, 2022

Roll Plugins from c9addb447354 to 3e1f7acf2c0b (8 revisions) flutter/flutter#102154

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 19, 2022

06257ddbf [video_player_avfoundation] Applies the standardized transf…

502f44d

…orm for videos with different orientations (flutter/plugins#5069)

AlexV525 deleted the fix-ios-video-transform branch April 25, 2022 04:55

aboyo mentioned this pull request Aug 7, 2022

[video_player] m3u8 videos are distorted when played on iOS flutter/flutter#109116

Closed

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

[video_player_avfoundation] Applies the standardized transform for vi…

92d3120

…deos with different orientations (flutter#5069)

Comments

Conversation

AlexV525 commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

AlexV525 commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexV525 commented Mar 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmagman commented Mar 22, 2022

Uh oh!

AlexV525 commented Mar 23, 2022

Uh oh!

AlexV525 commented Mar 23, 2022

Uh oh!

hellohuanlin commented Mar 24, 2022

Uh oh!

AlexV525 commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hellohuanlin commented Mar 24, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexV525 commented Mar 25, 2022

Uh oh!

This comment was marked as duplicate.

AlexV525 commented Mar 25, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

AlexV525 Apr 13, 2022 • 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!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

AlexV525 commented Mar 18, 2022 •

edited

Loading

AlexV525 commented Mar 18, 2022 •

edited

Loading

AlexV525 commented Mar 24, 2022 •

edited

Loading

AlexV525 Apr 13, 2022 •

edited

Loading