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.

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

Conversation

frankplow
Copy link
Collaborator

@frankplow frankplow commented Mar 23, 2023

This PR adds CI for windows using FFVS-Project-Generator. The steps are generally similar to the existing linux CI jobs, with one significant difference: as FFVS-Project-Generator does not currently support disabling assembly optimisations (see ShiftMediaProject/FFVS-Project-Generator#62), the Windows build is currently performed with assembly enabled and assembled using NASM.

One test (LTRP_A_ERICSSON_3) is actually failing right now, and due to a mismatch rather than a timeout. I don't believe this is an issue with the CI configuration however. I would imagine the issue is in the assembly optimisations, rather than some OS/compiler-specific idiosyncrasy.

The solution generated by FFVS-Project-Generator feels a little fragile to me as I was setting this up. I had to do a fair bit of fiddling – some of the flags are interpreted slightly differently to the ./configure script etc. I think in the long-term it may be better to perform MSVC builds using MSYS, which is also recommended in the FFmpeg compilation guide. This would also allow a matrix of compilers on windows as MinGW's Windows gcc could also be used. For now though, FFVS-Project-Generator seems to be working as a quick-and-dirty solution.

This PR will close #33.

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.

@nuomi2021 nuomi2021 merged commit b617f5c into ffvvc:main Mar 23, 2023
@nuomi2021
Copy link
Member

@frankplow thank you for the PR. Please continue to apply for the GSOC contributor

@frankplow frankplow deleted the windows-ci branch March 23, 2023 15:39
@nuomi2021
Copy link
Member

The solution generated by FFVS-Project-Generator feels a little fragile to me as I was setting this up

The project has one important benefit, it can generate a solution file. so we can use visual studio to debug it.

One test (LTRP_A_ERICSSON_3) is actually [failing right now]

It may be related to uninitialized variables. Not sure valgrind will capture it or not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up windows ci
2 participants