Skip to content

Rework defining output stream formats#25

Open
Noarkhh wants to merge 11 commits into
masterfrom
transcoder-api-rework
Open

Rework defining output stream formats#25
Noarkhh wants to merge 11 commits into
masterfrom
transcoder-api-rework

Conversation

@Noarkhh

@Noarkhh Noarkhh commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown

closes membraneframework/membrane_core#1152

Changes introduced by this PR:

  • Previously output format specification relied on stream formats, now OutputFormat provides module to be used when specifying the desired output format
  • Wrapped instantiation of VKVideo structs with Code.ensure_loaded?/1 calls
  • Previously, when input and output formats were structs of the same module, the transcoder just forwarded buffers and didn't create any elements. This approach ignored any differences between fields of these two stream formats. Now a parser is always inserted, making the stream conformant with output format

@Noarkhh Noarkhh self-assigned this Jun 25, 2026
@Noarkhh Noarkhh added this to Smackore Jun 25, 2026
@Noarkhh Noarkhh moved this to In Progress in Smackore Jun 25, 2026
@Noarkhh Noarkhh changed the title wrap VKVideo elements with ensure_loaded? Rework defining output stream formats Jun 25, 2026
@Noarkhh
Noarkhh force-pushed the transcoder-api-rework branch from d976cf3 to 5b20a5c Compare June 26, 2026 13:28
@Noarkhh
Noarkhh requested review from FelonEkonom and varsill June 29, 2026 16:08
@Noarkhh Noarkhh moved this from In Progress to In Review in Smackore Jun 29, 2026
FelonEkonom
FelonEkonom reviewed Jun 30, 2026
View reviewed changes
Comment thread lib/transcoder/audio.ex Outdated
Comment thread lib/transcoder/audio.ex
Comment thread lib/transcoder/audio.ex
Comment thread lib/transcoder/video.ex
Comment thread lib/transcoder/video.ex Outdated
Comment thread lib/transcoder/video.ex Outdated
Comment thread lib/transcoder/video.ex Outdated
Comment thread test/integration_test.exs Outdated
@FelonEkonom

FelonEkonom commented Jun 30, 2026

Copy link
Copy Markdown
Member

Please, fix CI

@Noarkhh
Noarkhh requested a review from FelonEkonom July 1, 2026 09:10
FelonEkonom
FelonEkonom reviewed Jul 1, 2026
View reviewed changes

@FelonEkonom FelonEkonom left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There one last thread unresolved

@Noarkhh
Noarkhh requested a review from FelonEkonom July 1, 2026 09:42
FelonEkonom
FelonEkonom approved these changes Jul 1, 2026
View reviewed changes

@FelonEkonom FelonEkonom left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 🚀 🚀

varsill
varsill requested changes Jul 1, 2026
View reviewed changes
Comment thread mix.exs
defp deps do
[
{:membrane_vk_video_plugin, "~> 0.2.0", optional: true},
# {:membrane_vk_video_plugin, "~> 0.2.0", optional: true},

@varsill varsill Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you commit it ;)

Comment thread test/integration_test.exs Outdated
@test_cases @video_cases ++ @audio_cases

Enum.map(@test_cases, fn test_case ->
@tag :base

@varsill varsill Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@tag :base

Comment thread lib/transcoder.ex
@Noarkhh
Noarkhh requested a review from varsill July 1, 2026 13:25
@FelonEkonom
FelonEkonom self-requested a review July 1, 2026 13:49
FelonEkonom
FelonEkonom requested changes Jul 1, 2026
View reviewed changes

@FelonEkonom FelonEkonom left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Noarkhh
Noarkhh requested a review from FelonEkonom July 2, 2026 07:36
FelonEkonom
FelonEkonom requested changes Jul 2, 2026
View reviewed changes

@FelonEkonom FelonEkonom left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What with running examples on CI? We could have a test, that runs both examples and checks if there is no error

Comment thread examples/multivariant_output.exs Outdated
defmodule Example do
alias Membrane.{H264, H265, VP8, RCPipeline}
alias Membrane.{H264, RCPipeline}
alias Membrane.Transcoder.OutputFormat

@FelonEkonom FelonEkonom Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

varsill
varsill requested changes Jul 2, 2026
View reviewed changes
Comment thread lib/transcoder/audio.ex Outdated
Comment thread lib/transcoder/audio.ex Outdated
Comment thread lib/transcoder/audio.ex Outdated
Comment thread lib/transcoder/output_format.ex Outdated
@Noarkhh

Noarkhh commented Jul 2, 2026

Copy link
Copy Markdown
Author

What with running examples on CI? We could have a test, that runs both examples and checks if there is no error

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

@Noarkhh
Noarkhh requested a review from varsill July 6, 2026 13:55
@Noarkhh
Noarkhh requested a review from FelonEkonom July 6, 2026 13:55
varsill
varsill approved these changes Jul 7, 2026
View reviewed changes

@varsill varsill left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥈 - just fix the CI ;)

@FelonEkonom

FelonEkonom commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

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 😉)

@khamilowicz khamilowicz mentioned this pull request Jul 15, 2026
Open
FelonEkonom
FelonEkonom reviewed Jul 16, 2026
View reviewed changes

@FelonEkonom FelonEkonom left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FelonEkonom FelonEkonom FelonEkonom requested changes
@varsill varsill varsill approved these changes

Assignees

@Noarkhh Noarkhh

Labels

None yet

Projects

Smackore
Status: In Review

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Transcoder] Rework output stream format specification

3 participants

@Noarkhh @FelonEkonom @varsill