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

Hide VP8 and Opus Payloader/Depayloader modules #154

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

mickel8
Copy link
Member

@mickel8 mickel8 commented Aug 8, 2024

Sorry, I don't want to mess in your PR but I have a couple of observations:

  • depayloaders does not take any options right now so we can get rid of them
  • all payloaders can take the same argument - max_payload_size
  • if the two above are true, we can hide everything except ExWebRTC.RTP.Payloader and ExWebRTC.RTP.Depayloader - user doesn't have to know about specific payloader/depayloader modules
  • we can extract payloader/depayloader behaviours into separate modules so they are private too. If we want to provide automatic dispatching, user won't be able to implement those behaviours as our dispatcher's new function has to be modified anyway

An alternative approach would be to use Protocols. The API would look like this:

defprotocol ExWebRTC.RTP.Payloader do
  def payload(t(), data) 
end

defprotocol ExWebRTC.RTP.Depayloader do
  def depayload(t(), data)
end

defmodule ExWebRTC.RTP.Utils do
  alias ExWebRTC.RTPCodecParameters
  
  @spec new_payloader(RTPCodecParameters.t()) :: ExWebRTC.RTP.Payloader.t()
  def new_payloader(codec_params) do
  end
  
  @spec new_depayloader(RTPCodecParameters.t()) :: ExWebRTC.RTP.Depayloader.t()
  def new_depayloader(codec_params) do
  end
end

This way, user can use RTP utils to automatically find payloader/depayloader or implement their own payloade/depayloader and still use our protocols.
This would be beneficial if we used those protocols internally, in PeerConnection but we don't right now.

Any thoughts?

Copy link

codecov bot commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.52%. Comparing base (b142d04) to head (be6c244).

Additional details and impacted files
@@                    Coverage Diff                    @@
##           sgfn/payloader-behaviour     #154   +/-   ##
=========================================================
  Coverage                     88.51%   88.52%           
=========================================================
  Files                            38       38           
  Lines                          1907     1908    +1     
=========================================================
+ Hits                           1688     1689    +1     
  Misses                          219      219           
Files Coverage Δ
lib/ex_webrtc/rtp/depayloader.ex 100.00% <100.00%> (ø)
lib/ex_webrtc/rtp/opus/depayloader.ex 100.00% <ø> (ø)
lib/ex_webrtc/rtp/opus/payloader.ex 100.00% <ø> (ø)
lib/ex_webrtc/rtp/payloader.ex 100.00% <100.00%> (ø)
lib/ex_webrtc/rtp/vp8/depayloader.ex 77.27% <100.00%> (ø)
lib/ex_webrtc/rtp/vp8/payload.ex 90.00% <ø> (ø)
lib/ex_webrtc/rtp/vp8/payloader.ex 100.00% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b142d04...be6c244. Read the comment docs.

with {:ok, module} <- match_payloader_module(codec_params.mime_type) do
payloader = if is_nil(options), do: module.new(), else: module.new(options)

def new(codec_params, max_payload_size \\ 1000) do
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather see the max_payload_size as an option in a keyword list. It makes it easier to add/remove options later on and is more conventional.

Copy link
Member

Choose a reason for hiding this comment

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

Unless you expect that there will never be a new option for the payloaders..

Copy link
Member

@sgfn sgfn left a comment

Choose a reason for hiding this comment

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

LGTM. I thought about using protocols, too, but right now behaviours seem like the better approach

@mickel8 mickel8 merged commit 9488e17 into sgfn/payloader-behaviour Aug 9, 2024
3 checks passed
@mickel8 mickel8 deleted the mickel8/payloader-behaviour branch August 9, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants