feat(egfx): add openh264 feature-gated H.264 decoder

[glamberson]

Adds an optional openh264 feature that provides a concrete H264Decoder
implementation using Cisco's OpenH264 library (BSD-2-Clause).

Depends on #1103 which introduces the H264Decoder trait and client core.

The decoder handles the format conversion required by the RDP protocol:
AVC-format NAL units (4-byte BE length prefix per [2.2.4.4]) are converted
to Annex B start codes before passing to OpenH264, then YUV420p output is
converted to RGBA for the client pipeline.

New optional dependency:

• openh264 = { version = "0.6", optional = true }
• Feature: openh264 = ["dep:openh264"]

4 tests generate valid H.264 bitstreams via the OpenH264 encoder, convert
to AVC format, and decode through the full pipeline to verify round-trip
correctness. Tests only run with --features openh264.

Without the feature enabled, all existing tests continue to pass unchanged.

[Richard Markiewicz (thenextman)]

First, glamberson I know that we all really appreciate your contributions so far to IronRDP. Impressive work! I won't comment on the technical aspects; but there are some things that I want to at least highlight to the other maintainers.

If we're assuming a world where IronRDP is being compiled into third-party RDP clients (rather than just being built and run by individuals for personal use), the default configuration of the openh264 crate makes it impossible for those clients to comply with Cisco's licensing terms. Now, that may or may not matter, depending on if you ship software in a region where software patents are enforceable. For Devolutions, that certainly does matter and I would suggest we toe the line here (and make it straightforward for users of the library to comply with Cisco's licensing).

I'd recommend the following - and I don't know if what I'm saying has technical merit, but hopefully the spirit of what I'm saying should be clear:

• openh264 is already an optional dependency. Is it enabled by default? If it is, I think it should not be.
• By default, we should be using the libloading configuration for openh264. The default (source) configuration could be optional (if we can expose a dependency option like that through the build system) but I don't think it should be default
• Even if openh264 support is enabled, there should be a runtime switch to enable or disable it. I would suggest it should be opt-in.
• Clients should probably have the ability to specify the path to the openh264 binary
  • This isn't strictly needed for license compliance, but from our side (Devolutions) this is useful to have. FreeRDP looks for the binary in known locations, in our (Devolutions) builds we also allow specifying the path to the binary
• I think we should document, either in the code, configuration or README, that there are special licensing considerations for using OpenH264 in an RDP client that's using the feature in IronRDP

Cisco license OpenH254 under a BSD license with additional clauses. Note especially the H264 Patent License conditions. As I wrote, none of this really matters as a library but I think we should make it the "default" case that a client build with IronRDP is compliant with Cisco's terms.

[glamberson]

Hi,

Thanks for the feedback. I completely agree and appreciate you drawing attention to this. I'll review this tomorrow as it's very late where I live.

It's really in my interest personally to make IronRDP as successful and compliant as it can be. This is the impetus behind this work as this particular code has been sitting mostly unused in my own products. But for adoption as a client library, anyone is going to expect this functionality. So considering how it relates to IronRDP is crucial.

Thanks.

Greg Lamberson

[glamberson]

Richard Markiewicz (@thenextman) Thank you for the thorough and well-considered feedback. I completely agree with every point and have spent the day doing exhaustive research on this exact topic — Cisco's binary license terms, the MPEG-LA patent pool structure, how FreeRDP/Firefox/Fedora handle it, and the implications for IronRDP consumers.

Your concerns are entirely correct:

1. source feature = zero patent coverage. Compiling OpenH264 from source gives only BSD-2-Clause copyright, not Cisco's patent license from MPEG-LA. The four conditions (separate download, user control, attribution, license reproduction) cannot be met with a statically compiled binary.

2. libloading must be the default. The openh264 crate's DynamicAPI enum already supports both paths cleanly — from_source() vs from_blob_path(). Switching the default is straightforward.

I'll revise this PR to address all your recommendations:

• Switch from source to libloading as the default feature for openh264-sys2. The source feature can remain available behind an explicit opt-in flag for development/testing environments where patent compliance isn't a concern.
• Add runtime enable/disable: The H264Decoder trait from feat(egfx): add client-side EGFX with surface management and AVC420 decode #1103 already provides the abstraction boundary — consumers that don't load the library simply don't get H.264 decode. I'll add configuration for this.
• Add path specification: Allow consumers to specify the path to the OpenH264 binary, similar to how FreeRDP handles it (searching known system locations with an override).
• Add attribution mechanism: Surface the "OpenH264 Video Codec provided by Cisco Systems, Inc." notice when the library is loaded.
• Document licensing considerations: Add clear documentation in the crate explaining the patent implications of source vs libloading and what consumers need to do for compliance.

On my own products (lamco-rdp-server), I've already adopted a hardware-first encoding strategy (VAAPI/NVENC) with dynamic OpenH264 as the software fallback, and documented this as a firm architectural decision. I'm fully committed to making IronRDP's default path compliant with Cisco's terms — it's in everyone's interest.

I'll push the revised PR once the changes are ready. Happy to discuss any of the specifics.

[Benoît Cortier (CBenoit)]

Thank you for looking into this. I’m okay with the changes in this PR otherwise. Since the PR is big, I’ll ask a Copilot review as well.

[Copilot]

Pull request overview

This pull request adds an optional openh264 feature to ironrdp-egfx that provides a concrete H.264 decoder implementation using Cisco's OpenH264 library. This complements PR #1103 which introduced the H264Decoder trait and EGFX client core. The implementation handles AVC-to-Annex-B format conversion and YUV420p-to-RGBA color space conversion as required by the RDP Graphics Pipeline Extension protocol.

Changes:

• Introduces a new decode module with H264Decoder trait, DecoderError, and DecodedFrame types for pluggable codec support
• Adds optional OpenH264Decoder implementation (feature-gated) that converts AVC-format NAL units to Annex B, decodes via OpenH264, and converts YUV to RGBA
• Updates GraphicsPipelineClient to accept an optional decoder and handle AVC420/uncompressed codecs with proper frame cropping for macroblock alignment
• Adds comprehensive test coverage including round-trip encoding/decoding tests and client state management tests

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
crates/ironrdp-egfx/src/lib.rs	Exports the new decode module publicly
crates/ironrdp-egfx/src/decode.rs	Implements core decoder traits (H264Decoder, DecoderError, DecodedFrame) and optional OpenH264Decoder with AVC↔Annex-B conversion; includes 4 feature-gated tests
crates/ironrdp-egfx/src/client.rs	Expands client from 74-line stub to full implementation with surface management, codec dispatch, H.264 decode integration, frame acknowledgment, and comprehensive tests
crates/ironrdp-egfx/Cargo.toml	Adds optional openh264 = "0.6" dependency with feature gate
Cargo.lock	Adds openh264 dependencies (openh264-sys2, nasm-rs, wide, safe_arch)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This PR introduces openh264 = "0.6" as a dependency, but crates/ironrdp-glutin-renderer/Cargo.toml already uses openh264 = "0.4". Having multiple versions of the same dependency in the workspace can lead to:

1. Larger binary sizes (both versions compiled in)
2. Type incompatibility issues if types from different versions need to interact
3. Confusion about which version is actually being used

Consider either:

• Updating ironrdp-glutin-renderer to use openh264 = "0.6" as well
• Using openh264 = "0.4" here to match the existing dependency
• If the versions are intentionally different due to breaking changes, document why

If version 0.6 has important fixes or features needed for this implementation, the existing usage should be updated in the same PR for consistency.

Suggested change

	openh264 = { version = "0.6", optional = true, default-features = false, features = ["libloading"] }
	openh264 = { version = "0.4", optional = true, default-features = false, features = ["libloading"] }

[Richard Markiewicz (thenextman)]

Benoît Cortier (@CBenoit)

but crates/ironrdp-glutin-renderer/Cargo.toml already uses openh264 = "0.4"

Do you know where / what this is used for? Is there a danger we've shipped OpenH264 and are using it for decoding?

[glamberson]

Yeah, libloading requires version 0.6. Swithcing to 0.4 is not the right way to go. ironrdp-glutin-renderer's usage is exactly what you're talking about. I'm trvelling and will be out of conect for about 3 days (Africa to the US takes awhile) which is why I'm dumping so much on you right now. Anyway, I'd offer ot PR that fix but I can't get to it til i get back.

[Benoît Cortier (CBenoit)]

Richard Markiewicz (@thenextman) This was contributed a while back, but we are not using it nor we ship it, it's kind of a separate project. We need to update the README to properly document that though

[Richard Markiewicz (thenextman)]

Richard Markiewicz (Richard Markiewicz (@thenextman)) This was contributed a while back, but we are not using it nor we ship it, it's kind of a separate project. We need to update the README to properly document that though

Thanks! Ok, as I look there is danger here because it's decoding with OH264. If that was compiled into a client it puts the client in violation of Cisco's license (maybe depending on if you live in a region where software patents are enforceable, but that's not a line we want to toe). So we just need to be careful (without understanding the purpose of the crate 😺)

[Copilot]

The allocation vec![0u8; width * height * 4] on line 227 could potentially cause an integer overflow if width * height * 4 exceeds usize::MAX, leading to a panic or unexpected behavior. While H.264 frame dimensions are typically reasonable, malformed or malicious input could specify extremely large dimensions.

Consider using checked multiplication:

let size = width.checked_mul(height)
    .and_then(|s| s.checked_mul(4))
    .ok_or_else(|| DecoderError::msg("frame dimensions too large"))?;
let mut rgba = vec![0u8; size];

This would make the failure explicit rather than panicking.

[glamberson]

ok sure.

[Richard Markiewicz (thenextman)]

• Add path specification: Allow consumers to specify the path to the OpenH264 binary, similar to how FreeRDP handles it (searching known system locations with an override).

In FreeRDP, the override was something that I specifically added in our (proprietary) client. It was for convenience more than anything, especially on platforms like macOS where FreeRDP is not smart enough to check homebrew, ports, etc....

(I'm just pointing that out so you don't go looking for it in the FreeRDP sources)

• Add attribution mechanism: Surface the "OpenH264 Video Codec provided by Cisco Systems, Inc." notice when the library is loaded.
• Document licensing considerations: Add clear documentation in the crate explaining the patent implications of source vs libloading and what consumers need to do for compliance.

Excellent. Any consumer is also supposed to reproduce the Cisco license "in the EULA and/or in another location where licensing information is to be presented". As a library I don't think that concerns IronRDP.

At Devolutions (again, in our proprietary product) we didn't want to include the license alongside other open source licenses because we didn't want to leave an impression that we were distributing OH264. Instead, in the place in the application where you can enable the feature, we reproduce the disclaimer but it's a clickable link that takes you to the full license. Our legal team determined that put us in compliance.

Overall, I think all we can do is help IronRDP client builders to be in good standing, but we can't do everything :)

[glamberson]

Linux CI failure — unrelated to this PR

The Checks [linux] job is failing at the Install devel packages step with:

E: Failed to fetch https://security.ubuntu.com/ubuntu/pool/main/a/alsa-lib/libasound2-dev_1.2.11-1ubuntu0.1_amd64.deb  404  Not Found
E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?

The ubuntu-latest runner's apt package index has a cached reference to libasound2-dev 1.2.11-1ubuntu0.1, which has been superseded on the Ubuntu mirrors. The CI workflow at .github/workflows/ci.yml:79-80 runs apt-get install without a preceding apt-get update, so it tries to fetch a package version that no longer exists.

All code-related checks pass: macOS, Windows, FFI, Fuzzing, formatting, and typo checks are all green.

Fix: Add sudo apt-get update -qq before the install step in .github/workflows/ci.yml:

      - name: Install devel packages
        if: ${{ runner.os == 'Linux' }}
        run: |
          sudo apt-get update -qq
          sudo apt-get -y install libasound2-dev

Happy to open a separate PR for this if you'd like, or it can be folded into any upcoming CI change.