Repository: aiortc/aiortc

Issue:
End of candidates type error
I am connecting to JS WebRTC, after gathering it gives end of candidates with empty candidate but still has sdpMid and sdpMLineIndex, so I can still make the candidate class with it and the rest is empty. The addIceCandidate() checks whether there is sdpMid and sdpMLineIndex:

https://github.com/aiortc/aiortc/blob/9664b669bcee50dc41e341656b2f8ab19ce69623/src/aiortc/rtcpeerconnection.py#L405-L406

next it will pass the candidate to addRemoteCandidate(), but in there, it checks if the candidate is None as the end of candidates, but the parameter is always a class object in addIceCandidate() so it will never know if its end of candidates:

https://github.com/aiortc/aiortc/blob/9664b669bcee50dc41e341656b2f8ab19ce69623/src/aiortc/rtcicetransport.py#L282-L291

the addIceCandidate() requires the RTCIceCandidate as parameter, even if I pass a None type, it will be cause an error because it checks the sdpMid and sdpMLineIndex. aiortc gatherer doesn't seem to make end of candidates, but Isn't it better to either check for end of candidates in addIceCandidate() too or checks for sdpMid and sdpMLineIndex on the addRemoteCandidate() instead of addIceCandidate()?

Sorry if I made a mistake, I'm a beginner

Hints from issue comments:
@fippo: There was some discussion and a spec change (and a lot of back and forth) back in 2018:
https://github.com/w3c/webrtc-pc/issues/2039

Making the candidate optional in RTCPeerConnection makes sense and one also needs to handle Firefox' candidate-with-empty-string:
```
RTCIceCandidate { candidate: "", sdpMid: "0", sdpMLineIndex: 0, foundation: null, 
component: null, priority: null, address: null, protocol: null, port: null, type: null }
```
@jlaine: I'm not sure I understand what you are saying @fippo , if we allowed `addIceCandidate(None)` how do we know which ICE transport we should pass this end-of-candidates to?@wrangelvid: Joining in on this discussion since I had a hacky workaround ◡̈

We could add the candidate string to RTCIceCandidate, and set it to empty to signal end of candidate similar to [this](https://developer.mozilla.org/en-US/docs/Web/API/RTCIceCandidate/candidate):

> A string representing the transport address for the candidate that can be used for connectivity checks. The format of this address is a candidate-attribute as defined in [RFC 5245](https://datatracker.ietf.org/doc/html/rfc5245). This string is empty ("") if the RTCIceCandidate is an "end of candidates" indicator.

We still pass in `sdpMid` and `sdpMLineIndex` to select the right transport.

Alternatively, we can break parity with the web by having an `signalEndOfCandidate(sdpMid, sdpMLinIndex)` function.@fippo: > if we allowed addIceCandidate(None) how do we know which ICE transport we should pass this end-of-candidates to?

None -> all of the ice candidates get end-of-candidates. This is a bit of a hack in the spec which has a [note](https://w3c.github.io/webrtc-pc/#dom-peerconnection-addicecandidate):
> Due to WebIDL processing, [addIceCandidate](https://w3c.github.io/webrtc-pc/#dom-peerconnection-addicecandidate)(null) is interpreted as a call with the default dictionary present, which, in the above algorithm, indicates end-of-candidates for all media descriptions and ICE candidate generation. This is by design for legacy reasons.

Firefox emits:
* one "" candidate for each (non-bundled) transport
* null candidate after that@jlaine: @fippo in addition to your PR #1318 should we also have a change to both RTCPeerTransport and RTCIceTransport which treats a "" candidate as an end-of-candidates for a specific transport?@fippo: Will work on that. I wonder why nobody complained about interop woes with Firefox yet!@fippo: hrm... the  spec is a [bit weasely](https://w3c.github.io/webrtc-pc/#constructor-0)I, returning early in the algorithm and leaving a lot of non-optional attributes as null.

So this would require changing the addIceCandidate signature in aiortc from
```
addIceCandidate(self, candidate: Optional[RTCIceCandidate])
```
to (for backward compat reasons)
```
addIceCandidate(self, candidate: Optional[RTCIceCandidateInit|RTCIceCandidate])
```@jlaine: I'm fine with going in increments. Supporting `Optional[RTCIceCandidate]` is already a step in the right direction, especially as you caught the fact we were not updating the remote description!

Solution patch:
diff --git a/examples/webcam/webcam.py b/examples/webcam/webcam.py
index 53a44124..dd0fd77e 100644
--- a/examples/webcam/webcam.py
+++ b/examples/webcam/webcam.py
@@ -5,25 +5,39 @@
 import os
 import platform
 import ssl
+from typing import Optional
 
 from aiohttp import web
-from aiortc import RTCPeerConnection, RTCRtpSender, RTCSessionDescription
+from aiortc import (
+    MediaStreamTrack,
+    RTCPeerConnection,
+    RTCRtpSender,
+    RTCSessionDescription,
+)
 from aiortc.contrib.media import MediaPlayer, MediaRelay
 
 ROOT = os.path.dirname(__file__)
 
-
+pcs = set()
 relay = None
 webcam = None
 
 
-def create_local_tracks(play_from, decode):
+def create_local_tracks(
+    play_from: str, decode: bool
+) -> tuple[Optional[MediaStreamTrack], Optional[MediaStreamTrack]]:
     global relay, webcam
 
     if play_from:
+        # If a file name was given, play from that file.
         player = MediaPlayer(play_from, decode=decode)
         return player.audio, player.video
     else:
+        # Otherwise, play from the system's default webcam.
+        #
+        # In order to serve the same webcam to multiple users we make use of
+        # a `MediaRelay`. The webcam will stay open, so it is our responsability
+        # to stop the webcam when the application shuts down in `on_shutdown`.
         options = {"framerate": "30", "video_size": "640x480"}
         if relay is None:
             if platform.system() == "Darwin":
@@ -40,7 +54,7 @@ def create_local_tracks(play_from, decode):
         return None, relay.subscribe(webcam.video)
 
 
-def force_codec(pc, sender, forced_codec):
+def force_codec(pc: RTCPeerConnection, sender: RTCRtpSender, forced_codec: str) -> None:
     kind = forced_codec.split("/")[0]
     codecs = RTCRtpSender.getCapabilities(kind).codecs
     transceiver = next(t for t in pc.getTransceivers() if t.sender == sender)
@@ -49,17 +63,17 @@ def force_codec(pc, sender, forced_codec):
     )
 
 
-async def index(request):
+async def index(request: web.Request) -> web.Response:
     content = open(os.path.join(ROOT, "index.html"), "r").read()
     return web.Response(content_type="text/html", text=content)
 
 
-async def javascript(request):
+async def javascript(request: web.Request) -> web.Response:
     content = open(os.path.join(ROOT, "client.js"), "r").read()
     return web.Response(content_type="application/javascript", text=content)
 
 
-async def offer(request):
+async def offer(request: web.Request) -> web.Response:
     params = await request.json()
     offer = RTCSessionDescription(sdp=params["sdp"], type=params["type"])
 
@@ -67,7 +81,7 @@ async def offer(request):
     pcs.add(pc)
 
     @pc.on("connectionstatechange")
-    async def on_connectionstatechange():
+    async def on_connectionstatechange() -> None:
         print("Connection state is %s" % pc.connectionState)
         if pc.connectionState == "failed":
             await pc.close()
@@ -105,15 +119,16 @@ async def on_connectionstatechange():
     )
 
 
-pcs = set()
-
-
-async def on_shutdown(app):
-    # close peer connections
+async def on_shutdown(app: web.Application) -> None:
+    # Close peer connections.
     coros = [pc.close() for pc in pcs]
     await asyncio.gather(*coros)
     pcs.clear()
 
+    # If a shared webcam was opened, stop it.
+    if webcam is not None:
+        webcam.video.stop()
+
 
 if __name__ == "__main__":
     parser = argparse.ArgumentParser(description="WebRTC webcam demo")
diff --git a/src/aiortc/rtcpeerconnection.py b/src/aiortc/rtcpeerconnection.py
index b34ed3da..69540232 100644
--- a/src/aiortc/rtcpeerconnection.py
+++ b/src/aiortc/rtcpeerconnection.py
@@ -393,30 +393,38 @@ def signalingState(self) -> str:
         """
         return self.__signalingState
 
-    async def addIceCandidate(self, candidate: RTCIceCandidate) -> None:
+    async def addIceCandidate(self, candidate: Optional[RTCIceCandidate]) -> None:
         """
         Add a new :class:`RTCIceCandidate` received from the remote peer.
 
         The specified candidate must have a value for either `sdpMid` or
         `sdpMLineIndex`.
 
-        :param candidate: The new remote candidate.
+        :param candidate: The new remote candidate or `None` to signal
+                            end-of-candidates.
         """
-        if candidate.sdpMid is None and candidate.sdpMLineIndex is None:
+        if (
+            candidate is not None
+            and candidate.sdpMid is None
+            and candidate.sdpMLineIndex is None
+        ):
             raise ValueError("Candidate must have either sdpMid or sdpMLineIndex")
 
         for transceiver in self.__transceivers:
             if (
-                candidate.sdpMid == transceiver.mid
-                or candidate.sdpMLineIndex == transceiver._get_mline_index()
-            ) and not transceiver._bundled:
+                candidate is None
+                or (
+                    candidate.sdpMid == transceiver.mid
+                    or candidate.sdpMLineIndex == transceiver._get_mline_index()
+                )
+                and not transceiver._bundled
+            ):
                 iceTransport = transceiver.receiver.transport.transport
                 await iceTransport.addRemoteCandidate(candidate)
-                return
 
-        if (
-            self.__sctp
-            and (
+        if self.__sctp and (
+            candidate is None
+            or (
                 candidate.sdpMid == self.__sctp.mid
                 or candidate.sdpMLineIndex == self.__sctp_mline_index
             )
@@ -425,6 +433,17 @@ async def addIceCandidate(self, candidate: RTCIceCandidate) -> None:
             iceTransport = self.__sctp.transport.transport
             await iceTransport.addRemoteCandidate(candidate)
 
+        # Update the remote description.
+        media = self.__remoteDescription().media
+        for sdp_m_line_index in range(0, len(media)):
+            if candidate is None:
+                media[sdp_m_line_index].ice_candidates_complete = True
+            elif (
+                candidate.sdpMLineIndex == sdp_m_line_index
+                or candidate.sdpMid == media[sdp_m_line_index].rtp.muxId
+            ):
+                media[sdp_m_line_index].ice_candidates.append(candidate)
+
     def addTrack(self, track: MediaStreamTrack) -> RTCRtpSender:
         """
         Add a :class:`MediaStreamTrack` to the set of media tracks which


Test patch:
diff --git a/tests/test_rtcpeerconnection.py b/tests/test_rtcpeerconnection.py
index 2ef1cd5c..b32995b2 100644
--- a/tests/test_rtcpeerconnection.py
+++ b/tests/test_rtcpeerconnection.py
@@ -575,6 +575,39 @@ def tearDown(self) -> None:
         aioice.stun.RETRY_MAX = self.retry_max
         aioice.stun.RETRY_RTO = self.retry_rto
 
+    @asynctest
+    async def test_addIceCandidate(self) -> None:
+        pc = RTCPeerConnection()
+        pc.createDataChannel("test")
+        offer = await pc.createOffer()
+        await pc.setRemoteDescription(offer)
+        self.assertFalse("a=candidate:" in pc.remoteDescription.sdp)
+        candidate_with_index = RTCIceCandidate(
+            component=1,
+            foundation="0",
+            ip="192.168.99.7",
+            port=33543,
+            priority=2122252543,
+            protocol="UDP",
+            type="host",
+            sdpMLineIndex=0,
+        )
+        await pc.addIceCandidate(candidate_with_index)
+        self.assertTrue("a=candidate:" in pc.remoteDescription.sdp)
+
+        candidate_with_mid = RTCIceCandidate(
+            component=1,
+            foundation="0",
+            ip="192.168.99.7",
+            port=33544,
+            priority=2122252543,
+            protocol="UDP",
+            type="host",
+            sdpMid=pc.sctp.mid,
+        )
+        await pc.addIceCandidate(candidate_with_mid)
+        self.assertEqual(pc.remoteDescription.sdp.count("a=candidate:"), 2)
+
     @asynctest
     async def test_addIceCandidate_no_sdpMid_or_sdpMLineIndex(self) -> None:
         pc = RTCPeerConnection()
@@ -594,6 +627,18 @@ async def test_addIceCandidate_no_sdpMid_or_sdpMLineIndex(self) -> None:
             str(cm.exception), "Candidate must have either sdpMid or sdpMLineIndex"
         )
 
+    @asynctest
+    async def test_addIceCandidate_null(self) -> None:
+        pc = RTCPeerConnection()
+        pc.createDataChannel("test")
+        pc.addTransceiver("audio")
+        pc.addTransceiver("video")
+        offer = await pc.createOffer()
+        await pc.setRemoteDescription(offer)
+        self.assertFalse("a=end-of-candidates" in pc.remoteDescription.sdp)
+        await pc.addIceCandidate(None)
+        self.assertTrue("a=end-of-candidates" in pc.remoteDescription.sdp)
+
     @asynctest
     async def test_addTrack_audio(self) -> None:
         pc = RTCPeerConnection()