Skip to content

Add output resolution support via output format struct width/height#23

Open
khamilowicz wants to merge 8 commits into
masterfrom
add-resolution
Open

Add output resolution support via output format struct width/height#23
khamilowicz wants to merge 8 commits into
masterfrom
add-resolution

Conversation

@khamilowicz

@khamilowicz khamilowicz commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Resolution is specified directly on the output format struct (e.g. %H264{width: 160, height: 90}) rather than as a separate option
  • SWScale.Converter handles scaling when non-nil dimensions are present
  • Parser-only shortcuts for H264→H264 and H265→H265 are blocked when output has non-nil dimensions, since scaling requires full decode+encode

Test plan

  • Scaling integration tests added with reference fixtures (160x90, half of 320x180 source) for software path (H264↔H264, H264↔RawVideo)
  • Fixture filenames include resolution and proper format extension
  • Run mix test to verify all integration tests pass

🤖 Generated with Claude Code

@khamilowicz khamilowicz force-pushed the add-resolution branch 2 times, most recently from 9a2754b to 7591111 Compare June 3, 2026 13:58
@khamilowicz khamilowicz requested a review from varsill June 3, 2026 14:04
Comment thread test/integration_test.exs Outdated
Comment thread test/integration_test.exs
Comment thread lib/transcoder.ex Outdated
Comment thread lib/transcoder.ex
Comment thread lib/transcoder.ex

defp resolve_output_stream_format(nil, input_format) do
case input_format do
%{width: _width, height: _height} -> %{input_format | width: nil, height: nil}

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.

why would we discard width and height from the input_format?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well, we either have defined resolution for the output, in which case we only care about it (to pass to the scaler), or we don't, so we skip scaling resolution, so we indicate it by passing nils. I don't think we need input at all.

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.

let's add a comment saying that, it's not 100% obvious at the first sight that leaving width: nil, height: nil will cause the scaler not to be used if the output_spec doesn't define width and height :D

Comment thread lib/transcoder/video.ex Outdated
@khamilowicz khamilowicz requested a review from varsill June 10, 2026 10:02
@khamilowicz khamilowicz force-pushed the add-resolution branch 2 times, most recently from 90789b3 to b4ea4c0 Compare June 17, 2026 11:52
Comment thread lib/transcoder.ex

defp resolve_output_stream_format(nil, input_format) do
case input_format do
%{width: _width, height: _height} -> %{input_format | width: nil, height: nil}

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.

let's add a comment saying that, it's not 100% obvious at the first sight that leaving width: nil, height: nil will cause the scaler not to be used if the output_spec doesn't define width and height :D

Comment thread test/integration_test.exs Outdated
Comment thread lib/transcoder.ex

defp apply_resolution(format, nil), do: format

defp apply_resolution(format, {_width, _height}), do: format

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.

Hmm, shouldn't we somehow use that _width and _height? I know that we cannot update these fields in format since they are not there, but I think that a scaler would need to be used

@khamilowicz khamilowicz requested a review from varsill June 18, 2026 10:14
khamilowicz and others added 6 commits June 22, 2026 12:03
Resolution is specified directly on the output format struct (e.g.
`%H264{width: 160, height: 90}`) rather than as a separate option.
SWScale.Converter handles scaling when non-nil dimensions are present.

Parser-only shortcuts for H264→H264 and H265→H265 are blocked when
output has non-nil dimensions, since scaling requires full decode+encode.

Vulkan path uses the same SWScale-based scaling approach. Converter
no-op detection is consolidated into named defguardp helpers:
is_nv12_native/1, raw_video_passthrough/2, h26x_compatible_input/1.

Scaling integration tests added with reference fixtures (160x90, half
of the 320x180 source) for software path (H264↔H264, H264↔RawVideo).
Fixture filenames include resolution and proper format extension.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Complete the API change to remove output_stream_format and resolution
from %Membrane.Transcoder{} bin options. All output-related
configuration must now go through output_stream_format or output pad
options via via_out.

Changes:
- Updated pad option descriptions to remove bin inheritance references
- Moved output_stream_format and resolution from bin options to pad
  options in all integration tests
- Regenerated scaling fixtures to match new behavior
- Fixed typo in transcoding_policy description

Generated by Mistral Vibe.
Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
…ions

These options are now strictly bin-level options and cannot be overridden
per-output. The pad options for outputs are now limited to:
- output_stream_format: the desired output format
- resolution: per-output video resolution override

Changes:
- Removed transcoding_policy and native_acceleration from output pad options
  in lib/transcoder.ex
- Updated documentation examples to not pass these via via_out
- Updated handle_pad_added to use bin-level values directly instead of
  falling back from pad options
- Removed test 'multivariant output: per-output transcoding_policy is respected'
  as this feature is no longer supported

Generated by Mistral Vibe.
Co-Authored-By: Mistral Vibe <vibe@mistral.ai>
Comment thread lib/transcoder.ex Outdated
* VP8/VP9 (libvpx): VBR mode with auto target bitrate
"""
],
resolution: [

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.

Are we using this option anywhere? 🤔
It looks that in handle_pad_added we just do resolution: pad_opts.resolution (so we don't fallback to this resolution from element options)

@khamilowicz khamilowicz requested a review from varsill June 25, 2026 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants