Add :force_transcoding option #65
Conversation
lib/boombox.ex
Outdated
| @spec run(Enumerable.t() | nil, | ||
| input: input(), | ||
| output: output(), | ||
| enforce_transcoding?: boolean() |
There was a problem hiding this comment.
I think we should make it possible to enforce transcoding of only audio or video and also specify the desired output codec
There was a problem hiding this comment.
I will make it possible to enforce transcoding of only video or audio, but IMO specifying the desired output codec should be done in a completely separate option, because
- For now it makes sense only for WebRTC output (so maybe it should be a webrtc output option?)
- Sbd could want to specify the output codec without enforcing transcoding or vice versa, so what is the point of merging these 2 things into one?
There was a problem hiding this comment.
I'm not saying that it should be one option, but they are somehow related, so we should make sure that they make sense together. For example, having the codec as an output option and enforce_transcoding as a top-level option would be confusing to me.
FelonEkonom
changed the title
FelonEkonom
changed the title
lib/boombox.ex
Outdated
| @@ -9,6 +9,8 @@ defmodule Boombox do | |||
|
|
|||
| alias Membrane.RTP | |||
|
|
|||
| @type force_transcoding_value() :: boolean() | :audio | :video | |||
There was a problem hiding this comment.
I'd make it full tuple
| @type force_transcoding_value() :: boolean() | :audio | :video | |
| @type force_transcoding() :: {:force_transcoding, boolean() | :audio | :video} |
lib/boombox.ex
Outdated
| If `:enforce_audio_transcoding?` or `:enforce_video_transcoding?` option is set to `true`, | ||
| boombox will perform audio and/or video transcoding, even if it is not necessary. By default | ||
| both options are set to `false`. | ||
|
|
||
| ``` | ||
| Boombox.run(input: "path/to/file.mp4", output: {:webrtc, "ws://0.0.0.0:1234"}, enforce_transcoding?: true) | ||
| Boombox.run( | ||
| input: "path/to/file.mp4", | ||
| output: {:webrtc, "ws://0.0.0.0:1234"}, | ||
| enforce_video_transcoding?: true, | ||
| enforce_audio_transcoding?: true |
lib/boombox.ex
Outdated
| defp resolve_force_transcoding(opts) do | ||
| maybe_keyword = | ||
| opts.output | ||
| |> Tuple.to_list() | ||
| |> List.last() | ||
|
|
||
| force_transcoding = | ||
| Keyword.keyword?(maybe_keyword) && Keyword.get(maybe_keyword, :force_transcoding, false) | ||
|
|
||
| opts | ||
| |> Map.put(:force_transcoding, force_transcoding) | ||
| |> Map.update!(:output, fn | ||
| {:webrtc, signaling, _opts} -> {:webrtc, signaling} | ||
| {:hls, location, _opts} -> {:hls, location} | ||
| {:mp4, location, _opts} -> {:mp4, location} | ||
| other -> other | ||
| end) |
There was a problem hiding this comment.
This looks bad. Why not keep enforce_transcoding like all other options?
Depends on membraneframework/membrane_transcoder_plugin#6
Closes membraneframework/membrane_core#938