Add encoder by varsill · Pull Request #2 · membraneframework/membrane_vk_video_plugin

varsill · 2025-12-12T12:35:44Z

This PR:

adds encoder and closes Create Membrane.VKVideo.Encoder membrane_core#1041
implements change concerning handling errors, requested in Initial implementation #1 (comment)

…handling of tune in the encoder

…s instead of returning the error tuple

…name

mat-hek · 2025-12-17T09:49:39Z

lib/encoder.ex

+                  | {:variable_bitrate, Membrane.VKVideo.Encoder.VariableBitrate.t()}
+                  | {:constant_bitrate, Membrane.VKVideo.Encoder.ConstantBitrate.t()},


Suggested change

| {:variable_bitrate, Membrane.VKVideo.Encoder.VariableBitrate.t()}

| {:constant_bitrate, Membrane.VKVideo.Encoder.ConstantBitrate.t()},

| {:variable_bitrate, __MODULE__.VariableBitrate.t()}

| {:constant_bitrate, __MODULE__.ConstantBitrate.t()},

mat-hek · 2025-12-17T09:51:32Z

lib/encoder/constant_bitrate.ex

+  @type t :: %__MODULE__{bitrate: non_neg_integer(), virtual_buffer_size_ms: non_neg_integer()}
+  defstruct [:bitrate, :virtual_buffer_size_ms]


If there are no default values for the fields, add @enforce_keys. Also, add some description for the fields, especially virtual_buffer_size_ms

mat-hek · 2025-12-17T09:58:40Z

lib/encoder.ex

+                Framerate of the stream expressed in number of frames per second.
+                If nil, the framerate will be read from the stream format's structure.


Is it only for the sake of rate control, I'd mention it in the description and probably call it approx_framerate or alike

mat-hek · 2025-12-17T09:59:04Z

lib/encoder.ex

+        Membrane.Logger.warning("""
+        Framerate received within stream format: #{inspect(stream_format.framerate)} was overriden by the value provided via options:
+        #{inspect(state.framerate)}
+        """)


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

mat-hek · 2025-12-17T10:00:57Z

test/encoder_test.exs

+      # assert_sink_playing(pid, :sink)
+      #
+      # assert_sink_stream_format(
+      #   pid,
+      #   :sink,
+      #   %Membrane.H264{width: 1280, height: 720, alignment: :au, stream_structure: :annexb}
+      # )


some leftovers here

mat-hek · 2025-12-17T10:02:16Z

native/vkvideo_encoder/src/lib.rs

+    let device = adapter
+        .create_device(wgpu::Features::empty(), wgpu::Limits::default())


what about the device server?

It requires putting everything into a single NIF so I am implementing it separately here: #3

mat-hek · 2025-12-17T10:05:09Z

lib/encoder.ex

+          %Membrane.Buffer{
+            payload: encoded_frame.payload,
+            pts: pts,
+            dts: buffer.dts


Does the decoder support B frames? If so, it should output DTS, if not, we should use PTS

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 🤔 ).

mat-hek · 2025-12-22T16:26:22Z

lib/encoder.ex

+        |> put_in([:width], stream_format.width)
+        |> put_in([:height], stream_format.height)


why use put_in instead of map update syntax?

It takes 2 lines less then :D

mat-hek · 2025-12-22T16:33:36Z

lib/encoder.ex

+            alignment: :au,
+            width: state.width,
+            height: state.height,
+            framerate: state.framerate


I'm not sure if we should ever set the framerate to approx_framerate specified via options

mat-hek · 2025-12-22T16:33:57Z

lib/encoder.ex

+                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.


what if none is given?

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 <[email protected]>

…ia stream format nor options

WIP

f0a2487

varsill mentioned this pull request Dec 12, 2025

Initial implementation #1

Closed

root and others added 11 commits December 12, 2025 17:40

Return error message from all vk-video errors. Update typespecs. Add …

74f18ad

…handling of tune in the encoder

Add RateControl option

b2d90c6

Add test checking different configuration of the encoder. Raise error…

e8a391d

…s instead of returning the error tuple

Merge branch 'initial_implementation' into add_encoder

dec65bd

Use DirtyIo scheduler for all NIF functions. Adjust tests to new file…

50cbb7d

…name

Merge branch 'initial_implementation' into add_encoder

b43188d

Add :requires_gpu tag to encoder tests

0eef17a

Add encoder element

b4778a3

Update typespecs. Add tests for encoder. Add support for RateControl

2620a98

Fix misspelled module name

f87cd6d

Add more tests for encoder

657619c

varsill requested a review from mat-hek December 16, 2025 16:34

varsill marked this pull request as ready for review December 17, 2025 09:13

mat-hek requested changes Dec 17, 2025

View reviewed changes

root added 2 commits December 22, 2025 14:39

Add docs for rate control structs

a0d0849

Implement reviewers suggestions

ad3b53c

varsill requested a review from mat-hek December 22, 2025 14:56

mat-hek reviewed Dec 22, 2025

View reviewed changes

varsill and others added 3 commits December 23, 2025 11:59

Fallback to 30 and warn if the framerate is needed but not provided v…

b65f73a

…ia stream format nor options

Fix credo

b26f1d9

varsill requested a review from mat-hek January 8, 2026 13:05

mat-hek approved these changes Jan 8, 2026

View reviewed changes

varsill merged commit 9d097e9 into initial_implementation Jan 9, 2026
3 checks passed

		\| {:variable_bitrate, Membrane.VKVideo.Encoder.VariableBitrate.t()}
		\| {:constant_bitrate, Membrane.VKVideo.Encoder.ConstantBitrate.t()},

		@type t :: %__MODULE__{bitrate: non_neg_integer(), virtual_buffer_size_ms: non_neg_integer()}
		defstruct [:bitrate, :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.

		let device = adapter
		.create_device(wgpu::Features::empty(), wgpu::Limits::default())

		\|> put_in([:width], stream_format.width)
		\|> put_in([:height], stream_format.height)

Conversation

varsill commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varsill Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

varsill commented Dec 12, 2025 •

edited

Loading

varsill Dec 17, 2025 •

edited

Loading