[video_player] Fix aspect ratio #690

recastrodiaz · 2018-07-28T11:13:03Z

This PR adds a rotationDegrees field to the VideoPlayerValue class and fixes the aspectRatio field so it takes the rotation of the video into account. This ensures that videos are properly displayed when wrapped with an AspectRatio Widget. (Currently portrait videos look squashed).

Android videos were no longer displayed with the correct aspect ratio when running against the new ExoPlayer. This seems to be a regression introduced in video_player v0.6.0.
iOS videos were never shown with the correct aspect ratio in portrait mode (Adding Video Capability to image_picker #565 (comment))

Test Methodology:

Android:
- Took 4 videos with a Samsung S6 camera in all 4 possible orientations. Took 4 more videos in All 4 possible orientations with the frontal camera
- Ran the image_picker example and validated all 8 videos displayed correctly (cd packages/image_picker/example && flutter run)
iOS:
- Same approach as above. 8 videos recorded with the default iOS app on an iPhone 4s

This PR fixes flutter/flutter#17606.

…ssible values are: 0, 90, 180 and 270. * Updated the aspectRatio getter to use the video rotation. * Fixed the aspect ratio of videos in the video_player plugin example. It now properly displays 'rotated' videos.

…ratio in image_picker

googlebot · 2018-07-28T11:13:05Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

recastrodiaz · 2018-07-28T11:18:48Z

I signed it!

googlebot · 2018-07-28T11:18:52Z

CLAs look good, thanks!

…ing displayed

…_player plugin on iOS devices. The fix sets the videoComposition of the respective AVPlayerItem to use the preferredTransform of the video (the rotation matrix for the video)

…race_condition

…tion in Android (possibly happening in iOS too)

…deo is in portratit mode then height > width

…e. Plus remove extra sendInitialized call - Add support for playing PHAssets from localIdentifier. Currently used from flutter like File('phasset://LOCAL_IDENTIFIER')

…ot have the tx and ty values populated, which in turn renders black videos. We work around this by settings the tx and ty values ourselves

…added on a different branch (phAsset)

cbracken · 2018-09-06T23:14:16Z

Apologies for the delay in getting to this PR. We're slowly making our way through. I'll take a look at this in the next few days. Thanks for your contribution!

recastrodiaz · 2018-09-07T10:32:25Z

Thanks @cbracken! Looking forward to your review.

ghost · 2018-12-10T14:02:43Z

@mnazim You can meanwhile use this like so I just tried that and worked perfectly!

  video_player:
    git:
      url: https://github.com/recastrodiaz/plugins.git
      path: packages/video_player  
      ref: 42ad92ffeab0f78baae289757e0ea121491b0c60
    version: ^0.7.3

ghost · 2018-12-11T03:24:25Z

What is going on with 0.8.0 aspectRatio is not fixed at all.

rlee1990 · 2018-12-12T16:50:00Z

Any updates on this?

uiboy · 2018-12-21T10:07:52Z

@recastrodiaz I was testing network videos with this PR and I run into this error, I understand that this only fixes file videos, but I think it should still at least display network videos correctly.

NoSuchMethodError: The method 'toInt' was called on null. Receiver: null Tried calling: toInt() #0 Object.noSuchMethod (dart:core/runtime/libobject_patch.dart:50:5) #1 VideoPlayerController.initialize.eventListener (package:video_player/video_player.dart:245:53) #2 _RootZone.runUnaryGuarded (dart:async/zone.dart:1314:10) #3 _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:336:11) #4 _DelayedData.perform (dart:async/stream_impl.dart:591:14) #5 _StreamImplEvents.handleNext (dart:async/stream_impl.dart:707:11) #6 _PendingEvents.schedule.<anonymous closure> (dart:async/stream_impl.dart:667:7) #7 _microtaskLoop (dart:async/schedule_microtask.dart:41:21) #8 _startMicrotaskLoop (dart:async/schedule_microtask.dart:50:5)

The offending line seems to be 245 in video_player which has
rotationDegrees: map['rotationDegrees'].toInt() ?? 0.0,

It seems map['rotationDegrees'] is null.. I also could not find a setRotationDegrees method but I assume this is intended to be handled internally that is why.

Any clues on why this would be happening ?

my pubspec.yml:

video_player:
    git:
      url: https://github.com/recastrodiaz/plugins.git
      path: packages/video_player
      ref: 42ad92ffeab0f78baae289757e0ea121491b0c60
    version: ^0.7.3

recastrodiaz · 2018-12-26T17:56:41Z

@uiboy 42ad92 is a very old revision. Can you check if the latest commit also throws an error?

ref: d589ee54ce405e38ebc42b1961e54273cd375726

uiboy · 2018-12-28T09:48:19Z

@recastrodiaz oops my bad, yes that seems to work. Thanks for your response. Happy Holidays!

ghost · 2018-12-28T13:59:47Z

@recastrodiaz
I tried to get like so.

  video_player:
    git:
      url: https://github.com/recastrodiaz/plugins.git
      path: packages/video_player  
      ref: d589ee54ce405e38ebc42b1961e54273cd375726
    version: ^0.7.3

But error saying

Because MyApp depends on video_player ^0.7.3 from git which doesn't match any versions, version solving failed.
pub get failed (1)
exit code 1

Any idea?
Thanks for your work!

ghost · 2018-12-28T14:21:55Z

@uiboy Could you share your pubspec.yml?

uiboy · 2018-12-28T15:12:23Z

@Tj543 I have this in my pubspec.yml (0.8.0) , maybe that is why it doesnt work for you.

  video_player:
    git:
      url: https://github.com/recastrodiaz/plugins.git
      path: packages/video_player
      ref: d589ee54ce405e38ebc42b1961e54273cd375726
    version: ^0.8.0

…ctratio

- addListener is not required to render the first video frame - call setState() on play/pause to update the UI - dispose() the controller when the widget is disposed - fix video urls

dnfield · 2019-01-24T00:44:29Z

Can you merge this in with master to get CI doing what it needs to?

Sorry for the delay again. We haven't lost track of this (completely)!

dnfield · 2019-01-24T00:56:37Z

packages/video_player/README.md

+            _controller.value.isPlaying
+                ? _controller.pause()
+                : _controller.play();
+            setState(() {});


Why are we calling setState empty here? Can we either move the lines above this to the closure for setState, or comment on the unusual use of setState here?

I'll move them inside the setState. We need setState as otherwise the play/pause button UI does not get updated.

dnfield · 2019-01-24T01:00:53Z

packages/video_player/pubspec.yaml

@@ -2,7 +2,7 @@ name: video_player
 description: Flutter plugin for displaying inline video with other Flutter
  widgets on Android and iOS.
 author: Flutter Team <flutter-dev@googlegroups.com>
-version: 0.8.0
+version: 0.8.1


Should this be a 0.9.0 bump? Could we end up surprising anyone with this change if they had some other work around in place to handle it?

Agree. Updated to 0.9.0

dnfield · 2019-01-24T01:02:03Z

Just a couple small things and a merge (or rebase) needed, and this should be good to go.

…ctratio

- Move play/pause logic within setState closure in video_player example

recastrodiaz · 2019-01-24T03:49:14Z

@dnfield Thanks for looking at this again! Very much appreciated! Any other issues please let me know.

dnfield · 2019-01-24T04:38:53Z

LGTM

franzejr · 2019-02-14T17:57:20Z

Hey @recastrodiaz , the version 0.9.0 is not here: https://pub.dartlang.org/packages/video_player#-versions-tab- ?

dnfield · 2019-02-14T18:11:02Z

The latest version on pub should have this change. Looks like no one ever published 0.9.0 but we can fix that.

dnfield · 2019-02-14T18:20:52Z

@mklim has taken care of this - 0.9.0 is now available on pub. See flutter/flutter#27258 - we're working on making sure these don't get missed in the future.

* * Added access to the video rotation in degrees (rotationDegrees). Possible values are: 0, 90, 180 and 270. * Updated the aspectRatio getter to use the video rotation. * Fixed the aspect ratio of videos in the video_player plugin example. It now properly displays 'rotated' videos. * Ensure videos are not upside down nor streteched when using the video_player plugin on iOS devices. The fix sets the videoComposition of the respective AVPlayerItem to use the preferredTransform of the video (the rotation matrix for the video) * - Bump video_player version to 0.9.0 - Move play/pause logic within setState closure in video_player example

This reverts commit 1da1ae5.

recastrodiaz added 2 commits July 28, 2018 10:27

Bugfix: rotated videos will now be displayed with the correct aspect …

bb8e0b5

…ratio in image_picker

googlebot added the cla: no label Jul 28, 2018

googlebot added cla: yes and removed cla: no labels Jul 28, 2018

recastrodiaz added 4 commits July 28, 2018 13:30

Fix format. Use clang-format-5.0 instead of clang-format (7.0)

f385849

Fix format. Remove spurious extra line.

7943a8f

Fix race condition in video_player that prevented some videos from be…

9a7a974

…ing displayed

Merge branch 'video_player_aspectratio' into develop

230c7e0

recastrodiaz mentioned this pull request Aug 7, 2018

video_player aspect ratio and orientation messed up on iOS flutter/flutter#17606

Closed

recastrodiaz added 3 commits August 13, 2018 13:22

Ensure videos are not upside down nor streteched when using the video…

0332538

…_player plugin on iOS devices. The fix sets the videoComposition of the respective AVPlayerItem to use the preferredTransform of the video (the rotation matrix for the video)

Ensure videos are not upside down nor streteched when using the video…

7ef3092

…_player plugin on iOS devices. The fix sets the videoComposition of the respective AVPlayerItem to use the preferredTransform of the video (the rotation matrix for the video)

Merge branch 'master' into video_player_aspectratio

27f55da

recastrodiaz changed the title ~~[WIP] Fix video player aspect ratio~~ Fix video player aspect ratio Aug 13, 2018

recastrodiaz added 9 commits August 13, 2018 15:03

Merge remote-tracking branch 'upstream/master' into video_player_fix_…

2238d06

…race_condition

Add support for 'sendInitialized' flutter event that fixes race condi…

9e74b42

…tion in Android (possibly happening in iOS too)

Merge branch 'video_player_aspectratio' into develop

d0ecc95

Merge branch 'video_player_fix_race_condition' into develop

f390cc2

Ensure with and height of a video is given after rotation. i.e. If vi…

fd5c298

…deo is in portratit mode then height > width

- Merge fix_race_condition branch so sendInitialized is only sent onc…

f0ade3c

…e. Plus remove extra sendInitialized call - Add support for playing PHAssets from localIdentifier. Currently used from flutter like File('phasset://LOCAL_IDENTIFIER')

Sometimes, the preferredTransformed returned by the videoTrack does n…

c55c5c2

…ot have the tx and ty values populated, which in turn renders black videos. We work around this by settings the tx and ty values ourselves

Merge remote-tracking branch 'upstream/master' into phAsset

65e19ff

- Removed support for phasset:// files. Support for phAssets will be …

0943513

…added on a different branch (phAsset)

recastrodiaz mentioned this pull request Sep 6, 2018

Race in VideoPlayer initialization flutter/flutter#21483

Open

cbracken self-requested a review September 6, 2018 23:13

recastrodiaz added 3 commits January 6, 2019 15:46

Merge remote-tracking branch 'upstream/master' into video_player_aspe…

7e265a5

…ctratio

Merge remote-tracking branch 'upstream/master' into video_player_aspe…

232fec9

…ctratio

video_player examples:

8d571fb

- addListener is not required to render the first video frame - call setState() on play/pause to update the UI - dispose() the controller when the widget is disposed - fix video urls

dnfield reviewed Jan 24, 2019

View reviewed changes

recastrodiaz added 2 commits January 23, 2019 21:41

Merge remote-tracking branch 'upstream/master' into video_player_aspe…

6792cd9

…ctratio

- Bump video_player version to 0.9.0

40f756c

- Move play/pause logic within setState closure in video_player example

- Bump plugin to 0.9.0 (not just the changelog)

055cb53

dnfield merged commit 014e373 into flutter:master Jan 24, 2019

recastrodiaz deleted the video_player_aspectratio branch January 24, 2019 17:02

andreidiaconu added a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019

Revert "[video_player] Fix aspect ratio (flutter#690)"

11774d3

This reverts commit 1da1ae5.

bparrishMines mentioned this pull request Apr 2, 2019

[video_player]Sample Videos Link Fix #982

Closed

jmagman mentioned this pull request Oct 12, 2022

video_player fixTransform error in iOS flutter/flutter#93371

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[video_player] Fix aspect ratio #690

[video_player] Fix aspect ratio #690

recastrodiaz commented Jul 28, 2018 •

edited

Loading

googlebot commented Jul 28, 2018

recastrodiaz commented Jul 28, 2018

googlebot commented Jul 28, 2018

cbracken commented Sep 6, 2018 •

edited

Loading

recastrodiaz commented Sep 7, 2018

ghost commented Dec 10, 2018

ghost commented Dec 11, 2018

rlee1990 commented Dec 12, 2018

uiboy commented Dec 21, 2018 •

edited

Loading

recastrodiaz commented Dec 26, 2018

uiboy commented Dec 28, 2018

ghost commented Dec 28, 2018 •

edited by ghost

Loading

ghost commented Dec 28, 2018

uiboy commented Dec 28, 2018

dnfield commented Jan 24, 2019

dnfield Jan 24, 2019

recastrodiaz Jan 24, 2019

dnfield Jan 24, 2019

recastrodiaz Jan 24, 2019

dnfield commented Jan 24, 2019

recastrodiaz commented Jan 24, 2019

dnfield commented Jan 24, 2019

franzejr commented Feb 14, 2019

dnfield commented Feb 14, 2019

dnfield commented Feb 14, 2019 •

edited

Loading

[video_player] Fix aspect ratio #690

[video_player] Fix aspect ratio #690

Conversation

recastrodiaz commented Jul 28, 2018 • edited Loading

Test Methodology:

googlebot commented Jul 28, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

recastrodiaz commented Jul 28, 2018

googlebot commented Jul 28, 2018

cbracken commented Sep 6, 2018 • edited Loading

recastrodiaz commented Sep 7, 2018

ghost commented Dec 10, 2018

ghost commented Dec 11, 2018

rlee1990 commented Dec 12, 2018

uiboy commented Dec 21, 2018 • edited Loading

recastrodiaz commented Dec 26, 2018

uiboy commented Dec 28, 2018

ghost commented Dec 28, 2018 • edited by ghost Loading

ghost commented Dec 28, 2018

uiboy commented Dec 28, 2018

dnfield commented Jan 24, 2019

dnfield Jan 24, 2019

Choose a reason for hiding this comment

recastrodiaz Jan 24, 2019

Choose a reason for hiding this comment

dnfield Jan 24, 2019

Choose a reason for hiding this comment

recastrodiaz Jan 24, 2019

Choose a reason for hiding this comment

dnfield commented Jan 24, 2019

recastrodiaz commented Jan 24, 2019

dnfield commented Jan 24, 2019

franzejr commented Feb 14, 2019

dnfield commented Feb 14, 2019

dnfield commented Feb 14, 2019 • edited Loading

recastrodiaz commented Jul 28, 2018 •

edited

Loading

cbracken commented Sep 6, 2018 •

edited

Loading

uiboy commented Dec 21, 2018 •

edited

Loading

ghost commented Dec 28, 2018 •

edited by ghost

Loading

dnfield commented Feb 14, 2019 •

edited

Loading