Encapsulate video support with compilation variable EXV_ENABLE_VIDEO #2448
Conversation
… add variable to unit test files
|
What's the motivation for the flag? Why wouldn't we have video support always enabled? |
just like support for png (EXV_HAVE_LIBZ) or BMF support (EXV_ENABLE_BMFF) ... It seems that every major functionality is encapsulated with a compilation variable. I believe that goal is to let the user choose which functionality should be included in his compiled binary... |
Wasn't video support taken out in 0.27 because it was not "substantial" and/or not being used? I like what has been done here, but the original motivation in removing video support must be revisited to understand why it was done. |
There was a problem hiding this comment.
There is some missing place where you should also deactivate some tests. Right now we have some CI jobs failing with this:
======================================================================
ERROR: small_video.mp4_test (test_regression_allfiles.TestAllFiles)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/runner/work/exiv2/exiv2/tests/regression_tests/test_regression_allfiles.py", line 186, in test_func
BT.reportTest(os.path.basename(filename), out)
File "/home/runner/work/exiv2/exiv2/tests/bash_tests/utils.py", line 567, in reportTest
raise RuntimeError('\n' + log.to_str())
RuntimeError:
Error: The output of the testcase mismatch the reference
[INFO] The output has been saved to file /home/runner/work/exiv2/exiv2/test/tmp/small_video.mp4.out
I think it is a good idea to add this option to the CMake code, since we could consider the video support experimental and only useful to a handful of users. Since we merged back to main the video support, the fuzzers are now reporting some issues with the code. Therefore, I think we should disable it by default in our release jobs.
The main reason was Robin planning to retire and a lack of contributors to the project. I think I also remember reading that the video code was not being updated to support new video formats and standards. Since then, more contributors have come forward which has improved the situation. However, as the project has no employees, the work is still completed in volunteer's free time. More contributors would be appreciated. See the discussion in #1748. |
… encapsulate in makefile
Codecov Report
@@ Coverage Diff @@
## main #2448 +/- ##
=======================================
Coverage 62.10% 62.10%
=======================================
Files 122 122
Lines 22937 22940 +3
Branches 11244 11245 +1
=======================================
+ Hits 14245 14248 +3
Misses 6475 6475
Partials 2217 2217
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
… fix PR windows matrix
|
@sridharb1 gerbera (a upnp server) has support for exiv2 (disabled by default) and it would be great to add support for reading tags from matroska files. libmatroska is not ideal for this purpose. |
I am all for adding video support because it is high time that exiv2 supports video. I realize that video support will have to be coded and that requires volunteers. If gerbera or other such projects would like to contribute video support, that should be welcome. In another thread, it was proposed that these additional support be done in the main/trunk so as to not destabilize current releases until it is deemed stable for production purposes. Agreed. |
… activate only for concerned CI tests
… desable for linux release
|
Hello @piponazo , |
Those are flaky tests (tests that fail from time to time in CI due to a dependency of the tests on time). I just re-triggered them and hopefully they should pass :) |
There was a problem hiding this comment.
Thanks for the contribution!
I am happy with the current way in which we are handling that new variable, but there is one important workflow in which you forgot to enable the VIDEO option:
.github/workflows/on_PR_linux_fuzz.yml
This is one of the most important ones, because we would like to analyze with the fuzzers defects on that area of the code.
|
Video metadata support is very hard. Just look at the FFmpeg evolution. Anyway thank you so much for the contribution. And HNY++ |
Hello @everybody ,
I added a compilation variable to encapsulate the video support EXV_ENABLE_VIDEO.
Please advise If we should add It or not , otherwise If you judge that is not necessary, don't hesitate to reject this pull request.
Regards