immich
immich copied to clipboard
immich-app
feat(mobile): native_video_player
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
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
Wow! Thanks a lot for your amazing work! Can this pull request fix issue #5120?
@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 @shenlong-tanwen videos have also wrong ratio (both local and remote). Attaching two images (one from Google Photos, another one from Immich).
@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?
@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.
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:
IOS show width and height:
@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 |
|---|---|
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
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
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 I don't think so
what does that mean? bo0tzz declared this as part of the bug and if this is the intended fix this PR must solve the BasicAuth issue. Therefore the BasicAuth test just became mandatory for this PR.
@poisonnuke
- You don't get to dictate how we should work
- A PR doesn't have to fix every possible angle. If appropriate, the bug report or FR can stay open after the PR is merged.
The ground work seems to be done though, so I assume there are chances to get Basic Authentication eventually: https://github.com/albemala/native_video_player/pull/25/files
Not sure there's also something for mTLS, which is what my family is eagerly waiting for.
@rovo89 neat, although it might be more applicable to be a subsequent pr since this pr is focusing on getting the player working first. There seems to be issue with playing remote videos on iOS 18 so we might have to split to use two players. So essentially the issue with the basic auth header won’t get resolved. But let’s not discussing that here to leave room for code related discussion ☺️
The ground work seems to be done though, so I assume there are chances to get Basic Authentication eventually: albemala/native_video_player#25 (files)
Not sure there's also something for mTLS, which is what my family is eagerly waiting for.
Potentially, but this is not the aim of the PR in its current state, so lets keep discussion relevant to the PR for now and discussion around these improvements can stay in the existing github discussions :slightly_smiling_face:
it might be more applicable to be a subsequent pr
Sure - just wanted to give back some hope. 😉 And now I'll shut up and let you get this finalized.
@poisonnuke
- You don't get to dictate how we should work
just to conclude this statement, this is wrong, you dictated it. Youve made a lot of mistakes that lead to this. The rest in private if youre really interessted in improving yourself. I was in your place 20 years ago, Ive made the same mistakes, too.
@poisonnuke
- You don't get to dictate how we should work
just to conclude this statement, this is wrong, you dictated it. Youve made a lot of mistakes that lead to this. The rest in private if youre really interessted in improving yourself. I was in your place 20 years ago, Ive made the same mistakes, too.
I'm not really sure what you're saying here. But this kind of input isn't appreciated and isn't valuable to the ongoing discussion. That statement is not wrong, you don't get to dictate how people work on their own open source projects, the level of entitlement here is astounding. Please enjoy continuing to use Immich, however we will no longer be accepting your input here. Have a nice day! :slightly_smiling_face:
I made a bunch of improvements to avoid animation stuttering / jank when opening or swiping a video, unnecessary async calls, repeated video initialization, etc. It works quite well in my testing at this point. However, there are quite a few changes I made to bring it to this point and I've only tested on iOS, so @alextran1502 please let me know if you notice any bugs. I also don't know how to fix the build failure for Android.
The only thing left on my mind is that seeking is not as responsive as I'd like, especially for remote videos. You have to let go or at least hold still for a moment for it to actually seek. This can probably be improved with better throttling / debouncing, but either way I don't think it's a blocker.
I added throttling for the seek and it's a lot more responsive now. Not sure if a throttle interval of 200ms is optimal, but it seems fine for now.
I made a few more improvements, particularly with seeking. I'm pretty happy with where it's at for iOS. There's a small issue in that there's no audio in silent mode, which we may address in this PR or a later one. Android still needs to be tested.
Also, the PR will need to be rebased after #14086 is merged as there are a few changes that rely on it.