Rework defining output stream formats - #25
Conversation
d976cf3 to
5b20a5c
Compare
|
Please, fix CI |
FelonEkonom
left a comment
There was a problem hiding this comment.
There one last thread unresolved
| defp deps do | ||
| [ | ||
| {:membrane_vk_video_plugin, "~> 0.2.0", optional: true}, | ||
| # {:membrane_vk_video_plugin, "~> 0.2.0", optional: true}, |
| @test_cases @video_cases ++ @audio_cases | ||
|
|
||
| Enum.map(@test_cases, fn test_case -> | ||
| @tag :base |
There was a problem hiding this comment.
| @tag :base |
FelonEkonom
left a comment
There was a problem hiding this comment.
Now I see that examples are not updated to the new API. Take a look at examples dir. It would be nice to have a test that runs examples and checks whenever they crash, to detect situations like this one.
FelonEkonom
left a comment
There was a problem hiding this comment.
What with running examples on CI? We could have a test, that runs both examples and checks if there is no error
| defmodule Example do | ||
| alias Membrane.{H264, H265, VP8, RCPipeline} | ||
| alias Membrane.{H264, RCPipeline} | ||
| alias Membrane.Transcoder.OutputFormat |
There was a problem hiding this comment.
I think that having alias for Membrane.Transcoder would be the prettiest in the example, right now we have Membrane.Transcoder in the child spec and OutputFormat.* in its opts, Trascoder and Transcoder.OutputFormat.* would be cleaner.
EDIT: it applies to the second example as well
I think this something we should address in general, not just in this plugin. Maybe add a CI step that does that in test, it should be pretty straightforward |
What with examples that require some interaction, like opening a browser tab to inject a WebRTC stream? Please, open an issue in membrane_core documenting this idea (or just implement it here 😉) |
FelonEkonom
left a comment
There was a problem hiding this comment.
As far as I understood you correctly during todays meeting, this won't be ready to review until you implement these phillars, am I right?
c0ebaa7 to
b295c5d
Compare
| format when is_struct(format) -> | ||
| format | ||
|
|
||
| module when is_atom(module) -> | ||
| struct(module) |
There was a problem hiding this comment.
Shouldn't we check agains a list of available OutputFormats here?
If somebody passes the old Membrane.H264, it will silently pass.
| ) | ||
|
|
||
| _other -> | ||
| builder |
There was a problem hiding this comment.
does it mean that e.g. for %Membrane.RemoteStream{content_format: Membrane.VP8} input and OutputFormat.V8 output configuration we will receive "unparsed" %Membrane.RemoteStream{content_format: Membrane.VP8} stream format from the output pad?
(I know that we don't have VP8 parser but I am just asking :D)
There was a problem hiding this comment.
that's right, for now I thought that transcoding the stream just to parse it is ugly, but I'm not sure what would be the best
| ) :: boolean() | ||
| defp should_be_transcoded(input_format, output_format, transcoding_policy, output_spec) do | ||
| transcoding_policy == :always or | ||
| output_spec.resolution != :keep or |
There was a problem hiding this comment.
And what if output resolution is explicitly provided, but equal to the input format resolution?
There was a problem hiding this comment.
then it will be transcoded and rescaled nonetheless, I didn't add it for now
|
|
||
| defmodule MPEGAudio do | ||
| @moduledoc """ | ||
| Struct defining the desired output Membrane stream format. |
There was a problem hiding this comment.
| Struct defining the desired output Membrane stream format. | |
| Struct defining the desired output MPEGAudio stream format. |
| spec: bitrate_option() | :default, | ||
| default: :default, | ||
| description: """ | ||
| Per-output bitrate setting for video streams. Inherits from bin's `bitrate` option if nil. | ||
| Per-output bitrate setting for video streams. | ||
|
|
||
| Can be either: | ||
| * a `Membrane.Transcoder.Video.ConstantBitrate` struct for constant bitrate encoding | ||
| * a `Membrane.Transcoder.Video.VariableBitrate` struct for variable bitrate encoding | ||
| * `:default` - use encoder defaults |
There was a problem hiding this comment.
Perhaps we could raise if this option is set for audio pad? It might be misleading otherwise
| | %Membrane.RemoteStream{content_format: Membrane.H264 | Membrane.H265} | ||
|
|
||
| An alternative to `t:#{inspect(__MODULE__)}.stream_format/0`. | ||
| @type audio_input_format :: |
There was a problem hiding this comment.
It's a duplicate of
| @@ -259,18 +300,40 @@ defmodule Membrane.Transcoder do | |||
|
|
|||
| spec = child(funnel_name, Funnel) |> bin_output(pad_ref) | |||
There was a problem hiding this comment.
[NIT] I know it's not a part of your change, but this Funnel could be Membrane.Connector
|
|
||
| other_format -> | ||
| module_suffix = other_format.__struct__ |> Module.split() |> List.last() | ||
| struct!(Module.concat(OutputFormat, module_suffix)) |
There was a problem hiding this comment.
Perhaps it would be good to have a whitelist of expected struct modules?
closes membraneframework/membrane_core#1152
Changes introduced by this PR:
OutputFormatprovides module to be used when specifying the desired output formatCode.ensure_loaded?/1calls