bump elixir webrtc to 0.3 #3
Conversation
There was a problem hiding this comment.
LGTM but I just want to make sure that you know that we introduced MediaStreams in 0.3 and right now, your implementation alaways adds tracks separately. So on the FE in ontrack callback, those tracks won't have any streams assigned.
Your examples will work as you do videoPlayer.srcObject.addTrack but I am just mentioning.
You can read more here: https://github.com/elixir-webrtc/ex_webrtc/releases/tag/v0.3.0
|
@mickel8 I have implemented handling keyframe requests since your last review, could you review my changes? |
| rtcp_packets | ||
| |> Enum.reduce(false, fn packet, request_keyframe? -> | ||
| case packet do | ||
| {_track_id, %ExRTCP.Packet.PayloadFeedback.PLI{}} -> |
There was a problem hiding this comment.
I think I would make sure that track_id refers to video track. Just to be sure
There was a problem hiding this comment.
In fact, there is no track_id here
| true | ||
|
|
||
| _other -> | ||
| Membrane.Logger.debug("Ignoring RTCP packet: #{inspect(packet)}") |
| actions = | ||
| if request_keyframe? do | ||
| ctx.pads | ||
| |> Enum.flat_map(fn {pad_ref, pad_data} -> |
There was a problem hiding this comment.
Why sending keyframe on all video pads? Just send on a pad that refers to track_id from line 142
There was a problem hiding this comment.
In fact, there is notrack_id there
| ctx.pads | ||
| |> Enum.flat_map(fn {pad_ref, pad_data} -> | ||
| case pad_data.options.kind do | ||
| :video -> [event: {pad_ref, %Membrane.KeyframeRequestEvent{}}] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Sounds resonable but how to distinct between PLI retransmission and real PLI 🤔
Before merging, we need to figure out the ex_sdp version conflicts, see membraneframework/ex_sdp#50