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

chore(android): update UI controller according to media3 #3914

Open
wants to merge 54 commits into
base: master
Choose a base branch
from

Conversation

seyedmostafahasani
Copy link
Collaborator

@seyedmostafahasani seyedmostafahasani commented Jun 14, 2024

Summary

Upgrade player UI to align with media3 demo app

Motivation

Using the native controller to close these issues. (#4019, #3301, #3232, #2599, #4043)

Changes

Test plan

@seyedmostafahasani seyedmostafahasani marked this pull request as draft June 15, 2024 06:20
Copy link
Collaborator

@freeboub freeboub left a comment

Choose a reason for hiding this comment

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

It looks like you didn't merge conflicts correctly (around drm props and allowchunklesspreparation. Can you please check that ?
I would like to test it afterward

@bulkinav
Copy link

Hi @seyedmostafahasani. Can you also add a screenshot of the new UI here? I would like to see what it looks like now )

@seyedmostafahasani
Copy link
Collaborator Author

Hi @seyedmostafahasani. Can you also add a screenshot of the new UI here? I would like to see what it looks like now )

@bulkinav thanks for your suggestion. I think it is better to implement the basics of the UI first, then we can implement the screenshot feature in the future in a new PR.
@freeboub what do you think?

@seyedmostafahasani
Copy link
Collaborator Author

@freeboub I think we can remove the fullscreen prop in V7. The user can handle fullscreen mode with the setFullScreen method, so we don’t need an additional prop like fullscreen. What is your opinion?

@seyedmostafahasani seyedmostafahasani marked this pull request as ready for review June 16, 2024 23:00
@seyedmostafahasani
Copy link
Collaborator Author

@freeboub, if everything works correctly, please don’t merge this branch to master yet. First, let me remove the additional code, then we can merge this branch to master.

@seyedmostafahasani
Copy link
Collaborator Author

@freeboub, did you have enough time to test this PR?

@ashnfb
Copy link
Contributor

ashnfb commented Jun 20, 2024

I've tried this version of UI with a video that has captions and multiple audio options
https://bitmovin-a.akamaihd.net/content/sintel/hls/playlist.m3u8

Does the new UI have the ability to show a textTracks selection menu? I could not see a way to turn on / off captions

The audio selection menu also appears buggy, as it repeats the options twice

@seyedmostafahasani
Copy link
Collaborator Author

seyedmostafahasani commented Jun 21, 2024

I've tried this version of UI with a video that has captions and multiple audio options https://bitmovin-a.akamaihd.net/content/sintel/hls/playlist.m3u8

Does the new UI have the ability to show a textTracks selection menu? I could not see a way to turn on / off captions

The audio selection menu also appears buggy, as it repeats the options twice

@ashnfb Yeah, it's possible to show the caption button in the new UI. I added it. Please check it out.

@seyedmostafahasani
Copy link
Collaborator Author

seyedmostafahasani commented Jun 21, 2024

@ashnfb regarding the audio selection menu, I think this URL has an issue: https://bitmovin-a.akamaihd.net/content/sintel/hls/playlist.m3u8. I've tested it with other URLs, as shown below, and the audio menu works correctly.

  1. https://flipfit-cdn.akamaized.net/flip_hls/661f570aab9d840019942b80-473e0b/video_h1.m3u8
  2. https://devstreaming-cdn.apple.com/videos/streaming/examples/img_bipbop_adv_example_fmp4/master.m3u8
    What do you think?

@freeboub
Copy link
Collaborator

Just tested, I found 2 issues fo now:

  • in the sample app, the controls are displayed by default even if controls={false} by default
  • in the sample app, when I go to "no view" item and then channel up to 'another live sample', the video shows a big resizing glitch

@seyedmostafahasani
Copy link
Collaborator Author

@JerakRus
Thank you for testing. Please test it again.

@JerakRus
Copy link

I tested it and now it works, thanks for the work done!

@seyedmostafahasani
Copy link
Collaborator Author

@freeboub @KrzysztofMoch
Please review this PR, as it will allow us to close some issues after merging.

# Conflicts:
#	android/src/main/java/com/brentvatne/common/api/ControlsConfig.kt
#	android/src/main/java/com/brentvatne/exoplayer/ExoPlayerView.java
#	android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java
#	docs/pages/component/props.mdx
# Conflicts:
#	android/src/main/java/com/brentvatne/exoplayer/ExoPlayerView.java
#	android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java
@freeboub
Copy link
Collaborator

freeboub commented Sep 4, 2024

@seyedmostafahasani I would like to focus on this PR for my next task, can you please remerge with master ?
I would like to handle open PRs before doing anything else :)

@seyedmostafahasani
Copy link
Collaborator Author

@seyedmostafahasani I would like to focus on this PR for my next task, can you please remerge with master ? I would like to handle open PRs before doing anything else :)

Thanks @freeboub .
For sure! I will do it.

# Conflicts:
#	android/src/main/java/com/brentvatne/common/api/ControlsConfig.kt
#	android/src/main/java/com/brentvatne/exoplayer/FullScreenPlayerView.kt
#	android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerView.java
#	docs/pages/component/props.mdx
#	src/specs/VideoNativeComponent.ts
#	src/types/video.ts
@seyedmostafahasani
Copy link
Collaborator Author

@freeboub
I think we need to include a comment about the subtitlesFollowVideo property when releasing a new version with these changes. The comment should read: This property was effective before version 6.4.6.

@freeboub
Copy link
Collaborator

freeboub commented Sep 4, 2024

@seyedmostafahasani following text is presented twice in the doc and is not true anymore, can you please have a look to this point ?

On iOS, this displays the video in a fullscreen view controller with controls.
On Android, this puts the navigation controls in fullscreen mode. It is not a complete fullscreen implementation, so you will still need to apply a style that makes the width and height match your screen dimensions to get a fullscreen video.

@freeboub
Copy link
Collaborator

freeboub commented Sep 5, 2024

sorry @seyedmostafahasani but we cannot merge this PR as is.
look the zapping with the sample when we are in stretch mode:
With your version:
newVersion.webm
With current master:
oldVersion.webm

On new version there is a remaining black square (I think it is the shutterView) during content change

@freeboub
Copy link
Collaborator

freeboub commented Sep 5, 2024

Another video with a 'normal' usage, We also see strange resizing during navigation, please see the video:
Normal.webm

If we want to progress with this PR, I think we should split it in sub PRs, maybe move in another PR the new features would be a good first step. (showSubtitleButton and showSettingButton)

@seyedmostafahasani
Copy link
Collaborator Author

sorry @seyedmostafahasani but we cannot merge this PR as is. look the zapping with the sample when we are in stretch mode: With your version: newVersion.webm With current master: oldVersion.webm

On new version there is a remaining black square (I think it is the shutterView) during content change

I am checking it.

# Conflicts:
#	android/src/main/java/com/brentvatne/exoplayer/FullScreenPlayerView.kt
@seyedmostafahasani
Copy link
Collaborator Author

@freeboub
Did you see my changes?

@freeboub
Copy link
Collaborator

@seyedmostafahasani No sorry, no time to test for now. I'll do it soon !

@@ -7,6 +7,8 @@ class ControlsConfig {
var hideSeekBar: Boolean = false
var seekIncrementMS: Int = 10000
var hideDuration: Boolean = false
var showSubtitleButton: Boolean = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

@seyedmostafahasani Can you please start by extracting showSubtitleButton & showSettingButton in a separated PR please ?
I am not sure this PR will me merged unfortunnatly, I am afraid of posssible regressions :/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for reviewing again.
Yeah, sure.
I will do it.

@KrzysztofMoch
Copy link
Member

KrzysztofMoch commented Oct 28, 2024

@seyedmostafahasani I would like to merge this, if you don't have time to resolve conflicts please let me know I will clone PR and do It by myself

@seyedmostafahasani
Copy link
Collaborator Author

@KrzysztofMoch
I will do it tonight.

@guihendias
Copy link

Just a comment: This PR will not close #3232 since the iOS implementation has not been added. If someone could add the same feat on iOS, it would be ACE! 🙏

Great work all around!

@fredrifoUni
Copy link

@seyedmostafahasani @freeboub

Hi, I just wanted to check in about the status on this PR. These are some great changes, and I would love to see them merged in.

@seyedmostafahasani
Copy link
Collaborator Author

@fredrifoUni
I will work on it over the weekend. Sorry, I’ve been very busy these days, but I’ll resolve all conflicts then. I think we can merge it afterward.

@fredrifoUni
Copy link

@fredrifoUni I will work on it over the weekend. Sorry, I’ve been very busy these days, but I’ll resolve all conflicts then. I think we can merge it afterward.

That's great to hear. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To Triage
Development

Successfully merging this pull request may close these issues.

9 participants