diff --git a/README.md b/README.md index 84b9467..3237960 100644 --- a/README.md +++ b/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. diff --git a/patches/0001-webrtcsink-no-converter-for-mpph264enc.patch b/patches/0001-webrtcsink-no-converter-for-mpph264enc.patch new file mode 100644 index 0000000..8936a08 --- /dev/null +++ b/patches/0001-webrtcsink-no-converter-for-mpph264enc.patch @@ -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)?; diff --git a/patches/README.md b/patches/README.md new file mode 100644 index 0000000..8e2d443 --- /dev/null +++ b/patches/README.md @@ -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. diff --git a/scripts/build.sh b/scripts/build.sh index c02d0e5..2b25cab 100755 --- a/scripts/build.sh +++ b/scripts/build.sh @@ -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`.