remove native nasm #6263

hgy59 · 2024-10-07T19:56:04Z

Description

remove native nasm, it is a prerequisite in the dev. environment since update to debian 12
remove native yasm
update related Makefiles

Fixes #

Affected Libraries

Direct dependencies

cross/dav1d
cross/ffmpeg*
cross/lame
cross/libaom
cross/libvpx
cross/openh264
cross/svt-av1
cross/x264
cross/x265

Further findings

cross/zstd

Affected Packages

By direct dependencies

ffmpeg*
chromaprint
comskip
tvheadend
imagemagick
mpd
rutorrent

Checklist

Build rule all-supported completed successfully
New installation of package completed successfully
Package upgrade completed successfully (Manually install the package again)
Package functionality was tested
Any needed documentation is updated/created

Type of change

Includes small framework changes

- remove native nasm, it is a prerequisite in the dev. environment since update to debian 12 - remove native yasm - update related Makefiles

This reverts commit 2c4a9cb.

- change affected packages to trigger build

th0ma7

Beside possible issue found with openh264 that your change brought to light, LGTM.

cross/openh264/Makefile

…native_nasm

- update cross/dav1d - remove definitions for nasm, meson build completely auto detects nasm

- remove definitions for nasm, meson build completely auto detects nasm - create optimized code

- cmake builds completely auto detect nasm

hgy59 · 2024-12-17T20:27:52Z

Special NASM definitions for meson and cmake build systems are obsolete and removed now.
The auto configuration for nasm works as expected.

further analysis for gcc builds (spksrc.cross-cc.mk) is WIP.

- disable support for OLD_PPC_ARCHS - build with assembler, except for PPC_ARCHS - remove obsolete configure args --enable-pic is forced with --enable-shared --cross-prefix is not required - remove definition of GNU_CONFIGURE and define --host - simplify definition of gcc as assembler if not using x86asm

- define --as=auto to use yasmm (prefered over nasm) - document why we don't define GNU_CONFIGRE - remove concurring patch - avoid duplicate definition of --disable-neon

hgy59 · 2024-12-20T08:31:09Z

cross/libvpx/Makefile

 ifeq ($(findstring $(ARCH),$(i686_ARCHS)),$(ARCH))
 CONFIGURE_ARGS += --target=x86-linux-gcc
+# --as=auto: prefers yasm over nasm
+CONFIGURE_ARGS += --as=auto
+CONFIGURE_ARGS += --disable-sse4_1


@th0ma7 can you remember why you disabled sse4_1 for intel archs?
it builds just fine without disabling it

This change comes from this PR: #6148
Specifically from this commit: c4c0d44

But I haven't left any descriptions as to why I disabled it? Maybe it failed on DSM6, again using gcc <= 4.9.x for that specific libvpx version (i.e 1.14.1)?

Looking a bit further this may have been a mistake from my part as SSE4.1 (Streaming SIMD Extensions 4.1) has nothing to do with newer AVX extensions and may have mixed them somehow. A bit more reading on it makes it that pretty much all "recent" intel based nas from synology (i.e. less than 10-12 years old) should support this extension...

I believe it is safe to remove that.

hgy59 · 2024-12-20T08:56:06Z

@th0ma7 the libraries and *.mk files are ready for review.

On the ffmpeg* packages there is some pending work...

OLD_PPC_ARCHS fail to build ffmpeg since 4.4.x (4.3.x are the last working releases, newer have issue with stdatomic)
There are some conditional dependencies for alpine comcerto2k and monaco archs.

example:
```
ifeq ($(findstring $(ARCH),alpine comcerto2k monaco $(ARMv8_ARCHS) $(i686_ARCHS) $(x64_ARCHS)),$(ARCH))
DEPENDS += cross/libaom
CONFIGURE_ARGS += --enable-libaom
endif
```
Except for arch-comcerto2k-7.1 we do not build additional packages for alpine and monaco
If we would build dedicated packages for alpine and monaco we would need to exclude those archs in the armv7 generic packages.
Despite we have an option to exclude dedicated archs from generic ones with REMOVE_FROM_GENERIC_ARCHS, we cannot use github build to create and publish dedicated archs.
And we never planned to created a single package for a list of dedicated archs...

hgy59 · 2024-12-20T08:59:39Z

and the third issue is with cross/lame:

even we configure with --enable-asm (and configure finds /usr/bin/nasm) it does not use the assembler.

th0ma7 · 2024-12-29T23:09:34Z

cross/dav1d/Makefile

-CONFIGURE_ARGS += -Denable_asm=true
-
-# Allow ASM on aarch64, disable on all others
-else ifeq ($(findstring $(ARCH),$(ARMv8_ARCHS)),$(ARCH))
-CONFIGURE_ARGS += -Denable_asm=true
-else
-CONFIGURE_ARGS += -Denable_asm=false


nasm is only used for x86/x86_64, while for aarch64 it uses the gcc assembler (as). Considering the code above, you should have kept something such as:

ifeq ($(findstring $(ARCH),$(ARMv8_ARCHS) $(i686_ARCHS) $(x64_ARCHS)),$(ARCH)) CONFIGURE_ARGS += -Denable_asm=true else CONFIGURE_ARGS += -Denable_asm=false endif

While it probably builds ok, I recall code to fail on armv7 in multiple cases which led to enable assembly optimizations only on aarch64 and x86 archs. I would suggest keeping this ifeq else usecase (and maybe elsewhere, haven't looked at all the code changes).

EDIT: On another comment I realized that issues with as on armv7 related to gcc <= 4.9, rather suggesting:

include ../../mk/spksrc.common.mk ifeq ($(findstring $(ARCH),$(ARMv8_ARCHS) $(i686_ARCHS) $(x64_ARCHS)),$(ARCH)) CONFIGURE_ARGS += -Denable_asm=true # only enable `asm` on armv7 when using gcc > 4.9.* (i.e. DSM7+) else if ($(findstring $(ARCH),$(ARMv7_ARCHS) $(ARMv7L_ARCHS)),$(ARCH)) ifeq ($(call version_lt, ${TC_GCC}, 5),1) CONFIGURE_ARGS += -Denable_asm=true endif else CONFIGURE_ARGS += -Denable_asm=false endif

th0ma7 · 2024-12-29T23:10:55Z

cross/ffmpeg4/Makefile

+NASM_BINARY = $(shell which nasm)
+ifeq ($(NASM_BINARY),)
+$(error nasm not found. Please intall NASM assembler)
+endif
+ENV += AS=$(NASM_BINARY)


Is that test even needed? I did not bother with that for ffmpeg5-6-7, maybe worth either using it everywhere or I would suggest not at all.

th0ma7 · 2024-12-29T23:19:28Z

cross/libvpx/Makefile

 ADDITIONAL_CXXFLAGS = -std=c++0x
 endif

 ifeq ($(findstring $(ARCH),$(ARMv7_ARCHS) $(ARMv7L_ARCHS)),$(ARCH))
 CONFIGURE_ARGS += --target=armv7-linux-gcc
-CONFIGURE_ARGS += --disable-neon


Disabling of neon was done in this PR #5620
This was the comit: 6ad7f5f

So from the commit log it states disabling neon (i.e. which includes as optimizations) on armv7 when using a compiler <= 4.9, basically on dsm6. I suggest the following change:

include ../../mk/spksrc.common.mk ifeq ($(findstring $(ARCH),$(ARMv7_ARCHS) $(ARMv7L_ARCHS)),$(ARCH)) CONFIGURE_ARGS += --target=armv7-linux-gcc ifeq ($(call version_lt, ${TC_GCC}, 5),1) CONFIGURE_ARGS += --disable-neon endif endif

th0ma7 · 2024-12-29T23:28:38Z

cross/libvpx/Makefile

 ifeq ($(findstring $(ARCH),$(i686_ARCHS)),$(ARCH))
 CONFIGURE_ARGS += --target=x86-linux-gcc
+# --as=auto: prefers yasm over nasm
+CONFIGURE_ARGS += --as=auto
+CONFIGURE_ARGS += --disable-sse4_1


This change comes from this PR: #6148
Specifically from this commit: c4c0d44

But I haven't left any descriptions as to why I disabled it? Maybe it failed on DSM6, again using gcc <= 4.9.x for that specific libvpx version (i.e 1.14.1)?

Looking a bit further this may have been a mistake from my part as SSE4.1 (Streaming SIMD Extensions 4.1) has nothing to do with newer AVX extensions and may have mixed them somehow. A bit more reading on it makes it that pretty much all "recent" intel based nas from synology (i.e. less than 10-12 years old) should support this extension...

I believe it is safe to remove that.

th0ma7 · 2024-12-29T23:37:58Z

cross/openh264/Makefile

@@ -15,14 +15,10 @@ LICENSE  = BSD
 UNSUPPORTED_ARCHS = $(PPC_ARCHS)

 CONFIGURE_ARGS += -Dtests=disabled
+ADDITIONAL_CPPFLAGS = -O


Why? is it to diable optimizations (i.e. -O0)

th0ma7 · 2024-12-29T23:40:22Z

cross/x264/Makefile

+# Define x86asm or use gcc as assembler
+ifeq ($(findstring $(ARCH),$(i686_ARCHS) $(x64_ARCHS)),$(ARCH))
+NASM_BINARY = $(shell which nasm)
+ifeq ($(NASM_BINARY),)
+$(error nasm not found. Please install NASM assembler)
+endif
+ENV += AS=$(NASM_BINARY)
+else


Similarly to ffmpeg, isn't this too much as nasm is always installed?
If anything, I'd rather have that test being done outside from individual cross but within the framework to avoid code duplication if feasible.

th0ma7 · 2024-12-29T23:49:17Z

mk/spksrc.cross-cmake.mk

-ifeq ($(strip $(CMAKE_USE_NASM)),1)
-ifeq ($(findstring $(ARCH),$(i686_ARCHS) $(x64_ARCHS)),$(ARCH))
-	@echo "# set assembly compiler" ; \
-	echo "set(ENABLE_ASSEMBLY $(ENABLE_ASSEMBLY))" ; \
-	echo "set(CMAKE_ASM_COMPILER $(CMAKE_ASM_COMPILER))" ; \
-	echo
-endif
-endif


This is rather interesting... indeed cmake should auto-detect nasm as being in default PATH.

th0ma7 · 2024-12-29T23:58:45Z

@th0ma7 the libraries and *.mk files are ready for review.

I did a first pass, overall looking good. However I believe it does require minor adjustments for armv7 with gcc <= 4.9.x pertaining to assembly optimizations (i.e. neon).

On the ffmpeg* packages there is some pending work...

OLD_PPC_ARCHS fail to build ffmpeg since 4.4.x (4.3.x are the last working releases, newer have issue with stdatomic)

I would suggest removing OLD_PPC_ARCHS from all ffmpeg makefiles and declaring it as unsupported.

There are some conditional dependencies for alpine comcerto2k and monaco archs.
Except for arch-comcerto2k-7.1 we do not build additional packages for alpine and monaco
If we would build dedicated packages for alpine and monaco we would need to exclude those archs in the armv7 generic packages.
Despite we have an option to exclude dedicated archs from generic ones with REMOVE_FROM_GENERIC_ARCHS, we cannot use github build to create and publish dedicated archs.
And we never planned to created a single package for a list of dedicated archs...

Wow, that brings me a while back... when I took over the ffmpeg maintenance (which I didn't new what I was up against), I thought we absolutely needed arch specific packages for acceleration purposes... Not totally wrong but unsustainable as we a) don't have the manpower to support that (and not sure of the benefits) and b) don't have the storage space to do so.

Honestly this can be cleaned-up, it's just crap left-over. Good catch thoughg 🤷

th0ma7 · 2024-12-30T00:25:47Z

and the third issue is with cross/lame:

even we configure with --enable-asm (and configure finds /usr/bin/nasm) it does not use the assembler.

I tried enabling the following options to check if nasm would be used somewhere, but nothing (btw, nevertless, mp3rtp could be enabled with the addition of bin/mp3rtp to the PLIST):

CONFIGURE_ARGS  = --enable-mp3rtp
CONFIGURE_ARGS += --enable-shared
CONFIGURE_ARGS += --enable-dynamic-frontends

Looking at arch linux and gentoo they both enable nasm...? Even default debian rules files enables it. btw lame-3.100/debian/rules uses the following:

                --with-fileio=sndfile \
                --enable-nasm \
                --with-pic \
                --disable-mp3x \
                --disable-mp3rtp \
                --disable-gtktest \
                --enable-dynamic-frontends \
                --enable-expopt=full

Tried them all, nothing shows up from a nasm usage ... So I tried compiling on the host to see what hapens, and it does enter into libmp3lame/i386 where the assembly files are but does nothing:

Making all in libmp3lame
make[2]: Entering directory '/home/spksrc/python-wheels4/spksrc/distrib/lame-3.100/libmp3lame'
Making all in i386 
make[3]: Entering directory '/home/spksrc/python-wheels4/spksrc/distrib/lame-3.100/libmp3lame/i386'
make[3]: Nothing to be done for 'all'.
make[3]: Leaving directory '/home/spksrc/python-wheels4/spksrc/distrib/lame-3.100/libmp3lame/i386'

Honestly, I don't believe it ever worked or I'm missing something?

hgy59 added 5 commits October 7, 2024 21:55

remove native nasm

7889de7

- remove native nasm, it is a prerequisite in the dev. environment since update to debian 12 - remove native yasm - update related Makefiles

temp add output to analyze prepare action

2c4a9cb

Revert "temp add output to analyze prepare action"

d880967

This reverts commit 2c4a9cb.

avoid variable in DEPENDS

448c879

- change affected packages to trigger build

revert unintended changes

52b31b8

hgy59 requested a review from th0ma7 October 9, 2024 19:05

th0ma7 requested changes Oct 9, 2024

View reviewed changes

cross/openh264/Makefile Outdated Show resolved Hide resolved

hgy59 added 4 commits December 17, 2024 18:24

Merge branch 'master' of https://github.com/hgy59/spksrc into remove_…

db7c4ba

…native_nasm

cross/dav1d: update from v1.4.2 to v1.4.3

e5b50fa

- update cross/dav1d - remove definitions for nasm, meson build completely auto detects nasm

cross/openh264: remove nasm definitions

db9b00f

- remove definitions for nasm, meson build completely auto detects nasm - create optimized code

remove CMAKE_USE_NASM

f7381e6

- cmake builds completely auto detect nasm

hgy59 added 2 commits December 19, 2024 19:37

update build of cross/libvpx

49d2ca7

- define --as=auto to use yasmm (prefered over nasm) - document why we don't define GNU_CONFIGRE - remove concurring patch - avoid duplicate definition of --disable-neon

hgy59 commented Dec 20, 2024

View reviewed changes

th0ma7 requested changes Dec 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove native nasm #6263

remove native nasm #6263

hgy59 commented Oct 7, 2024 •

edited

Loading

th0ma7 left a comment

hgy59 commented Dec 17, 2024

hgy59 Dec 20, 2024

th0ma7 Dec 29, 2024

hgy59 commented Dec 20, 2024

hgy59 commented Dec 20, 2024

th0ma7 Dec 29, 2024

th0ma7 Dec 29, 2024

th0ma7 Dec 29, 2024

th0ma7 Dec 29, 2024

th0ma7 Dec 29, 2024

th0ma7 Dec 29, 2024

th0ma7 Dec 29, 2024

th0ma7 commented Dec 29, 2024

th0ma7 commented Dec 30, 2024

remove native nasm #6263

Are you sure you want to change the base?

remove native nasm #6263

Conversation

hgy59 commented Oct 7, 2024 • edited Loading

Description

Affected Libraries

Affected Packages

Checklist

Type of change

th0ma7 left a comment

Choose a reason for hiding this comment

hgy59 commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hgy59 commented Dec 20, 2024

hgy59 commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

th0ma7 commented Dec 29, 2024

th0ma7 commented Dec 30, 2024

hgy59 commented Oct 7, 2024 •

edited

Loading