Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(mobile): native_video_player #12104

Open
wants to merge 52 commits into
base: main
Choose a base branch
from

Conversation

shenlong-tanwen
Copy link
Member

@shenlong-tanwen shenlong-tanwen commented Aug 28, 2024

Changes made in the PR:

  • replaced video_player with native_video_player to fix HDR playback on mobile devices
  • buffering is handled by using a timer to periodically check if the playback position has been updated or not

Fixes #2130
Fixes #3321
Fixes #5120
Fixes #5651
Fixes #7617
Fixes #11229
Fixes #11400
Fixes #11689

Maybe #5553? Need to test.

@alextran1502
Copy link
Contributor

Good work as always!

A couple of issues I noticed

  • After the video is uploaded, when viewing the video, it loads the remote video instead of the local one.
  • The aspect ratio calculation for the remote video is not correct. It looks distorted
  • When taping on the video from the timeline, you will often hear the sounds of the player getting initialized twice

@hsu1943
Copy link

hsu1943 commented Aug 30, 2024

Wow! Thanks a lot for your amazing work! Can this pull request fix issue #5120?

@phajduk
Copy link

phajduk commented Aug 30, 2024

@shenlong-tanwen @alextran1502 any chance this PR will fix #11689 ?

@alextran1502
Copy link
Contributor

@phajduk yes

@phajduk
Copy link

phajduk commented Aug 30, 2024

@alextran1502 I will test this in a minute as I struggle with that bug hard and it's a bit of a showstopper to migrate to Immich.

@alextran1502
Copy link
Contributor

@phajduk The pr is not finished yet, but yah, welcome to try out 😁

@phajduk
Copy link

phajduk commented Aug 30, 2024

@alextran1502 @shenlong-tanwen videos have also wrong ratio (both local and remote). Attaching two images (one from Google Photos, another one from Immich).
Screenshot_2024-08-30-21-45-26-17_965bbf4d18d205f782c6b8409c5773a4
Screenshot_2024-08-30-21-44-58-57_d8aea95ef633eb6b7eb1cae13c34b40e

@RaulGw4
Copy link

RaulGw4 commented Sep 6, 2024

Any chance this will also get fixed? #12006

@shenlong-tanwen
Copy link
Member Author

shenlong-tanwen commented Sep 10, 2024

@alextran1502 I cannot reproduce the issue where remote video is preferred instead of the local one and the audio issue. Found a possible root cause for the aspect ratio issue in case of local assets and handle it now. Can you please verify if you could still reproduce the other issues?

@phajduk Can you verify if you still face the aspect ratio issue with the recent change?

@phajduk
Copy link

phajduk commented Sep 10, 2024

@shenlong-tanwen I've tested and 90% cases work. Thanks for doing this. I have some random private videos that have aspect ratio still broken but I can't upload it nor give specification because other, similar videos, work. I think we're good to merge for now.

@dvbthien
Copy link
Contributor

dvbthien commented Sep 11, 2024

Hi @shenlong-tanwen
When I tested the last commit, I encountered the same aspect ratio problem. I discovered that it occurs when I play remote videos that record by iphone. Upon debugging, I noticed that asset.width and asset.height return exif Info width and height for resolution video, so when used for AspectRatio, it will render incorrectly.
When I check the EXIF information online, I notice that MOV videos recorded by iPhones have a rotation value. For example, when I record a vertical video, the rotation value is 90 degrees. This means the video's dimensions need to be swapped, so the width becomes the height and vice versa, resulting in a 9:16 aspect ratio. However, if I record a horizontal video, the rotation value is 0, so the width and height do not need to change.
Link file video: https://drive.google.com/file/d/16iZXe7gjTKgoFanclhH5r39RjaIWpxZi/view?usp=sharing
Exif Info:
image
IOS show width and height:
image

@alextran1502
Copy link
Contributor

@shenlong-tanwen So here is what I see, the aspect ratio is incorrect for vertical video when playing remote only asset. I am using the default transcoding.

Local Remote
local remote

@alextran1502
Copy link
Contributor

I am also seeing errors when playing videos that were uploaded from iOS and play on Android (remote asset)

F/MPEG4Extractor(32196): frameworks/av/media/module/sec_extractors/mp4/MPEG4Extractor.cpp:7317 CHECK(AMediaFormat_getBuffer(format, AMEDIAFORMAT_KEY_CSD_HEVC, &data, &size)) failed.
V/MediaPlayerNative(30878): message received msg=100, ext1=100, ext2=1
E/MediaPlayerNative(30878): error (100, 1)
V/MediaPlayerNative(30878): message received msg=100, ext1=1, ext2=-2147483648
E/MediaPlayerNative(30878): error (1, -2147483648)
E/MediaPlayer(30878): Error (100,1)
D/VideoView(30878): Error: 100,1
E/MediaPlayer(30878): Error (1,-2147483648)
D/VideoView(30878): Error: 1,-2147483648

@shenlong-tanwen
Copy link
Member Author

The orientation issue should be fixed now.

@poisonnuke
Copy link

As this PR is supposed to fix #5553 , please verify that all parts of the issue are fixed, that includes the BasicAuth issue on Android, ExoPlayer cannot play assets that use BasicAuth HTTP.
See also #12985 for more informations.

Im happy to provide an public accessible testing-URL on a private channel

@alextran1502
Copy link
Contributor

@poisonnuke I don't think so

@alextran1502
Copy link
Contributor

I've just finished another round of testing.

  • On the emulator (iOS 17), the player can play the remote-only video.
  • On the physical device (iOS 18), the player cannot play the remote-only video. It can play the video that is on the device just fine. Here is a maybe relevant error message.
#0      ChangeNotifier.debugAssertNotDisposed.<anonymous closure> (package:flutter/src/foundation/change_notifier.dart:183:9)
#1      ChangeNotifier.debugAssertNotDisposed (package:flutter/src/foundation/change_notifier.dart:190:6)
#2      ChangeNotifier.notifyListeners (package:flutter/src/foundation/change_notifier.dart:416:27)
#3      ValueNotifier.value= (package:flutter/src/foundation/change_notifier.dart:559:5)
#4      NativeVideoViewerPage.build.initController (package:immich_mobile/pages/common/native_video_viewer.page.dart:214:18)
<asynchronous suspension>

Let me know if you need more information.

@poisonnuke

This comment was marked as off-topic.

@bo0tzz

This comment was marked as off-topic.

@rovo89

This comment was marked as off-topic.

@alextran1502

This comment was marked as off-topic.

@zackpollard

This comment was marked as off-topic.

@rovo89

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment