Skip to content

MSYS2 build of SVT JPEG XS codec #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

vasilich-tregub
Copy link

The edits here enable use of MSYS2 build environment for SVT JPEG XS codec repository. The base repository recommends use of MSYS2 to build FFMPEG with libsvtjpegxs enabled; in the process, SVT-JPEG-XS has to be built in the MSYS2 environment.

A lot of lines in the root CMakeLists.txt and Source/Lib/Common/ASM_SSE2/x64inc.asm had to be edited to enable this build with MSYS2 (UCRT) terminal. In this fork, I made the necessary edits and edited the related sections of the README files. For convenience of users with MSVC background, the sections describing Linux and MSYS builds are written assuming the fresh installs of these systems. I expanded the section on building the project in the MSYS2 (UCRT) environment and transferred this section from the ffmpeg-plugin README to the root README. Also, the test file 200.jxs from ISO_IEC 21122 Part 4 test files is added to the repo for the expedient testing of a sample decoder.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this file jxs-test-files/200.jxs

@vasilich-tregub
Copy link
Author

vasilich-tregub commented Mar 18, 2025 via email

@tszumski
Copy link
Collaborator

Nothing special, I simply documented my workflow as it was saved in the console stack. You are right, the use of Build/linux/build.sh <release | debug> is more clear and straightforward. Should I edit my text per your comment or would you please do it?

Please do it

@vasilich-tregub
Copy link
Author

vasilich-tregub commented Mar 18, 2025 via email

%elifidn __OUTPUT_FORMAT__,win64
%define WIN64 1
%elifidn __OUTPUT_FORMAT__,x64
%define WIN64 1
Copy link
Collaborator

@tszumski tszumski Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for modifying/commenting-out x64inc.asm ? Is it necessary?

@vasilich-tregub
Copy link
Author

vasilich-tregub commented Mar 19, 2025 via email

@tszumski
Copy link
Collaborator

tszumski commented Mar 19, 2025

CMakeLists.txt of your repo DEFINEs symbols WIN64 [line 105: set(CMAKE_ASM_NASM_FLAGS "${CMAKE_ASM_NASM_FLAGS} -DWIN64")] and UNIX64 [line 107: set(CMAKE_ASM_NASM_FLAGS "${CMAKE_ASM_NASM_FLAGS} -DUNIX64")]. Notice that the very first statements in Source/Lib/ASM_SSE2/x64inc.asm starts UNDEFINE these symbols, in line 12: %undef WIN64 and in line 13: %undef UNIX64. After that, one of these symbols is defined anew, depending on the *value *of variable OUTPUT_FORMAT. Now, OUTPUT_FORMAT in x64inc.asm is defined by a *host *environment (where the compiler and linker work)) and not by a *target *environment (where the code will be used). It may seem unusual, but MSYS2 build system should be considered a 'cross compiler' even when MSYS2 builds for the Windows platform. And while this technique (cross compilation) may appear to be imposing unnecessary complications, it is no more nuisance than the existence of multiple platforms and architectures (UNIX, Windows, ARM64) per se. Think of the development tools for embedded systems, they inherently require 'cross-compilers'. Back to my edit of x64inc.asm: instead of unearthing CMake-related, assembler, or other variables suitable for passing the target environment variables to preprocessor and on, I assigned the symbols WIN64 and UNIX64 their values in the root CMakeLists.txt, like set(CMAKE_ASM_NASM_FLAGS "${CMAKE_ASM_NASM_FLAGS} -DWIN64=1 -DUNIX64=0") in one branch and switching 1 and 0 in another. In fact, I brought back the intentions of original developers of this *.asm file: now you need not undefine the symbols, you just use these as they are passed from CMakeLists.txt. Only do not forget to replace ifdef-s with ifidn-s for these symbols. It reminds me of your original question about 'benefits of cross-compilers (like MSYS) vs. native tools (like MSVC for Windows, gnu-compilers for NIX-es): indeed, there exists a real benefit of building with two different build systems for the same platform. You see how this procedure can reveal some intricate details of code behaviour and strengthen the code health.

Thank you for contribution and detailed explanation, Let me test your changes(it may take a few days)

@vasilich-tregub
Copy link
Author

vasilich-tregub commented Mar 20, 2025 via email

README.md Outdated
- __You are ready to build the SVT-JPEG-XS package__
- `git clone https://github.com/OpenVisualCloud/SVT-JPEG-XS.git`
- `cd SVT-JPEG-XS`
- `cd Build/linux/build.sh <release | debug>`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cd Build/linux && build.sh <release | debug>

@vasilich-tregub vasilich-tregub marked this pull request as ready for review March 25, 2025 10:57
@vasilich-tregub
Copy link
Author

Added (maybe excessively detailed) readme-s. The gist for building with universal c runtime in MSYS2 is to always work with UCRT64 terminal and install only packets and tools from mingw-w64-ucrt-x86_64 'namespace', the only exception maybe is the 'git' tool. In doing so, you'll never receive messages like 'Native MSYS builds are discouraged, please use the MINGW environment'. Added the folder ffmpeg_enable-libsvtjpegxs explaining MSYS2- and WDL2-builds plus data for FFMPEG plugin

@tszumski
Copy link
Collaborator

Hi @vasilich-tregub , unfortunately I cannot accept whole ffmpeg repository here (folder ffmpeg_enable-libsvtjpegxs)

…eplaced my fancy ci/cd instructions with standard ci/cd patch instructions of ffmpeg-plugin
@vasilich-tregub
Copy link
Author

vasilich-tregub commented Mar 28, 2025 via email

@vasilich-tregub
Copy link
Author

vasilich-tregub commented Mar 28, 2025 via email

@tszumski
Copy link
Collaborator

tszumski commented Apr 1, 2025

Thank you @vasilich-tregub for your contribution, readme/instruction looks fine, just one more question is CMakeSettings.json file required ? I do not see any reference in readme to that file

@vasilich-tregub
Copy link
Author

vasilich-tregub commented Apr 1, 2025 via email

@tszumski
Copy link
Collaborator

tszumski commented Apr 1, 2025

Thanks,
@MateuszGrabuszynski can you check this PR in your spare time ?

@MateuszGrabuszynski
Copy link
Collaborator

Hi, I will do my best to test it in the beginning of the next week (April 7th or 8th). Sorry for that delay.

@vasilich-tregub
Copy link
Author

vasilich-tregub commented Apr 1, 2025 via email

@vasilich-tregub
Copy link
Author

vasilich-tregub commented Apr 4, 2025 via email

@vasilich-tregub
Copy link
Author

vasilich-tregub commented Apr 5, 2025 via email

@vasilich-tregub
Copy link
Author

vasilich-tregub commented Apr 6, 2025 via email

@tszumski
Copy link
Collaborator

tszumski commented Apr 7, 2025

Hi @vasilich-tregub , few comments:

  1. Can you please split your changes into multiple PR ? When you issue a PR, you should not add new features; instead, you should only make corrections.
  2. Could you please put yout comments directly in github website ? Your comments/replies via email are unreadable in github notifications ?

@tszumski
Copy link
Collaborator

tszumski commented Apr 7, 2025

Please create another PR with FFmpeg 7.1 patches(and revert changes in this PR). Patches should follow the same scheme as for 6.1/7.0, do not to create single large patch-file.

@vasilich-tregub
Copy link
Author

vasilich-tregub commented Apr 8, 2025 via email

@tszumski
Copy link
Collaborator

tszumski commented Apr 8, 2025

Hi @vasilich-tregub,
With regards to ffmpeg patches:

  1. Create another subdirecotry called 7.1
  2. Modify 3 patches that should land there (based on 6.1/7.0)
  3. Keep in mind that libsvtjpegxsenc.c and libsvtjpegxsdec.c should not be include in patch itself. This way in case of any error in plugin implementation, single change in that files will fix all ffmpeg revisions

With regards to multiple PR:
Right now you have pushed 33 commits, the only way to "merge" it is to squash all of them into single commit. You touched multiple aspect of code. My Ask is to split your PR into multiple PR, or cleanup your commits, and leave just single commit for each type of change (this way I will be able to merge them without squashing)
I see at least 3 separate changes:

  1. Fix MSYS2 build
  2. Fix CMake version 4.0 ()
  3. Add support for FFmpeg 7.1

Also once again I am asking you to not to reply via email, thats how github notification looks like:
image

Copy link
Collaborator

@MateuszGrabuszynski MateuszGrabuszynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review - 6 out of 11 files viewed.

Hello,

Thank you for the contribution. I am sorry, but I stopped somewhere in the middle. I am not able to spend more time on this PR. The change is not going to be accepted in the current form. Some of the issues I found are:

  1. The PR message mentions MSYS2 build of SVT JPEG XS - but the change does cover a lot more - WSL builds, patches to FFmpeg 7.1, etc.

  2. Those patches to FFmpeg 7.1 are not to be inserted in the patch file (please see the associated comment). In my opinion, the proper folder structure is a must as well. The folders and files for FFmpeg 6.1 and 7.0 are already provided there, please follow your changes to be done in the same manner.

  3. Mostly subjective, but the instructions are a bit too chaotic for my liking - the installation process should cover the bare minimum version, not adding or removing any FFmpeg dependencies, like AV1 (dav1d). Also, if something can be done using pacman alone, I do not see a need to install pacboy.

  4. And the most important, I have tried to build the JPEG XS according to the provided instructions, but am not able to, encountering a following error:

    Logs
    $ cmake --build ~/install-dir/builddebug -j4 --config Debug --target install
    System is unknown to cmake, create:
    Platform/MINGW64_NT-10.0-22631 to use this system, please post your config file on discourse.cmake.org so it can be added to cmake
    Your CMakeCache.txt file was copied to CopyOfCMakeCache.txt. Please post that file on discourse.cmake.org.
    -- BUILD_SHARED_LIBS: OFF
    (...)
    
    CMake Warning at third_party/cpuinfo/CMakeLists.txt:61 (MESSAGE):
      Target processor architecture "unknown" is not supported in cpuinfo.
      cpuinfo will compile, but cpuinfo_initialize() will always fail.
    
    
    CMake Warning at third_party/cpuinfo/CMakeLists.txt:74 (MESSAGE):
      Target operating system "MINGW64_NT-10.0-22631" is not supported in
      cpuinfo.  cpuinfo will compile, but cpuinfo_initialize() will always fail.
    
    
    -- Configuring done (0.4s)
    -- Generating done (0.6s)
    (...)
    [ 84%] Building C object Source/Lib/Encoder/Codec/CMakeFiles/ENCODER_CODEC.dir/encoder_dsp_rtcd.c.obj
    [ 84%] Built target DECODER_CODEC
    [ 85%] Building C object Source/App/SampleDecoder/CMakeFiles/SvtJpegxsSampleDecoder.dir/main.c.obj
    [ 87%] Linking C executable /home/usr123/jxs-ffmpeg-8/SVT-JPEG-XS/Bin/Debug/SvtJpegxsSampleDecoder
    [ 87%] Built target ENCODER_CODEC
    C:/msys64/ucrt64/bin/../lib/gcc/x86_64-w64-mingw32/14.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: ../../../third_party/cpuinfo/CMakeFiles/cpuinfo.dir/src/init.c.obj:init.c:(.rdata$.refptr.cpuinfo_x86_windows_init[.refptr.cpuinfo_x86_windows_init]+0x0): undefined reference to `cpuinfo_x86_windows_init'
    C:/msys64/ucrt64/bin/../lib/gcc/x86_64-w64-mingw32/14.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: ../../Lib/Common/Codec/CMakeFiles/COMMON_CODEC.dir/common_dsp_rtcd.c.obj:common_dsp_rtc:(.rdata$.refptr.cpuinfo_isa[.refptr.cpuinfo_isa]+0x0): undefined reference to `cpuinfo_isa'
    [ 88%] Linking C static library /home/usr123/jxs-ffmpeg-8/SVT-JPEG-XS/Bin/Debug/libSvtJpegxs.a
    collect2.exe: error: ld returned 1 exit status
    [ 89%] Building C object Source/App/SampleEncoder/CMakeFiles/SvtJpegxsSampleEncoder.dir/main.c.obj
    make[2]: *** [Source/App/SampleDecoder/CMakeFiles/SvtJpegxsSampleDecoder.dir/build.make:138: /home/usr123/jxs-ffmpeg-8/SVT-JPEG-XS/Bin/Debug/SvtJpegxsSampleDecoder] Error 1
    make[1]: *** [CMakeFiles/Makefile2:866: Source/App/SampleDecoder/CMakeFiles/SvtJpegxsSampleDecoder.dir/all] Error 2
    make[1]: *** Waiting for unfinished jobs....
    [ 90%] Linking C executable /home/usr123/jxs-ffmpeg-8/SVT-JPEG-XS/Bin/Debug/SvtJpegxsSampleEncoder
    C:/msys64/ucrt64/bin/../lib/gcc/x86_64-w64-mingw32/14.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: ../../../third_party/cpuinfo/CMakeFiles/cpuinfo.dir/src/init.c.obj:init.c:(.rdata$.refptr.cpuinfo_x86_windows_init[.refptr.cpuinfo_x86_windows_init]+0x0): undefined reference to `cpuinfo_x86_windows_init'
    C:/msys64/ucrt64/bin/../lib/gcc/x86_64-w64-mingw32/14.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: ../../Lib/Common/Codec/CMakeFiles/COMMON_CODEC.dir/common_dsp_rtcd.c.obj:common_dsp_rtc:(.rdata$.refptr.cpuinfo_isa[.refptr.cpuinfo_isa]+0x0): undefined reference to `cpuinfo_isa'
    collect2.exe: error: ld returned 1 exit status
    make[2]: *** [Source/App/SampleEncoder/CMakeFiles/SvtJpegxsSampleEncoder.dir/build.make:149: /home/usr123/jxs-ffmpeg-8/SVT-JPEG-XS/Bin/Debug/SvtJpegxsSampleEncoder] Error 1
    make[1]: *** [CMakeFiles/Makefile2:832: Source/App/SampleEncoder/CMakeFiles/SvtJpegxsSampleEncoder.dir/all] Error 2
    [ 90%] Built target SvtJpegxsLib
    make: *** [Makefile:136: all] Error 2
    

I do not think that splitting into multiple PRs is reasonable: this repo is
already (using git parlance) many 'commits' behind ISO21122-ed3, we should
move forward as soon as possible.

Is this exact change adding anything to the repository's ISO compliance?


Let me, however, note that the
21122 standard and consequently its SVT-JPEG-XS implementation are in their
experimental phase, so we can have a quite experienced developer (as user)
in mind.

Either way, we should not give up on clarity and maintainability. After the PR is merged, we do not know when it will be changed again, so (if possible) we should prepare it in the best possible way, from the very start.


Also, let me note that I filled in the PR template form with the option
'Allow edits from maintainers' set. You, the maintainer, can select from my
fork only items that you approve for merge, and also edit them any way you
like.

We are aware of that option. :) We can split the PR into multiple ones ourselves, but please keep in mind, changing anything from our end requires more time.

@@ -94,20 +94,22 @@ set(CMAKE_CXX_STANDARD 11)

# BUILD_SHARED_LIBS is a standard CMake variable, but we declare it here to
# make it prominent in the GUI.
option(BUILD_SHARED_LIBS "Build shared libraries (DLLs)." ON)
option(BUILD_SHARED_LIBS "Build shared libraries (DLLs)." OFF)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[q] BUILD_SHARED_LIBS is turned OFF, and there is never a way to turn it back ON. Is that an expected behavior?

else()
set(CMAKE_ASM_NASM_FLAGS "${CMAKE_ASM_NASM_FLAGS} -DUNIX64")
set(CMAKE_ASM_NASM_FLAGS "${CMAKE_ASM_NASM_FLAGS} -DUNIX64=1 -DWIN64=0")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] Keep the same order for readability

Suggested change
set(CMAKE_ASM_NASM_FLAGS "${CMAKE_ASM_NASM_FLAGS} -DUNIX64=1 -DWIN64=0")
set(CMAKE_ASM_NASM_FLAGS "${CMAKE_ASM_NASM_FLAGS} -DWIN64=0 -DUNIX64=1")

Comment on lines -214 to +220
set(CMAKE_ASM_NASM_FLAGS_DEBUG "${CMAKE_ASM_NASM_FLAGS_DEBUG} -gcv8")
if(MSVC)
set(CMAKE_ASM_NASM_FLAGS_DEBUG "${CMAKE_ASM_NASM_FLAGS_DEBUG} -gcv8")
else()
set(CMAKE_ASM_NASM_FLAGS_DEBUG "${CMAKE_ASM_NASM_FLAGS_DEBUG} -gcv8")
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[q] Is it if something do x, else do x case? I do not see any difference.
image

MSYS2build.md Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand, this is supposed to cover building with MSYS2 on Windows, but the doc is much longer and covers supported formats and Linux-related topics as well. Please avoid duplicating information that is already in the main README.md file.

@@ -65,6 +65,18 @@ Supported OS versions:
- __API headers location__
- API headers can be found under `Source/API`

### Windows Services for Linux
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### Windows Services for Linux
### Windows Subsystem for Linux (WSL)

Comment on lines +65 to +66
sudo apt install libdav1d-dev
sudo apt install libsdl2-dev
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sudo apt install libdav1d-dev
sudo apt install libsdl2-dev
sudo apt install libdav1d-dev libsdl2-dev

Comment on lines +68 to +69
The library libsvtav1 can also be apt installed, only you have to install two
packages:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The library libsvtav1 can also be apt installed, only you have to install two
packages:
If checking out to version 7.1 of FFmpeg (`release/7.1`), then install additional packages:

```
sudo apt install libsvtav1-dev libsvtav1enc-dev
```
You only may need the libsvtav1 codec if you plan to checkout ffmpeg to `release/7.1`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
You only may need the libsvtav1 codec if you plan to checkout ffmpeg to `release/7.1`.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strictly do not agree with that approach.

  1. This patch file should be placed under a proper version folder (ffmpeg-plugin/7.1/0001-commit-to-enable-libsvtjpegxs.patch), to avoid confusion.
  2. There is a way to use the libavcodec/libsvtjpegxsdec.c and libavcodec/libsvtjpegxsenc.c files without the need of copying their contents into the patch. Although it is more manual, it avoids misalignment between the commits. Modifying anything inside those two C files would not propagate into the patch. A mirror of the change inside the patch file would be required, which is unwanted.

--disable-doc --disable-everything --disable-network \
--enable-libdav1d --enable-libsvtjpegxs \
--enable-decoder='aac,ac3,h264,hevc,libdav1d,libsvtjpegxs' \
--enable-encoder='aac,ac3,wrapped_avframe,libsvtjpegxs'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
--enable-encoder='aac,ac3,wrapped_avframe,libsvtjpegxs'
--enable-encoder='aac,ac3,wrapped_avframe,libsvtjpegxs' \

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MateuszGrabuszynski,

All your comments are fair and useful; I'm working on satisfying your requirements, but the test of build-ability is paramount for my work. To begin with, I edited the lines

50 MESSAGE(STATUS "CMAKE_SYSTEM_PROCESSOR is "${CMAKE_SYSTEM_PROCESSOR}"")

and

66 MESSAGE(STATUS "CMAKE_SYSTEM_NAME is "${CMAKE_SYSTEM_NAME}"")

of SVT-JPEG-XS/third_party/cpuinfo/CMakeLists.txt (in the original CMakeLists, lines 50 and 66 are blank), and ran

$ cmake -S . -B builddebug -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=OFF -DCMAKE_INSTALL_PREFIX=$HOME/install-dir > generatebuilddebug.log

in MSYS2 UCRT64 terminal. It reports

...
-- C:/msys64/home/User/SVT-JPEG-XS/Source/Lib/Decoder/ASM_AVX512 CXXFLAGS: -Wall -Wextra -Wformat -Wformat-security -mxsave -fno-asynchronous-unwind-tables -Wno-unused-parameter -mavx512f -mavx512bw -mavx512dq -mavx512vl
-- CMAKE_SYSTEM_PROCESSOR is "AMD64"
-- CMAKE_SYSTEM_NAME is "Windows"
-- Configuring done (32.4s)
...

while your log has

CMake Warning at third_party/cpuinfo/CMakeLists.txt:61 (MESSAGE):
Target processor architecture "unknown" is not supported in cpuinfo.
cpuinfo will compile, but cpuinfo_initialize() will always fail.

CMake Warning at third_party/cpuinfo/CMakeLists.txt:74 (MESSAGE):
Target operating system "MINGW64_NT-10.0-22631" is not supported in
cpuinfo. cpuinfo will compile, but cpuinfo_initialize() will always fail.

i.e., CMAKE_SYSTEM_PROCESSOR is "unknown" and CMAKE_SYSTEM_NAME is "MINGW64_NT-10.0-22631".

I'm struggling to find out how your system would have come up with these entirely different values for CMAKE_SYSTEM_PROCESSOR and CMAKE_SYSTEM_NAME variables this place. The SVT-JPEG-XS sources never define these variables explicitly. I gather, their values could come from the toolset.

I'm investigating hypothetical scenarios of your possible MSYS2 install and preliminary actions that may result in this disagreement. Maybe you have any ideas? What cmake flavor does your pacman -Q show? does it show mingw-w64-ucrt-x86_64-cmake 3.31.6-1 or mingw-w64-ucrt-x86_64-cmake <other version> or what? I installed toolchain and cmake two weeks ago in the MSYS2 UCRT64 terminal with commands

pacboy -S toolchain:u
pacboy -S cmake:u

aiming intentionally to delegate decision making, on what versions and triplets is to be installed, to the MSYS2 UCRT64 terminal. 'Make things, especially with installs, as simple as it is reasonable'.

It is important that the toolchain components and cmake are all from the 'mingw-w64-ucrt-x86_64-' triplet. I see that your ld.exe is of UCRT64 toolchain but cannot understand what cmake you use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants