An example of Unity native plugin of webrtc for Windows OS

An example of Unity native plugin of webrtc for Windows OS
Unity native plugin has to use Pinvoke technology in its APIs
This plugin dll can also be used by Windows C# applications other than
Unity.

BUG=webrtc:7389
Review-Url: https://codereview.webrtc.org/2823783002
Cr-Commit-Position: refs/heads/master@{#18108}
Committed: https://chromium.googlesource.com/external/webrtc/+/ad7cad8aba7ec1973be263ab5fd330b4a2244e19

[GeorgeZ, 2017-04-17 20:42:40]

The CQ bit was checked by gyzhou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 20:42:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-17 20:46:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 20:46:56]

Dry run: Try jobs failed on following builders:
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/18237)
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
  win_x64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/8314)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/24742)

[GeorgeZ, 2017-04-17 22:58:34]

The CQ bit was checked by gyzhou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 22:58:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-17 23:15:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 23:15:56]

Dry run: This issue passed the CQ dry run.

[GeorgeZ, 2017-04-19 17:00:09]

gyzhou@chromium.org changed reviewers:
+ niklase@chromium.org, tommi@webrtc.org

[GeorgeZ, 2017-04-19 17:00:09]

Niklas and Tommi,

Would you please have a review of this CL?

Thanks,

[niklas.enbom, 2017-04-19 17:19:11]

Quick high level comment, add a README file that explains what this is and how
it can be used. Also add an OWNERS file with yourself in it.

On 2017/04/19 17:00:09, GeorgeZ wrote:
> Niklas and Tommi,
> 
> Would you please have a review of this CL?
> 
> Thanks,

[GeorgeZ, 2017-04-19 22:16:50]

Patchset #3 (id:40001) has been deleted

[GeorgeZ, 2017-04-20 15:47:06]

On 2017/04/19 17:19:11, niklas.enbom wrote:
> Quick high level comment, add a README file that explains what this is and how
> it can be used. Also add an OWNERS file with yourself in it.
> 
> On 2017/04/19 17:00:09, GeorgeZ wrote:
> > Niklas and Tommi,
> > 
> > Would you please have a review of this CL?
> > 
> > Thanks,

Niklas,

The README and OWNERS files have added.

[niklas.enbom, 2017-04-20 23:08:50]

Still high level review. This is essentially a Windows Peerconnection DLL.
Tommi, do you see any other use case of this than Unity that would motivate a
different naming? Also, I realize we need a reviewer that knows the
PeerConnection C++ API better than I, can you add Lu as well?


On 2017/04/20 15:47:06, GeorgeZ wrote:
> On 2017/04/19 17:19:11, niklas.enbom wrote:
> > Quick high level comment, add a README file that explains what this is and
how
> > it can be used. Also add an OWNERS file with yourself in it.
> > 
> > On 2017/04/19 17:00:09, GeorgeZ wrote:
> > > Niklas and Tommi,
> > > 
> > > Would you please have a review of this CL?
> > > 
> > > Thanks,
> 
> Niklas,
> 
> The README and OWNERS files have added.

[tommi, 2017-04-21 09:47:51]

https://codereview.webrtc.org/2823783002/diff/60001/webrtc/examples/unityplug...
File webrtc/examples/unityplugin/simple_peer_connection.h (right):

https://codereview.webrtc.org/2823783002/diff/60001/webrtc/examples/unityplug...
webrtc/examples/unityplugin/simple_peer_connection.h:101: static int
peer_count_;
should these static vars perhaps be in an anonymous namespace in the cc file?
Any chance we can avoid having globals at all?

https://codereview.webrtc.org/2823783002/diff/60001/webrtc/examples/unityplug...
webrtc/examples/unityplugin/simple_peer_connection.h:107:
rtc::scoped_refptr<webrtc::PeerConnectionInterface> peer_connection_ =
initializing scoped_refptr like this, isn't necessary since the default ctor
does this anyway.

[GeorgeZ, 2017-04-21 22:00:03]

gyzhou@chromium.org changed reviewers:
+ lliuu@webrtc.org

[GeorgeZ, 2017-04-21 22:00:04]

I updated the CL to address review comments and added lliuu@ as a reviewer as
Niklas suggested.

https://codereview.webrtc.org/2823783002/diff/60001/webrtc/examples/unityplug...
File webrtc/examples/unityplugin/simple_peer_connection.h (right):

https://codereview.webrtc.org/2823783002/diff/60001/webrtc/examples/unityplug...
webrtc/examples/unityplugin/simple_peer_connection.h:101: static int
peer_count_;
On 2017/04/21 09:47:51, tommi (wëbrtc) wrote:
> should these static vars perhaps be in an anonymous namespace in the cc file?
> Any chance we can avoid having globals at all?

I moved those static variables to the cc file.

Class Conductor represents a peerconnection. An application may have multiple
peerconnections. All those peerconnections should share one peerconnection
factory. Therefore peer_connection_factory_ and its associated threads should be
static.

One way to avoid this is to add one more layer/class to managed all instances of
class conductor. Then peerconnection factory can be the managed class's none
static member variable. However this will lead quite a bit of code change.

https://codereview.webrtc.org/2823783002/diff/60001/webrtc/examples/unityplug...
webrtc/examples/unityplugin/simple_peer_connection.h:107:
rtc::scoped_refptr<webrtc::PeerConnectionInterface> peer_connection_ =
On 2017/04/21 09:47:51, tommi (wëbrtc) wrote:
> initializing scoped_refptr like this, isn't necessary since the default ctor
> does this anyway.

Done.

[lliuu, 2017-04-24 17:25:33]

George, could you clarify the intention of the CL a bit more?

If it is only an example (as opposed to ready-to-use native wrapper code for
unity), I think we need to be more explicit in comments about it; also there
does not seem to be anything specific to Unity in either code or README

https://codereview.webrtc.org/2823783002/diff/80001/webrtc/examples/unityplug...
File webrtc/examples/unityplugin/simple_peer_connection.h (right):

https://codereview.webrtc.org/2823783002/diff/80001/webrtc/examples/unityplug...
webrtc/examples/unityplugin/simple_peer_connection.h:24: class Conductor :
public webrtc::PeerConnectionObserver,
Can we call this SimplePeerConnection instead? Conductor as a name is very vague

[GeorgeZ, 2017-04-24 22:56:06]

Description was changed from

==========
An example of Unity native plugin of webrtc for Windows OS
Unity native plugin has to use Pinvoke technology in its APIs

BUG=webrtc:7389
==========

to

==========
An example of Unity native plugin of webrtc for Windows OS
Unity native plugin has to use Pinvoke technology in its APIs
This plugin dll can also be used by Windows C# applications other than
Unity.

BUG=webrtc:7389
==========

[GeorgeZ, 2017-04-24 22:57:35]

Hi Lu,

I updated the CL to address your comments.

Thanks

https://codereview.webrtc.org/2823783002/diff/80001/webrtc/examples/unityplug...
File webrtc/examples/unityplugin/simple_peer_connection.h (right):

https://codereview.webrtc.org/2823783002/diff/80001/webrtc/examples/unityplug...
webrtc/examples/unityplugin/simple_peer_connection.h:24: class Conductor :
public webrtc::PeerConnectionObserver,
On 2017/04/24 17:25:32, lliuu wrote:
> Can we call this SimplePeerConnection instead? Conductor as a name is very
vague

I modify the class name as you suggested.
Conductor was from the webrtc/examples/Peerconnection/client/conductor.h.

[lliuu, 2017-04-25 01:56:43]

Thanks George.

A high level comment, if this is only intended as an example, I think it will
make more sense if we include a functional demo, as other projects under
webrtc/examples do?

https://codereview.webrtc.org/2823783002/diff/100001/webrtc/examples/unityplu...
File webrtc/examples/unityplugin/unity_plugin_apis.cc (right):

https://codereview.webrtc.org/2823783002/diff/100001/webrtc/examples/unityplu...
webrtc/examples/unityplugin/unity_plugin_apis.cc:21: g_SimplePeerConnection_map;
I think the notation style should be something like g_simple_peer_connection_map

[GeorgeZ, 2017-04-25 23:52:45]

Hi Lu,

A functional Demo for Unity exists in a git repository that is outside of webrtc
code base. The demo is in C# and Unity specific stuff which cannot build in
webrtc code base. Therefore, the demo is not recommended to put into the webrtc
code base.

https://codereview.webrtc.org/2823783002/diff/100001/webrtc/examples/unityplu...
File webrtc/examples/unityplugin/unity_plugin_apis.cc (right):

https://codereview.webrtc.org/2823783002/diff/100001/webrtc/examples/unityplu...
webrtc/examples/unityplugin/unity_plugin_apis.cc:21: g_SimplePeerConnection_map;
On 2017/04/25 01:56:43, lliuu wrote:
> I think the notation style should be something like
g_simple_peer_connection_map

Done.

[lliuu, 2017-04-26 19:06:53]

On 2017/04/25 23:52:45, GeorgeZ wrote:
> Hi Lu,
> 
> A functional Demo for Unity exists in a git repository that is outside of
webrtc
> code base. The demo is in C# and Unity specific stuff which cannot build in
> webrtc code base. Therefore, the demo is not recommended to put into the
webrtc
> code base.
> 
>
https://codereview.webrtc.org/2823783002/diff/100001/webrtc/examples/unityplu...
> File webrtc/examples/unityplugin/unity_plugin_apis.cc (right):
> 
>
https://codereview.webrtc.org/2823783002/diff/100001/webrtc/examples/unityplu...
> webrtc/examples/unityplugin/unity_plugin_apis.cc:21:
g_SimplePeerConnection_map;
> On 2017/04/25 01:56:43, lliuu wrote:
> > I think the notation style should be something like
> g_simple_peer_connection_map
> 
> Done.

Hi,

I understand that putting the Unity project into webrtc repo will be
problematic. My main reservation is that IMHO this CL won't be very helpful for
3rd party developers without any Unity side hook-up or a functional demo to see
what is going on.

I think https://github.com/webrtc would be a better place for these files along
with a working Unity project; another option we have is making this into a more
generic C# webrtc peer connection plugin dll for Windows, but I think more code
cleanup will be required in that case.

But of course I won't have any objections if Niklas and Tommy think this CL is
fine as-is :)

[GeorgeZ, 2017-05-01 15:00:43]

Ping Niklas and Tommi.

[niklas.enbom, 2017-05-03 17:55:57]

I think this is a useful example to have, and I don't think it make sense to add
C# code that won't compile and be tested in any way. So adding just the C++ code
(like it's now) is the best option imo.

On 2017/05/01 15:00:43, GeorgeZ wrote:
> Ping Niklas and Tommi.

[lliuu, 2017-05-05 20:04:11]

On 2017/05/03 17:55:57, niklas.enbom wrote:
> I think this is a useful example to have, and I don't think it make sense to
add
> C# code that won't compile and be tested in any way. So adding just the C++
code
> (like it's now) is the best option imo.
> 
> On 2017/05/01 15:00:43, GeorgeZ wrote:
> > Ping Niklas and Tommi.

Thanks! Sorry for the delay, LGTM

[GeorgeZ, 2017-05-10 00:19:30]

Ping Tommi.

[tommi, 2017-05-10 17:46:24]

oh man, I'm sorry for the delay on this :-/
lgtm!

[GeorgeZ, 2017-05-10 23:57:30]

Niklas,

tommi and lliuu are fine with this CL. What's your opinion.

Thanks,

[niklas.enbom, 2017-05-10 23:59:18]

lgtm
On 2017/05/10 23:57:30, GeorgeZ wrote:
> Niklas,
> 
> tommi and lliuu are fine with this CL. What's your opinion.
> 
> Thanks,

[GeorgeZ, 2017-05-11 20:47:22]

The CQ bit was checked by gyzhou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 20:47:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-11 22:44:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 22:44:06]

Dry run: This issue passed the CQ dry run.

[GeorgeZ, 2017-05-11 23:06:58]

The CQ bit was checked by gyzhou@chromium.org

[GeorgeZ, 2017-05-11 23:06:59]

The patchset sent to the CQ was uploaded after l-g-t-m from lliuu@webrtc.org,
niklas.enbom@webrtc.org, tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2823783002/#ps140001 (title:
"Sync to head")

[commit-bot: I haz the power, 2017-05-11 23:07:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-11 23:10:03]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494544018956370,
"parent_rev": "38605965bd68ad1e376c530883041b9c12f26d3b", "commit_rev":
"ad7cad8aba7ec1973be263ab5fd330b4a2244e19"}

[commit-bot: I haz the power, 2017-05-11 23:10:07]

Description was changed from

==========
An example of Unity native plugin of webrtc for Windows OS
Unity native plugin has to use Pinvoke technology in its APIs
This plugin dll can also be used by Windows C# applications other than
Unity.

BUG=webrtc:7389
==========

to

==========
An example of Unity native plugin of webrtc for Windows OS
Unity native plugin has to use Pinvoke technology in its APIs
This plugin dll can also be used by Windows C# applications other than
Unity.

BUG=webrtc:7389

Review-Url: https://codereview.webrtc.org/2823783002
Cr-Commit-Position: refs/heads/master@{#18108}
Committed:
https://chromium.googlesource.com/external/webrtc/+/ad7cad8aba7ec1973be263ab5...
==========

[commit-bot: I haz the power, 2017-05-11 23:10:09]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/ad7cad8aba7ec1973be263ab5...