Unity-Technologies / com.unity.webrtc

WebRTC package for Unity
Other
739 stars 185 forks source link

Add method to import/export audio track from/to external #919

Closed Nemo-G closed 1 year ago

Nemo-G commented 1 year ago

Make package work with other sound engine.

  1. Add GetData method so that data can be pulled from audio track
  2. Add UnsafeSetData method so that we can feed external data into audio track
unity-cla-assistant commented 1 year ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Nemo Guan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Nemo-G commented 1 year ago

Hi @karasusan , please refer to this issue for details.

karasusan commented 1 year ago

Thanks for your contribution!

I have a question, AudioStreamTrack already has SetData API, why do you want to add UnsafeSetData?

Nemo-G commented 1 year ago

Thanks for your contribution!

I have a question, AudioStreamTrack already has SetData API, why do you want to add UnsafeSetData?

This is because that inside SetData, NativeArray is used. And the allocator limit the caller context to be a Unity managed thread. However this API would be called inside Wwise Plugin C++ which using NativeArray(NativeContainer) will give a complier error.

karasusan commented 1 year ago

And the allocator limit the caller context to be a Unity managed thread.

Oh, I misunderstood that NativeArray is thread safe. Do you mean your issue is related this post in Unity Forum? https://forum.unity.com/threads/is-nativearray-thread-safe-for-its-allocation-and-deallocation.822846/

I would like to fix SetData method to make thread-safe instead of creating new method.

karasusan commented 1 year ago

How about Span in .Net? We may can replace Span instead of NativeArray.

Nemo-G commented 1 year ago

@karasusan You mean you want to change NativeArray data structure to Span in the original SetData ? I think that seems better and by that time we don't need add UnsafeSetData.

I think maybe we can merge it and when Span change is ready, we remove this? Or you would like me to update this PR and modify SetData directly?

karasusan commented 1 year ago

@Nemo-G I'm going to fix SetData, and if it works well, we can remove this PR.

karasusan commented 1 year ago

I fixed the issue in this draft PR. https://github.com/Unity-Technologies/com.unity.webrtc/pull/926

karasusan commented 1 year ago

@Nemo-G One question, you added GetData method in the AudioStreamTrack class, but I think we should make a delegate method to fetch audio buffer from an other peer. What do you think about that?

Nemo-G commented 1 year ago

@Nemo-G One question, you added GetData method in the AudioStreamTrack class, but I think we should make a delegate method to fetch audio buffer from an other peer. What do you think about that?

@karasusan

That sounds good. I'm fine with the delegation. In fact GetData in the PR, is not a good function name as it actually feeds the data to track instead of getting data. And this method implicitly assumes that the _streamRenderer is correctly initialized.

But I would like to point out that the delegation should be used only when there is no internally attached audio clip/source to the AudioStreamTrack. One track now only allow one source. Because of this, maybe the delegator could be added into the AudioStreamTrack constructor? Like new AudioStreamTrack( AudioDelegator delegator) as a new type of track?

karasusan commented 1 year ago

@Nemo-G I agree with you. I'm going to try your idea that making the new constructor with a callback argument.

karasusan commented 1 year ago

I made a PR for onReceive delegation. https://github.com/Unity-Technologies/com.unity.webrtc/pull/927

karasusan commented 1 year ago

@Nemo-G I pushed two draft PR but I would like to know your issues are fixed by these changes. Can you review them?

One note, if you want to check on your environment, you need to replace native plugins with this commit. https://github.com/Unity-Technologies/com.unity.webrtc/compare/main...chore/update-plugin

Nemo-G commented 1 year ago

@Nemo-G I pushed two draft PR but I would like to know your issues are fixed by these changes. Can you review them?

One note, if you want to check on your environment, you need to replace native plugins with this commit. main...chore/update-plugin

Hi @karasusan, I'm fine with those changes. It fully meets my demands.

I'll try with your latest libwebrtc later.

karasusan commented 1 year ago

@Nemo-G Thanks for checking. I am opening these draft PRs.

karasusan commented 1 year ago

We merged other PRs to fix this issue. I am closing this.