Patch webrtcsink to put no converter in front of mpph264enc
`make_converter_for_video_caps` builds the chain webrtcsink inserts ahead of an encoder it selected, special-casing hardware it knows — NVMM, D3D11, CUDA, GL, VA, and on main also v4l2h264enc — and falling back to software `videoconvert ! videoscale` for anything else. Rockchip's MPP encoder takes NV12, I420, YUY2 and more directly and converts on the SoC's 2D accelerator, so the fallback adds a full CPU pass over every frame to do work the hardware was going to do anyway, on the four A55s robotd's 50 Hz loop shares. The reason this matters more than CPU: the robot currently avoids the whole question by pre-encoding and handing webrtcsink finished H.264. That works, and it means webrtcsink cannot reach the encoder — so congestion control cannot adapt the bitrate to the link, and a peer's PLI cannot produce a keyframe, which leaves a viewer that lost one broken until the next periodic GOP. Letting webrtcsink own the encoder fixes both. This patch is what makes that affordable. **This repository is no longer patch-free, and says so.** MPL-2.0 asks that modifications be identifiable, so the README states it, patches/README.md gives each patch's reasoning, and build.sh records every applied patch in the release MANIFEST beside the upstream ref. `git apply --check` runs first, so a patch that stops applying fails the build naming itself rather than yielding a plugin quietly missing the change it was carried for. The trade-off is written down rather than glossed: without videoscale the bin cannot resize, so the negotiated resolution must be one the source produces. True on this robot, which pins its caps upstream of the tee — and the reason upstream may want RGA-backed scaling instead of nothing before taking it. The v4l2h264enc arm on main is the same shape for another hardware encoder, so the precedent exists, and if it lands this file is deleted at the next bump. Verified to apply cleanly against a real 0.15.3 checkout. Assisted-by: Claude:claude-opus-5[1m] shellcheck
This commit is contained in:
parent
fe51a2aa55
commit
b81b50199a
16
README.md
16
README.md
@ -8,7 +8,7 @@ Two plugins, for two unrelated reasons. Neither is packaged anywhere we can inst
|
||||
| plugin | provides | why it is here |
|
||||
|---|---|---|
|
||||
| `libgstrockchipmpp.so` | `mpph264enc`, `mpph265enc`, `mppjpegenc`, `mppvp8enc`, `mppvideodec`, `mppjpegdec` | Debian ships no Rockchip encoder in any suite. Radxa's own `gstreamer1.0-rockchip1_1.14-4` does contain them, so this build is about the pin, dropping `libx11-6`, and riding along with the plugin below — see [below](#the-permission-trap-that-hid-all-of-this). |
|
||||
| `libgstrswebrtc.so`, `libgstrsrtp.so` | `webrtcsink`, `webrtcsrc`, `rsrtp*` | `gstreamer1.0-plugins-rs` does not exist in **any** Debian suite — not trixie, backports, sid or experimental. |
|
||||
| `libgstrswebrtc.so`, `libgstrsrtp.so` | `webrtcsink`, `webrtcsrc`, `rsrtp*` | `gstreamer1.0-plugins-rs` does not exist in **any** Debian suite — not trixie, backports, sid or experimental. Patched; see [`patches/`](patches/). |
|
||||
|
||||
`webrtcbin` is **not** here: it comes from `gstreamer1.0-plugins-bad` in Debian and needs no
|
||||
build.
|
||||
@ -104,8 +104,18 @@ These are binaries built from other people's source, so where that source is mat
|
||||
[the upstream repository](https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs) at the tag in
|
||||
`pins.env`.
|
||||
|
||||
Nothing here is modified — no patches, no forks. Each release's `MANIFEST` names the repository
|
||||
and the exact ref per plugin, which is both the licence answer and the reason a media bug found on
|
||||
**`gst-plugins-rs` is patched, and that matters for more than tidiness.** MPL-2.0 asks that
|
||||
modifications be identifiable, so it is stated here, listed in [`patches/`](patches/) with what
|
||||
each one buys and how it ends, and recorded in every release's `MANIFEST` — a release names the
|
||||
upstream ref *and* every patch applied over it. `gstreamer-rockchip` is unmodified.
|
||||
|
||||
There is one patch today: `webrtcsink` inserts a software `videoconvert ! videoscale` in front of
|
||||
any encoder it does not recognise, and `mpph264enc` converts on the SoC's 2D accelerator instead —
|
||||
so a CPU pass over every frame is added to work the hardware was going to do anyway, on cores the
|
||||
robot's control loop shares. [`patches/README.md`](patches/README.md) has the reasoning, the
|
||||
trade-off it accepts, and the route upstream that would delete it.
|
||||
|
||||
Together the ref and the patch list are both the licence answer and the reason a media bug found on
|
||||
a robot can be traced to a specific build.
|
||||
|
||||
This repository's own build scripts are Apache-2.0, matching the daemon.
|
||||
|
||||
28
patches/0001-webrtcsink-no-converter-for-mpph264enc.patch
Normal file
28
patches/0001-webrtcsink-no-converter-for-mpph264enc.patch
Normal file
@ -0,0 +1,28 @@
|
||||
--- a/net/webrtc/src/webrtcsink/imp.rs
|
||||
+++ b/net/webrtc/src/webrtcsink/imp.rs
|
||||
@@ -685,6 +685,25 @@
|
||||
|
||||
ret.add_many([&vapostproc])?;
|
||||
(vapostproc.clone(), vapostproc)
|
||||
+ } else if codec
|
||||
+ .encoder_factory()
|
||||
+ .is_some_and(|factory| factory.name() == "mpph264enc")
|
||||
+ {
|
||||
+ // Rockchip's MPP encoder accepts NV12, I420, YUY2 and a dozen more formats on its
|
||||
+ // sink pad, and performs any conversion it needs on the SoC's 2D accelerator
|
||||
+ // (RGA) rather than on the CPU. So unlike the NVIDIA and VA cases above, what it
|
||||
+ // wants is not a *better* converter but no converter at all: a software
|
||||
+ // `videoconvert ! videoscale` in front of it is a full CPU pass over every frame,
|
||||
+ // which on the four Cortex-A55s of an RK3566 is precisely the cost the hardware
|
||||
+ // encoder exists to avoid.
|
||||
+ //
|
||||
+ // The trade-off, stated plainly: without `videoscale` this bin cannot resize, so
|
||||
+ // the negotiated resolution has to be one the source already produces. That is
|
||||
+ // true of the robot this is used on, which pins its caps upstream of the tee.
|
||||
+ let identity = make_element("identity", None)?;
|
||||
+
|
||||
+ ret.add_many([&identity])?;
|
||||
+ (identity.clone(), identity)
|
||||
} else {
|
||||
let convert = make_element("videoconvert", None)?;
|
||||
let scale = make_element("videoscale", None)?;
|
||||
42
patches/README.md
Normal file
42
patches/README.md
Normal file
@ -0,0 +1,42 @@
|
||||
# Patches
|
||||
|
||||
Applied to the upstream checkout by `scripts/build.sh`, in filename order, and recorded in every
|
||||
release's `MANIFEST` so a binary can be traced to the exact source that produced it.
|
||||
|
||||
**Carrying a patch is a cost, so each one has to say what it buys and how it ends.** A patch with no
|
||||
route upstream is a fork with extra steps: it has to be re-cut at every version bump, and the
|
||||
binary stops being something anyone else can reproduce from a public ref alone.
|
||||
|
||||
`build.sh` runs `git apply --check` first, so a patch that no longer applies fails the build naming
|
||||
itself, rather than silently producing a plugin missing the change it was carried for.
|
||||
|
||||
---
|
||||
|
||||
## `0001-webrtcsink-no-converter-for-mpph264enc.patch`
|
||||
|
||||
**What it changes.** `make_converter_for_video_caps` in `net/webrtc/src/webrtcsink/imp.rs` builds
|
||||
the chain `webrtcsink` inserts in front of an encoder it selected. It special-cases hardware it
|
||||
knows — NVMM, D3D11, CUDA, GL, VA, and on `main` also `v4l2h264enc` — and falls back to software
|
||||
`videoconvert ! videoscale` for anything else. This adds an arm for `mpph264enc` that inserts
|
||||
nothing.
|
||||
|
||||
**Why.** Rockchip's MPP encoder takes NV12, I420, YUY2 and more directly, and converts on the SoC's
|
||||
2D accelerator rather than the CPU. A software convert-and-scale pass in front of it costs a full
|
||||
CPU traversal of every frame on four Cortex-A55s — which is exactly what the hardware encoder is
|
||||
there to avoid, and which shares those cores with `robotd`'s 50 Hz control loop.
|
||||
|
||||
**Why not just keep pre-encoding.** Because the robot did, and it costs more than it looks.
|
||||
Handing `webrtcsink` finished H.264 means it cannot reach the encoder, so two things it normally
|
||||
does silently do not happen: congestion control cannot adapt the bitrate to the link, and a peer's
|
||||
PLI cannot produce a keyframe — a viewer that loses one stays broken until the next periodic GOP.
|
||||
Letting `webrtcsink` own the encoder fixes both, and this patch is what makes that affordable here.
|
||||
|
||||
**The trade-off it makes.** Without `videoscale` the bin cannot resize, so the negotiated
|
||||
resolution has to be one the source already produces. True on this robot, which pins its caps
|
||||
upstream of its tee — and the honest reason this may need discussion before upstream takes it, since
|
||||
a general fix would want RGA-backed scaling rather than none. `mpph264enc` has `width` and `height`
|
||||
properties that scale on the VPU, but nothing in `webrtcsink` sets them.
|
||||
|
||||
**How it ends.** Upstream. The `v4l2h264enc` arm on `main` is the same shape for another hardware
|
||||
encoder, so the precedent exists; if it is taken, this file is deleted at the next version bump.
|
||||
Until then it is re-cut per bump, which `--check` will demand rather than let slide.
|
||||
@ -173,6 +173,21 @@ build_webrtc() {
|
||||
git clone -q --depth 1 --branch "$GST_PLUGINS_RS_REF" "$GST_PLUGINS_RS_REPO" "${src}/s" \
|
||||
|| die "cannot clone ${GST_PLUGINS_RS_REPO} at ${GST_PLUGINS_RS_REF}"
|
||||
|
||||
# Our patches, applied in order and recorded in the MANIFEST. `--check` first so a patch that
|
||||
# no longer applies stops the build here, naming itself, rather than producing a plugin that is
|
||||
# quietly missing the change it was carried for.
|
||||
for patch in "${ROOT}"/patches/*.patch; do
|
||||
[ -e "$patch" ] || continue
|
||||
name="$(basename "$patch")"
|
||||
say "applying ${name}"
|
||||
( cd "${src}/s" && git apply --check "$patch" ) \
|
||||
|| die "${name} does not apply to ${GST_PLUGINS_RS_REF}.
|
||||
It was written against a specific version of the file it touches. Re-cut it against this ref, or
|
||||
drop it if upstream has taken the change — see patches/README.md."
|
||||
( cd "${src}/s" && git apply "$patch" ) || die "${name} failed to apply"
|
||||
printf 'patch %s\n' "$name" >> "${DIST}/MANIFEST"
|
||||
done
|
||||
|
||||
install -d "$DIST" "$STAGE"
|
||||
|
||||
# Two crates, not one: the same stack wants `libgstrswebrtc.so` *and* `libgstrsrtp.so`.
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user