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

Build system improvements and video reading fix for bin/tld.c #38

Open
wants to merge 18 commits into
base: unstable
Choose a base branch
from
Open

Build system improvements and video reading fix for bin/tld.c #38

wants to merge 18 commits into from

Conversation

smvv
Copy link
Contributor

@smvv smvv commented Nov 13, 2012

This build system will enable parallelized builds and tracks changes in header files properly. Changing an header file will result in a rebuild of the library and/or binaries.

The first commit fixes the video reading part of bin/tld.c. Due to incorrect usage of the libav API, it was not possible for me to use the demo. Symptoms were complaining about incorrect nalsizes and not flipping "got_picture" to true. The first commit should resolve that issue.

The undeclared constants are defined in zlib.h. Previously, png.h included
zlib.h, but recent versions of libpng (version >= 1.5) do not include zlib.h
anymore.

This commit will solve the following errors:

lib/io/_ccv_io_libpng.c: In function ‘_ccv_write_png_fd’:
lib/io/_ccv_io_libpng.c:55:23: error: ‘MAX_MEM_LEVEL’ undeclared (first use in
this function)
lib/io/_ccv_io_libpng.c:55:23: note: each undeclared identifier is reported
only once for each function it appears in
lib/io/_ccv_io_libpng.c:63:38: error: ‘Z_BEST_SPEED’ undeclared (first use in
this function)
lib/io/_ccv_io_libpng.c:65:40: error: ‘Z_HUFFMAN_ONLY’ undeclared (first use in
this function)
This commit makes it also possible to build all binaries and the ccv library in
parallel using e.g. make -j4.
@liuliu
Copy link
Owner

liuliu commented Nov 13, 2012

This looks good, I will do a manual merge tonight. BTW, do you know if this builds runs with clang's static analyzer now? I am trying to get it running on the old build sys, but has no luck.

@smvv
Copy link
Contributor Author

smvv commented Nov 13, 2012

Please include these two commits as well: eb13376 and 33e1c65. Without the first patch, the build system does not look on the right location for the libccv.a file. The second commit is just some code cleanup.

Is there a specific reason why you do not have -Wextra and -fPIC in CFLAGS? Using -fPIC allows us to create shared libraries, instead of compiling static builds. Currently, the size of each binary in bin/ is >2.0mb, which is rather large.

I tried "scan-build make" but it says "scan-build: Removing directory '/tmp/scan-build-2012-11-13-1' because it contains no reports.". Did you have a similar error? It tried it with clang 2.9.

If you have any questions about the new build system, feel free to ask of course.

@liuliu
Copy link
Owner

liuliu commented Nov 14, 2012

I don't think that this diff does the right thing.

It invokes cc (alias to gcc) to compile, rather than the designated clang compiler, there are redundant rules that I cannot figure out why is not called. The ./configure shell-script doesn't read old config.mk and skip properly. Trying to fix these issues now.

@liuliu
Copy link
Owner

liuliu commented Nov 14, 2012

Need to spend more time to improve my GNU make skill, before that, there are a few issues in this diff makes me uncomfortable to merge yet (the main makefile's %.o: %.c rule never get called, a few %.o: %c rules in suffix.mk is executed, no libccv.a object get emitted, maybe a make version problem?: GNU Make 3.81)

Also, the ffmpeg part is tricky too, it cannot now execute on my mpg file. Note that my tld.c is known to have memory issues and such, I need to fix these, meanwhile, the tld implementation itself is separately tested with valgrind to make sure no memory leak in the implementation.

Thanks for the diff, I just need more time to make sure everything I understands ;-( a night is just not enough.

@liuliu
Copy link
Owner

liuliu commented Nov 14, 2012

also, to answer the question about why not a shared object, because ccv uses some global data, and it would be troublesome to make it a shared object. Personally, for any serious project, ccv should be just included in the project and use link-time-optimization to remove all these unused code if you care about binary size.

@smvv
Copy link
Contributor Author

smvv commented Nov 14, 2012

I fixed the issue with libccv.a and integrated test/ in the new central build system. Both "make test" as well as "make" work as expected.

I also ran make through clang's static analyser (use "scan-build make STATIC_ANALYZER=1") and it produced an error report. Make sure that you first remove /config.mk and generate it again using "./configure". Otherwise, config.mk will not have the proper content.

There is a null pointer dereference issue in lib/ccv_swt.c on line 730:

730: for (i = 0; i < all_words->rnum; i++)

where all_words is null. There are a couple of checks above involving scale_invariant and when scale_invariant is false, all_words is set. However, when scale_invariant is true, all_words remains null and that will cause a null pointer dereference on line 730. See clang's static analyser report (use "scan-view /tmp/scan-build-2012-11-14-1" or a similar path) for more details.

@liuliu
Copy link
Owner

liuliu commented Nov 15, 2012

thanks for the pointer, I am running trunk version of scan-build with clang now, quite a few warnings, no surprisingly.

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