Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[video_player] Fixed orientation and position issue for some videos metadata. #1451

Closed

Conversation

quentinleguennec
Copy link
Contributor

@quentinleguennec quentinleguennec commented Apr 5, 2019

Description

The video was not always displaying properly on iOS depending on the rotation metadata (iOS need the tx and ty of the transform to have an offset if the video is rotated, unlike Android or VLC). This fixes it.

Related Issues

Fixes flutter/flutter#29951
Fixes flutter/flutter#27201
Fixes flutter/flutter#29500

NOTE: The camera plugin also need an update to add rotation metadata to the recorded videos. This is done in this other PR: #1452

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your 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.

@quentinleguennec quentinleguennec force-pushed the tengio_improved_video_player branch from 0e4caa2 to a6de6a7 Compare April 5, 2019 16:28
@ened
Copy link
Contributor

ened commented Apr 6, 2019

Do you have a sample video to show before/after?

@quentinleguennec
Copy link
Contributor Author

quentinleguennec commented Apr 8, 2019

Do you have a sample video to show before/after?

I don't have one no, but what was happening is that when rotated on portrait left the video was not visible (out of the screen). Sound was working.

This just adds the missing orientation (the fix for portrait up and down was already there).

@quentinleguennec
Copy link
Contributor Author

I did this fix when working on this PR for the camera plugin on iOS #1452
When I add the translation directly in the metadata it displays properly (and the above fix is not needed). This translation is only needed on iOS though and displays properly on Android without it. I think it's best to fix it in the video_player directly because some videos (like the one produced by the camera plugin on Android) don't have the translation metadata and will not display properly on iOS without this fix.

transform.tx = videoTrack.naturalSize.height;
transform.ty = 0;
} else if (rotationDegrees == 180) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@quentinleguennec Thanks for your PR. I just tested your fix and unfortunately this line is not rendering videos taken at 180°. tx and ty must be 0 for videos in landscape and reverse landscape mode. Furthermore, some videos metadata include incorrect tx, ty values when in portrait mode. In https://github.com/flutter/plugins/pull/1307/files I've added a fix to properly display these videos.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be because I was using the fixed rotation metadata from this PR: #1452
On my side with both those PRs the videos display properly. (even when the video is recorded with a rotation).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried recording your videos with the native Camera app? That's the one I used to record a video in 180° that displays a black screen with tx and ty set to non 0.

Copy link
Contributor Author

@quentinleguennec quentinleguennec Apr 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@recastrodiaz Good point, I didn't think about checking with the native camera app.
I just did and it is working with the current code when you record a video on your iPhone and play it with the video_player. But it is not working (black screen) if you record the video from the native camera app of an Android an play it with the video_player on an iPhone.

That's because the native iPhone camera app add the translation metadata to the video file (which can be seen with ffprobe). But Android doesn't add it (and VLC doesn't need it either, just iOS).

With the fix I added it displays the video recorded on Android properly when played on an iPhone. And it also works with a video recorded from an iPhone and played on an iPhone from the video_player.

@quentinleguennec
Copy link
Contributor Author

quentinleguennec commented May 21, 2019

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

@cbenhagen
Copy link
Contributor

@quentinleguennec thank you for your contribution! Unfortunately the structure of the plugin has changed lately to support implementations on more platforms. Mainly the plugin was moved to a new subdirectory and now uses a common platform interface. Adapting your PR to the new structure should be straight forward. If you have questions please ask!

Please also make sure to include tests for the new functionality.

@kuyazee
Copy link

kuyazee commented Oct 28, 2020

Currently using the ff

  • camera: 0.5.8+11
  • video_player: 0.10.12+5

Issue still persists

@stuartmorgan-g
Copy link
Contributor

Thanks for the contribution! Based on the discussion above, it sounds like this PR may not actually work in isolation, which would be a blocker for moving forward with review and landing. It also would need to be updated for the changes in plugin structure as noted above. Given that, and the long period without updates, I'm going to close this, but if you're still interested in moving forward with this please do open a new PR (which should be triaged quickly under our new process) with a version that works in isolation—combining fixes if necessary.

We apologize for the long delay in triaging this PR. We’re in the process of overhauling our PR triage system to respond much more quickly, as well as working through the backlog.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants