Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bump elixir webrtc to 0.3 #3

Merged
merged 17 commits into from
Aug 2, 2024
3 changes: 3 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,15 @@ workflows:
filters: &filters
tags:
only: /v.*/
cache-version: 2
- elixir/test:
filters:
<<: *filters
cache-version: 2
- elixir/lint:
filters:
<<: *filters
cache-version: 2
- elixir/hex_publish:
requires:
- elixir/build_test
Expand Down
32 changes: 32 additions & 0 deletions lib/membrane_webrtc/ex_webrtc/sink.ex
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,38 @@ defmodule Membrane.WebRTC.ExWebRTCSink do
raise "Track #{inspect(track_id)} was rejected by the other peer"
end

@impl true
def handle_info({:ex_webrtc, _from, {:rtcp, rtcp_packets}}, ctx, state) do
request_keyframe? =
rtcp_packets
|> Enum.reduce(false, fn packet, request_keyframe? ->
case packet do
{_track_id, %ExRTCP.Packet.PayloadFeedback.PLI{}} ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would make sure that track_id refers to video track. Just to be sure

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, there is no track_id here

Membrane.Logger.debug("Keyframe request received: #{inspect(packet)}")
true

_other ->
Membrane.Logger.debug("Ignoring RTCP packet: #{inspect(packet)}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be pretty verbose

request_keyframe?
end
end)

actions =
if request_keyframe? do
ctx.pads
|> Enum.flat_map(fn {pad_ref, pad_data} ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why sending keyframe on all video pads? Just send on a pad that refers to track_id from line 142

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, there is notrack_id there

case pad_data.options.kind do
:video -> [event: {pad_ref, %Membrane.KeyframeRequestEvent{}}]
Copy link
Member Author

@mat-hek mat-hek Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we somehow 'debounce' the keyframe requests? AFAIR the browser is going to spam us with PLIs when it's unable to decode the stream to make sure we receive a PLI ASAP. And we shouldn't generate as many keyframes as PLIs. cc @mickel8

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds resonable but how to distinct between PLI retransmission and real PLI 🤔

:audio -> []
end
end)
else
[]
end

{actions, state}
end

@impl true
def handle_info({:ex_webrtc, _from, message}, _ctx, state) do
Membrane.Logger.debug("Ignoring ex_webrtc message: #{inspect(message)}")
Expand Down
2 changes: 1 addition & 1 deletion lib/membrane_webrtc/ex_webrtc/source.ex
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ defmodule Membrane.WebRTC.ExWebRTCSource do
end

@impl true
def handle_info({:ex_webrtc, _from, {:rtp, id, packet}}, _ctx, state) do
def handle_info({:ex_webrtc, _from, {:rtp, id, _rid, packet}}, _ctx, state) do
buffer = %Membrane.Buffer{
payload: packet.payload,
metadata: %{rtp: packet |> Map.from_struct() |> Map.delete(:payload)}
Expand Down
28 changes: 4 additions & 24 deletions lib/membrane_webrtc/ex_webrtc/utils.ex
Original file line number Diff line number Diff line change
Expand Up @@ -15,41 +15,21 @@ defmodule Membrane.WebRTC.ExWebRTCUtils do
]

def codec_params(:h264) do
pt = 96
rtx_pt = 97

[
%RTPCodecParameters{
payload_type: pt,
payload_type: 96,
mime_type: "video/H264",
clock_rate: codec_clock_rate(:h264),
rtcp_fbs: [%ExSDP.Attribute.RTCPFeedback{pt: pt, feedback_type: :nack}]
},
%RTPCodecParameters{
payload_type: rtx_pt,
mime_type: "video/rtx",
clock_rate: codec_clock_rate(:h264),
sdp_fmtp_line: %ExSDP.Attribute.FMTP{pt: rtx_pt, apt: pt}
clock_rate: codec_clock_rate(:h264)
}
]
end

def codec_params(:vp8) do
pt = 102
rtx_pt = 103

[
%RTPCodecParameters{
payload_type: pt,
payload_type: 102,
mime_type: "video/VP8",
clock_rate: codec_clock_rate(:vp8),
rtcp_fbs: [%ExSDP.Attribute.RTCPFeedback{pt: pt, feedback_type: :nack}]
},
%RTPCodecParameters{
payload_type: rtx_pt,
mime_type: "video/rtx",
clock_rate: codec_clock_rate(:vp8),
sdp_fmtp_line: %ExSDP.Attribute.FMTP{pt: rtx_pt, apt: pt}
clock_rate: codec_clock_rate(:vp8)
}
]
end
Expand Down
4 changes: 2 additions & 2 deletions mix.exs
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ defmodule Membrane.WebRTC.Plugin.Mixfile do
defp deps do
[
{:membrane_core, "~> 1.0"},
{:ex_webrtc, "~> 0.2.0"},
{:membrane_rtp_plugin, "~> 0.27.1"},
{:ex_webrtc, "~> 0.3.0"},
{:membrane_rtp_plugin, "~> 0.29.0"},
{:membrane_rtp_h264_plugin, "~> 0.19.0"},
{:membrane_rtp_vp8_plugin, "~> 0.9.0"},
{:membrane_rtp_opus_plugin, "~> 0.9.0"},
Expand Down
Loading