Reapply "Add NVENC AV1 hardware video encoding for WebRTC"

[jemoreira]

No description provided.

[Databean]

Don't have insight into the webrtc implementation, reviewed mostly the external dependencies.

[Databean]

What does this have to do with nvenc? It looks like this is the only change to vhost_user_input in this PR.

[larsers-googl]

I had some issues with the CI itself...

My understanding is that grpcio-sys depends on libz-sys with features = ["static"]. The static feature makes libz-sys's build script compile zlib from vendored C sources in src/zlib/ ... But that directory is a git submodule that is not afaik included in the crates.io tarball (the published package contains like 13 files). So any build of that crate from a clean state fails.

The Cuttlefish CI doesn't normally hit this because the build script output is probably kept in the GitHub Actions disk cache. But I got the failure on a fresh clone of unmodified main with a cold cache. This PR's MODULE.bazel changes invalidate that cache, so without the annotation the PR fails CI on a bug I (afaik) didn't introduce.

The annotation disables the broken build script and links the system zlib instead. It lives in vhost_user_input.MODULE.bazel (rather than netsim's or casimir's) because all three crate registries share the same crate_universe extension instance. So one annotation covers all three, and adding it to a second file errors with "duplicate annotation".

[Databean]

This seems backwards: shouldn't the target with hdrs be the one that is included elsewhere, and it can internally have deps on other targets that provide the sources?

[larsers-googl]

The direction is needed by a real compilation dependency, not just header re-export... dav1d's own sources include the public API headers (e.g.src/lib.c has #include "dav1d/dav1d.h" and #include "dav1d/data.h")...And those includes only resolve through :dav1d_api's strip_include_prefix = "include". So afaik :dav1d (srcs) needs :dav1d_api (hdrs) to compile.

Inverting it (:dav1d_api as the public entry with an internal dep on a sources target) would make the sources target depend on the api target for its includes while the api target depends on it for linkage...Creating a cycle.

So the split exists because the two file sets need different include path treatment. The public headers get strip_include_prefix (consumers write #include <dav1d/dav1d.h>), while the internal headers
shouldn't be exported via includes/-isystem (it will cause a problem where config.h gets a shadowing conflict with libsrtp2 that this layout avoids). Consumers depend on @dav1d and get the API headers transitively, which also keeps the conventional @dav1d shorthand label working.

[Databean]

My understanding of bazel is that the same headers can be attributed to multiple targets, so the api target could have hdrs, and the implementation target could list all of the .h files in its own srcs target as well. That doesn't solve the strip_include_prefix problem though.

[larsers-googl]

I'm not by any means an expert on these things... But you're right in that multiple attribution is doable, so the impl target could carry the public headers in its own srcs and the cycle goes away. But as you note, that still leaves the include path problem and I don't know if there's a clean mechanism for it...

• Headers listed in srcs are plain private files. So strip_include_prefix only applies to hdrs...So #include "dav1d/dav1d.h" in the sources won't resolve, only the literal "include/dav1d/dav1d.h" path would.
• Rewriting the 21 public-header includes across 18 source files to "include/dav1d/..." via sed would work, but that would grow the patch command surface (which was already large :) )
• includes = ["include"] on the impl resolves it I think it would propagate -isystem include/ to every transitive dependent, exposing the whole directory tree rather than the globbed header list...That would cause a leak similar to the config.h shadowing conflict with libsrtp2 that this layout exists to avoid.
• A nonpropagating copts = ["-iquote", "external/.../include"] needs a hardcoded external repo path, which is brittle under bzlmod's repo name handling.

So to me deps = [":dav1d_api"] seems like the least bad way to hand the impl the dav1d/rooted include paths. Only the declared public headers are reachable, nothing leaks to dependents, and the impl consumes the api through the same contract external consumers do.

[Databean]

Can the http_archive rule get a checksum to validate the file hasn't changed?

[larsers-googl]

Yes, it probably should have one...

We probably could switch to videolan's official static release tarball, which has a published checksum and a stable URL.

So, something like this:

urls = ["https://downloads.videolan.org/pub/videolan/dav1d/1.5.3/dav1d-1.5.3.tar.xz"],
sha256 = "732010aa5ef461fa93355ed2c6c5fedb48ddc4b74e697eaabe8907eaeb943011",

[Databean]

This is a lot for patch_cmds. Is it possible to include the newly defined files as targets of build_external/dav1d that are referenced by the targets created in dav1d?

[larsers-googl]

Well...the commands fall into two groups:

• The three generated headers (dav1d_config.h, dav1d_vcs_version.h, include/dav1d/version.h) could become checked-in files underbuild_external/dav1d/, copied into the external repo by a small copy_file/genrule in BUILD.dav1d.bazel. That would work, they'd be reviewable as plain files but we have to do some rule plumbing then... (they have to be materialized inside the @dav1d repo for #include "dav1d_config.h" to resolve via -iquote).

• The mv include/common src/common + sed include rewrites can't be afaik expressed as file targets. They modify the fetched sources. The only alternative is a conventional patches file, but since the rewrite touches an include line in nearly every file under src/, that patch would imho more brittle. The sed commands are at least self describing and survive version bumps.

How do you want to do it?

[larsers-googl]

lgtm