Skip to content
This repository has been archived by the owner on Mar 22, 2022. It is now read-only.

Missing headers in C++ API #123

Closed
tigerw opened this issue Nov 9, 2019 · 18 comments · Fixed by #190
Closed

Missing headers in C++ API #123

tigerw opened this issue Nov 9, 2019 · 18 comments · Fixed by #190
Assignees
Labels
bug Something isn't working
Milestone

Comments

@tigerw
Copy link

tigerw commented Nov 9, 2019

Hi, thanks for the great work!

I'm using Microsoft.MixedReality.WebRTC.Native.UWP in a UWP C++/WinRT project that targets Desktop and Mobile.

After the latest (1.0.1) update and with the removal of api.h, I attempted to include the C++ header peer_connection.h. However, compilation fails because that file includes audio_frame_observer.h which itself includes api/mediastreaminterface.h which appears to be an upstream Google header file.

I do not have the upstream WebRTC sources, since I understand the purpose of the NuGet package is to provide a prebuilt solution.

Please advise, many thanks!

@djee-ms
Copy link
Member

djee-ms commented Nov 9, 2019

Right. Unfortunately the C++ API is not very stable and not well tested at the moment, as efforts are focusing on the C# and Unity use cases. The C++ API as of today is tightly coupled with Google's WebRTC implementation, because I expected, for good reason or not, that C++ users might be more power users who want to switch back and forth between the two APIs. And therefore, the C++ API of MixedReality-WebRTC requires the associated C++ headers from the Google codebase, which are not currently present in the NuGet packages. Unlike the C#/Unity APIs where we completely wrap the Google API and expose only an alternate one we control.

Thinking out loud here, I am not sure how best to approach this issue:

  1. One possibility is to ship the headers in the NuGet packages. However because webrtc.lib is statically linked, this effectively means that the usable symbols from Google are only the one not pruned out by the linker, so the ones used by Microsoft.MixedReality.WebRTC.Native.dll. This is error-prone and very restrictive.
  2. Another avenue is completely hiding the Google API surface like we do for C#. This solves the issue, but means no possible way for power users to cast back to Google API types and use advanced features exposed by webrtc.lib when directly accessing its API surface. That being said, users cannot really do that at the moment because of linker pruning as in 1., so maybe that's OK.
  3. Finally, an alternative that both fixes the header issue above and retain the full Google API exposure is to build webrtc.lib into a DLL and consume it that way. The drawback is that the DLL size is one order of magnitude larger : expect a 500MB webrtc.dll instead of a 10MB Microsoft.MixedReality.WebRTC.dll. This is probably prohibitive for a lot of users.

@tigerw what do you think about those? What is your use case and what would you think is the best avenue going forward. I will talk internally with the team next week to brainstorm on that, but feedback is much welcome!

Tagging as "bug" because this makes the NuGet packages unusable 😥

@djee-ms djee-ms added the bug Something isn't working label Nov 9, 2019
@djee-ms djee-ms self-assigned this Nov 9, 2019
@tigerw
Copy link
Author

tigerw commented Nov 9, 2019

Thank you for such a quick response!

I must admit what I'm using this project for is much less advanced than that, we're working on a university project that aims to build a peer-to-peer drawing app (like Microsoft Whiteboard). Since it has to work on the web, we're using WebRTC data channels to transmit inking data, which means the UAP side of things also needs a native WebRTC library—I hear MS knows a little something about UWP, so here I am :P

For the version before 1.0.1, I was using the C API which was self-contained, but alas distinctly C.

I would argue that wrapping the Google API surface completely and only providing what's necessary for (presumably HoloLens/MR developers?) to use is best. C++ developers who find something missing could ask for it to be included, which then benefits C# and Unity too. I would say having two APIs for the same underlying library is confusing at best, and a massive increase in bundle size to support the "full" functionality would be unnecessary in most cases and certainly detrimental to Mobile app installs.

For what I'm doing now at least, the surface you expose is completely sufficient. I would say turning the NuGet packages into a build server for webrtc.dll is probably unnecessary, so option (2) might be better.

@djee-ms
Copy link
Member

djee-ms commented Nov 10, 2019

Ok thanks for the feedback. Any reason not to use C# in your particular case?

As a workaround until this is fixed you can grab the header files manually:

  • Checkout the repository recursively for the Google headers.
  • Grab and unzip Microsoft.MixedReality.WebRTC.Native.Core.UWP.nupkg (rename to .zip for unzipping) for the C++/WinRT generated headers, so you don't have to run the prebuild step which generates them. I don't remember on top of my head if those are used publicly, so might be unnecessary.

@djee-ms djee-ms added this to the M2 milestone Nov 13, 2019
@djee-ms
Copy link
Member

djee-ms commented Dec 9, 2019

Some update on this, we discussed internally and we are considering dropping support for DLL for C++ to avoid inherent limitations related to template/inline calls across shared modules being unsafe due to different compiler implementations and different heaps for memory allocations. The plan would be instead to ship a DLL of the core implementation and C-style interop layer, very similar to what exists now, and then have the C++ library sit on top of this and distributed as source only. This has several advantages:

  • no need to ship WebRTC headers (the current bug).
  • no need to care about template/inline calls across shared modules since the interface is C only.
  • the C++ library is small enough to be quick to compile, and can be integrated at will in projects, either linked statically or wrapped into a DLL via the user's choice (generic no-template/inline, per-compiler variant, custom standard library or C++ standard library types, etc.).

The drawbacks are that for us this requires writing more boilerplate code, as the existing C++ library moves above the interop layer, and we need a second (internal) layer of C++ code to interface the interop API with the underlying core implementation from Google (written in C++).

@djee-ms djee-ms mentioned this issue Dec 10, 2019
22 tasks
@drejx
Copy link

drejx commented Dec 10, 2019

Just adding my 2 cents here in agreeing with your approach. Generally C++ users are more comfortable with compiling from source and doing so will provide the most flexibility. Correct me if I'm wrong, but it would provide a base layer for integration into almost any platform (e.g. mobile) if one is willing to roll up their sleeves and build a proper interface for that platform and compile it.

@djee-ms
Copy link
Member

djee-ms commented Dec 11, 2019

Yes and no.

No because the problem of new platforms is (almost) never the C++ library we ship itself (MixedReality-WebRTC), but the underlying C++ code from Google, and the A/V integration which is platform-specific. Google provides some A/V integration on some platforms, which are generally OK for simple video chat, but can be limiting for other scenarios (e.g. raw audio access, see #92 for example). On other platforms, nothing is provided or you want to be integrating into an existing A/V stack (e.g. another game engine than Unity). So this refactor doesn't significantly change the work needed from that perspective.

Yes because it still makes it easier since the DLL boundary is the C-style interop API, and one can choose to either statically link the MixedReality-WebRTC C++ library if it is found to be a suitable integration, or even rewrite their own tight integration with some other engine/framework by directly talking to the C-style interop API (and therefore bypassing the C++ library, not using it at all) and translating that directly to types native of that engine/framework (especially container types etc.). One good example is some app using custom container types instead of the C++ standard library (as common in game engines), which could directly interop those containers with the C-style layer, without the need to have an extra layer using the standard C++ library:

const char* => MyCustomString

instead of

const char* => std::string => MyCustomString

In short this transitions from a stacked model where the user is forced to use the C++ library, and where the DLL boundary is in C++ land (with all the problems it brings):

| C# Library | C++ user app  |
|----------------------------| <= DLL boundary
|         C++ Library        |
|----------------------------|
| Google core implementation |

to a more horizontal model with possibility of tighter integration (more work), or still use the C++ library (convenience), and where the DLL boundary is in pure C land, avoiding most of the issues:

             |            C++ user app           |
|------------------------------------------------|
| C# Library | C++ Library | User custom interop |
|------------------------------------------------| <= DLL boundary
|                   Interop API                  |
|------------------------------------------------|
|           Google core implementation           |

Of course writing against the interop API means manipulating raw C functions, so less C++ friendly.

Does that make sense?

@drejx
Copy link

drejx commented Dec 11, 2019

I appreciate the detailed reply :) Yes it makes total sense to have that Interop C layer for maximum flexibility, for example to incorporate in other game engines as you mentioned. Certainly all the work having to provide raw video frames and audio buffers to the Interop+WebRTC layer is new work that would have to be done by the user (hopefully contributed back) :)

I think you touched on this, but in order for that Interop+WebRTC DLL to not be a huge 500 MB (i.e. no symbol stripping) it would require the user to build that DLL themselves and would look something like:

|------------------------------------------------|
|  User C#/C++ layer (references 50/200 symbols) |
|------------------------------------------------| <= DLL boundary (built from source by user)
|                   Interop API                  |
|------------------------------------------------|
|           Google core implementation           |
|             (assume 200 symbols)               |

So in the above example since the user is only using 50 out of 200 symbols (made up #) the footprint will be much smaller. The user should also be able to directly integrate the Interop+WebRTC source directly into their application rather than building a DLL and importing it (if they want to go that route).

Cheers,
Andrej

@djee-ms
Copy link
Member

djee-ms commented Dec 12, 2019

Well not really, we (MixedReality-WebRTC) decide on the interop API so we decide what symbols are used. Currently this strips a large chunk of symbols we are not using, and gets us ~10MB DLLs, and I don't expect much change after the refactor.

If a user really has troubles with DLL size (e.g. for mobile/embedded) then they should probably recompile and link statically the interop for further stripping. But given we already use a subset of Google's overall API, I am not sure how much more can be stripped. Recompiling the interop DLL themselves will not change anything unless they modify the interop to remove some functions, and then we're talking about forking and modifying this project so that's out of scope for this project support. Or did I misunderstand your explanation?

@djee-ms
Copy link
Member

djee-ms commented Dec 12, 2019

Note: UWP x64 is currently the largest with 9 MB in Release, other configs are more typically around 6 MB.

@drejx
Copy link

drejx commented Dec 12, 2019

Yes indeed a user would have to modify the actual interop layer to bring the size down, but then it would be up to them to fork it and make custom modifications.

The MixedReality DLLs are a decently low size at least for desktop apps. Curiously what types of things/features don't you take from Google's API into the interop layer? It would be useful from a consumer standpoint to clearly know if certain things couldn't be done with the MixedReality-WebRTC SDK that are expected of a standard WebRTC implementation.

Cheers,
Andrej

@djee-ms
Copy link
Member

djee-ms commented Dec 13, 2019

It's not so much that we are missing features, although there might be some minor things we are not using (can't think of any right now). I don't know of an exhaustive list of Google's features to compare with. It's more that by exporting only a handful of C functions you reduce the number of exported symbols quite a lot, which reduces the DLL size eventually. I have no direct point of comparison aside from the WebRTC UWP wrappers (Org.WebRtc.dll) coming at 12 MB vs. 9 MB for us in UWP x64, but they ship a full WinRT layer that we are excluding, hence I think the difference.

The main win is that the alternative for our users is either to compile the entire Google code (time-consuming, complex) or to get pre-compiled libraries (.lib/.a), and the latter culminates at 500 MB, which will get stripped when linking but remains an issue for distribution (we had to split those into 17 separate NuGet packages because of size limit on nuget.org). Getting a 10 MB NuGet package of MixedReality-WebRTC instead is a net win in terms of productivity.

djee-ms added a commit that referenced this issue Jan 16, 2020
- NuGet packages are built from release branches only, and are not
guaranteed to be compatible with the code on the master branch.
- C++ NuGet packages are missing some headers, and require cloning this
repository.

Bugs: #123, #166, #168
@domfaber
Copy link

Hi there, I'm working with unreal engine and would like to integrate the ms-webrtc c++ library to our project. But I also have the missing API issues with NuGet package. Could you describe the way how to add the WebRTC headers to the project manually? Sadly I'm not a power user with c++ yet. Thank you for your support!

fibann added a commit that referenced this issue Feb 17, 2020
Fix CI after #190 breaks. Related to #123.
djee-ms pushed a commit that referenced this issue Feb 17, 2020
Fix CI after #190 breaks. Related to #123.
@djee-ms
Copy link
Member

djee-ms commented Feb 17, 2020

@domfaber : we removed the extra C++ includes, and are now exposing only a pure C API. You can work with that, taking example on how the native tests use the C API.

@djee-ms
Copy link
Member

djee-ms commented Feb 17, 2020

Reopening for visibility, as NuGet packages still exhibit the issue.

@djee-ms djee-ms reopened this Feb 17, 2020
@domfaber
Copy link

Thanks a lot @djee-ms! Do you know when the NuGet package update will be available? BTW.: This project is really great! Thank you for doing this.

@djee-ms
Copy link
Member

djee-ms commented Feb 21, 2020

Thanks @domfaber!
For NuGet packages, we only build them when we release, and currently we have 2-3 major features with breaking changes coming to master so we plan to ship a 2.0 release when all of that is done and polished. Which means in the meantime there won't be any NuGet packages unfortunately.

As a workaround you can recompile the project via the provided Visual Studio solution in the meantime. Because we didn't touch the underlying implementation (WebRTC UWP) its NuGet packages (Microsoft.MixedReality.WebRTC.Native.Core.*) are still valid, so you only need to recompile the MixedReality-WebRTC libraries, which is done via the Visual Studio solution and only takes a few minutes.

@domfaber
Copy link

@djee-ms Ok great. Thank you. I will try this.

@fibann
Copy link
Member

fibann commented Jul 28, 2020

This is fixed in latest NuGet packages.

@fibann fibann closed this as completed Jul 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants