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: Add video loop options #653

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

Conversation

Yashraj254
Copy link
Contributor

Fix #572 added a functionality to play videos in loop

sample8.mp4

@drogga
Copy link

drogga commented Oct 3, 2023

Minor thing, but in my opinion it would've looked a lot better if the icon wasn't fill color, in the video it outstands from the others...

@Yashraj254
Copy link
Contributor Author

yeah i was thinking the same but the problem was there was no separate icon availble in vector assets for LoopOff button, so i just changed the color, now i have replace that icon with a different one... tell me how it looks now

sample9.mp4

@drogga
Copy link

drogga commented Oct 3, 2023

Good job. It looks better now.

@anilbeesetti
Copy link
Owner

Hey @Yashraj254, What i'm thinking will be good is loop options like "loop one video", "loop all videos", "loop off".

@anilbeesetti anilbeesetti changed the title Loop video feature added Feat: Add video loop options Oct 9, 2023
@Yashraj254
Copy link
Contributor Author

Yashraj254 commented Oct 9, 2023

@anilbeesetti alright.. working on it

Comment on lines +958 to +959
VideoLoop.LOOP_ALL -> {
player.repeatMode = Player.REPEAT_MODE_OFF
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
VideoLoop.LOOP_ALL -> {
player.repeatMode = Player.REPEAT_MODE_OFF
VideoLoop.LOOP_ALL -> {
player.repeatMode = Player.REPEAT_MODE_ALL

Copy link
Contributor Author

@Yashraj254 Yashraj254 Oct 14, 2023

Choose a reason for hiding this comment

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

If we try using it here, Player.REPEAT_MODE_ALL will keep playing the same video infinitely; it acts the same as Player.REPEAT_MODE_ONE because, if you try to print the player's mediaItemCount, it returns 1, which is the currently playing media item.

In the playVideo() method, this line specifies that only one media item is added in the player's mediaItems list and not the whole playlist.
player.setMediaItem(mediaStream, viewModel.currentPlaybackPosition ?: C.TIME_UNSET)

Player.REPEAT_MODE_ALL would have worked if player.setMediaItems(mediaStreamList) would have been used instead of player.setMediaItem(mediaStream), i.e. , adding the entire playlist at once and not clearing up the player's mediaItems list and then adding a single mediaItem every time playVideo() is called.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it

# Conflicts:
#	feature/player/src/main/java/dev/anilbeesetti/nextplayer/feature/player/PlayerActivity.kt
#	feature/player/src/main/res/layout/exo_player_control_view.xml
@Yashraj254
Copy link
Contributor Author

hey @anilbeesetti conflicts resolved. Ready for merge. Any further changes or feedback?

@drogga
Copy link

drogga commented Jan 25, 2024

@anilbeesetti Are you still around ?

@anilbeesetti
Copy link
Owner

Hey @drogga, I'm here. Thanks for checking in. I've been caught up with my new job and other stuff for the last two months. I'll be more engaged starting next month.

# Conflicts:
#	core/model/src/main/java/dev/anilbeesetti/nextplayer/core/model/PlayerPreferences.kt
#	feature/player/src/main/res/layout/exo_player_control_view.xml
@Ausk8815
Copy link

Ausk8815 commented May 3, 2024

Can we please get this merged? this basic feature is all that is stopping myself and i am sure many others from using this player right now.

@apprehensions
Copy link

mpv-android and just (video) player has this feature.

@Ausk8815
Copy link

Is there an issue with merging this request? I would really like to switch to this player. I am considering just forking it and manually copying the pull request and compiling an apk at this point.

If there is an actual merge issue maybe i can help?

@anilbeesetti
Copy link
Owner

Is there an issue with merging this request? I would really like to switch to this player. I am considering just forking it and manually copying the pull request and compiling an apk at this point.

If there is an actual merge issue maybe i can help?

Hey, I will try to merge this for the upcoming release

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

Successfully merging this pull request may close these issues.

Having a loop video option
6 participants