diff --git a/README.md b/README.md index 3237960..34926e3 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,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). | +| `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). Patched; see [`patches/`](patches/). | | `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 @@ -104,16 +104,23 @@ 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`. -**`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. +**Both upstreams are patched, and that matters for more than tidiness.** MPL-2.0 asks that +modifications be identifiable, so they are 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 each +upstream ref *and* every patch applied over it. -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. +Two patches today, both about getting hardware H.264 to a browser: + +- **`gst-plugins-rs`** — `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. +- **`gstreamer-rockchip`** — `mpph264enc`'s pad template omitted `constrained-baseline` from its + profile list, which is the one profile WebRTC asks for. Without this, `webrtcsink` cannot offer + H.264 on an RK3566 at all, and the only trace is a `videorate` complaining about NV12. + +[`patches/README.md`](patches/README.md) has the reasoning for each, 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. diff --git a/patches/README.md b/patches/README.md index 8e2d443..603bcfb 100644 --- a/patches/README.md +++ b/patches/README.md @@ -1,7 +1,12 @@ # 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. +One directory per upstream — `gst-plugins-rs/`, `gstreamer-rockchip/` — applied to that +project's checkout by `scripts/build.sh` in filename order, and recorded in every release's +`MANIFEST` as `patch /` so a binary can be traced to the exact source that +produced it. + +The routing is explicit rather than glob-everything, because a patch aimed at the wrong tree fails +the same way a stale one does, and the two want different fixes. **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 @@ -12,7 +17,7 @@ itself, rather than silently producing a plugin missing the change it was carrie --- -## `0001-webrtcsink-no-converter-for-mpph264enc.patch` +## `gst-plugins-rs/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 @@ -40,3 +45,38 @@ 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. + +--- + +## `gstreamer-rockchip/0001-mpph264enc-advertise-constrained-baseline.patch` + +**What it changes.** One word in `mpph264enc`'s src pad template: its profile list was +`{ baseline, main, high }` and is now `{ constrained-baseline, baseline, main, high }`. + +**Why.** Without it `webrtcsink` cannot offer H.264 on this SoC at all, and says so only at +`GST_DEBUG=*:WARNING`. Its codec discovery pass builds the encoding chain with no output caps, so +`force_profile` is true and it inserts a capsfilter demanding +`video/x-h264, stream-format=avc, profile=constrained-baseline` — WebRTC's interoperable floor. +`h264parse` strips `alignment`, `stream-format` and `parsed` from a caps query but **not +`profile`**, so that demand reaches the encoder's src pad, whose template could not satisfy it. +The intersection is empty, `GstVideoEncoder`'s sink getcaps returns nothing, and the failure +surfaces far upstream as `videorate` reporting it "could not transform NV12 … in anything we +support". Discovery then drops H.264 with a warning, VP8 is negotiated instead, and the session +dies in `rtpvp8pay`. Nothing in the error names the profile. + +**Why it is true and not a convenient claim.** `mpph264enc profile=baseline` sets `h264:cabac_en` +and the 8x8-transform flag off, and MPP emits no FMO, ASO or redundant slices — so the SPS it +writes really does carry `profile_idc=66` with `constraint_set1_flag`. Measured on an RK3566 +rather than reasoned about: `videotestsrc ! mpph264enc profile=baseline ! h264parse` negotiates +`profile=(string)constrained-baseline` on the parser's src pad. The element could always produce +this; only its template denied it. A pad template is a capability set, not current state — the +same template already advertises all three other profiles regardless of which one the property +selects. + +**The trade-off it makes.** None that we can find, which is itself worth stating: the change only +widens what the pad may agree to, and the encoder's own src caps still come from its `profile` +property. A pipeline that asked for `baseline` before still gets it. + +**How it ends.** Upstream, at `JeffyCN/mirrors` or whichever Rockchip tree succeeds it. It is a +one-word capability fix with a reproducer, which is the easiest kind to land; if it is taken, this +file is deleted at the next pin bump. Until then `--check` demands it be re-cut per bump. diff --git a/patches/0001-webrtcsink-no-converter-for-mpph264enc.patch b/patches/gst-plugins-rs/0001-webrtcsink-no-converter-for-mpph264enc.patch similarity index 100% rename from patches/0001-webrtcsink-no-converter-for-mpph264enc.patch rename to patches/gst-plugins-rs/0001-webrtcsink-no-converter-for-mpph264enc.patch diff --git a/patches/gstreamer-rockchip/0001-mpph264enc-advertise-constrained-baseline.patch b/patches/gstreamer-rockchip/0001-mpph264enc-advertise-constrained-baseline.patch new file mode 100644 index 0000000..513268c --- /dev/null +++ b/patches/gstreamer-rockchip/0001-mpph264enc-advertise-constrained-baseline.patch @@ -0,0 +1,11 @@ +--- a/gst/rockchipmpp/gstmpph264enc.c ++++ b/gst/rockchipmpp/gstmpph264enc.c +@@ -98,7 +98,7 @@ GST_STATIC_PAD_TEMPLATE ("src", + GST_MPP_H264_ENC_SIZE_CAPS "," + "stream-format = (string) { byte-stream }, " + "alignment = (string) { au }, " +- "profile = (string) { baseline, main, high }")); ++ "profile = (string) { constrained-baseline, baseline, main, high }")); + + static GstStaticPadTemplate gst_mpp_h264_enc_sink_template = + GST_STATIC_PAD_TEMPLATE ("sink", diff --git a/scripts/build.sh b/scripts/build.sh index 2b25cab..71b6647 100755 --- a/scripts/build.sh +++ b/scripts/build.sh @@ -37,6 +37,34 @@ case "$WANT" in *) die "unknown target: ${WANT} (both, rockchip, webrtc)" ;; esac +# Apply this project's patches to a fresh checkout, and record each in the MANIFEST. +# +# apply_patches +# +# `` is a directory under `patches/`, named for the upstream it patches — the routing has +# to be explicit, because a patch against `net/webrtc/src/webrtcsink/imp.rs` applied to the +# rockchip tree fails in a way that reads like a stale patch rather than a misdirected one. +# +# `--check` runs first so a patch that no longer applies stops the build naming itself, rather +# than producing a plugin quietly missing the change it was carried for. +apply_patches() { + project="$1" + checkout="$2" + dir="${ROOT}/patches/${project}" + [ -d "$dir" ] || return 0 + for patch in "$dir"/*.patch; do + [ -e "$patch" ] || continue + name="$(basename "$patch")" + say "applying ${project}/${name}" + ( cd "$checkout" && git apply --check "$patch" ) \ + || die "${project}/${name} does not apply to this ref. + It was written against a specific version of the file it touches. Re-cut it against the pin, or + drop it if upstream has taken the change — see patches/README.md." + ( cd "$checkout" && git apply "$patch" ) || die "${project}/${name} failed to apply" + printf 'patch %s/%s\n' "$project" "$name" >> "${DIST}/MANIFEST" + done +} + check_environment() { [ "$(id -u)" = 0 ] || die "run as root — it installs build dependencies" arch="$(uname -m)" @@ -112,6 +140,8 @@ build_rockchip() { git -C "${src}/s" checkout -q "$GST_ROCKCHIP_REF" \ || die "${GST_ROCKCHIP_REF} is not on ${GST_ROCKCHIP_BRANCH}" + apply_patches gstreamer-rockchip "${src}/s" + # `rkximage` and `kmssrc` are the X11 and KMS *sinks* in the same tree. A headless robot has # no use for either, and they are why the prebuilt Radxa deb depends on libx11-6. Dropping # them is the concrete thing building ourselves buys, beyond provenance. @@ -173,20 +203,7 @@ 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 + apply_patches gst-plugins-rs "${src}/s" install -d "$DIST" "$STAGE"