Add encoder#2
Conversation
…handling of tune in the encoder
…s instead of returning the error tuple
| | {:variable_bitrate, Membrane.VKVideo.Encoder.VariableBitrate.t()} | ||
| | {:constant_bitrate, Membrane.VKVideo.Encoder.ConstantBitrate.t()}, |
There was a problem hiding this comment.
| | {:variable_bitrate, Membrane.VKVideo.Encoder.VariableBitrate.t()} | |
| | {:constant_bitrate, Membrane.VKVideo.Encoder.ConstantBitrate.t()}, | |
| | {:variable_bitrate, __MODULE__.VariableBitrate.t()} | |
| | {:constant_bitrate, __MODULE__.ConstantBitrate.t()}, |
| @type t :: %__MODULE__{bitrate: non_neg_integer(), virtual_buffer_size_ms: non_neg_integer()} | ||
| defstruct [:bitrate, :virtual_buffer_size_ms] |
There was a problem hiding this comment.
If there are no default values for the fields, add @enforce_keys. Also, add some description for the fields, especially virtual_buffer_size_ms
| Framerate of the stream expressed in number of frames per second. | ||
| If nil, the framerate will be read from the stream format's structure. |
There was a problem hiding this comment.
Is it only for the sake of rate control, I'd mention it in the description and probably call it approx_framerate or alike
| Membrane.Logger.warning(""" | ||
| Framerate received within stream format: #{inspect(stream_format.framerate)} was overriden by the value provided via options: | ||
| #{inspect(state.framerate)} | ||
| """) |
There was a problem hiding this comment.
I don't think it's worth issuing a warning in such case. Also, there's code duplication with the other clause that seems easily avoidable
| # assert_sink_playing(pid, :sink) | ||
| # | ||
| # assert_sink_stream_format( | ||
| # pid, | ||
| # :sink, | ||
| # %Membrane.H264{width: 1280, height: 720, alignment: :au, stream_structure: :annexb} | ||
| # ) |
| let device = adapter | ||
| .create_device(wgpu::Features::empty(), wgpu::Limits::default()) |
There was a problem hiding this comment.
It requires putting everything into a single NIF so I am implementing it separately here: #3
| %Membrane.Buffer{ | ||
| payload: encoded_frame.payload, | ||
| pts: pts, | ||
| dts: buffer.dts |
There was a problem hiding this comment.
Does the decoder support B frames? If so, it should output DTS, if not, we should use PTS
There was a problem hiding this comment.
The encoder doesn't support B-frames.
If it would, it should be enough to put the incoming buffer DTS/PTS as the output DTS (if I get it right, if the decoder was to output DTS, it would do it in the same manner as I do it here 🤔 ).
| |> put_in([:width], stream_format.width) | ||
| |> put_in([:height], stream_format.height) |
There was a problem hiding this comment.
why use put_in instead of map update syntax?
There was a problem hiding this comment.
It takes 2 lines less then :D
| alignment: :au, | ||
| width: state.width, | ||
| height: state.height, | ||
| framerate: state.framerate |
There was a problem hiding this comment.
I'm not sure if we should ever set the framerate to approx_framerate specified via options
| description: """ | ||
| Framerate of the stream expressed in number of frames per second. | ||
| It's only used by the rate control mechanism and therefore it does not need to be an exact | ||
| value. If nil, the framerate will be read from the stream format's structure. |
There was a problem hiding this comment.
as discuss, we will warn and fallback to 30 fps
* Puts encoder and decoder into a single NIF * Make Resource an Enum * Fix function names * Add device server (#4) * Add create_device * Fix the resource * Add Device server and destroy function --- Co-authored-by: Jerzy Wilczek <jerzy.wilczek@swmansion.com>
…ia stream format nor options
2 participants
This PR: