diff --git a/python-webrtc/cpp/NATIVE.md b/python-webrtc/cpp/NATIVE.md index 5fb5370..50ea603 100644 --- a/python-webrtc/cpp/NATIVE.md +++ b/python-webrtc/cpp/NATIVE.md @@ -102,7 +102,7 @@ One direction: Python → C++ → libwebrtc. unregistered is ended by `Disarm()` at the start of the destructor, which then waits for the callback's thread once. - Raw pointers never own: they point to libwebrtc observers and callbacks, or into C APIs. A raw pointer libwebrtc hands over to own (`OnSuccess(SessionDescriptionInterface *)`) goes into a `std::unique_ptr` right away. -- An event's arguments are native values copied at `Emit`; they're converted to Python on the loop thread. +- An event's arguments are native values copied or moved at `Emit`; they're converted to Python on the loop thread. - Registries and the Dispatcher are leaked on purpose (`static auto *x = new ...`, `ForkLocal`): native threads can outlive static destructors at exit. diff --git a/python-webrtc/cpp/src/interfaces/peer_connection_factory.cpp b/python-webrtc/cpp/src/interfaces/peer_connection_factory.cpp index 2924d3f..bb94767 100644 --- a/python-webrtc/cpp/src/interfaces/peer_connection_factory.cpp +++ b/python-webrtc/cpp/src/interfaces/peer_connection_factory.cpp @@ -17,7 +17,6 @@ #include "../utils/port_allocator.h" #include "../utils/task_queue_factory.h" -#include #include #include #include @@ -84,7 +83,7 @@ namespace python_webrtc { } } // namespace - // webrtc::CreatePeerConnectionFactory without the RTC event log, which keeps every packet's buffer in memory + // webrtc::CreatePeerConnectionFactory without the RTC event log (holds every packet) or audio processing (no capture) webrtc::PeerConnectionFactoryDependencies PeerConnectionFactory::Dependencies() { webrtc::PeerConnectionFactoryDependencies dependencies; dependencies.network_thread = _workerThread.get(); @@ -95,7 +94,6 @@ namespace python_webrtc { dependencies.adm = _audioDeviceModule; dependencies.audio_encoder_factory = webrtc::CreateBuiltinAudioEncoderFactory(); dependencies.audio_decoder_factory = webrtc::CreateBuiltinAudioDecoderFactory(); - dependencies.audio_processing_builder = std::make_unique(); dependencies.video_encoder_factory = std::make_unique(); dependencies.video_decoder_factory = std::make_unique(); webrtc::EnableMedia(dependencies); @@ -117,9 +115,8 @@ namespace python_webrtc { PeerConnectionFactory::PeerConnectionFactory() : _fieldTrials(_trials), _networkIgnoreMask(_loopbackAllowed ? 0 : webrtc::kDefaultNetworkIgnoreMask) { - _workerThread = Started(std::make_unique(std::make_unique()), - "PeerConnectionFactory:workerThread"); - _signalingThread = Started(webrtc::Thread::Create(), "PeerConnectionFactory:signalingThread"); + _workerThread = Started(std::make_unique(std::make_unique()), "wrtc-network"); + _signalingThread = Started(webrtc::Thread::Create(), "wrtc-signaling"); // sanitizer builds check that these threads never enter Python _workerThread->PostTask(&TagNativeThread); _signalingThread->PostTask(&TagNativeThread); diff --git a/python-webrtc/cpp/src/interfaces/rtc_data_channel.cpp b/python-webrtc/cpp/src/interfaces/rtc_data_channel.cpp index 2a66038..2362506 100644 --- a/python-webrtc/cpp/src/interfaces/rtc_data_channel.cpp +++ b/python-webrtc/cpp/src/interfaces/rtc_data_channel.cpp @@ -14,6 +14,7 @@ #include "../exceptions.h" #include "../utils/gil.h" #include "../utils/libwebrtc_thread.h" +#include "../utils/parking.h" namespace python_webrtc { @@ -61,12 +62,13 @@ namespace python_webrtc { void RTCDataChannel::Init(pybind11::module &m) { pybind11::class_(m, "DataChannelMessage") .def_property_readonly("data", [](const DataChannelMessage &message) -> pybind11::object { + const auto *data = message.data.cdata(); if (message.binary) { - return pybind11::bytes(message.data); + return pybind11::bytes(data, message.data.size()); } // the upper layers verify UTF-8, as libwebrtc doesn't return pybind11::reinterpret_steal( - PyUnicode_DecodeUTF8(message.data.data(), static_cast(message.data.size()), "replace")); + PyUnicode_DecodeUTF8(data, static_cast(message.data.size()), "replace")); }); DefineBinding(pybind11::class_>(m, "RTCDataChannel")) @@ -84,7 +86,15 @@ namespace python_webrtc { .def_property("bufferedAmountLowThreshold", nogil_fn(&RTCDataChannel::GetBufferedAmountLowThreshold), nogil_fn(&RTCDataChannel::SetBufferedAmountLowThreshold)) .def_property("binaryType", nogil_fn(&RTCDataChannel::GetBinaryType), nogil_fn(&RTCDataChannel::SetBinaryType)) - .def("send", &RTCDataChannel::Send, nogil(), pybind11::arg("data"), pybind11::arg("binary")) + .def( + "send", + // only bytes: the view must outlive the call without the GIL, which a bytearray's resize breaks + [](RTCDataChannel &self, const pybind11::bytes &data, bool binary) { + const std::string_view view = data; + const gil_release release; + return self.Send(view, binary); + }, + pybind11::arg("data"), pybind11::arg("binary")) .def("close", &RTCDataChannel::Close, nogil()) .def("_surfaceState", &RTCDataChannel::SurfaceState, nogil(), pybind11::arg("state")) .def("_decreaseBufferedAmount", &RTCDataChannel::DecreaseBufferedAmount, nogil(), pybind11::arg("sent")); @@ -116,17 +126,22 @@ namespace python_webrtc { Emit("open", state); break; case DataState::kClosing: + Parking::At("datachannel.closing"); Emit("closing", state); break; case DataState::kClosed: { auto error = _channel->error(); - // closing locally isn't an error, even if queued messages couldn't be sent bool closedLocally = false; { const std::scoped_lock lock(_closeMutex); closedLocally = _closeRequested; } - if (!error.ok() && !closedLocally) { + const bool detailed = error.error_detail() != webrtc::RTCErrorDetailType::NONE; + // closing locally isn't an error + if (!error.ok() && (detailed || !closedLocally)) { + if (!detailed) { + error.set_error_detail(webrtc::RTCErrorDetailType::DATA_CHANNEL_FAILURE); + } Emit("error", RTCCallbackException(std::move(error))); } Emit("close", state); @@ -141,8 +156,7 @@ namespace python_webrtc { } void RTCDataChannel::OnMessage(const webrtc::DataBuffer &buffer) { - Emit("message", - DataChannelMessage{.data = std::string(buffer.data.cdata(), buffer.size()), .binary = buffer.binary}); + Emit("message", DataChannelMessage{.data = buffer.data, .binary = buffer.binary}); } void RTCDataChannel::OnBufferedAmountChange(uint64_t sentDataSize) { @@ -204,7 +218,7 @@ namespace python_webrtc { } webrtc::Priority RTCDataChannel::GetPriority() { - // the ranges Chromium maps priority values to + // splits at the midpoints between the DCEP priority values of W3C webrtc-priority (128, 256, 512, 1024) constexpr int veryLowMax = 192; constexpr int lowMax = 384; constexpr int mediumMax = 768; @@ -277,7 +291,7 @@ namespace python_webrtc { } // namespace - bool RTCDataChannel::Send(const std::string &data, bool binary) { + bool RTCDataChannel::Send(std::string_view data, bool binary) { if (GetReadyState() != DataState::kOpen) { throw RTCException(webrtc::RTCErrorType::INVALID_STATE, "RTCDataChannel.readyState is not 'open'"); } @@ -307,14 +321,8 @@ namespace python_webrtc { } void RTCDataChannel::SendNow(webrtc::DataBuffer buffer) { - auto size = buffer.size(); - // posted to the network thread, in call order; a failed send closes the channel - _channel->SendAsync(std::move(buffer), [weak = weak_from_this(), size](const webrtc::RTCError &error) { - auto self = weak.lock(); - if (!error.ok() && self) { - self->DecreaseBufferedAmount(size); - } - }); + // posted in call order; libwebrtc reports a failed send's bytes through OnBufferedAmountChange too + _channel->SendAsync(std::move(buffer), nullptr); } void RTCDataChannel::SetStateLocked(DataState state) { diff --git a/python-webrtc/cpp/src/interfaces/rtc_data_channel.h b/python-webrtc/cpp/src/interfaces/rtc_data_channel.h index bd8e647..7d0b95b 100644 --- a/python-webrtc/cpp/src/interfaces/rtc_data_channel.h +++ b/python-webrtc/cpp/src/interfaces/rtc_data_channel.h @@ -15,8 +15,10 @@ #include #include #include +#include #include +#include #include @@ -32,7 +34,8 @@ namespace python_webrtc { // A received message, converted to str or bytes by Python struct DataChannelMessage { - std::string data; + // refcounted and only read, so it's shared across threads without a copy + webrtc::CopyOnWriteBuffer data; bool binary; }; @@ -113,8 +116,8 @@ namespace python_webrtc { // whether bufferedAmount dropped to the threshold bool DecreaseBufferedAmount(uint64_t sent); - // whether the message is the only one queued: the channel's activity changes only then - bool Send(const std::string &data, bool binary); + // whether the message is the only one queued: the channel's activity changes only then; data is copied + bool Send(std::string_view data, bool binary); void Close(); diff --git a/python-webrtc/cpp/src/interfaces/rtc_peer_connection/rtc_peer_connection.cpp b/python-webrtc/cpp/src/interfaces/rtc_peer_connection/rtc_peer_connection.cpp index 39e33ce..03cf622 100644 --- a/python-webrtc/cpp/src/interfaces/rtc_peer_connection/rtc_peer_connection.cpp +++ b/python-webrtc/cpp/src/interfaces/rtc_peer_connection/rtc_peer_connection.cpp @@ -32,6 +32,8 @@ namespace python_webrtc { auto configuration = webrtc::PeerConnectionInterface::RTCConfiguration(); configuration.sdp_semantics = webrtc::SdpSemantics::kUnifiedPlan; configuration = _configuration.Apply(configuration); + // renders each decoded frame at once, without a smoothing queue per video receiver + configuration.set_prerenderer_smoothing(false); webrtc::PeerConnectionDependencies dependencies(this); dependencies.allocator = _factory->CreatePortAllocator(configuration); diff --git a/python-webrtc/cpp/src/interfaces/rtc_rtp_sender.cpp b/python-webrtc/cpp/src/interfaces/rtc_rtp_sender.cpp index 4805562..34d9cf7 100644 --- a/python-webrtc/cpp/src/interfaces/rtc_rtp_sender.cpp +++ b/python-webrtc/cpp/src/interfaces/rtc_rtp_sender.cpp @@ -22,7 +22,7 @@ namespace python_webrtc { namespace { - // only what setParameters may change, as in Chromium + // only the encoding members setParameters may change, the rest keep their current values void applySettable(const webrtc::RtpEncodingParameters &requested, webrtc::RtpEncodingParameters ¤t) { current.active = requested.active; current.request_key_frame = requested.request_key_frame; diff --git a/python-webrtc/cpp/src/utils/mailbox.h b/python-webrtc/cpp/src/utils/mailbox.h index f506514..d10d3b5 100644 --- a/python-webrtc/cpp/src/utils/mailbox.h +++ b/python-webrtc/cpp/src/utils/mailbox.h @@ -130,8 +130,9 @@ namespace python_webrtc { template class Emitter : public Binding { protected: + // an rvalue argument is moved into the record, and from it into Python template - void Emit(const char *name, const Args &...args) { + void Emit(const char *name, Args &&...args) { CheckNativeThreadDetached("Emitter::Emit"); if (!IsBound()) { return; @@ -141,8 +142,13 @@ namespace python_webrtc { if (!self) { return; } - Post(Record{.source = self, .target = self->Id(), .name = name, .args = [self, args...]() { - return pybind11::make_tuple(self, args...); + // Take converts a record once + Post(Record{.source = self, + .target = self->Id(), + .name = name, + // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks): the analyzer loses a moved shared_ptr + .args = [self, ... args = std::forward(args)]() mutable { + return pybind11::make_tuple(self, std::move(args)...); }}); } diff --git a/python-webrtc/python/webrtc/exceptions.py b/python-webrtc/python/webrtc/exceptions.py index 481b5a0..6e1ceea 100644 --- a/python-webrtc/python/webrtc/exceptions.py +++ b/python-webrtc/python/webrtc/exceptions.py @@ -212,7 +212,7 @@ def __init__(self, init: RTCErrorInit, message: str = '') -> None: _BY_RTC_ERROR_TYPE = { - # the same mapping as Chromium's + # the W3C error names for libwebrtc's RTCErrorType values 'UNSUPPORTED_OPERATION': OperationError, 'UNSUPPORTED_PARAMETER': InvalidAccessError, 'INVALID_PARAMETER': InvalidAccessError, diff --git a/python-webrtc/python/webrtc/interfaces/rtc_data_channel.py b/python-webrtc/python/webrtc/interfaces/rtc_data_channel.py index 1552ed3..c8c5cf5 100644 --- a/python-webrtc/python/webrtc/interfaces/rtc_data_channel.py +++ b/python-webrtc/python/webrtc/interfaces/rtc_data_channel.py @@ -23,7 +23,7 @@ from webrtc.models.events import Event, MessageEvent, RTCErrorEvent, RTCErrorEventInit from webrtc.utils.events import AnyHandler, EventTarget, HandlerDecorator from webrtc.utils.names import Alias, alias -from webrtc.utils.strings import usv_string +from webrtc.utils.strings import usv_utf8 if TYPE_CHECKING: import webrtc @@ -43,7 +43,7 @@ def check_utf8_length(name: str, value: str) -> None: Raises: TypeError: If it's longer. """ - if len(usv_string(value).encode()) > MAX_UNSIGNED_SHORT: + if len(usv_utf8(value)) > MAX_UNSIGNED_SHORT: msg = f'{name} is longer than {MAX_UNSIGNED_SHORT} bytes' raise TypeError(msg) @@ -399,7 +399,7 @@ def send(self, data: str | bytes | bytearray | memoryview | Blob) -> None: webrtc.OperationError: If the send queue is full. """ if isinstance(data, str): - message, binary = usv_string(data).encode(), False + message, binary = usv_utf8(data), False elif isinstance(data, (bytes, bytearray, memoryview, Blob)): message, binary = bytes(data), True else: diff --git a/python-webrtc/python/webrtc/models/rtp_parameters.py b/python-webrtc/python/webrtc/models/rtp_parameters.py index 3b29d4a..e7be6cd 100644 --- a/python-webrtc/python/webrtc/models/rtp_parameters.py +++ b/python-webrtc/python/webrtc/models/rtp_parameters.py @@ -31,7 +31,7 @@ _NativeCodecT = TypeVar('_NativeCodecT', bound='wrtc.RtpCodec') -# the bitrate priorities of libwebrtc for RTCRtpEncodingParameters.priority, as Chromium maps them +# libwebrtc's bitrate priorities for RTCRtpEncodingParameters.priority, doubling per level from low (the default) at 1.0 _BITRATE_PRIORITY = { RTCPriorityType.very_low: 0.5, RTCPriorityType.low: 1.0, diff --git a/python-webrtc/python/webrtc/utils/strings.py b/python-webrtc/python/webrtc/utils/strings.py index 6842b2b..122e1cb 100644 --- a/python-webrtc/python/webrtc/utils/strings.py +++ b/python-webrtc/python/webrtc/utils/strings.py @@ -17,3 +17,11 @@ def usv_string(value: str) -> str: except UnicodeEncodeError: return value.encode('utf-16-le', 'surrogatepass').decode('utf-16-le', 'replace') return value + + +def usv_utf8(value: str) -> bytes: + """Encodes a USVString in UTF-8, encoding only once unless there are lone surrogates.""" + try: + return value.encode() + except UnicodeEncodeError: + return usv_string(value).encode() diff --git a/stubs/wrtc/__init__.pyi b/stubs/wrtc/__init__.pyi index 5216641..1b0c1ea 100644 --- a/stubs/wrtc/__init__.pyi +++ b/stubs/wrtc/__init__.pyi @@ -644,7 +644,7 @@ class RTCDataChannel(Binding): ... def close(self) -> None: ... - def send(self, data: str | bytes, binary: bool) -> bool: + def send(self, data: bytes, binary: bool) -> bool: ... @property def bufferedAmount(self) -> int: diff --git a/tests/test_data_channel.py b/tests/test_data_channel.py index 68c605a..e724e0d 100644 --- a/tests/test_data_channel.py +++ b/tests/test_data_channel.py @@ -12,6 +12,7 @@ import asyncio import math import re +import sys from typing import cast import pytest @@ -166,6 +167,131 @@ async def test_remote_connection_closed_fires_error( assert channel.ready_state == webrtc.RTCDataChannelState.closed +races_remote_close = pytest.mark.skipif( + sys.platform == 'win32', reason="libwebrtc's Windows socket server runs posted tasks after pending socket events" +) + + +def send_racing_remote_close( + channel: webrtc.RTCDataChannel, remote: webrtc.RTCDataChannel, messages: list[str | bytes] +) -> None: + """Sends once libwebrtc has the channel closing by the remote end, before its closing event is emitted.""" + wrtc._testing.park('datachannel.closing') + try: + remote.close() + assert wrtc._testing.parked('datachannel.closing', seconds(10)) + # still open for Python, so libwebrtc fails these sends + for message in messages: + channel.send(message) + finally: + wrtc._testing.release('datachannel.closing') + + +@races_remote_close +@pytest.mark.asyncio +async def test_send_racing_a_remote_close_fires_error( + caller: webrtc.RTCPeerConnection, callee: webrtc.RTCPeerConnection, monkeypatch: pytest.MonkeyPatch +) -> None: + """A send libwebrtc fails as the remote end closes fires error with data-channel-failure, then close.""" + channel, remote = await open_pair(caller, callee) + unraisable: list[sys.UnraisableHookArgs] = [] + monkeypatch.setattr(sys, 'unraisablehook', unraisable.append) + loop = asyncio.get_running_loop() + reported: list[dict[str, object]] = [] + loop.set_exception_handler(lambda _loop, context: reported.append(context)) + try: + events: list[webrtc.Event] = [] + channel.on('error', events.append) + closed = wait_for_event(channel, 'close') + + send_racing_remote_close(channel, remote, ['late']) + events.append(await closed) + finally: + loop.set_exception_handler(None) + + error_event, close_event = events + assert isinstance(error_event, webrtc.RTCErrorEvent) + assert isinstance(error_event.error, webrtc.RTCError) + assert error_event.error.error_detail == webrtc.RTCErrorDetailType.data_channel_failure + assert close_event.type == 'close' + assert channel.ready_state == webrtc.RTCDataChannelState.closed + assert reported == [] + assert unraisable == [] + + +@races_remote_close +@pytest.mark.asyncio +async def test_buffered_amount_when_sends_race_a_remote_close( + caller: webrtc.RTCPeerConnection, callee: webrtc.RTCPeerConnection +) -> None: + """The bytes of sends libwebrtc fails stay in bufferedAmount, which libwebrtc never reports sent.""" + channel, remote = await open_pair(caller, callee) + channel.send(b'drained') + await wait_for_event(channel, 'bufferedamountlow') + assert channel.buffered_amount == 0 + + observed: list[tuple[str, int]] = [] + + def observe(event: webrtc.Event) -> None: + observed.append((event.type, channel.buffered_amount)) + + for name in ('bufferedamountlow', 'closing', 'error'): + channel.on(name, observe) + closed = wait_for_event(channel, 'close') + messages: list[str | bytes] = ['late', bytes(1000), 'é'] + pending = len('late') + 1000 + len('é'.encode()) + + send_racing_remote_close(channel, remote, messages) + assert channel.buffered_amount == pending + observe(await closed) + + assert observed == [('closing', pending), ('error', pending), ('close', pending)] + + +@pytest.mark.asyncio +async def test_native_send_takes_only_bytes(caller: webrtc.RTCPeerConnection, callee: webrtc.RTCPeerConnection) -> None: + """The native send reads its data without the GIL, so it refuses a bytearray that could be resized meanwhile.""" + channel, _ = await open_pair(caller, callee) + native = channel._native_obj + for data in (bytearray(b'data'), 'data'): + with pytest.raises(TypeError): + native.send(data, binary=True) # pyrefly: ignore[bad-argument-type] + + +@pytest.mark.asyncio +async def test_messages_round_trip(caller: webrtc.RTCPeerConnection, callee: webrtc.RTCPeerConnection) -> None: + """Text and binary messages arrive unchanged, empty and as large as the SCTP transport takes.""" + channel, remote = await open_pair(caller, callee) + assert caller.sctp is not None + large = 256 * 1024 + max_message_size = caller.sctp.max_message_size + assert max_message_size is not None + assert max_message_size >= large + messages: list[str | bytes] = [ + '', + b'', + 'héllo, мир, 世界 🎉', + bytes(range(256)) * (large // 256), + 'é' * (large // 2), + '\x00text', + ] + received: list[str | bytes | webrtc.Blob] = [] + got_all = asyncio.get_running_loop().create_future() + + @remote.on('message') + def on_message(event: webrtc.MessageEvent) -> None: + received.append(event.data) + if len(received) == len(messages) + 1 and not got_all.done(): + got_all.set_result(None) + + for message in messages: + channel.send(message) + # a lone surrogate becomes U+FFFD + channel.send('a\ud800b') + await asyncio.wait_for(got_all, seconds(10)) + assert received == [*messages, 'a\ufffdb'] + + @pytest.mark.parametrize( ('init', 'error'), [