Modernize

[ibc]

Working on v3 branch.

Tasks:

• [x] Add more debug logs (for RTP parameters, etc).
• [x] Must validate all ORTC objects before using them. We may have separate validators that check mandatory fields, add default values and fill optional arrays with empty arrays, etc.
• [x] Update code to be in sync with mediasoup-client (this task involves most of the other open issues in GitHub).
• [x] Make scripts/format.sh work. I've changed the bash for which didn't work but not sure whether clang-format is really working as expected.
  • DONE: It's been replaced with a gulpfile.js that has "lint" and "format" tasks as in mediasoup.
• [x] Really do clang-tidy.
  • I'm changing readability-identifier-naming.GlobalFunctionCase to camelBack (instead of CamelCase) since that's how all the code is written, even public API.
  • Need a way to make clang-tidy ignore code in deps. We should build deps first, then run tidy.sh.
• [x] Deallocate closed MediaSections (delete mediaSection). See #69.
• [ ] Apply HandlerInterface as in mediasoup-client.
• [ ] Improve probation RTP parameters and modernize unit tests (commit in mediasoup-client).
• [ ] Allow calling producer.replaceTrack(null) (commit in mediasoup-client).
• [x] Enable codec selection in transport.produce()(commit in mediasoup-client).
• [x] Add DataChannel support. Ongoing PR #77 by @copiltembel.

[ibc]

Eventually I get this error when running the tests (just a few times):

libc++abi.dylib: terminating with uncaught exception of type nlohmann::detail::parse_error: [json.exception.parse_error.101] parse error at line 1, column 1194: syntax error while parsing value - invalid string: control character U+0000 (NUL) must be escaped to \u0000; last read: '"ebfv3f<U+0000>'

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test_mediasoupclient is a Catch v2.11.1 host application.
Run with -? for options

-------------------------------------------------------------------------------
mediasoupclient
  consumer.GetStats() succeeds
-------------------------------------------------------------------------------
/Users/ibc/src/v3-libmediasoupclient/test/src/mediasoupclient.test.cpp:656

So it happens when calling REQUIRE_NOTHROW(videoConsumer->GetStats());.

UPDATE: More detailed crash:

Crashed Thread:        53  signaling_thread

Exception Type:        EXC_CRASH (SIGABRT)
Exception Codes:       0x0000000000000000, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Application Specific Information:
abort() called
terminating with uncaught exception of type nlohmann::detail::parse_error: [json.exception.parse_error.101] parse error at line 1, column 1191: syntax error while parsing value - invalid string: control character U+0000 (NUL) must be escaped to \u0000; last read: '"DIG<U+0000>'

Thread 0:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	0x00007fff722a486a __psynch_cvwait + 10
1   libsystem_pthread.dylib       	0x00007fff7236356e _pthread_cond_wait + 722
2   libc++.1.dylib                	0x00007fff6f39da0a std::__1::condition_variable::wait(std::__1::unique_lock<std::__1::mutex>&) + 18
3   libc++.1.dylib                	0x00007fff6f3a696b std::__1::__assoc_sub_state::__sub_wait(std::__1::unique_lock<std::__1::mutex>&) + 45
4   test_mediasoupclient          	0x0000000107c279f6 std::__1::__assoc_state<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer> >::move() + 70
5   test_mediasoupclient          	0x0000000107c168b0 std::__1::future<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer> >::get() + 80
6   test_mediasoupclient          	0x0000000107c16c7a mediasoupclient::PeerConnection::GetStats(rtc::scoped_refptr<webrtc::RtpReceiverInterface>) + 266
7   test_mediasoupclient          	0x0000000107c03634 mediasoupclient::RecvHandler::GetReceiverStats(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 692
8   test_mediasoupclient          	0x0000000107c3ed9c mediasoupclient::RecvTransport::OnGetStats(mediasoupclient::Consumer const*) + 380
9   test_mediasoupclient          	0x0000000107c3edf3 non-virtual thunk to mediasoupclient::RecvTransport::OnGetStats(mediasoupclient::Consumer const*) + 51
10  test_mediasoupclient          	0x0000000107bf0358 mediasoupclient::Consumer::GetStats() const + 360

Again: it happens randomly. It seems there is some race condition in some thread.

[ibc]

We have a problem with the current API. This is device.createSendTransport() in mediasoup-client:

createSendTransport(
	{
		id,
		iceParameters,
		iceCandidates,
		dtlsParameters,
		sctpParameters,
		iceServers,
		iceTransportPolicy,
		additionalSettings,
		proprietaryConstraints,
		appData = {}
	}: TransportOptions
): Transport

However this is Device::CreateSendTransport in libmediasoupclient:

SendTransport* Device::CreateSendTransport(
  SendTransport::Listener* listener,
  const std::string& id,
  const json& iceParameters,
  const json& iceCandidates,
  const json& dtlsParameters,
  const PeerConnection::Options* peerConnectionOptions,
  const json& appData) const

We cannot extend the C++ method signature without breaking its API. Just wondering if we should have used an object from the very beginning instead of multiple arguments.

/cc @jmillan

[jmillan]

As commented via phone lets create this new signature and also maintain the previous one:

SendTransport* Device::CreateSendTransport(
  SendTransport::Listener* listener,
  const std::string& id,
  const json& iceParameters,
  const json& iceCandidates,
  const json& dtlsParameters,
  const json& sctpParameters,
  const PeerConnection::Options* peerConnectionOptions,
  const json& appData) const

[ibc]

BTW added a bullet in the list above. cc/ @jmillan

[tdrz]

Picking up on the discussion above, just my 2 cents:

If you want to go more in the direction of having a similar API with mediasoup-client, you will lose some of the opportunities that the (c++) language offers. Take for example the "produce" event vs. the OnProduce() callback. Since OnProduce() is pure virtual, the user has to implement it, otherwise the user's code won't compile. The mediasoup-client app will still "build" even if the "produce" event is not listened to.

Of course, there is no "better way", it really depends on what is more important to you.

[ibc]

What is the purpose of calling transport->Produce() if your app is not listening for the "produce" event? In fact, mediasoup-client will throw if you call transport.produce() if you did not add a listener for the "produce" event.

[tdrz]

True, but it will throw only at runtime. C++ won't even build.

Of course there's no logic in calling Produce without listening on the "produce" event. But since you can enforce it at compile time, it might make your library a bit more resilient (and possibly avoid some questions on the forums).

[ibc]

I don't understand your point now. In fact you said:

Since OnProduce() is pure virtual, the user has to implement it, otherwise the user's code won't compile.

now you say:

But since you can enforce it at compile time, it might make your library a bit more resilient (and possibly avoid some questions on the forums)

and that's exactly what we do now. Mandate the user to define onProduce. Otherwise it won't compile.

[tdrz]

I'm saying that there is a difference of behavior between mediasoup-client and libmediasoup.

Anyway, not important. It's good that you mandate the implementation of OnProduce().

[ibc]

@jmillan can you please update checkboxes in the issue description above?

[jmillan]

It's up to date.

[maxweisel]

Just to add to the OnProduce discussion. Maybe another way to look at it is with OnProduceData(). I don't use data producers or consumers and so it feels awkward that the compiler requires me to implement OnProduceData(). Especially because my implementation isn't just an empty method, it's a method that creates an exception that states that I haven't implemented data producers and returns a future that's already in an error state.

I think it would make sense for the interface to provide a default implementation that does this for all methods. If you call Produce or ProduceData at runtime and you haven't implemented one of the methods, it will throw and you can implement it. Applications that are only using a limited set of APIs won't be required to implement callbacks for all APIs.

Max

[tdrz]

that the compiler requires me to implement OnProduceData()

True. But I would actually aim for an even more integrated API, if possible. The way I see it there should be only one callback to be implemented: "OnProduce()". Logically "data" is just another type besides video and audio that mediasoup (WebRTC) can transport, so to me it just feels more natural to have a generic callback to handle all of them.

[maxweisel]

I disagree there. If mediasoup sent data via a Producer (as opposed to the separate DataProducer type), I'd agree with you, but they treat them as different paths, and I think that's the correct move given that libwebrtc treats audio/video and data channels separately almost in every layer. It maps nicely if you're working with clients/servers that aren't using mediasoup as well.

[tdrz]

I didn't say the Producer and DataProducer should be consolidated into a single class. I was talking only about the callback, which is mediasoup specific.

[tdrz]

I don't use data producers or consumers

Probably a corner case, but for someone who is ONLY using data producers/consumers might feel awkward that the compiler requires her to implement OnProduce().

[maxweisel]

If they're separate producer classes, but use the same listener type, but the listener is expected to branch based on what type of producer is making the callback you've defeated the purpose of using types in C++ imo.

The solution I proposed would not require someone using "ONLY using data producers/consumers" to implement OnProduce() fwiw. If OnProduce and OnProduceData just had their own default implementation that threw an exception, anyone using only audio/video or only data would only have to implement a single listener. And anyone using both wouldn't have to have a runtime check to branch based on which producer type is making the callback. They'd just implement both.

[tdrz]

If OnProduce and OnProduceData just had their own default implementation that threw an exception

Yes, this was my initial point. It would also bring it close to the mediasoup-client API, which doesn't enforce implementation of the callbacks.

but use the same listener type, but the listener is expected to branch based on what type of producer

Unsure if I understand the point. A branching will be needed on the server side to choose between produce() and produceData(), not on the C++ client side.

But thinking about it, it would complicate things in other ways like sctp parameters being mandatory for DataChannels but not for audio/video etc. At least now it's clear which parameters need to be sent to the other side.