search

millicast / millicast-player-unreal-engine-plugin

Millicast Player plugin for Unreal Engine
Other
19 stars 15 forks source link

ExternalAudioConsumer passed to PeerConnection::Init should be removed #54

Closed improbable-andreaskrugersen closed 1 year ago

improbable-andreaskrugersen commented 1 year ago

I recently updated to latest on the UE4.27 branch and adapted to all the changes that happened in the meantime (we previously were on version 1.1).

It seems the preferred way to handle video and audio now is to AddConsumer to UMillicastXXXTrack.

However, the TWeakInterfacePtr<IMillicastExternalAudioConsumer that I added to FWebRTCPeerConnection::Init is still available. I didn't realise this when upgrading and I ended up with garbage audio because I had the consumer registered twice (once through the connection and once through the audio track).

I don't think the consumer passed into the connection serves any purpose any longer? Audio works either way, if you pass the consumer into the connection or if you add it to the audio track. For consistency it should probably always be through the audio track

dbaldassi commented 1 year ago

I have left it like this, thinking it could be use if you want a consumer that globally receive all audio data, but, it can be removed indeed. I think it can be a bit confusing.

rweber89 commented 1 year ago

@improbable-andreaskrugersen Thank you for pointing this out. I have also noticed that we do still have backwards-compatible branches, like UE4.27 in your case.

Alongside with fixing this issue I'll also work on getting support for all versions into our dev branch, just like we have done recently for the publisher plugin.

rweber89 commented 1 year ago

The PR is now up: https://github.com/millicast/millicast-player-unreal-engine-plugin/pull/58