Skip to content
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.

Sign up for GitHub

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

Continuous integration job for Windows/MSVC #52

Merged
merged 8 commits into from
Mar 23, 2023
Merged

Continuous integration job for Windows/MSVC #52

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8435ede
Add Windows build & test to CI
frankplow Mar 22, 2023
653dee3
Add `workflow_dispatch` trigger to CI
frankplow Mar 22, 2023
e7c3997
Windows CI - Use `runs` and `uses` correctly
frankplow Mar 22, 2023
5661a62
Windows CI - Correct project generator path
frankplow Mar 22, 2023
c38dec6
Windows CI - Set up MSVC and modify build flags
frankplow Mar 22, 2023
89d0ac2
Windows CI - Set FFMPEG_PATH per-job and install NASM for Windows build
frankplow Mar 23, 2023
9769798
Windows CI - Place `--enable-ffmpeg` after `--disable-everything` and
frankplow Mar 23, 2023
d277866
Windows CI - Reduce number of threads used running tests
frankplow Mar 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 46 additions & 7 deletions .github/workflows/makefile.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,20 @@ on:
branches: [ main ]
pull_request:
branches: [ main ]
workflow_dispatch:

env:
FFMPEG_PATH: FFmpeg/ffmpeg

jobs:

linux:
name: ${{ matrix.os }}.${{ matrix.compiler.compiler }}
name: Linux.${{ matrix.compiler.compiler }}
strategy:
fail-fast: false
matrix:
compiler:
- { compiler: GNU, CC: gcc }
- { compiler: LLVM, CC: clang}
env:
FFMPEG_PATH: FFmpeg/ffmpeg

runs-on: ubuntu-latest
steps:
Expand All @@ -34,7 +34,46 @@ jobs:
run: cd .. && git clone https://github.com/ffvvc/tests.git tests

- name: Unit test
run: cd .. && python3 tests/tools/ffmpeg.py tests/conformance/passed
run: cd .. && python3 tests/tools/ffmpeg.py --threads 4 tests/conformance/passed

- name: Negative test
run: cd .. && python3 tests/tools/ffmpeg.py --threads 4 tests/conformance/failed || true

windows:
name: Windows.MSVC
runs-on: windows-latest
env:
FFMPEG_PATH: msvc\bin\x64\ffmpeg.exe

steps:
- name: Set up MSVC
uses: ilammy/msvc-dev-cmd@v1

- name: Get FFVS-Project-Generator
run: Invoke-WebRequest -Uri https://github.com/ShiftMediaProject/FFVS-Project-Generator/releases/download/1.11.6/FFVS-Project-Generator_1.11.6_x64.zip -OutFile FFVS-Project-Generator.zip; Expand-Archive FFVS-Project-Generator.zip

- name: Get NASM
run: Invoke-WebRequest -Uri https://github.com/ShiftMediaProject/VSNASM/releases/download/0.9/VSNASM.zip -OutFile VSNASM.zip; Expand-Archive VSNASM.zip; Start-Process .\VSNASM\install_script.bat

- name: Get repository
uses: actions/checkout@v3
with:
path: src/main

- name: Generate visual studio project
run: .\FFVS-Project-Generator\project_generate.exe --rootdir=src/main --disable-everything --enable-ffmpeg --enable-decoder=vvc --enable-parser=vvc --enable-demuxer=vvc --enable-protocol=file --enable-protocol=pipe --enable-encoder=rawvideo --enable-muxer=rawvideo --enable-muxer=md5
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the pr.
Could we use some macros for this?
So Linux and windows can share the same command line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can do, although it is worth noting as I say above FFVS-Project-Generator actually interprets some arguments differently to the unix ./configure script. Specifically:

  • It does not support commas, for example --enable-muxer=rawvideo,md5 does not work.
  • --disable-everything and --disable-all are synonymous, whereas in the unix version --disable-everything disables all components, but not all programs and other aspects of the build.
  • Some command line options such as --disable-asm are not supported.

I actually kept the two separate because of this, but would you still like to try to have some flags shared? Maybe there could be a workflow-wide variable for shared flags and then job-specific variables. I will also put a comment warning anyone editing the workflow about this problem.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for explaining. Considering the difference, we can keep these two command lines.
Seems some cases failed. it may be related to memory issues.
Maybe we can think about reducing worker number on windows. https://github.com/ffvvc/tests/blob/87ae9fed1283b1d101ef0c44609cb3739ed81779/tools/ffmpeg.py#L123

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why do you think WPP_B_Sharp_2 and LTRP_A_ERICSSON_3 are failing? I see the other four fails result from the runner running out of memory, but these appear to complete successfully and then have a md5 mismatch. LTRP_A_ERICSSON_3 failed on my fork as well, where I didn't have any memory issues.

Copy link
Member

Choose a reason for hiding this comment

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

There are two+ issues here.

  1. Memory allocation issue. We need to reduce memory in code. But the current quick solution is to reduce worker threads.
  2. Random failure. It may be related to valgrind issue for SAO filter. #26 and libavcodec/vvc_filter: Fix unintialized memory region at lc->sao_buffer #49 (comment).

Let us fix the first one and merge the PR. Then we can think about how to fix the second one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let us fix the first one and merge the PR. Then we can think about how to fix the second one.

👍 Would you like me to make that change to ffvvc/tests or will you?

Copy link
Member

Choose a reason for hiding this comment

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

Please help do this.
thank you

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changes to enable control over number of threads have been made and PR is open: ffvvc/tests#1.


- name: Build
run: msbuild -p:Configuration="Release" src\main\SMP\ffmpeg.sln

- name: Download Bitstream
uses: actions/checkout@v3
with:
repository: ffvvc/tests
path: tests

- name: Unit test
run: python3 tests/tools/ffmpeg.py --threads 2 tests/conformance/passed

- name: Negtive test
run: cd .. && python3 tests/tools/ffmpeg.py tests/conformance/failed || true
- name: Negative test
run: python3 tests/tools/ffmpeg.py --threads 2 tests/conformance/failed; if ( $? -eq $True ) { throw }