RTP input #50
RTP input #50
Conversation
Noarkhh
force-pushed
the
rtp-input
branch
2 times, most recently
from
a417940 to
94f4cc9
Compare
lib/boombox/rtp.ex
Outdated
| if state.track_builders == nil do | ||
| %{state | track_builders: %{media_type => spec}} | ||
| else | ||
| Bunch.Struct.put_in(state, [:track_builders, media_type], spec) | ||
| end |
There was a problem hiding this comment.
Endpoints shouldn't modify the boombox state, otherwise we get a spaghetti. If you need to keep some state, add rtp_state field to the boombox state and use that.
| """ | ||
| @type rtp_encoding_specific_params :: | ||
| {:AAC, [bitrate_mode: RTP.AAC.Utils.mode(), audio_specific_config: binary()]} | ||
| | {:H264, [ppss: [binary()], spss: [binary()]]} |
There was a problem hiding this comment.
- it would be nice if we could pass binaries encoded in HEX/base64 via cmdline, I think FFmpeg allows it too
- How about H265 and Opus?
There was a problem hiding this comment.
Maybe we could have some default mapping between cli and elixir apis when it comes to binaries, like base64 <-> binary?
There was a problem hiding this comment.
yup, good idea, it can be a separate task
There was a problem hiding this comment.
I'll add Opus, but we don't have a rtp_h265_plugin
There was a problem hiding this comment.
That's a lie :D https://hexdocs.pm/membrane_rtp_h265_plugin/readme.html
mix.exs
Outdated
| {:membrane_rtp_plugin, | ||
| github: "membraneframework/membrane_rtp_plugin", branch: "rtp-demuxer", override: true}, | ||
| {:membrane_rtp_format, | ||
| github: "membraneframework/membrane_rtp_format", branch: "resolve-function", override: true}, |
There was a problem hiding this comment.
Shouldn't we also add codec-specific plugins?
|
Whoops didn't mean to ask for a rereview just yet, sry |
|
A test will be added in #51 |
lib/boombox/rtp.ex
Outdated
| :H265 -> | ||
| {Membrane.RTP.H265.Depayloader, Membrane.H265.Parser} |
lib/boombox/rtp.ex
Outdated
| end) | ||
|
|
||
| if opts[:track_configs] == [] do | ||
| raise "No media configured" |
There was a problem hiding this comment.
| raise "No media configured" | |
| raise "No RTP media configured" |
| t = | ||
| Task.async(fn -> | ||
| Boombox.run( | ||
| input: {:rtp, port: rtp_port, track_configs: [audio: [encoding: :OPUS], video: [encoding: :H264]]}, |
There was a problem hiding this comment.
WDYT about making it more flat, meaning
| input: {:rtp, port: rtp_port, track_configs: [audio: [encoding: :OPUS], video: [encoding: :H264]]}, | |
| input: {:rtp, port: rtp_port, audio_encoding: :OPUS, video_encoding: :H264}, |
There was a problem hiding this comment.
it would work for this specific case, but not if we want to pass clock rate or payload type
There was a problem hiding this comment.
audio_clock_rate, video_payload_type etc?
There was a problem hiding this comment.
Maybe something like
input: {:rtp, port: rtp_port, audio_config: [encoding: :OPUS], video_config: [encoding: :H264]}
would be a good middleground
There was a problem hiding this comment.
Not sure how it would look like in the CLI interface
There was a problem hiding this comment.
and how would the least nested option look?
-i --rtp --port 1234 --audio_encoding OPUS --video_encoding H264?
There was a problem hiding this comment.
yeah, I don't think any nesting is feasible in CLI when using only -- args, but in Elixir API I think it adds clarity. I guess these are possible approaches that occur to me:
- the "flat" version - nicer CLI, not so nice Elixir API
- the single nest version - nicer Elixir API, would require us to rethink the CLI
- both - have both versions be available, allow for both
audio_clock_rate: 48_000andaudio_config: [clock_rate: 48_000]. This creates a bit of ambiguity, but could be reasonable.
what do you think?
There was a problem hiding this comment.
Personally, I'd go for the flat version and I'm definitely against having both versions at once :P If we go for a nested version, I guess we should rethink the whole API, for example https://github.com/membraneframework/boombox/blob/master/lib/boombox.ex#L12
No description provided.