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

[BUG]: Ios single video making or repeating multiple sound #3567

Open
keval7838 opened this issue Mar 6, 2024 · 28 comments · Fixed by #3875
Open

[BUG]: Ios single video making or repeating multiple sound #3567

keval7838 opened this issue Mar 6, 2024 · 28 comments · Fixed by #3875
Assignees
Labels
Accepted Issue is confirmed and accepted by maintainers team bug

Comments

@keval7838
Copy link

Version

v6 (Beta)

What platforms are you having the problem on?

iOS

Architecture

Old architecture

What happened?

whenever I play the video after changing the template then it will play normally but most of the time it play multiple sounds of single video(like reverb)

Reproduction

Steps to reproduce this bug are:

listen whole video.

WhatsApp.Video.2024-03-04.at.4.49.19.PM.mp4
@keval7838 keval7838 added the bug label Mar 6, 2024
@keval7838 keval7838 changed the title [BUG]: Ios single video making multiple sound [BUG]: Ios single video making or repeating multiple sound Mar 6, 2024
@KidsOnShred
Copy link

I am also experiencing this issue. I suspect it is caused by rerenders creating new native instances of the video which are not being destroyed properly.

I also have audio that is continuing to play after my component has been unmounted. I am working around this by just having the video muted on render and being manually unmuted and also checking focus to stop videos playing and muting them also. I think there must be memory leaking as a result of all this

@tdammy92
Copy link

@KidsOnShred Please where u able to fix this at your end ?

@freeboub
Copy link
Collaborator

Can you please clarify which beta version you use? And can you please try on the last one please ? (Beta.6)

@KidsOnShred
Copy link

I worked around it by auto muting the video in the start then manually unmute the one video which stops the sound duplicating but I suspect that multiple videos are playing in the background if the component ever rerenders. I am using 5.2.1

@coofzilla
Copy link
Contributor

this is an issue on version: 6.0.0-beta.8 as well. Basically, single player has two audio playbacks.

@rajgupta027
Copy link

Facing same issue still on IOS on changing url.
Version 6.1.2

@freeboub
Copy link
Collaborator

Can you please provide a sample? I don't think this is reproduced with the sample included in this repo ?

@mk0116
Copy link

mk0116 commented Jun 4, 2024

Facing same issue here too.
Version 6.1.2
It is reproducible when you play a video with a url, unmount, mount a video component and play video with a url again.(doesn't matter if the urls are same or not)

@rajgupta027
Copy link

@mk0116 @freeboub Its concerning now since I am unable to move to latest (6.1.2) package in my project. Seems I'll have to wait before more stable version is out.
Also, I need IMA google ads enabled for my project which is NOT present in previous stable version (5.2.1). Requesting the team to please look into it at the earliest.

@freeboub
Copy link
Collaborator

freeboub commented Jun 4, 2024

I agree this is critical, but are you able to reproduce it with a sample ? Last time I tried, I was not able to reproduce with the sample included in the repo. I don't really know ho to progress...

@freeboub
Copy link
Collaborator

freeboub commented Jun 4, 2024

Ok, I am able to reproduce the issue with following patch in the sample:

diff --git a/examples/basic/src/VideoPlayer.tsx b/examples/basic/src/VideoPlayer.tsx
index f712e013..4d720221 100644
--- a/examples/basic/src/VideoPlayer.tsx
+++ b/examples/basic/src/VideoPlayer.tsx
@@ -89,6 +89,7 @@ interface StateType {
   poster?: string;
   showNotificationControls: boolean;
   isSeeking: boolean;
+  _key: number;
 }
 
 class VideoPlayer extends Component {
@@ -120,6 +121,7 @@ class VideoPlayer extends Component {
     poster: undefined,
     showNotificationControls: false,
     isSeeking: false,
+    _key: 0,
   };
 
   // internal usage change to index if you want to select tracks by index instead of lang
@@ -557,6 +560,13 @@ class VideoPlayer extends Component {
     }
   };
 
+  componentDidMount(): void {
+      this.setState({_key: this.state._key + 1})
+      setTimeout(()=> {
+        this.setState({_key: this.state._key + 1})
+      })
+  }
+
   onSelectedTextTrackChange = (itemValue: string) => {
     console.log('on value change ' + itemValue);
     this.setState({
@@ -762,6 +772,7 @@ class VideoPlayer extends Component {
     return (
       <TouchableOpacity style={viewStyle}>
         <Video
+          key={this.state._key}
           showNotificationControls={this.state.showNotificationControls}
           ref={(ref: VideoRef) => {
             this.video = ref;

@mk0116
Copy link

mk0116 commented Jun 4, 2024

@freeboub I found a related code error while debugging the issue. Player objects are not popping properly from the observers.

So that it keeps adding player in the observers object.

-        if let observer = observers[players.hashValue] {
+        if let observer = observers[player.hashValue] {
             observer.invalidate()
         }
 
-        observers.removeValue(forKey: players.hashValue)
+        observers.removeValue(forKey: player.hashValue)
         players.remove(player)

But this is just a piece.

I found a main problem of the issue. The views do init -> registerPlayer -> deinit first time but second time, it does init the RCTPlayer view and registerPlayer twice and deinit just once. init -> registerPlayer -> init -> registerPlayer -> deinit.
This leads also to stacking player objects to the observers object and never pop out.

For me I was able to resolve the issue by patching the package with the code above and memo the react view so that view renders not too many times and ensured it does not detach and attach in a short time.

@freeboub
Copy link
Collaborator

freeboub commented Jun 4, 2024

@mk0116 on my side, I also see that we don't stop the player in deinit {
This is also a part as multiple reloads still reproduce the issue.
I'll keep you updated, and thank you for the patch !

@freeboub
Copy link
Collaborator

freeboub commented Jun 4, 2024

ping @KrzysztofMoch can you confirm it makes sense for you ?

@freeboub
Copy link
Collaborator

freeboub commented Jun 4, 2024

Here is an aditionnal patch which make sense I think.
I didn't find the real root cause, but I think there is an issue with some ref not uninitialized.
I don't reproduce the issue once this is added:
Let me know if it's also fixing the issue on your side !

@@ -1239,21 +1239,31 @@ class RCTVideo: UIView, RCTVideoPlayerViewControllerDelegate, RCTPlayerObserverH
     }
 
     // MARK: - Lifecycle
 
     override func removeFromSuperview() {
+        self._player?.replaceCurrentItem(with: nil)
         if let player = _player {
             player.pause()
             NowPlayingInfoCenterManager.shared.removePlayer(player: player)
         }
+        _playerItem = nil
+        _source = nil
+        _chapters = nil
+        _drm = nil
+        _textTracks = nil
+        _selectedTextTrackCriteria = nil
+        _selectedAudioTrackCriteria = nil
+        _presentingViewController = nil
 
         _player = nil
         _resouceLoaderDelegate = nil
         _playerObserver.clearPlayer()
 
         #if USE_GOOGLE_IMA
             _imaAdsManager.releaseAds()
+            _imaAdsManager = nil
         #endif
 
         self.removePlayerLayer()
 
         if let _playerViewController {

@freeboub
Copy link
Collaborator

freeboub commented Jun 4, 2024

BTW, I reproduce similar issue on android also :'(

@freeboub
Copy link
Collaborator

freeboub commented Jun 4, 2024

@mk0116 I pushed a branch with both fixes here: #3875, can you please test if it fixes your issue

@freeboub
Copy link
Collaborator

freeboub commented Jun 7, 2024

Will be integrated in 6.2.0

@mk0116
Copy link

mk0116 commented Jun 12, 2024

Thank you for your effort @freeboub I will test it soon and let you know.

@MTPL0005-AbhishekDube
Copy link

MTPL0005-AbhishekDube commented Jul 2, 2024

@freeboub I have encounter this issue when url for video changes the video plays two audios. version(6.2.0 & 6.3.0)

@rajgupta027
Copy link

@MTPL0005-AbhishekDube @freeboub I am still facing this issue. Applied the above patch as well.

@freeboub
Copy link
Collaborator

freeboub commented Jul 2, 2024

Can you provide a sample please ?

@MTPL0005-AbhishekDube
Copy link

MTPL0005-AbhishekDube commented Jul 2, 2024

@freeboub I use similar code as video example <Video bufferConfig={{ minBufferMs: 15000, maxBufferMs: 50000, bufferForPlaybackMs: 2500, bufferForPlaybackAfterRebufferMs: 5000, cacheSizeMB: 0, }} // fullscreen={fullscreen} muted={muted} onAspectRatio={this.onAspectRatio} onAudioTracks={this.onAudioTracks} onBuffer={this.onBuffer} onEnd={this.onEnd} onLoad={this.onLoad} onLoadStart={this.onVideoLoadStart} onProgress={this.onProgress} onReceiveAdEvent={this._onReceiveAdEvent} onTextTracks={this.onTextTracks} paused={paused} pictureInPicture={false} playInBackground={playInBackground} playWhenInactive={playInBackground} poster={courseImage} posterResizeMode="cover" preventsDisplaySleepDuringVideoPlayback={true} rate={rate} ref={(ref) => { this.videoPlayerRef = ref; }} resizeMode={ResizeMode.CONTAIN} selectedAudioTrack={selectedAudioTrack} selectedTextTrack={selectedTextTrack} showNotificationControls={playInBackground} source={{ uri: url, metadata: { title: title, artist: mentorName, imageUri: courseImage, }, }} style={fullscreen ? styles.fullscreen : styles.inlineScreen} volume={volume} /> I change the source uri onClick of list of lessons.

@freeboub
Copy link
Collaborator

freeboub commented Jul 2, 2024

We need a sample in a dedicated repository! Stop copy pasting extract of code please...

@rajgupta027
Copy link

rajgupta027 commented Sep 5, 2024

@freeboub Try this snippet. I am able to reproduce this issue 100% times in ios.
React Native: 0.71
React Native Video: 6.1.2
Apparently, whenever there is instant mounting/unmounting of video player, audio leak is observed.

//App.js

import React, { useState ,useEffect} from "react";
import { Dimensions } from "react-native";
import Video from "react-native-video";

const App = () => {
const [mount, setMount] = useState(true);
useEffect(() => {
setMount(false);
}, []);

return (
mount && (
<Video
source={{
uri: "http://content.jwplatform.com/manifests/vM7nH0Kl.m3u8",
}}
style={{
height: Dimensions.get("screen").width,
width: Dimensions.get("screen").width,
backgroundColor: "red",
}}
/>
)
);
};

In my case,I prevented instant mount/unmounting of player and issue is resolved for me for the time being.
In my understanding, deinit method is not being called when this scenario is observed.

@freeboub
Copy link
Collaborator

freeboub commented Sep 5, 2024

@rajgupta027 on 6.5.0 ?

@rajgupta027
Copy link

@rajgupta027 on 6.5.0 ?
Yes, its happening on latest version(6.5.0) as well.

Screenshot 2024-09-06 at 15 49 53

Normal Case: Video Playback works fine.

WhatsApp.Video.2024-09-06.at.3.49.21.PM.mp4

Instant Mount/Unmount Case: Audio Leak is observed.

WhatsApp.Video.2024-09-06.at.3.49.14.PM.mp4

@freeboub freeboub reopened this Sep 6, 2024
Copy link

github-actions bot commented Oct 7, 2024

This issue is stale because it has been open for 30 days with no activity. If there won't be any activity in the next 14 days, this issue will be closed automatically.

@github-actions github-actions bot added the stale Closed due to inactivity or lack or resources label Oct 7, 2024
@freeboub freeboub self-assigned this Oct 7, 2024
@freeboub freeboub added Accepted Issue is confirmed and accepted by maintainers team and removed stale Closed due to inactivity or lack or resources labels Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Issue is confirmed and accepted by maintainers team bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants