Pocock/cmake boost #4
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When mDNS resolution is enabled in the system, reception of Chrome's mDNS ICE candidates works fine. However, in environments where mDNS is not available: * Docker images * Ubuntu systems where the package libnss-mdns is not installed * Systems where /etc/nsswitch.conf doesn't contain mdns4_minimal in the list of host resolution engines the resolution of Chrome's mDNS candidate will fail, as expected, but the candidate itself will be considered valid. Considering it valid means that it is processed, stored in the queue of candidates to be added to libnice, and eventually added. libnice rejects adding an mDNS candidate because it cannot parse it, so it returns an error that ends up unwrapping as a Kurento API Error message, and this will make most applications abort because errors are not expected, even though invalid mDNS candidates is a perfectly valid situation. The solution is to simply reject mDNS candidates in the case their address cannot be resolved. Fixes Kurento/bugtracker#432
The code was first removing the attribute, then adding a new one. This means that the loop logic would step over the next attribute that might come next, because the index variable gets incremented, while the array of attributes has been modified in place. This changes the attribute struct in place, so the loop logic is not altered by the modification.
The if-else blocks were wrongly disposed, so when the stunServerPort config wasnt set, but the IP was, a STUN server would not be setup in the NiceAgent interface Also some identation changes according to the git hook suggestions around the code I changed
Use the names of the Client API events to group different debug messages with the same prefix. This way, it is easy for a user to identify which log messages correspond to the "action" that is related to the given event. There are 3 levels at which the ICE messages can occur, from internal to external: * libnice agent control (kmsiceniceagent) * WebRTC session handling (kmswebrtcsession) * WebRtcEndpoint implementation (kmswebrtcendpoint) The most internal ones are set to DEBUG level, while the endpoint implementation is the one providing information to the user by default, and is set to work on the INFO level. With this, the default log levels for a Kurento installation contain all details about the ICE candidate gathering, and all messages come from the same source (kmswebrtcendpoint). The only exception is from Peer-Reflexive ICE candidates, which are detected and processed only at the most internal layer (kmsiceniceagent), so these are logged at the INFO level.
nice_agent_remove_stream(), called from kms_ice_nice_agent_remove_stream(), was silently converted from synchronous to asynchronous in libnice 0.1.16. I know that Semantic Versioning tells us that 0.X.Y should be considered as unstable API, but come on, that change wasn't *nice* of libnice ;-) To solve the issue, we need to run a last iteration of its GMainLoop context in order to allow it release all resources and object references. Otherwise, libnice takes a reference to the NiceAgent object in nice_agent_remove_stream(), and it never gets to release it, because the unref is done in the callback function on_stream_refreshes_pruned(). Thanks to @omgold for finding the root cause of the issue and proposing a solution! Kurento/bugtracker#460 (comment) A bug report has been filled in libnice as a result of this commit: https://gitlab.freedesktop.org/libnice/libnice/issues/110 Fixes Kurento/bugtracker#253 Fixes Kurento/bugtracker#460
Defaulting to INFO for some basic operations ends up logging thousands of lines that don't really convey anything useful in a typical deployment, and can make it harder to find other more interesting messages.
The previous method of relying on a limited number of ports was working only for IPv4. As soon as IPv6 is used, this was confusing and prone to errors. Also it failed because with IPv6 it was actually finding an UDP candidate for the RTCP component (number 2) of the second WebRtcEndpoint. This method is more robust: just store all seen candidates, and check that they never get to repeat. It does the same as before, but it works better for both IPv4 and IPv6.
We had some WebRTC tests that had been commented out to disable, due to various causes. According to the comments there were some libnice issues that would cause some of these tests to fail. The issues must have been resolved by now, because all these test now pass successfully. Currently Kurento is using libnice 0.1.17. Also, the check about gathering state of libnice was superfluous and unneeded, because that's already controlled from the previous layer, at KmsWebrtcSession. This was also breaking some tests, but now that the condition is removed, the tests pass.
Users were assuming that this setting was mandatory. Move it to the end of the settings file and explain better how this is a hack that should be used only under complete understanding of the feature.
It is possible to configure an external certificate to be used by DTLS. This is important because some browsers such as Firefox won't allow to establish multiple connections from the same tab to different peers that use different DTLS certificates.
Previous code would reuse the turn_url, after it had been parsed by the input regex. It is safer to rely on the parsing step and only handle well delimited fields. Also the code is much clearer this way.
What is the current behavior you want to change? When the relative X/Y is greater than zero but less than the relative width/height, the video is cropped incorrectly. Fixes Kurento/bugtracker#63. ie) Given setPortProperties(0.1, 0.1, 0, 0.5, 0.5, port) results in the video being positioned at 0.1, 0.1 but only a small slice of the video is showing, the rest is cropped. What is the new behavior provided by this change? Removing this cropping produces the expected behaviour with correct relative X, Y, width, height. ie) Given setPortProperties(0.1, 0.1, 0, 0.5, 0.5, port) results in the video being positioned at 0.1, 0.1 with the entire video showing, correctly scaled to 50% of the master port size. How has this been tested? Tested in Ubuntu 18 with both latest KMS source and as a module in KMS 6.14.1 installed through apt.
This setting follows the same rationale of the externalAddress setting, but the idea here is to make it dual stack friendly by splitting it into IPv4 and IPv6 specific configs and mangling compatible candidates with them For the sake of backwards compatibility, externalAddress has preference over these two. So when it`s set, they will be ignored
…nalIPv6 Add Javadoc/Jsdoc texts for new parameters externalIPv4, externalIPv6. Edit externalAddress to make all of them homogeneous.
This is now superseded by the combination of externalIPv4 and externalIPv6.
Add externalIPv4/externalIPv6 configs and API methods for them (deprecates `externalAddress`)
When IPv6 is not available, this test would get stuck forever because the GSTreamer pipeline keeps failing with a warning (not shown with the default log level) and the Kurento garbage collector would not kick in and delete the elements until the default timeout of 4 minutes. Fixes Kurento/bugtracker#560
If the recorder is already stopped, just warn about it but don't raise an error event.
This prevents language-specific tools from allowing a too wide range of compatible versions. Specifically, Maven will attempt to access ALL versions in the range, which meant that it would download manifests of all artifacts since 6.7.0. kms-omni-build/bin/set-versions.sh has been updated too, so future releases will have this range automatically updated to the newest release version. Fixes https://groups.google.com/g/kurento/c/7B5k_cZ2Ya0/
…nto#26) * Add niceAgentIceTcp configuration option and API methods for it This adds the ability to configure libnice`s NiceAgent ice-tcp property through WebRtcEndpoint.conf.ini or via API The rationale behind this is to give the ability to deactivate TCP candidate gathering in setups where they aren`t needed, thus potentially speeding up the gathering process and connectivity checks Property reference: https://libnice.freedesktop.org/libnice/NiceAgent.html#NiceAgent--ice-tcp * rename to "iceTcp": don't leak name of internal class Avoid leaking that the feature is internally implemented by a class named NiceAgent, which the users don't care about. * review docs, use same text in API and config Co-authored-by: Juan Navarro <[email protected]>
## What is the current behavior you want to change? Allow choosing between the original broken behavior or the recorder (keep gaps) and the new one introduced in Kurento 6.15.0 (remove gaps in recordings): https://doc-kurento.readthedocs.io/en/6.15.0/project/relnotes/v6_15_0.html#fixed-recorder-synchronization-on-streaming-gaps ## What is the new behavior provided by this change? * New CONFIG FILE: **RecorderEndpoint.conf.ini** * New CONFIG OPTION: **gapsFix = (NONE | GENPTS)** * DEFAULT setting: `gapsFix=NONE` ## How has this been tested? Debug log to show that the feature is enabled. Not really tested yet (will publish experimental packages for it) ## Types of changes Kurento 6.15.0 changed the recording behavior because a recording with gaps in it should be considered a bug, thus an invalid file. So the breaking change was allowed within minor versions because it was considered as a bug fix. But XKCD 1172 came to bite us because we need that the total duration of the recordings matches with the total duration of the sessions that were recorded, which does not happen if you remove timestamp gaps from recordings. So we need to roll back the change. Doc changes already included in the PR.
Usage of DEBUG_MEDIASET was removed from code in this commit: Kurento/kms-core@6bff421
Prevent a thread race detected by ThreadSanitizer: the libnice Agent is calling our callbacks from different threads, instead of the specific KmsLoop thread that we expected (where the GMainContext instance lives). Using `g_main_context_invoke_full` is the best way to ensure that the signal emission is performed from our context thread. More info: https://lists.freedesktop.org/archives/nice/2021-May/001493.html
Using G_MAXUINT16 was semantically confusing. It conveys something like "this value is limited by the range of 16bit integers", when that's just a coincidence. We want to say "max port as defined by Linux", so if tomorrow they change to 65539, then that's the new value to use. The fact that Linux happens to use MAXUINT16 as their max port number, is just an implementation detail on their part.
Follow the same naming used in other similar properties, where "Id" is used rather than "ID". This was making direct JSON-RPC implementations very confusing because some properties were using the "Id" style, while just these two were using "ID".
…mponentId" Leave this change for the major release 7.0, which can contain breaking changes in the API. Meanwhile, mark these as deprecated. This reverts commit fe060fc.
…entId Follow the same naming used in other similar properties, where "Id" is used rather than "ID". This was making direct JSON-RPC implementations confusing because some properties were using the "Id" style, while just these were using "ID".
… componentId The change was missing corresponding implementation in C++-land
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
already submitted upstream
Kurento#35
Please also cherry-pick into your bionic-gstreamer branch