search

elixir-webrtc / ex_webrtc

An Elixir implementation of the W3C WebRTC API
Apache License 2.0
363 stars 14 forks source link

Add behaviour and dynamic dispatch for (de)payloaders #147

Closed sgfn closed 3 months ago

sgfn commented 3 months ago

Closes #143

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.51%. Comparing base (af1fec4) to head (b142d04). Report is 4 commits behind head on master.

Files Patch % Lines
lib/ex_webrtc/rtp/vp8/depayloader.ex 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #147 +/- ## ========================================== + Coverage 88.31% 88.51% +0.20% ========================================== Files 36 38 +2 Lines 1891 1907 +16 ========================================== + Hits 1670 1688 +18 + Misses 221 219 -2 ``` | [Files](https://app.codecov.io/gh/elixir-webrtc/ex_webrtc/pull/147?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-webrtc) | Coverage Δ | | |---|---|---| | [lib/ex\_webrtc/rtp/depayloader.ex](https://app.codecov.io/gh/elixir-webrtc/ex_webrtc/pull/147?src=pr&el=tree&filepath=lib%2Fex_webrtc%2Frtp%2Fdepayloader.ex&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-webrtc#diff-bGliL2V4X3dlYnJ0Yy9ydHAvZGVwYXlsb2FkZXIuZXg=) | `100.00% <100.00%> (ø)` | | | [lib/ex\_webrtc/rtp/opus/depayloader.ex](https://app.codecov.io/gh/elixir-webrtc/ex_webrtc/pull/147?src=pr&el=tree&filepath=lib%2Fex_webrtc%2Frtp%2Fopus%2Fdepayloader.ex&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-webrtc#diff-bGliL2V4X3dlYnJ0Yy9ydHAvb3B1cy9kZXBheWxvYWRlci5leA==) | `100.00% <100.00%> (+100.00%)` | :arrow_up: | | [lib/ex\_webrtc/rtp/opus/payloader.ex](https://app.codecov.io/gh/elixir-webrtc/ex_webrtc/pull/147?src=pr&el=tree&filepath=lib%2Fex_webrtc%2Frtp%2Fopus%2Fpayloader.ex&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-webrtc#diff-bGliL2V4X3dlYnJ0Yy9ydHAvb3B1cy9wYXlsb2FkZXIuZXg=) | `100.00% <100.00%> (+100.00%)` | :arrow_up: | | [lib/ex\_webrtc/rtp/payloader.ex](https://app.codecov.io/gh/elixir-webrtc/ex_webrtc/pull/147?src=pr&el=tree&filepath=lib%2Fex_webrtc%2Frtp%2Fpayloader.ex&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-webrtc#diff-bGliL2V4X3dlYnJ0Yy9ydHAvcGF5bG9hZGVyLmV4) | `100.00% <100.00%> (ø)` | | | [lib/ex\_webrtc/rtp/vp8/payloader.ex](https://app.codecov.io/gh/elixir-webrtc/ex_webrtc/pull/147?src=pr&el=tree&filepath=lib%2Fex_webrtc%2Frtp%2Fvp8%2Fpayloader.ex&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-webrtc#diff-bGliL2V4X3dlYnJ0Yy9ydHAvdnA4L3BheWxvYWRlci5leA==) | `100.00% <ø> (ø)` | | | [lib/ex\_webrtc/rtp/vp8/depayloader.ex](https://app.codecov.io/gh/elixir-webrtc/ex_webrtc/pull/147?src=pr&el=tree&filepath=lib%2Fex_webrtc%2Frtp%2Fvp8%2Fdepayloader.ex&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-webrtc#diff-bGliL2V4X3dlYnJ0Yy9ydHAvdnA4L2RlcGF5bG9hZGVyLmV4) | `77.27% <0.00%> (ø)` | | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/elixir-webrtc/ex_webrtc/pull/147?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-webrtc). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-webrtc) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/elixir-webrtc/ex_webrtc/pull/147?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-webrtc). Last update [af1fec4...b142d04](https://app.codecov.io/gh/elixir-webrtc/ex_webrtc/pull/147?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-webrtc). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=elixir-webrtc).
sgfn commented 3 months ago

@LVala I, too, thought that ex_doc would handle it, but alas

Without @spec in implementations:

image

With:

image
LVala commented 3 months ago

Removing the @doc will automatically add a doc with "Callback implementation for ". Indeed, not @spec in such a case.

Not sure which one is better, but having explicit spec seems more useful.

Edit: You also need to remove @impl true to generate the default doc, which is kinda yucky.

mickel8 commented 3 months ago

I think that at the end of the day, we don't want to document functions from Opus/VP8 payloaders/depaylaoders. Instead, the whole documentation should be in the behaviour module or in the module that is responsible for dispatching or in both.

E.g.

d = ExWebRTC.RTP.Depayloader.new(ExWebRTC.RTP.Depayloader.VP8, some_options)
{frame, d} = ExWebRTC.RTP.Depayloader.depayload(d, packet)

In such case, we only need to document ExWebRTC.RTP.Depayloader.VP8 module. Its functions shouldn't appear in docs. But this is the final outcome.

sgfn commented 3 months ago

@mickel8 Could you please reiterate on how exactly do you suggest the docs should look like? Specifically, which parts should I get rid of?

mickel8 commented 3 months ago

@sgfn please take a look at #154