Repository: aiortc/aiortc

Issue:
Setting RTCRtpTransceiver.direction to `inactive` does not have any effect
We have a transceiver that was generated out of a `pc.addTransceiver(track)`, meaning the transceiver has only a receiver.

When setting the transceiver direction to `inactive`, while we keep sending RTP to this endpoint we expect it to just ignore the traffic, but `RTCRtpReceiver.__handle_rtp_packet()` simply processes the traffic normally.

My expectation is that the receiver should ignore the incoming RTP packet. In our case we look for the receiver not to process anything, that's why we set the direction to 'inactive'. 


Discussion:
jmillan: I've used an Intel laptop since with the M1 I was unable to run the tests.
jmillan: I'm willing to work on the tests but I'm still unable to build the package in my M1, which is where I have my dev setup.
jmillan: @jlaine would you please take a couple of minutes to check this PR?

It's straight forward and quite clean.

The bug this PR fixes is preventing us from using aiortc for load testing a WebRTC environent.
ibc: Just a friendly ping because we are in a situation in which we already had to fork `aiortc` fo have this issue fixed.
jlaine: Hi! I'm currently away but will definitely take a look at this PR as soon as possible.

One thing which is not clear to me is what the expected behaviour is for a disabled *receiver*: return silent/blank frames or nothing at all. Could you please provide some links to specs?

By the way two weeks seems like a low bar for a fork :)
Any interest in co-maintaining aiortc?
ibc: > One thing which is not clear to me is what the expected behaviour is for a disabled _receiver_: return silent/blank frames or nothing at all. Could you please provide some links to specs?

Perhaps you mean "disabled _sender_"? If so it shouldn't send anything. In WebRTC there is no need to artificially send RTP all the time (silence or empty video frames while disabled) since there is already mandatory ICE consent mechanism running so there is proper keepalive system already. I don't know if I could be able to find links to spec about this.


> By the way two weeks seems like a low bar for a fork :) Any interest in co-maintaining aiortc?

Last thing we want is to maintain a fork. Sorry for the rush. Not sure if we have bandwidth now to maintain yet another project. We'll keep contributing to it as much as we can but we are not Python guys at all.
jmillan: > One thing which is not clear to me is what the expected behaviour is for a disabled receiver: return silent/blank frames or nothing at all. Could you please provide some links to specs?

Setting the direction of a `RtcRtpTransceiver` will take effect in future calls of `createOffer` or `createAnswer` in order to mark the corresponding media description as sendrecv, sendonly, recvonly or inactive. So no RTP sending/procesing is expected when disabling sender or receiver respectively.
jlaine: On the whole, I'm not against this PR, but it feels like one of those cases where we are going to run against limits of how aiortc's "media pipeline" was designed. I am starting to feel like the media pipeline just doesn't match how other frameworks implement it, but I really don't have a good view of what I *should* have implemented.
jlaine: You can ignore the linter error for now, I'm fixing this in #1058 
jmillan: @jlaine, all the concerns are fixed now :-)
jlaine: OK the code looks like something I'm happy to merge, do you mind if I take over the branch to edit some comments?
jmillan: > OK the code looks like something I'm happy to merge, do you mind if I take over the branch to edit some comments?

Sure, please.
jlaine: OK, I've made the changes I wanted:

- I've added some comments.
- I changed the `_currentDirection` setter to a method, because I don't understand why `@currentDirection.setter` (no leading underscore) worked
- I introduced a slight optimisation: if the sender is disabled, don't run frames through the encoder.

Let's see how CI feels about the changes!

Patch:
diff --git a/src/aiortc/rtcpeerconnection.py b/src/aiortc/rtcpeerconnection.py
index 738d486..17e06cb 100644
--- a/src/aiortc/rtcpeerconnection.py
+++ b/src/aiortc/rtcpeerconnection.py
@@ -789,7 +789,7 @@ class RTCPeerConnection(AsyncIOEventEmitter):
         # configure direction
         for t in self.__transceivers:
             if description.type in ["answer", "pranswer"]:
-                t._currentDirection = and_direction(t.direction, t._offerDirection)
+                t._setCurrentDirection(and_direction(t.direction, t._offerDirection))
 
         # gather candidates
         await self.__gather()
@@ -871,7 +871,7 @@ class RTCPeerConnection(AsyncIOEventEmitter):
                 # configure direction
                 direction = reverse_direction(media.direction)
                 if description.type in ["answer", "pranswer"]:
-                    transceiver._currentDirection = direction
+                    transceiver._setCurrentDirection(direction)
                 else:
                     transceiver._offerDirection = direction
 
diff --git a/src/aiortc/rtcrtpreceiver.py b/src/aiortc/rtcrtpreceiver.py
index 87d7399..09181ee 100644
--- a/src/aiortc/rtcrtpreceiver.py
+++ b/src/aiortc/rtcrtpreceiver.py
@@ -263,6 +263,7 @@ class RTCRtpReceiver:
         if transport.state == "closed":
             raise InvalidStateError
 
+        self._enabled = True
         self.__active_ssrc: Dict[int, datetime.datetime] = {}
         self.__codecs: Dict[int, RTCRtpCodecParameters] = {}
         self.__decoder_queue: queue.Queue = queue.Queue()
@@ -450,6 +451,10 @@ class RTCRtpReceiver:
         """
         self.__log_debug("< %s", packet)
 
+        # If the receiver is disabled, discard the packet.
+        if not self._enabled:
+            return
+
         # feed bitrate estimator
         if self.__remote_bitrate_estimator is not None:
             if packet.extensions.abs_send_time is not None:
diff --git a/src/aiortc/rtcrtpsender.py b/src/aiortc/rtcrtpsender.py
index 0d02064..6ce431b 100644
--- a/src/aiortc/rtcrtpsender.py
+++ b/src/aiortc/rtcrtpsender.py
@@ -78,6 +78,7 @@ class RTCRtpSender:
         self._rtx_ssrc = random32()
         # FIXME: how should this be initialised?
         self._stream_id = str(uuid.uuid4())
+        self._enabled = True
         self.__encoder: Optional[Encoder] = None
         self.__force_keyframe = False
         self.__loop = asyncio.get_event_loop()
@@ -268,6 +269,13 @@ class RTCRtpSender:
     async def _next_encoded_frame(self, codec: RTCRtpCodecParameters):
         # get [Frame|Packet]
         data = await self.__track.recv()
+
+        # If the sender is disabled, drop the frame instead of encoding it.
+        # We still want to read from the track in order to avoid frames
+        # accumulating in memory.
+        if not self._enabled:
+            return None
+
         audio_level = None
 
         if self.__encoder is None:
@@ -325,7 +333,12 @@ class RTCRtpSender:
                     await asyncio.sleep(0.02)
                     continue
 
+                # Fetch the next encoded frame. This can be `None` if the sender
+                # is disabled, in which case we just continue the loop.
                 enc_frame = await self._next_encoded_frame(codec)
+                if enc_frame is None:
+                    continue
+
                 timestamp = uint32_add(timestamp_origin, enc_frame.timestamp)
 
                 for i, payload in enumerate(enc_frame.payloads):
diff --git a/src/aiortc/rtcrtptransceiver.py b/src/aiortc/rtcrtptransceiver.py
index 8363795..d9af67a 100644
--- a/src/aiortc/rtcrtptransceiver.py
+++ b/src/aiortc/rtcrtptransceiver.py
@@ -29,6 +29,7 @@ class RTCRtpTransceiver:
         sender: RTCRtpSender,
         direction: str = "sendrecv",
     ):
+        self.__currentDirection: Optional[str] = None
         self.__direction = direction
         self.__kind = kind
         self.__mid: Optional[str] = None
@@ -37,7 +38,6 @@ class RTCRtpTransceiver:
         self.__sender = sender
         self.__stopped = False
 
-        self._currentDirection: Optional[str] = None
         self._offerDirection: Optional[str] = None
         self._preferred_codecs: List[RTCRtpCodecCapability] = []
         self._transport: RTCDtlsTransport = None
@@ -54,7 +54,7 @@ class RTCRtpTransceiver:
 
         One of `'sendrecv'`, `'sendonly'`, `'recvonly'`, `'inactive'` or `None`.
         """
-        return self._currentDirection
+        return self.__currentDirection
 
     @property
     def direction(self) -> str:
@@ -130,6 +130,22 @@ class RTCRtpTransceiver:
         await self.__sender.stop()
         self.__stopped = True
 
+    def _setCurrentDirection(self, direction: str) -> None:
+        self.__currentDirection = direction
+
+        if direction == "sendrecv":
+            self.__sender._enabled = True
+            self.__receiver._enabled = True
+        elif direction == "sendonly":
+            self.__sender._enabled = True
+            self.__receiver._enabled = False
+        elif direction == "recvonly":
+            self.__sender._enabled = False
+            self.__receiver._enabled = True
+        elif direction == "inactive":
+            self.__sender._enabled = False
+            self.__receiver._enabled = False
+
     def _set_mid(self, mid: str) -> None:
         self.__mid = mid
 


Test patch:
diff --git a/tests/test_rtcrtpreceiver.py b/tests/test_rtcrtpreceiver.py
index 51d5b71..fa3aa47 100644
--- a/tests/test_rtcrtpreceiver.py
+++ b/tests/test_rtcrtpreceiver.py
@@ -449,6 +449,28 @@ class RTCRtpReceiverTest(CodecTestCase):
             packet = RtpPacket(payload_type=123)
             await receiver._handle_rtp_packet(packet, arrival_time_ms=0)
 
+    @asynctest
+    async def test_rtp_disabled(self):
+        async with create_receiver("audio") as receiver:
+            self.assertEqual(receiver._enabled, True)
+            receiver._track = RemoteStreamTrack(kind="audio")
+
+            await receiver.receive(RTCRtpReceiveParameters(codecs=[PCMU_CODEC]))
+            receiver._enabled = False
+            self.assertEqual(receiver._enabled, False)
+
+            # receive RTP while disabled.
+            packet = RtpPacket(payload_type=0)
+            await receiver._handle_rtp_packet(packet, arrival_time_ms=0)
+
+            # check stats
+            report = await receiver.getStats()
+            self.assertIsInstance(report, RTCStatsReport)
+            self.assertEqual(
+                sorted([s.type for s in report.values()]),
+                ["transport"],
+            )
+
     @asynctest
     async def test_rtp_rtx(self):
         async with create_receiver("video") as receiver:
diff --git a/tests/test_rtcrtpsender.py b/tests/test_rtcrtpsender.py
index c02f8a4..65aad45 100644
--- a/tests/test_rtcrtpsender.py
+++ b/tests/test_rtcrtpsender.py
@@ -389,6 +389,31 @@ class RTCRtpSenderTest(TestCase):
             self.assertEqual(found_rtx.ssrc, 2345)
             self.assertEqual(found_rtx.payload[0:2], pack("!H", packet.sequence_number))
 
+    @asynctest
+    async def test_disabled(self):
+        async with dummy_dtls_transport_pair() as (local_transport, _):
+            sender = RTCRtpSender(AudioStreamTrack(), local_transport)
+            self.assertEqual(sender.kind, "audio")
+            self.assertEqual(sender._enabled, True)
+
+            await sender.send(RTCRtpParameters(codecs=[PCMU_CODEC]))
+            sender._enabled = False
+            self.assertEqual(sender._enabled, False)
+
+            # check stats
+            report = await sender.getStats()
+            self.assertIsInstance(report, RTCStatsReport)
+            self.assertEqual(
+                sorted([s.type for s in report.values()]),
+                ["outbound-rtp", "transport"],
+            )
+
+            outbound_rtp = report["outbound-rtp_" + str(id(sender))]
+            self.assertEqual(outbound_rtp.packetsSent, 0)
+
+            # clean shutdown
+            await sender.stop()
+
     @asynctest
     async def test_stop(self):
         async with dummy_dtls_transport_pair() as (local_transport, _):