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

Crunchyroll Android Improvements #2689

Merged

Conversation

armands-malejevs
Copy link
Collaborator

This is a collection of improvements we've done for the Crunchyroll Android app:

  • Support disabling buffering
  • Fix AudioFocus bug that could cause the player to stop responding to play/pause in some instances.
  • Fix player crashing when it is being cleared.
  • Add support for customising back buffer duration and handle network errors gracefully to prevent releasing the player when network is lost.
  • Allow player to be init before source is provided, and later update once a source is provided.
  • Adds handling for providing a empty source in order to stop playback and clear out any existing content
  • Add support for onBufferProgress prop on Android to get buffer data even when the player is paused.
  • Add support for detecting if format is supported and exclude unsupported resolutions from auto quality selection and video track info in RN.
  • Improve error handling
  • Add support for L1 to L3 Widevine fallback if playback fails initially.
  • Reduce buffer size based on available heap
  • Force garbage collection when there is no available memory
  • Improve memory usage
  • Support disabling screen recording
  • Improved error capturing
  • Fix DRM init crashes
  • Improve progress reporting
  • Fix progress loss when network connection is regained

Testing

Either use the test apps in this project or link your own project to this PR and test everything related to playback.

armands-malejevs and others added 30 commits March 15, 2021 16:51
Fix AudioFocus bug that could cause the player to stop responding to play/pause in some instances.

Fixes issue react-native-video#1945

This was caused by the player requesting audio focus on each play (un-pause) and that resulted in a small window of Audio focus loss and then gain. The focus loss results in the player being paused while the player was supposed to play at the time. The solution is to keep track of Audio focus and not request new focus if we already have it.
Upgrade ExoPlayer from 2.11.4 to 2.13.2 and fix any related issues related to the upgrade and deprecated method use.
This PR adds the property disableBuffering: boolean for android. The PR was initally created as a personal fork and referenced in crunchyroll/vilos#1227

also updated RNVLoadControl constructor and method to reflect new ExoPlayer.LoadControl api

related ticket: https://jira.tenkasu.net/browse/VEX-3776
Fix player crashing when it is being cleared.
Add support for customizing back buffer duration and handle network errors gracefully to prevent releasing the player when network is lost.
- Allow player to be init before source is provided, and later update once a source is provided.
- Adds handling for providing a empty source in order to stop playback and clear out any existing content
Add support for onBufferProgress prop on Android to get buffer data even when the player is paused.
Add support for detecting if format is supported and exclude unsupported resolutions from auto quality selection and video track info in RN.
Add support for L1 to L3 Widevine fallback if playback fails initially.
Converts ios implementation from objective-c to swift.
Adds offline decryption key and uses it to decrypt content during offline playback

Jira: VEX-5938
https://jira.tenkasu.net/browse/VEX-5938

- Update to accept scheme for key required to play offline playback
- Uses provided scheme to intercept call from player and return the key
- Fixes player item observer removal pattern

### Reviews
- Major reviewer (domain expert): @armadilio3
#14)

Add support for content tracks and improve track selection to work even during ads playback.
This PR changes the behavior on old devices that have poor memory management.

Jira: VEX-6030
https://jira.tenkasu.net/browse/VEX-6030

The solution implied customizing the method shouldContinueLoading from RNVLoadControl to use only the available heap, performing tests on an old Nexus 5 it was determined the ideal bytes allocation to half the reported heap, that provided some buffering during ads but smooth playback during the video with no crashes (23:39 of 23:39 at the moment of writing this, video kept playing as expected after 3 ad breaks)

The fix is only targeting Marshmallow as the reduction of buffer is substantial and other versions that work properly should not get affected.

Depending on the test result of VEX-5758, this same fix can be applied to Nougat

Reviews
Major reviewer (domain expert): @armadilio3
Minor reviewer: @nickfujita
This PR is intended to prevent memory crashes when the free memory runs out

Jira: VEX-6030
https://jira.tenkasu.net/browse/VEX-6030

Velocity PR: crunchyroll/velocity#2043

By forcefully calling the garbage collector the memory crashes, specially on lower resolutions (<720p) are gone on devices with 2GB of ram or less and Android 6, also testing 3GB Android 7 with this patch, so far is working as intended

Reviews
Major reviewer (domain expert): @nickfujita
Minor reviewer: @jctorresM
Improve memory management to reduce pressure on low end devices.

JIRA: VEX-6365
This PR adds core actions to enable or disable screenshots

Jira: VEX-6540
https://jira.tenkasu.net/browse/VEX-6540

velocity PR: crunchyroll/velocity#2248

New core actions were added to control the player surface and mark it as secure. Sending the prop to react-native-video will recreate the surface of the player changing it to a SurfaceView and enabling security

Reviews
Major reviewer (domain expert): @nickfujita
Minor reviewer: @armadilio3
Updates disableDisconnectError retry condition to be more specific.

Jira: VEX-6518

Reviews
Major reviewer (domain expert): @armadilio3
Minor reviewer: @gabriel-rivero
public void run() {
if (player != null) {
double bufferedDuration = (double) (player.getBufferedPercentage() * player.getDuration() / 100);
eventEmitter.bufferProgress(0d, bufferedDuration);
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I understand you need additionnal informations on available buffer, but it should be added to onProgress event.
the new bufferProgress has 2 paramters: 0 Ok for futur use, so not needed now.
bufferedDuration which is exactly equals to bufferedDuration in onProgress.
The only difference is the type you use double instead of long.

Another difference is the frequency. If another issue for you is linked to the the missing report when playback is pause, I invite you to check following PR: #2664

For me this new event doesn't bring new infomation for application...

@cobarx
Copy link
Contributor

cobarx commented May 30, 2022

Agree with @armadilio3's suggestion that we record the improvements and take them up one by one. Our team has limited capacity to devote to changes, so picking them up incrementally would be much better.

Copy link
Contributor

@gabriel-rivero gabriel-rivero left a comment

Choose a reason for hiding this comment

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

double checked the changes in test-harness, features are working with no issues

@hueniverse hueniverse added 1 and removed 2 labels May 31, 2022
@armands-malejevs
Copy link
Collaborator Author

@freeboub I cleaned up some stuff 👍 I had some questions on some of your comments, let me know how you want to handle those

@armands-malejevs armands-malejevs requested a review from freeboub May 31, 2022 07:18
@armands-malejevs
Copy link
Collaborator Author

@freeboub Can you take another look? I fixed/responded to some of the feedback

@freeboub
Copy link
Collaborator

freeboub commented Jun 3, 2022

PR Tested on sample provided in exemple/basic:
I have following crash when playing local file (require('./broadchurch.mp4'))

[Fri Jun 03 2022 18:39:31.221] LOG {"error":{"errorStackTrace":"com.google.android.exoplayer2.ExoPlaybackException: Unexpected runtime error\n\tat com.google.android.exoplayer2.ExoPlayerImplInternal.handleMessage(ExoPlayerImplInternal.java:587)\n\tat android.os.Handler.dispatchMessage(Handler.java:102)\n\tat android.os.Looper.loop(Looper.java:223)\n\tat android.os.HandlerThread.run(HandlerThread.java:67)\nCaused by: java.lang.IllegalStateException\n\tat com.google.android.exoplayer2.util.Assertions.checkState(Assertions.java:86)\n\tat com.google.android.exoplayer2.source.ProgressiveMediaPeriod.selectTracks(ProgressiveMediaPeriod.java:281)\n\tat com.google.android.exoplayer2.source.MaskingMediaPeriod.selectTracks(MaskingMediaPeriod.java:186)\n\tat com.google.android.exoplayer2.MediaPeriodHolder.applyTrackSelection(MediaPeriodHolder.java:292)\n\tat com.google.android.exoplayer2.ExoPlayerImplInternal.reselectTracksInternal(ExoPlayerImplInternal.java:1643)\n\tat com.google.android.exoplayer2.ExoPlayerImplInternal.handleMessage(ExoPlayerImplInternal.java:503)\n\t... 3 more\n","errorCode":"2001","errorException":"com.google.android.exoplayer2.ExoPlaybackException: Unexpected runtime error","errorString":"ExoPlaybackException type : 2"}}

Please notice also that after the error occurs, onBufferProgress is still sent in loop

Edit: it is reproduced with all mp4 content
Edit2: In this PR I also add m3u and live streams samples it would be intersting to test with these new streams.

Thanks

@armands-malejevs
Copy link
Collaborator Author

@freeboub I fixed single track playback. Can you test again?

@freeboub
Copy link
Collaborator

freeboub commented Jun 7, 2022

@freeboub I fixed single track playback. Can you test again?

I confirm this is fixed, thanks

public void run() {
if (player != null) {
double bufferedDuration = (double) (player.getBufferedPercentage() * player.getDuration() / 100);
eventEmitter.bufferProgress(0d, bufferedDuration);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@armadilio3 This is the last point to handle I think.
I still don't understand why you need to duplicate this event as information is already available in onProgress event.
once again there is a bug in onProgress which is blocking even when player is paused, it has to be solved at least for live playback.
Duplicating event can overload the bridge, so having too much event is a bad idea.

The advatage of onProgress is that timeout can already be configured.
I also think having 1 event with multiple parameters is lighter than have 2 events in terms of CPU consumption

@armands-malejevs
Copy link
Collaborator Author

@freeboub I'm still not sold on having the an independent value buffer tied to on progress events which seems non intuitive if the two are unrelated to one another, but to get this PR moving forward I removed it. We can open a separate PR to properly implement the feature and discuss separating or leaving in the progress event.

@armands-malejevs armands-malejevs requested a review from freeboub June 8, 2022 08:33
@freeboub
Copy link
Collaborator

freeboub commented Jun 8, 2022

Thank you @armadilio3 !
In the same time, can you please review this PR and check if you are now well receiving the event even if player is paused or have connectivity issue : #2664

@hueniverse hueniverse merged commit bf11bb9 into TheWidlarzGroup:master Jun 9, 2022
@hueniverse hueniverse added this to the 6.0.0-alpha1 milestone Jun 9, 2022
@EyMaddis
Copy link

EyMaddis commented Dec 14, 2022

Force garbage collection when there is no available memory

I think this might be the culprit why our FireTV Sticks starting having frame drops, this works on 5.x versions of react-native-video

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

Successfully merging this pull request may close these issues.

10 participants