[V2]Update GStreamer packages to 1.28.7 + fix ARM32 - #30205
Conversation
3635ba9 to
0d37872
Compare
0d37872 to
6bcca65
Compare
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
There was a problem hiding this comment.
fixed, thanks — documented in the commit body and the plugin is kept available via the new gst1-mod-y4m in gst-plugins-good.
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
There was a problem hiding this comment.
83b691d to
5371599
Compare
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
|
I see two build targets timed out. It looks like the build succeeded in both cases. Can anyone else comment on this?Please add a reference to where you submitted/reported 100-audio-resampler-neon-a32-syntax.patch upstream. Thank you—I am very glad to see this progress. |
Right. |
90bbe40 to
26a541f
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed at 26a541f (previous review was at 5371599).
The only change to this feed since then is a rebase plus a rewrite of the commit body and the patch header on the last commit. git diff 5371599..26a541f -- multimedia/ touches nothing but the header of 100-audio-resampler-neon-a32-syntax.patch; all 18 add.w → add hunks are byte-identical, and git range-diff reports the six version/hash-bump commits as =.
Two things worth recording for this head:
Test arm_cortex-a15_neon-vfpv4completed successfully instead of being cancelled at the runner limit, so the NEON A32 target this patch exists for finally has a green build behind it. That was the main open unknown in my last two reviews.- I checked the "OpenWrt builds ARM in A32 mode" premise against
openwrt/openwrt— there is no-mthumborUSE_THUMBanywhere in the userspace build flags, onlyCONFIG_ARM_THUMBin kernel configs — so the reasoning in the patch header holds. The encoding claims check out too (e0888001isadd r8, r8, rNin A32; the 16-bit ThumbADD (register)form supports high registers and leaves flags alone).
Commit checks
26a541f"gst1-plugins-base: fix NEON resampler build in ARM A32 mode" — the reworded body now says "Backport a patch dropping the.wsuffix". "Backport" asserts the fix already exists upstream, but as of your 2026-08-24 reply upstreaming had not started, and the sentence that previously hedged this ("Reported upstream.") was dropped rather than replaced with a link. If the fix has landed upstream since, please cite the commit/MR in the body; if not, "Add a patch" is the accurate verb. I could not reach the GStreamer repo from this environment, so this is a question rather than an assertion. Same point inline on the patch header.
Still open from the first review and unchanged on this head: the undocumented removal of gst1-mod-y4mdec in gst1-plugins-bad, and the libgst1video → libgst1allocators (→ libdrm) dependency change plus the audio addition to pbtypes.
One CI note so it is not mistaken for a regression: Test x86_64 failed at the runtime-test install step with libmesa (no such package): required by: libgst1gl-1.28.6-r1[libmesa]. That +libmesa dependency is pre-existing on master (GstBuildLibrary,gl at gst1-plugins-base/Makefile:262, untouched by this PR), so it reads as a feed/index gap in the test harness rather than something this diff introduced. aarch64_generic was again cancelled at the 6h limit.
Nothing here blocks a merge from my side.
Generated by Claude Code
| sole 32-bit encoding (e0888001); in Thumb-2 it picks the 16-bit encoding | ||
| (4488) instead of add.w (eb08 0801), same result, flags untouched. | ||
|
|
||
| Signed-off-by: Alexandru Ardelean <alex@shruggie.ro> |
There was a problem hiding this comment.
The upstream reference is still missing, and this reword moved it backwards rather than forwards: the previous header ended with "Reported upstream.", and that sentence was deleted without a link taking its place — while the commit body now calls the change a "Backport". flyn-org asked for the reference explicitly and your reply said upstreaming had not started yet, so as the patch stands there is nothing a future maintainer can check to decide whether it can be dropped at the next version bump.
The minimum that keeps the patch retirable — swap Pending for the MR URL once it is filed:
| Signed-off-by: Alexandru Ardelean <alex@shruggie.ro> | |
| Signed-off-by: Alexandru Ardelean <alex@shruggie.ro> | |
| Upstream-Status: Pending |
Related, on line 1: From 3664d240c434969e6e460faa323704a4c579261e Mon Sep 17 00:00:00 2001 is the id of a commit in your own tree, not an upstream one (unless the fix has landed upstream since — I could not reach the GStreamer repo from this environment to check). Combined with "Backport a patch..." in the commit body, that header reads like an upstream commit someone is meant to be able to look up. Either drop the From <sha> line or make the commit body say "Add a patch", so the provenance is unambiguous.
Generated by Claude Code
There was a problem hiding this comment.
fixed, thanks — the patch is now a real upstream backport with Upstream-Status and the MR link, and the From line is an upstream SHA.
Generated by Claude Code
26a541f to
147f3c1
Compare
82e242b to
f7220d9
Compare
Upstream list of changes is available at https://gstreamer.freedesktop.org/releases/1.28/#1.28.7. Signed-off-by: W. Michael Petullo <mike@flyn.org> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
Upstream list of changes is available at https://gstreamer.freedesktop.org/releases/1.28/#1.28.7. Signed-off-by: W. Michael Petullo <mike@flyn.org> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
Upstream list of changes is available at https://gstreamer.freedesktop.org/releases/1.28/#1.28.7. Signed-off-by: W. Michael Petullo <mike@flyn.org> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
In 1.28 upstream merged the y4mdec element from gst-plugins-bad into the gst-plugins-good y4m plugin, which now registers both y4mdec and y4menc from a single libgsty4m. The Makefile has passed -Dy4m= since 1.16.2, but the package line stayed commented out, so the plugin was never built. Enable it, so the y4mdec element dropped from gst-plugins-bad in this series stays available. Anyone who selected gst1-mod-y4mdec needs to select gst1-mod-y4m instead. Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
Upstream list of changes is available at https://gstreamer.freedesktop.org/releases/1.28/#1.28.7. The y4m plugin is gone from gst-plugins-bad in 1.28: its y4mdec element moved into the gst-plugins-good y4m plugin, and neither gst/y4m nor the y4m meson option exists here any more. Drop the -Dy4m= argument, which meson would now reject as unknown, together with the gst1-mod-y4mdec package. The element stays available as gst1-mod-y4m from gst-plugins-good. Signed-off-by: W. Michael Petullo <mike@flyn.org> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
Upstream list of changes is available at https://gstreamer.freedesktop.org/releases/1.28/#1.28.7. Signed-off-by: W. Michael Petullo <mike@flyn.org> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
f7220d9 to
25eb66c
Compare
|
Sync-ed with upstream. |
Upstream list of changes is available at https://gstreamer.freedesktop.org/releases/1.28/#1.28.7. Drop 001-gst-libav-Fix-build-failure-with-FFmpeg-9.0.patch. The fix it carries is already in 1.28.7, so quilt refuses to apply it ("can be reverse-applied") and the build fails before it starts. Signed-off-by: W. Michael Petullo <mike@flyn.org> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
The NEON audio resampler has been compiled out of every 32-bit ARM build since 1.28. Its inline asm advances pointers with add.w, a Thumb-2 only mnemonic that gas rejects in ARM (A32) state, which is how OpenWrt builds ARM. The same add.w sits in the meson probe that gates HAVE_ARM_NEON, so the probe fails first and hides the broken asm behind the generic C path. Backport the upstream fix, which selects the operand form per ISA and drops add.w from the probe. Thumb-2 code generation is unchanged. Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
| PKG_NAME:=gst1-libav | ||
| PKG_VERSION:=1.26.4 | ||
| PKG_RELEASE:=2 | ||
| PKG_VERSION:=1.28.7 |
There was a problem hiding this comment.
Applying patch 001-gst-libav-Fix-build-failure-with-FFmpeg-9.0.patch
Hunk #1 FAILED at 2760.
1 out of 1 hunk FAILED -- rejects in file ext/libav/gstavviddec.c
Patch 001-gst-libav-Fix-build-failure-with-FFmpeg-9.0.patch can be reverse-applied
ERROR: package/feeds/packages_ci/gst1-libav failed to build.
That patch, added on master in 66c7c55e for 1.26.4, is MR !12217, which 1.28.7 already contains — so quilt reverse-applies it and all ten build jobs die at package/gst1-libav/refresh before anything compiles. Delete multimedia/gst1-libav/patches/001-gst-libav-Fix-build-failure-with-FFmpeg-9.0.patch in this commit.
Generated by Claude Code
There was a problem hiding this comment.
25eb66c to
b15ce0b
Compare
|
To the best of my knowledge, all matters of gstreamer have been solved; and the update should be good to go. So, if no objections, I will merge this in a couple of days. |
|
Thank you for your work on this! I think we should merge, assuming the build failures are due to the CI system. The failures look to be during the test phase after the build. |
They're CI related |
I think it will be fixed tomorrow as there are scheduled runs for new Docker images. |
|
Thank you, @commodo. |
📦 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