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

Buildscripts/automake for GitHub actions #51

Conversation

LostInKadath
Copy link
Contributor

This PR adds YML-script for building and testing the project on Ubuntu and MacOS. It also recombines sources putting them into src folder.

Also the distclean building step fixed -- some artifacts created by configure-script were not removed; that is not recommended by the official YML-manual.

The tests on MacOS build fail:

--- samples/x264_test.out	2023-11-05 13:03:26.000000000 +0000
+++ tmp2.out	2023-11-05 13:04:26.000000000 +0000
@@ -135,7 +135,7 @@
 7.5: sh->drpm.memory_management_control_operation[ n ]: 6 
 8.8: sh->drpm.long_term_frame_idx[ n ]: 0 
 8.7: sh->drpm.memory_management_control_operation[ n ]: 15 
-9.6: sh->drpm.memory_management_control_operation[ n ]: 182783 
+9.6: sh->drpm.memory_management_control_operation[ n ]: 182784 
 17.5: sh->cabac_init_idc: 0 
 17.4: sh->slice_qp_delta: 0 
 17.3: sh->disable_deblocking_filter_idc: 0 

That's not because of the PR -- maybe it's a bug and requires an issue.

- libtoolize can work improperly with non-Unix EOLs:
https://stackoverflow.com/a/51874066/3528675
- for Ubuntu and MacOS
- obsolete AC_PROG_LIBTOOL -> LT_INIT
- obsolete AC_PROG_LD -> LT_PATH_LD
- libh264bitstream.pc.in and libh264bitstream-uninstalled.sh are generated by configure and should be deleted by distclean
https://www.gnu.org/software/automake/manual/html_node/Clean.html
@aizvorski
Copy link
Owner

aizvorski commented Nov 5, 2023

@LostInKadath That's a very nice PR! I'd like to merge it right away

Just one question, are the file renames to src/ necessary or just convenient? Would prefer to keep the source top level, that's been a style choice for this project since a long time ago. Some very widely used C programs do this https://github.com/bminor/bash and some use src/ https://github.com/coreutils/coreutils so there isn't a single standard

@LostInKadath
Copy link
Contributor Author

are the file renames to src/ necessary or just convenient?

No, it's just convenient for me, I have another experience working with sources on top level. I'll revert this commit in a moment, no problem.

Could you please answer a couple of questions? I'm now working on a crossplatform build with CMake. I have another branch in my forked repo, where I try to run your diff-tests on Ubuntu (gcc and clang) and Windows (cl, that's main Visual Studio compiler). And autotests also fail on Windows with the same diff, on the same param check:

--- samples/x264_test.out	2023-11-05 13:03:26.000000000 +0000
+++ tmp2.out	2023-11-05 13:04:26.000000000 +0000
@@ -135,7 +135,7 @@
 7.5: sh->drpm.memory_management_control_operation[ n ]: 6 
 8.8: sh->drpm.long_term_frame_idx[ n ]: 0 
 8.7: sh->drpm.memory_management_control_operation[ n ]: 15 
-9.6: sh->drpm.memory_management_control_operation[ n ]: 182783 
+9.6: sh->drpm.memory_management_control_operation[ n ]: 182784 
 17.5: sh->cabac_init_idc: 0 
 17.4: sh->slice_qp_delta: 0 
 17.3: sh->disable_deblocking_filter_idc: 0 

Moreover, tests also fail while building with clang, but have no errors in case of gcc. That's a bit weird, I'm pretty sure there's an implementation defined behavior somewhere in the code.

So, how did you get your test files and their outputs? How do you know they are correct?

And would you like to have a crossplatform CMake build file, or should it be Automake for Unix and something else for Windows?

This reverts commit d3ae487.
@LostInKadath
Copy link
Contributor Author

I've debugged a little and found an arithmetic overflow bug in bs_read_ue() function of bs.h header file:

...
r = bs_read_u(b, i);
r += (1 << i) - 1;

If i==32, the right shift equals the width of the integer type, so the behavior is undefined.

Different compilers react in different ways: https://gcc.godbolt.org/z/3Pbj3zPdP

What behavior was implied here?

@aizvorski aizvorski merged commit 2f804f4 into aizvorski:master Nov 7, 2023
@aizvorski
Copy link
Owner

aizvorski commented Nov 7, 2023

If i==32, the right shift equals the width of the integer type, so the behavior is undefined.

Hello - Thanks for finding the bug/compiler-dependent behavior, that's really good debugging!

I think (1 << 32) was assumed to be zero, but I'll look into it. Opened an issue to track this #52

So, how did you get your test files and their outputs? How do you know they are correct?

It's been a while, but from what I remember we examined them by hand, and for some of them checked against a commercial GUI video analysis tool

@aizvorski
Copy link
Owner

aizvorski commented Nov 7, 2023

And would you like to have a crossplatform CMake build file, or should it be Automake for Unix and something else for Windows?

@LostInKadath A CMake would be great! I'm pretty happy to switch from Automake to CMake, if you feel like writing that

@LostInKadath LostInKadath deleted the buildscripts/automake-for-github-actions branch November 8, 2023 08:53
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.

2 participants