Image and video player improvements

hjiangsu commented 2 weeks ago

Pull Request Description

Review without whitespace

This PR builds upon the great video player work that @ggichure implemented in #1081 and also adds some simplification to the way we determine image dimensions.

To summarize, these are the high level changes:

Added back buttons to the video players (mainly to allow iOS to navigate back to the previous page).
Added a snackbar which appears if a video fails to load. Tapping on the snackbar will open the video link in a browser as a fallback.
Fixed an issue where some youtube links were not being detected properly (and thus, attempted to use the normal video player)
Adds a separate widget that displays in the feed if it detects that the url is a video. This also allows any existing thumbnail images to show up on the feed along with a play button that opens up the video player
Added logic in the parsePostView function to handle embedded video links postView.post.embedVideoUrl. In these cases, the postView.post.url may not directly point to a video link, but postView.post.embedVideoUrl may. We want these links to also open up the video player (e.g., embedded peertube video links)
- For reference: https://github.com/thunder-app/thunder/issues/1009
This is not directly related to videos, but I've improved the handling of determining image dimensions. It turns out there is an existing Flutter implementation that helps decode images, so fetching image dimensions should be more robust. This should reduce instances where images fail to load properly, or have the incorrect image dimensions.

I've also adjusted Media to have a thumbnailUrl parameter. This is to allow us to account for both images/videos

mediaUrl should now point to the url of the media (image or video urls)
thumbnailUrl should point to a valid image url for displaying on the feed
originalUrl is the original url of the post

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Checklist

[ ] Did you update CHANGELOG.md?
[ ] Did you use localized strings where applicable?
[ ] Did you add semanticLabels where applicable for accessibility?

micahmo commented 2 weeks ago

I haven't reviewed yet as this is still a draft, but I did have a couple initial thoughts from testing.

Do we need the play icon on top of the thumbnail when we already have the media type indicator? I understand the reasoning (it implies that pressing the thumbnail will play the video), but it also seems a bit redundant, often covers up a significant part of the thumbnail, and the black with the shadow often doesn't stand out very well. If you do want to keep it, maybe something lighter colored with opacity would look better.
Any idea why videos without thumbnails sometimes look different? The second one is the one I expect if there's no thumbnail.
I noticed that sharing a post with a video doesn't provide the option to share the video link, only the thumbnail link.

If you want me to help with any of these things after your PR, just let me know!

hjiangsu commented 1 week ago

Do we need the play icon on top of the thumbnail when we already have the media type indicator?

I can get rid of it! It just felt weird to not have anything in the box when there's no thumbnail

Any idea why videos without thumbnails sometimes look different?

Hmm, could you provide the community/links so that I can check this out?

I noticed that sharing a post with a video doesn't provide the option to share the video link, only the thumbnail link.

I think we can make this into a separate PR!

micahmo commented 1 week ago

Any idea why videos without thumbnails sometimes look different?

Hmm, could you provide the community/links so that I can check this out?

Here's an example I found in youtube@lemmy.fmhy.ml (I know that instance is down, but the community is available via other instances).

Another thing I see in this screenshot (and that I've been noticing a lot while browsing lately) is that URLs detected as non-video links with no thumbnail have the play icon instead of the web browser icon as the placeholder. I think that's a regression (maybe not in this PR though).

hjiangsu commented 1 week ago

Thanks, I'll check it out.

URLs detected as non-video links with no thumbnail have the play icon instead of the web browser icon as the placeholder

I noticed that too - it does seem to be a regression from the original PR (it'll be fixed in this PR though!)

hjiangsu commented 1 week ago

Ahh, okay - in this case, there was supposed to be a thumbnail associated with that video post, but it was unable to find it. Presumably, this is because the original instance is down so it can no longer fetch the thumbnail image.

When this happens, it shows the error icon with the play icon superimposed on it (so it looks like a circular play icon instead)

micahmo commented 1 week ago

Interesting, good catch!! I think I've seen that error placeholder for articles as well. Do we just need to supply our own widget for error cases (which can be the solid gray color, as though there's image at all)?

hjiangsu commented 1 week ago

A few updates:

I removed the extra play icon that appears when there's no thumbnail, it should be the default solid colour background
I added a way to open in browser when opening the video, in case the video does not load properly (for both youtube and the regular video player)
Fixed the issue where links would show the play icon
Played around with the UI for both compact/card mode

I think it's now at a place where you can try out the new changes and let me know if you see any new issues!

ggichure commented 1 week ago

The youtube player doesn't have an unmute which means should you want to enable audio you have to go to settings to disable auto mute.

update

We could do something like this

Center(
                child: ypf.YoutubePlayerBuilder(
                  player: ypf.YoutubePlayer(
                    onReady: () => _ypfController.addListener(listener),
                    aspectRatio: 16 / 10,
                    topActions: [
                      IconButton(
                          onPressed: () {
                            muted ? _ypfController.unMute() : _ypfController.mute();
                            setState(() {
                              muted = !muted;
                            });
                          },
                          icon: Icon(
                            muted ? Icons.volume_off : Icons.volume_up,
                            color: Colors.white,
                          ))
                    ],

Result. you can move it to the right or left.

Screenshot_20240521-142742_Thunder

micahmo commented 1 week ago

The youtube player doesn't have an unmute which means should you want to enable audio you have to go to settings to disable auto mute.

I noticed this as well, and I like your suggestion for adding a button!

P.S. One more thing I noticed on Android is that, once you enter fullscreen mode, Thunder stays in fullscreen mode even after exiting the video player. This means that the navigation and notification bars are hidden and have to be "swiped in". @ggichure Any ideas on that?

hjiangsu commented 1 week ago

Thanks for the feedback! I'll add in the mute button for the youtube player. It would be nice if we could combine both players into one to make everything more consistent but that can be explored in the future.

One more thing I noticed on Android is that, once you enter fullscreen mode, Thunder stays in fullscreen mode even after exiting the video player.

This also happens on iOS and I'm not too sure why - it's probably related to the internal logic of the video players.

ggichure commented 1 week ago

One more thing I noticed on Android is that, once you enter fullscreen mode, Thunder stays in fullscreen mode even after exiting the video player.

We can play around with this to achieve desired result.

For BetterPlayer , although this seems to be working correctly without this.

 List<SystemUiOverlay> systemOverlaysAfterFullScreen = SystemUiOverlay.values,
  List<DeviceOrientation> deviceOrientationsAfterFullScreen = const [DeviceOrientation.portraitUp, DeviceOrientation.portraitDown, DeviceOrientation.landscapeLeft, DeviceOrientation.landscapeRight],

 BetterPlayerConfiguration betterPlayerConfiguration = BetterPlayerConfiguration(
      aspectRatio: 16 / 10,
      fit: BoxFit.cover,
      autoPlay: autoPlayVideo(thunderBloc),
      fullScreenByDefault: thunderBloc.videoAutoFullscreen,
      looping: thunderBloc.videoAutoLoop,
      autoDetectFullscreenAspectRatio: true,
      useRootNavigator: true,
      systemOverlaysAfterFullScreen: [ //New
        SystemUiOverlay.bottom, //New
        SystemUiOverlay.top, //New
      ],

Youtube Player

child: ypf.YoutubePlayerBuilder(
                  onExitFullScreen: () {
                    SystemChrome.setEnabledSystemUIMode(SystemUiMode.edgeToEdge);
                  },

https://github.com/thunder-app/thunder/assets/31619962/f7a5b016-b5b4-4258-95fa-3ec86638d193

hjiangsu commented 1 week ago

Alright, I've applied more changes to fix the mute issue and some other various issues. It took a bit longer to implement the mute because the youtube player doesn't seem to provide a status on whether or not the video is muted, so the mute status is handled by externally.

We can play around with this to achieve desired result.

Thanks for providing those suggestions! I only applied the configuration for the youtube player as that was causing issues for me. Let me know if everything else looks good - I'll only focus on fixing existing issues rather than adding new things as those are better suited to a separate PR!

micahmo commented 1 week ago

I think the best way to review this is to test it. 😊 Since @ggichure approved, you can merge any time, but I will also build this and run it for a little while if that's ok. 😊

micahmo commented 1 week ago

One super small thing I just noticed is that playing a video doesn't respect the "Mark Read After Viewing Media" setting. Otherwise so far so good...

ggichure commented 1 week ago

One super small thing I just noticed is that playing a video doesn't respect the "Mark Read After Viewing Media" setting. Otherwise so far so good...

Working on this on a different branch

thunder-app / thunder