[V2]Update GStreamer packages to 1.28.6 + attempt fix ARM32 - #30205
[V2]Update GStreamer packages to 1.28.6 + attempt fix ARM32#30205commodo wants to merge 7 commits into
Conversation
3635ba9 to
0d37872
Compare
0d37872 to
6bcca65
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 7 commits (fresh review, full PR diff). Version/hash bumps are consistent across the six packages, PKG_RELEASE is correctly reset to 1 on gstreamer1 and gst1-plugins-base (both were at 2), no stale 1.26 references are left in the feed, and each commit subject matches the files it touches — no commit-message problems found. The new NEON patch is clean (18/18 add.w → add, no CRLF or trailing whitespace, diffstat matches).
Nothing here blocks a merge from my side. The three inline notes are: an undocumented user-visible package removal (gst1-mod-y4mdec) plus a question about whether the y4m functionality should move to gst-plugins-good, an undocumented dependency-graph change (libgst1video → libgst1allocators → libdrm), and a request for an upstream reference on the new patch.
Note that I could not reach the upstream GStreamer sources from this environment, so anything about what 1.28 changed upstream is phrased as a question rather than an assertion.
Generated by Claude Code
| $(eval $(call GstBuildPlugin,videoparsersbad,videoparsersbad support,codecparsers,,)) | ||
| $(eval $(call GstBuildPlugin,videosignal,videosignal support,,,)) | ||
| $(eval $(call GstBuildPlugin,vmnc,vmnc support,,,)) | ||
| $(eval $(call GstBuildPlugin,y4mdec,y4mdec support,,,)) |
There was a problem hiding this comment.
nit: this drops the user-selectable gst1-mod-y4mdec package (together with the -Dy4m= meson arg on line 159), which is a user-visible removal that the commit message ("update to 1.28.6" + release-notes link) does not mention. Worth a sentence in the commit body saying the y4m plugin is gone from gst-plugins-bad in 1.28 so anyone bisecting a missing package finds it.
Related question: gst-plugins-good already carries the plumbing for a y4m plugin — $(call GST_COND_SELECT,y4m) at gst1-plugins-good/Makefile:139 with the package itself commented out at line 265. If upstream folded y4mdec into the -good y4m plugin, enabling that entry would keep the functionality available instead of losing it in this bump. I could not verify the upstream source from this environment, so this is a question rather than a claim — is the drop intended to be a straight removal?
Generated by Claude Code
| $(eval $(call GstBuildLibrary,sdp,SDP,pbutils rtp,)) | ||
| $(eval $(call GstBuildLibrary,tag,tag support,,)) | ||
| $(eval $(call GstBuildLibrary,video,video,,)) | ||
| $(eval $(call GstBuildLibrary,video,video,allocators,)) |
There was a problem hiding this comment.
Adding allocators here makes libgst1video depend on libgst1allocators, which in turn carries +libdrm (GstBuildLibrary,allocators at line 258). Since practically every gst consumer in the feed pulls libgst1video (gst1-libav, the -good/-bad/-ugly plugins), this quietly adds libdrm to a lot of installs. If it reflects a real link-time dependency in 1.28 that is fine, but it is a dependency-graph change rather than a version bump, and neither it nor the audio addition on line 318 is mentioned in the "update to 1.28.6" commit message — please note both in the commit body so the reason survives.
Generated by Claude Code
|
|
||
| Drop the .w suffix: plain add selects the identical 32-bit encoding in A32, and | ||
| in Thumb-2 the high-register/shift operands force the wide encoding anyway, so | ||
| the generated code is unchanged on both. Reported upstream. |
There was a problem hiding this comment.
nit: "Reported upstream." without a pointer makes this hard to retire later. This patch is a real code change that is expected to go upstream (the PR body says so), so please add the GStreamer issue/MR URL — or an Upstream-Status: line — so a future bump can tell at a glance whether the patch can be dropped.
For context on validation: on this head the Test arm_cortex-a15_neon-vfpv4 job was cancelled at the 6h runner limit rather than completing, so the NEON target this patch exists for is not actually covered by a green run yet (aarch64_generic was cancelled the same way). Not something to fix in the diff, just worth knowing before merge.
Generated by Claude Code
16c687d to
83b691d
Compare
Upstream list of changes is available at https://gstreamer.freedesktop.org/releases/1.28/#1.28.6. Signed-off-by: W. Michael Petullo <mike@flyn.org>
Upstream list of changes is available at https://gstreamer.freedesktop.org/releases/1.28/#1.28.6. Signed-off-by: W. Michael Petullo <mike@flyn.org>
Upstream list of changes is available at https://gstreamer.freedesktop.org/releases/1.28/#1.28.6. Signed-off-by: W. Michael Petullo <mike@flyn.org>
Upstream list of changes is available at https://gstreamer.freedesktop.org/releases/1.28/#1.28.6. Signed-off-by: W. Michael Petullo <mike@flyn.org>
Upstream list of changes is available at https://gstreamer.freedesktop.org/releases/1.28/#1.28.6. Signed-off-by: W. Michael Petullo <mike@flyn.org>
Upstream list of changes is available at https://gstreamer.freedesktop.org/releases/1.28/#1.28.6. Signed-off-by: W. Michael Petullo <mike@flyn.org>
The NEON audio-resampler asm rewritten in 1.28 computes strides with the UAL
wide mnemonic add.w. OpenWrt builds ARM in A32 (non-Thumb) mode, where gas is
in divided syntax and rejects it ("bad instruction `add.w'"), breaking the
build on every NEON target (e.g. cortex-a15). Upstream only builds ARM in Thumb
mode so never hits this; 1.28.6 and main are still affected.
Add a patch dropping the .w suffix: plain add selects the identical 32-bit
encoding in A32, and in Thumb-2 the high-register/shift operands force the wide
encoding anyway, so the emitted code is unchanged on both.
Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
83b691d to
5371599
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed at 5371599 (previous review was at 6bcca65).
The push since my last review is a pure rebase onto a newer master: git range-diff 6bcca65~7..6bcca65 5371599~7..5371599 reports all seven commits as identical (=), and nothing landed on master in the rebase window that touches multimedia/gst*. No new commits and no content changes, so no new issues found.
The three inline notes from the previous review are still open and still apply to the current head:
- the undocumented removal of
gst1-mod-y4mdec(and the related question about they4mplugin in gst-plugins-good), - the undocumented
libgst1video→libgst1allocators(→ libdrm) dependency-graph change, plus theaudioaddition topbtypes, - the missing upstream reference /
Upstream-Status:on100-audio-resampler-neon-a32-syntax.patch.
One piece of context worth repeating for this head: Test arm_cortex-a15_neon-vfpv4 and Test aarch64_generic were again cancelled at the ~6h runner limit rather than completing, so the NEON target this patch exists for still has no completed build behind it. That is a harness limitation, not a defect in the diff — nothing here blocks a merge from my side.
Generated by Claude Code
3 participants
📦 Package Details
Maintainer: @thess @flyn-org @dangowrt
Description:
V2 of PR #29616
But trying to fix the ARM32 stuff here.
If this works, we will try to find an upstreamable fix for ARM32
🧪 Run Testing Details
✅ Formalities