elixir-webrtc / ex_webrtc

An Elixir implementation of the W3C WebRTC API
Apache License 2.0
269 stars 8 forks source link

VP8Depayloader.Write error case not guarded #118

Closed ndrean closed 4 days ago

ndrean commented 2 weeks ago

https://github.com/elixir-webrtc/ex_webrtc/blob/e6c8dadb2ee8ad2db5b5b9ae103917733cee804c/lib/ex_webrtc/rtp/vp8_depayloader.ex#L29

You have only the happy path. But with .... else ?? to guard for example {:error, :invalid_packet} returned from VP8Payload.parse.

so maybe you want this:

else
    {:error, msg}  ->
      Logger.warning("#{inspect(msg)}")
      {:ok, depayloader}
end

I saw when I tried to VP8Deplayload a packet with Safari: VP8Payload.parse failed. This does not happen with Chrome !?

An invalid packet (at least as seen by parse):

packet.payload == <<>>

but also this one:

%ExRTP.Packet{
  payload_type: 102,
  sequence_number: 15628,
  timestamp: 3223795332,
  ssrc: 3409997747,
  payload: <<56, 0, 21, 39, 66, 192, 30, 137, 138, 22, 4, 193, 79, 47, 53, 2, 2,
    2, 182, 30, 17, 8, 197, 192, 0, 4, 40, 206, 60, 128>>,
  version: 2,
  padding: false,
  extension: true,
  marker: false,
  csrc: [],
  extension_profile: 48862,
  extensions: [
    %ExRTP.Packet.Extension{id: 3, data: <<0, 4>>},
    %ExRTP.Packet.Extension{id: 4, data: "1"}
  ],
  padding_size: 0
}
mickel8 commented 5 days ago

Hi @ndrean, sorry fo the late reply :c Yeah, it looks like we have a bug. I will try to fix that

mickel8 commented 5 days ago

@ndrean

The second packet can't be parsed as R bit in its first byte is not equal to 0.

I checked what Pion does and it just ignores this bit without even checking its value. This is here

Before modyfing our code I have a couple of question:

  1. Did this occur when running one of our examples or demo applications or when running your own code?
  2. What is your Safari version?
  3. Are you sure that payload type 102 is really an VP8 codec?
mickel8 commented 5 days ago

I also checked Reco example on Safari and it works correctly to me

ndrean commented 5 days ago

Did this occur when running one of our examples or demo applications or when running your own code?

I could not run your example because Xav failed to build. My code but it is a copy of yours, almost literally.

What is your Safari version?

Version 17.5 (19618.2.12.11.6)

Are you sure that payload type 102 is really an VP8 codec?

You are right: ["a=rtpmap:102 H264", "102 H264"].

I will check all this again as I see you updated Xav. If it works for you..

mickel8 commented 4 days ago

Hi @ndrean, I am closing this issue for now as we merged #123 and it also looks like the problem is with invalid codec. However, please feel free to reopen if you still encounter the problem!