Ensure codec payload types and rtp header extension ids are not updated

without the need

Author: mickel8

lib/ex_webrtc/peer_connection/configuration.ex

@@ -263,10 +263,27 @@ defmodule ExWebRTC.PeerConnection.Configuration do
     |> Keyword.put(:audio_extensions, Enum.map(audio_extensions, fn {_, ext} -> ext end))
     |> Keyword.put(:video_extensions, Enum.map(video_extensions, fn {_, ext} -> ext end))
     |> then(&struct(__MODULE__, &1))
+    |> ensure_unique_payload_types()
     |> populate_feedbacks(feedbacks)
     |> add_features()
   end
 
+  defp ensure_unique_payload_types(config) do
+    audio_pt = Enum.map(config.audio_codecs, fn codec -> codec.payload_type end)
+
+    if length(audio_pt) != length(Enum.uniq(audio_pt)) do
+      raise "Payload types in audio codecs are not unique."
+    end
+
+    video_pt = Enum.map(config.video_codecs, fn codec -> codec.payload_type end)
+
+    if length(video_pt) != length(Enum.uniq(video_pt)) do
+      raise "Payload types in video codecs are not unique."
+    end
+
+    config
+  end
+
   defp add_features(config) do
     %__MODULE__{features: features} = config
 
@@ -436,21 +453,37 @@ defmodule ExWebRTC.PeerConnection.Configuration do
   defp do_update_extensions(extensions, sdp_extensions, free_ids) do
     # we replace extension ids in config to ids from the SDP
     # in case we have an extension in config but not in SDP, we replace
-    # its id to some free (not present in SDP) id, so it doesn't conflict
+    # its id only when it's occupied to some free (not present in SDP) id, so it doesn't conflict
     Enum.map_reduce(extensions, free_ids, fn ext, free_ids ->
-      sdp_extensions
-      |> Enum.find(&(&1.uri == ext.uri))
-      |> case do
-        nil ->
+      case find_in_sdp_rtp_extensions(sdp_extensions, ext) do
+        {nil, false} ->
+          {ext, free_ids}
+
+        {nil, true} ->
           [id | rest] = free_ids
           {%Extmap{ext | id: id}, rest}
 
-        other ->
+        {other, _id_used} ->
           {%Extmap{ext | id: other.id}, free_ids}
       end
     end)
   end
 
+  # Searches for rtp extension in sdp rtp extensions.
+  # If ext is not found, id_used determines whether ext's id
+  # is already present in sdp_extensions.
+  # Otherwise, id_used can have any value.
+  defp find_in_sdp_rtp_extensions(sdp_extensions, ext, id_used \\ false)
+  defp find_in_sdp_rtp_extensions([], _ext, id_used), do: {nil, id_used}
+
+  defp find_in_sdp_rtp_extensions([sdp_ext | sdp_extensions], ext, id_used) do
+    if sdp_ext.uri == ext.uri do
+      {sdp_ext, id_used}
+    else
+      find_in_sdp_rtp_extensions(sdp_extensions, ext, id_used || sdp_ext.id == ext.id)
+    end
+  end
+
   defp update_codecs(config, sdp) do
     %__MODULE__{audio_codecs: audio_codecs, video_codecs: video_codecs} = config
     sdp_codecs = SDPUtils.get_rtp_codec_parameters(sdp)
@@ -463,29 +496,27 @@ defmodule ExWebRTC.PeerConnection.Configuration do
   end
 
   defp do_update_codecs(codecs, sdp_codecs, free_pts) do
-    # we replace codec payload types in config to payload types from SDP
-    # both normal codecs and rtx (we also update apt FMTP attribute in rtxs)
-    # other codecs that are present in config but not in SDP
-    # are also updated with values from a pool of free payload types (not present in SDP)
-    # to make sure they don't conflict
-    {sdp_rtxs, sdp_codecs} = Enum.split_with(sdp_codecs, &rtx?/1)
+    # We replace codec payload types in config to payload types from SDP
+    # both for normal codecs and rtx (we also update apt FMTP attribute in rtxs).
+    # Other codecs that are present in config but not in SDP, and their
+    # payload type is already present in SDP, are also updated with values
+    # from a pool of free payload types (not present in SDP) to make sure they don't conflict
     {rtxs, codecs} = Enum.split_with(codecs, &rtx?/1)
 
     {codecs, {free_pts, mapping}} =
       Enum.map_reduce(codecs, {free_pts, %{}}, fn codec, {free_pts, mapping} ->
-        sdp_codecs
-        |> Enum.find(
-          &(String.downcase(&1.mime_type) == String.downcase(codec.mime_type) and
-              &1.clock_rate == codec.clock_rate and
-              &1.channels == codec.channels and fmtp_equal_soft?(codec, &1))
-        )
-        |> case do
-          nil ->
+        case find_in_sdp_codecs(sdp_codecs, codec) do
+          # there is no such codec and its payload type is not used
+          {nil, false} ->
+            {codec, {free_pts, Map.put(mapping, codec.payload_type, codec.payload_type)}}
+
+          # there is no such codec, but its payload type is used
+          {nil, true} ->
             [pt | rest] = free_pts
             new_codec = do_update_codec(codec, pt)
             {new_codec, {rest, Map.put(mapping, codec.payload_type, pt)}}
 
-          other ->
+          {other, _pt_used} ->
             new_codec = do_update_codec(codec, other.payload_type)
             {new_codec, {free_pts, Map.put(mapping, codec.payload_type, other.payload_type)}}
         end
@@ -497,15 +528,18 @@ defmodule ExWebRTC.PeerConnection.Configuration do
         %RTPCodecParameters{rtx | sdp_fmtp_line: %FMTP{fmtp | apt: Map.fetch!(mapping, apt)}}
       end)
       |> Enum.map_reduce(free_pts, fn rtx, free_pts ->
-        sdp_rtxs
-        |> Enum.find(&(&1.sdp_fmtp_line.apt == rtx.sdp_fmtp_line.apt))
-        |> case do
-          nil ->
+        case find_in_sdp_codecs(sdp_codecs, rtx) do
+          # there is no such codec and its payload type is not used
+          {nil, false} ->
+            {rtx, free_pts}
+
+          # thre is no such codec, but its payload type is used
+          {nil, true} ->
             [pt | rest] = free_pts
             rtx = do_update_codec(rtx, pt)
             {rtx, rest}
 
-          other ->
+          {other, _pt_used} ->
             rtx = do_update_codec(rtx, other.payload_type)
             {rtx, free_pts}
         end
@@ -514,6 +548,38 @@ defmodule ExWebRTC.PeerConnection.Configuration do
     {codecs ++ rtxs, free_pts}
   end
 
+  # Searches for codec in sdp_codecs.
+  # If codec is not found, pt_used determines whether
+  # codec's payload type is already present in sdp_codecs.
+  # Otherwise, pt_used can have any value.
+  defp find_in_sdp_codecs(sdp_codecs, codec, pt_used \\ false)
+
+  defp find_in_sdp_codecs([], _codec, pt_used), do: {nil, pt_used}
+
+  defp find_in_sdp_codecs([sdp_codec | sdp_codecs], codec, pt_used) do
+    if String.ends_with?(codec.mime_type, "/rtx") do
+      if sdp_codec.sdp_fmtp_line != nil && sdp_codec.sdp_fmtp_line.apt == codec.sdp_fmtp_line.apt do
+        {sdp_codec, pt_used}
+      else
+        find_in_sdp_codecs(
+          sdp_codecs,
+          codec,
+          pt_used || sdp_codec.payload_type == codec.payload_type
+        )
+      end
+    else
+      if codec_equal_soft?(sdp_codec, codec) do
+        {sdp_codec, pt_used}
+      else
+        find_in_sdp_codecs(
+          sdp_codecs,
+          codec,
+          pt_used || sdp_codec.payload_type == codec.payload_type
+        )
+      end
+    end
+  end
+
   defp do_update_codec(codec, new_pt) do
     %RTPCodecParameters{rtcp_fbs: fbs, sdp_fmtp_line: fmtp} = codec
     new_fbs = Enum.map(fbs, &%RTCPFeedback{&1 | pt: new_pt})
@@ -549,6 +615,7 @@ defmodule ExWebRTC.PeerConnection.Configuration do
     end)
   end
 
+  # soft functions does not compare payload types
   @doc false
   @spec codec_equal?(RTPCodecParameters.t(), RTPCodecParameters.t()) :: boolean()
   def codec_equal?(c1, c2) do
@@ -558,6 +625,14 @@ defmodule ExWebRTC.PeerConnection.Configuration do
       c1.channels == c2.channels and fmtp_equal?(c1, c2)
   end
 
+  @doc false
+  @spec codec_equal_soft?(RTPCodecParameters.t(), RTPCodecParameters.t()) :: boolean()
+  def codec_equal_soft?(c1, c2) do
+    String.downcase(c1.mime_type) == String.downcase(c2.mime_type) and
+      c1.clock_rate == c2.clock_rate and
+      c1.channels == c2.channels and fmtp_equal_soft?(c1, c2)
+  end
+
   defp fmtp_equal?(%{sdp_fmtp_line: nil}, _c2), do: true
   defp fmtp_equal?(_c1, %{sdp_fmtp_line: nil}), do: true
   defp fmtp_equal?(c1, c2), do: c1.sdp_fmtp_line == c2.sdp_fmtp_line

lib/ex_webrtc/rtp_sender.ex

@@ -146,9 +146,9 @@ defmodule ExWebRTC.RTPSender do
   defp log_rtx_codec_change(%{rtx_codec: rtx_codec} = sender, nil, neg_codecs)
        when rtx_codec != nil do
     Logger.warning("""
-    Unselecting RTP sender codec as it is no longer supported by the remote side.
+    Unselecting RTP sender RTX codec as it is no longer supported by the remote side.
     Call set_sender_codec again passing supported codec.
-    Codec: #{inspect(sender.codec)}
+    Codec: #{inspect(sender.rtx_codec)}
     Currently negotiated codecs: #{inspect(neg_codecs)}
     """)
   end

test/ex_webrtc/peer_connection/configuration_test.exs

@@ -115,16 +115,15 @@ defmodule ExWebRTC.PeerConnection.ConfigurationTest do
 
       assert length(video_codecs) == 4
 
-      Enum.map(video_codecs, fn %{mime_type: mime, rtcp_fbs: rtcp_fbs} ->
+      Enum.each(video_codecs, fn %{mime_type: mime, rtcp_fbs: rtcp_fbs} ->
         if String.ends_with?(mime, "/rtx") do
           assert rtcp_fbs == []
         else
           assert Enum.any?(rtcp_fbs, &(&1.feedback_type == :nack))
         end
       end)
 
-      [100, 101]
-      |> Enum.each(fn pt ->
+      Enum.each([100, 101], fn pt ->
         assert Enum.any?(
                  video_codecs,
                  &(String.ends_with?(&1.mime_type, "/rtx") and &1.sdp_fmtp_line.apt == pt)
@@ -161,9 +160,17 @@ defmodule ExWebRTC.PeerConnection.ConfigurationTest do
         end
       end
     end
+
+    test "with duplicated payload types" do
+      options = [audio_codecs: [@opus_codec, @opus_codec]]
+      assert_raise RuntimeError, fn -> Configuration.from_options!(options) end
+
+      options = [video_codecs: [@h264_codec, @vp8_codec]]
+      assert_raise RuntimeError, fn -> Configuration.from_options!(options) end
+    end
   end
 
-  describe "update!/2" do
+  describe "update/2" do
     test "updates RTP header extensions" do
       extensions = [
         %{type: :all, uri: @mid_uri},
@@ -294,7 +301,7 @@ defmodule ExWebRTC.PeerConnection.ConfigurationTest do
           ]
         )
 
-      # h264 and it's rtx both should change pt
+      # h264 and its rtx both should change pt
       # vp8 should stay as it is but its rtx should change pt as it conflicts with the new h264
       # vp9 should just change pt
       sdp =
@@ -329,6 +336,96 @@ defmodule ExWebRTC.PeerConnection.ConfigurationTest do
       assert %{mime_type: "video/rtx", payload_type: pt, sdp_fmtp_line: %{apt: 96}} = vp8_rtx
       assert pt not in [100, 101, 96, 110]
     end
+
+    test "does not update anything, when there are no common codecs" do
+      og_config = Configuration.from_options!(controlling_process: self())
+
+      sdp =
+        """
+        m=audio 9 UDP/TLS/RTP/SAVPF 126
+        a=rtpmap:126 newaudiocodec/48000/2
+        m=video 9 UDP/TLS/RTP/SAVPF 127
+        a=rtpmap:127 newvideocodec/90000
+        """
+        |> ExSDP.parse!()
+
+      assert Enum.all?(og_config.audio_codecs, fn codec ->
+               codec.payload_type not in [126, 127] and codec.mime_type != "audio/newaudiocodec"
+             end)
+
+      assert Enum.all?(og_config.video_codecs, fn codec ->
+               codec.payload_type not in [126, 127] and codec.mime_type != "video/newvideocodec"
+             end)
+
+      assert Configuration.update(og_config, sdp) == og_config
+
+      # make sure that RTX codecs and RTP header extensions were also present
+      assert og_config.audio_extensions != []
+      assert og_config.video_extensions != []
+
+      assert Enum.any?(og_config.video_codecs, fn codec ->
+               String.ends_with?(codec.mime_type, "/rtx")
+             end)
+    end
+
+    test "does not update codec payload type when FMTP does not match" do
+      h264_codec = %RTPCodecParameters{
+        payload_type: 98,
+        mime_type: "video/H264",
+        clock_rate: 90_000,
+        sdp_fmtp_line: %FMTP{
+          pt: 98,
+          level_asymmetry_allowed: true,
+          packetization_mode: 1,
+          profile_level_id: 0x42E01F
+        }
+      }
+
+      og_config =
+        Configuration.from_options!(
+          controlling_process: self(),
+          video_codecs: [h264_codec]
+        )
+
+      # packetization mode in fmtp differs
+      sdp =
+        """
+        m=video 58712 UDP/TLS/RTP/SAVPF 127
+        a=rtpmap:127 H264/90000
+        a=rtcp-fb:127 nack
+        a=rtcp-fb:127 nack pli
+        a=fmtp:127 profile-level-id=42e01f;packetization-mode=0;level-asymmetry-allowed=1
+        """
+        |> ExSDP.parse!()
+
+      assert Configuration.update(og_config, sdp) == og_config
+    end
+
+    test "updates codec payload type when there is no local FMTP" do
+      og_config =
+        Configuration.from_options!(controlling_process: self(), video_codecs: [@h264_codec])
+
+      assert @h264_codec.sdp_fmtp_line == nil
+      assert @h264_codec.payload_type != 96
+
+      sdp =
+        """
+        m=video 58712 UDP/TLS/RTP/SAVPF 96
+        a=rtpmap:96 H264/90000
+        a=rtcp-fb:96 nack
+        a=rtcp-fb:96 nack pli
+        a=fmtp:96 profile-level-id=42e01f;packetization-mode=0;level-asymmetry-allowed=1
+        """
+        |> ExSDP.parse!()
+
+      config = Configuration.update(og_config, sdp)
+
+      assert [h264, rtx] = config.video_codecs
+      assert h264.payload_type == 96
+      assert h264.sdp_fmtp_line == nil
+      assert rtx.payload_type != 96
+      assert rtx.sdp_fmtp_line.apt == 96
+    end
   end
 
   test "intersect_codecs/2" do