Add transcoder. Update vk-video dependency. by varsill · Pull Request #7 · membraneframework/membrane_vk_video_plugin

varsill · 2026-03-20T15:41:52Z

This PR:

adds Membrane.VKVideo.Transcoder along with Membrane.VKVideo.Transcoder.OutputConfig specifying desired output of the transcoder
adds transcoder.rs with transcoder-specific functions
adds new_transcoder(), transcoder and flush_transcoder() NIFs
updates the dependeny to vk-video, adjusts to its new API and update test fixtures
moves EncodedFrame struct from encoder.rs to common lib.rs
removes Cargo dependencies to wgpu and naga
adds transcoder integration tests
adds a helper mechanism to update reference files by setting TAKE_TEST_REFERENCES_SNAPSHOT=1 env

… lib.rs

Committer: varsill <lukasz.kita0@gmail.com>

…his behaviour. Make sure no output pads are added when playback is already playing. Add reference outputs. Bump version to v0.2.0

mat-hek · 2026-03-24T11:44:42Z

+  def_options output_specs: [
+                spec: [Membrane.VKVideo.Transcoder.OutputSpec.t()],
+                description: """
+                List of output specifications. Each entry defines the target resolution,
+                framerate, encoder tune, rate control, and scaling algorithm for one output
+                stream. Output pads are referenced as `Pad.ref(:output, index)` where
+                `index` is the zero-based position in this list.
+                """
+              ]


I'd make it a pad option

mat-hek · 2026-03-24T11:46:28Z

+  Fields:
+  * `width` - output frame width in pixels
+  * `height` - output frame height in pixels
+  * `frame_rate` - output frame rate as `{numerator, denominator}`


Shouldn't it be called approx_framerate and optional, like in the encoder?

yeah, especially that vk-video won't change the framerate. you need to pass the input framerate here. Perhaps not the best API on our part..

I fixed it, now there is a single approx_framerate element option which is propagated to all transcoder output configs.

mat-hek · 2026-03-24T11:50:34Z

+    Unexpected :output pad was linked: \
+    #{inspect(pad_ref)}


Suggested change

Unexpected :output pad was linked: \

#{inspect(pad_ref)}

Pad #{inspect(pad_ref)} was linked when playback already changed to `:playing`. Make sure to link all transcoder pads in the spec where the transcoder is spawned.

mat-hek · 2026-03-24T11:59:18Z

+    let transcoder = TranscoderResource { transcoder_mutex };
+
+    let resource = ResourceArc::new(Resource::Transcoder(transcoder));
+    Ok((ok(), resource))


If we always raise on errors, what's the point of returning an ok tuple? Applies to all functions

we don't alwasy raise, but I think we should

jerzywilczek · 2026-03-24T12:21:28Z

+  Fields:
+  * `width` - output frame width in pixels
+  * `height` - output frame height in pixels
+  * `frame_rate` - output frame rate as `{numerator, denominator}`


yeah, especially that vk-video won't change the framerate. you need to pass the input framerate here. Perhaps not the best API on our part..

jerzywilczek · 2026-03-24T12:23:37Z

 pub enum Resource {
    Encoder(EncoderResource),
    Decoder(DecoderResource),
+    Transcoder(TranscoderResource),
    Device(DeviceResource),
 }


I don't understand why this instead of implementing Resource for each thing. A limitation of rustler? can you only have a single resource per crate?

AFAIK indeed I can define a single resource per crate 🤔

jerzywilczek · 2026-03-24T12:27:46Z

+                .atom_to_string()?
+                .as_str()
+            {
+                "nearest_neighbour" => ScalingAlgorithm::NearestNeighbor,


british spelling?

jerzywilczek · 2026-03-24T12:28:41Z

+            let non_zero_width = std::num::NonZero::new(spec.width).ok_or(Error::BadArg)?;
+            let non_zero_height = std::num::NonZero::new(spec.height).ok_or(Error::BadArg)?;


maybe hard for someone to figure out which argument is bad based on the BadArg alone

In line 52 etc you raise strings instead of returning simple errors, this should be unified I think

I added RaiseTerms with string reason instead

jerzywilczek · 2026-03-24T12:32:16Z

+    let transcoder = TranscoderResource { transcoder_mutex };
+
+    let resource = ResourceArc::new(Resource::Transcoder(transcoder));
+    Ok((ok(), resource))


we don't alwasy raise, but I think we should

jerzywilczek · 2026-03-24T12:35:01Z

+        .transcoder_mutex
+        .lock()
+        .map_err(|err| Error::RaiseTerm(Box::new(err.to_string())))?;
+    let transcoder = guard.as_mut().ok_or(Error::BadArg)?;


Either rename guard to transcoder and remove this line, or add explicit typing in this line to unconfuse the language server. otherwise this line doesnt make sense, you can call guard.transcode() transparently

As discussed - we cannot get rid of this guard, I added an explicit typing

jerzywilczek · 2026-03-24T12:38:42Z

+    let mut guard = transcoder
+        .transcoder_mutex
+        .lock()
+        .map_err(|err| Error::RaiseTerm(Box::new(err.to_string())))?;


not sure if this map_err makes sense here (it might, I'm just not certain). If this is an Err, it means that some thread panicked while having the mutex locked. maybe this is the correct way of handling this, if it happens in rust code the normal thing is to panic this thread too (to propagate panics to everyone who wants to access the mutex), since something hard to detect has gone very wrong

Do I get it right that only if I panic when I encounter Err, other threads trying to get the lock will get Err too? (even though my thread didn't obtain the lock at all? 🤔)

no, sorry explained it wrong. if any thread gets an Err, all others will too

jerzywilczek · 2026-03-24T12:40:27Z

 rustler = "0.37.0"
-vk-video = { git = "https://github.com/software-mansion/smelter.git", features=["expose_parsers"] }
-wgpu = "25.0.2"
+termcolor = "1.4.1"


why termcolor? if you needed to add this it probably means that we need to fix something on our side, you shouldn't have to do that

removed, it was a leftover ;)

jerzywilczek · 2026-03-24T12:41:32Z

since you have these tests, does that mean you have a CI runner with a GPU? can I have it too???

…c field

…lling

jerzywilczek

Also, you don't run any rust checks on CI. I'd run at least cargo fmt --check and cargo clippy -- -D warnings

mat-hek · 2026-03-26T10:10:39Z

+      output_spec: [
+        spec: Membrane.VKVideo.Transcoder.OutputSpec.t(),
+        description: """
+        Output specification for this pad. Defines the target resolution,
+        encoder tune, rate control, and scaling algorithm for the output stream.
+        """


I'd rather put the fields of the OutputSpec as separate pad options

We need to keep OutputSpec as a struct for the sake of convenience in passing it via rustler, but I will make it a private struct and create it based on pad options

mat-hek · 2026-03-26T10:13:21Z

+    cond do
+      state.override_framerate? and is_nil(state.transcoder) ->
+        spawn_transcoder(state)
+
+      not state.override_framerate? ->
+        new_framerate = stream_format.framerate || @default_framerate
+
+        if is_nil(state.transcoder) or new_framerate != state.framerate do
+          %{state | framerate: new_framerate} |> spawn_transcoder()
+        else
+          {[], state}
+        end
+
+      true ->
+        {[], state}
    end


This is a bit convoluted, I'd first calculate the new framerate and then spawn the transcoder if it's not there or the framerate changed

mat-hek · 2026-03-26T10:17:36Z

+    {buffer_actions ++ eos_actions, %{state | transcoder: nil}}
+  end
+
+  defp build_buffer_actions(outputs, state) do


outputs isn't a fortunate name, as it can mean 'output pads'

mat-hek · 2026-03-26T10:18:55Z

+      Enum.with_index(frame_per_pads)
+      |> Enum.map(fn {frame, i} ->
+        {pad_ref, _spec} = Enum.at(state.output_specs, i)


Suggested change

Enum.with_index(frame_per_pads)

|> Enum.map(fn {frame, i} ->

{pad_ref, _spec} = Enum.at(state.output_specs, i)

Enum.zip(frame_per_pads, state.output_specs)

|> Enum.map(fn {frame, {pad_ref, _spec}} ->

mat-hek · 2026-03-26T10:26:13Z

+      output_specs: %{},
+      device: nil,
+      override_framerate?: opts.approx_framerate != nil,
+      framerate: opts.approx_framerate


maybe let's call it approx_framerate everywhere

… transcoder. Add circle ci linter config for rust

mat-hek · 2026-03-26T14:05:51Z

-  def_input_pad :input,
+  alias Membrane.VKVideo.{DeviceServer, Native, Transcoder.OutputSpec}
+
+  def_options(


formatting issue 🕵️‍♀️

mat-hek · 2026-03-26T14:16:04Z

+  def handle_init(_ctx, opts) do
+    state = %{
+      transcoder: nil,
+      output_specs: %{},


let's change to list

mat-hek · 2026-03-26T14:21:14Z

+    new_framerate = state.approx_framerate || stream_format.framerate || @default_framerate
+
+    needs_spawn? =
+      is_nil(state.transcoder) or
+        (not state.override_framerate? and new_framerate != state.approx_framerate)
+
+    if needs_spawn? do
+      %{state | approx_framerate: new_framerate} |> spawn_transcoder()
+    else
+      {[], state}
+    end


let's improve as we spoke

…-labs/membrane_vk_video_plugin into add_transcoder

mat-hek · 2026-03-27T09:13:37Z

  def handle_stream_format(:input, stream_format, _ctx, state) do
-    new_framerate = state.approx_framerate || stream_format.framerate || @default_framerate
+    new_framerate =
+      if not is_nil(state.approx_framerate_option) do
+        state.approx_framerate_option
+      else
+        stream_format.framerate || @default_framerate
+      end

-    needs_spawn? =
-      is_nil(state.transcoder) or
-        (not state.override_framerate? and new_framerate != state.approx_framerate)
+    old_framerate = state.approx_framerate

-    if needs_spawn? do
-      %{state | approx_framerate: new_framerate} |> spawn_transcoder()
+    state = %{state | approx_framerate: new_framerate}
+
+    if is_nil(state.transcoder) or new_framerate != old_framerate do
+      spawn_transcoder(state)
    else
      {[], state}
    end


beautiful ✨

varsill and others added 2 commits March 20, 2026 15:56

WIP - update deps, add transcoder module, move EncodedFrame to common…

08d244a

… lib.rs

Adjust to new vk-video

004767a

Committer: varsill <lukasz.kita0@gmail.com>

varsill mentioned this pull request Mar 20, 2026

Update vk-video #6

Closed

Add native transcoder functions

c711b84

varsill force-pushed the add_transcoder branch from 42b9a10 to c711b84 Compare March 24, 2026 09:18

varsill added 3 commits March 24, 2026 10:41

Add transcoder element. Add tests.

0766dfb

Raise error when unexpected pads are encountered. Add test checking t…

8286173

…his behaviour. Make sure no output pads are added when playback is already playing. Add reference outputs. Bump version to v0.2.0

Rename ref fixtures

90c55ce

varsill requested review from jerzywilczek and mat-hek and removed request for jerzywilczek March 24, 2026 11:25

Add requires_gpu tag

b1c8f73

mat-hek requested changes Mar 24, 2026

View reviewed changes

varsill added 4 commits March 24, 2026 14:46

Make OutputSpec pad's option

b995bca

Remove unecessary test

b7740ba

Simplify output spec handling

aea655b

Fix transcoder

6641332

jerzywilczek approved these changes Mar 24, 2026

View reviewed changes

varsill added 11 commits March 25, 2026 09:31

Add approx_framerate element option in favour of frame_rate OutputSpe…

92d9d1d

…c field

Impelment reviewer suggestions - remove termcolor dependency, fix spe…

ce2c23b

…lling

Remove frame_rate option from tests

f76673c

Make pad_ref key in output_specs map

2bc62e4

Make pad_ref key in output_specs map

47f9e9d

Recreate transcoder

52f13d9

Add framerate to stream format

3ef5fb0

Remove ok tuples

b8bfe9e

Adjust return types

b29511f

Add meaningful errors

39fcc3a

Add more meaningful error messages in encoder and decoder

bbe93fb

varsill requested a review from mat-hek March 26, 2026 08:29

jerzywilczek reviewed Mar 26, 2026

View reviewed changes

mat-hek reviewed Mar 26, 2026

View reviewed changes

varsill added 5 commits March 26, 2026 12:12

Make OutputSpec private. Accept all encoding params as pad options in…

35007fc

… transcoder. Add circle ci linter config for rust

Fix transcoder settings in tests

c856072

Fix clippy warnings

a786273

Fix clippy warning

8f81ed8

Supress clippy warning. Simplify spawning of a new transcoder

a1acca5

varsill requested a review from mat-hek March 26, 2026 13:30

mat-hek reviewed Mar 26, 2026

View reviewed changes

varsill added 2 commits March 26, 2026 15:33

implement CR suggestions

cc199c2

Fix credo warning

431d9bb

varsill requested a review from mat-hek March 26, 2026 15:22

varsill added 2 commits March 27, 2026 09:32

Format the code

dc91505

Merge branch 'add_transcoder' of https://github.com/membraneframework…

83c9613

…-labs/membrane_vk_video_plugin into add_transcoder

mat-hek approved these changes Mar 27, 2026

View reviewed changes

varsill merged commit 5c96478 into master Mar 27, 2026
4 checks passed

varsill self-assigned this Mar 27, 2026

varsill added this to Smackore Mar 27, 2026

varsill moved this to Done in Smackore Mar 27, 2026

varsill mentioned this pull request Mar 27, 2026

Add vk-video transcoder membraneframework/membrane_core#1101

Closed

	Unexpected :output pad was linked: \
	#{inspect(pad_ref)}
	Pad #{inspect(pad_ref)} was linked when playback already changed to `:playing`. Make sure to link all transcoder pads in the spec where the transcoder is spawned.

		let non_zero_width = std::num::NonZero::new(spec.width).ok_or(Error::BadArg)?;
		let non_zero_height = std::num::NonZero::new(spec.height).ok_or(Error::BadArg)?;

Conversation

varsill commented Mar 20, 2026 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

jerzywilczek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varsill Mar 26, 2026 • 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!

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

varsill commented Mar 20, 2026 •

edited

Loading

varsill Mar 26, 2026 •

edited

Loading