aizvorski / h264bitstream

A complete set of functions to read and write H.264 video bitstreams, in particular to examine or modify headers.
GNU Lesser General Public License v2.1
713 stars 237 forks source link

Buildscripts/automake for GitHub actions #51

Closed LostInKadath closed 8 months ago

LostInKadath commented 8 months ago

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.

aizvorski commented 8 months ago

@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 commented 8 months ago

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?

LostInKadath commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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