bbc / turingcodec

Source code for the Turing codec, an HEVC software encoder optimised for fast encoding of large resolution video content
http://turingcodec.org/
GNU General Public License v2.0
154 stars 39 forks source link

Remove in-tree external dependencies #11

Closed jdek closed 6 months ago

jdek commented 7 years ago

Both boost and havoc could be added as ExternalProject dependencies via CMake, with boost as a system dependency by default.

kupix commented 7 years ago

We actually moved dependencies' source into the tree so that turingcodec is a coherent project that only needs a compiler to build. Havoc has an upstream project because: (a) it could be useful for other codec projects and (b) it can be built and tested independently.

IIRC, it should already be possible to use system Boost by configuring CMake accordingly - although the way this works can likely be improved.

jdek commented 7 years ago

Only needs a compiler to build

This isn't really true as it requires CMake already, which has the functionality to include external source from git repositories pinned to a commit, or via release tarballs. Using CMake's ExternalProject doesn't change how it is built in visual studio/linux & gcc. The issue is that Boost is being distributed with turingcodec, which is unnecessary for most, and encourages bit-rotting of the dependency. Which in the long run will reduce compatibility with upstream, and discourage testing with multiple versions. It also adds more complexity to the maintaining of turingcodec, which is unneeded.

If turingcodec were using pure makefiles, then having all dependencies in-tree would make sense.

kupix commented 7 years ago

Point taken on need for CMake. Another option is a git submodule which we used to use for havoc. Either way, git itself becomes an additional dependency - but that's a minor point. Point taken also on bitrot - it doesn't feel quite right to have a subset of Boost in the tree but it seems the most practical arrangement for the time being. If ExternalProject can work well on all supported platforms then that might be the best way forward.

matteonaccari commented 7 years ago

At the moment the main use of Boost is for program options to parse the command line. We might want to drop and adopt a solution ala HM using a program_option lite thing. The same should apply also for the remaining uses of boost.

jdek commented 7 years ago

This brings me on to the another concern I and some others had. The API isn't very friendly really, it's essentially just a wrapper for the option parser. If you're looking to rewrite the command-line options parser, then it may be worth designing a functional library API, having the CLI program make use of that with something simple like public domain getopt (or similar) for option parsing.

kupix commented 7 years ago

@matteonaccari I have experimental forks that use additional Boost libraries (time, async, uuid, etc). Also I have dependent projects that themselves use Boost. "Lite" versions might reduce source code size (and initial checkout/build times) but in my opinion, that benefit is outweighed by the extra work, porting and maintenance issues that they'd involve.

@enoposix I'm interested in your friendliness concern - surely most people would consider a large and/or complex C struct less friendly than an options string? The textual settings API is by design to reduce maintenance, documentation, testing and risk of bugs. The cost of string manipulation and string lookup is insignificant against the codec workload and it's outweighed by the benefits of overall simplification.

jdek commented 7 years ago

@kupix the issue is with the level that the library is used. Most programs which need HEVC encoding will generally have a specific use-case minimal settings, or have it's own option parser within. A couple of example programs which may need HEVC encoding:

  1. A generic video recorder
    • uses mostly fixed settings
    • needs one flag for quality/resolution etc
    • will ship with it's own UI
  2. FFmpeg
    • needs
    • has it's own options parser

The level of abstraction is wrong for the settings, it would make more sense in my mind to not restrict the user to strings and have bindings/API users do this instead. Note that the issue isn't the cost of string manipulation here.