Hi Martin On Sat, Mar 21, 2026 at 08:15:10PM +0200, Martin Storsjö via ffmpeg-devel wrote: > On Sat, 21 Mar 2026, Michael Niedermayer via ffmpeg-devel wrote: > > > This is not a small refactor. It adds 651 commits, touches 117 files, and > > adds 19405 lines and removes 1940. > > That scope merits broader review and discussion before any merge decision. > > Yes, it's a somewhat large refactor of the checkasm core code. It is based > on the dav1d checkasm. Note; the checkasm we have in ffmpeg was inspired by > the x264 checkasm, but essentially rewritten from scratch for ffmpeg by > Henrik Gramner. Here, it evolved for a few years. > > When dav1d was started, Henrik relicensed parts of what ffmpeg had (from a > subset of authors, to ease the relicensing effort), to get a version of > checkasm suitable for that project. > > Since then, both projects' checkasm implementations have evolved a bit (with > a some part of that evolution being features being ported back and forth > between the two, with suitable permissions from authors). > > This effort was based on dav1d's checkasm implementation (which both had the > most compatible license, and the most featureful checkasm core). And as far > as I'm aware, Niklas had asked and received approval to relicense the other > checkasm features from ffmpeg's checkasm. >
My concern is not that code has historically moved between projects. My concern is how this is being integrated into FFmpeg now, and under what assumptions. > > First, a dependancy on external infrastructure is unnecceasry > > Nothing requires a dependency on external infrastructure. The imported tree clearly points users and contributors to external infrastructure: +tests/checkasm/ext/docs/getting-started.md:url = https://code.videolan.org/videolan/checkasm.git +tests/checkasm/ext/docs/getting-started.md:git submodule add -b release https://code.videolan.org/videolan/checkasm subprojects/checkasm +tests/checkasm/ext/docs/introduction.md:- [**GitLab Repository**](https://code.videolan.org/videolan/checkasm) +tests/checkasm/ext/docs/introduction.md:- [**Issue Tracker**](https://code.videolan.org/videolan/checkasm/issues) +tests/checkasm/ext/docs/introduction.md:- [VideoLAN Homepage](https://www.videolan.org/) +tests/checkasm/ext/src/html_data/body.html: This report was created using the <a id="checkasm-version" href="https://code.videolan.org/videolan/checkasm";>checkasm</a> So even if you do not consider that a hard technical dependency, it is still an integration centered around external infrastructure and external project entry points. That is a problem for FFmpeg. If we import code into FFmpeg, then bug reporting, contribution flow, and project identity should point to FFmpeg infrastructure, not to some external repository and issue tracker. This is not different from how we handle the current checkasm. > It can be treated > as a plain code drop, with no obligation to update further, if you don't > want to. A code drop still has maintenance consequences. If imported documentation, generated output, and contributor guidance point to an external repository and external bug tracker, then even as a nominal "code drop" it diverts bug reports and contributions away from FFmpeg. That is precisely the sort of thing we should avoid when integrating code we are expected to maintain locally. > > > Second, a dependancy on gitlab-CI is not acceptable > > There is literally no dependency on that? The imported tree also contains CI integration tied to VideoLAN GitLab infrastructure: +tests/checkasm/ext/.gitlab-ci.yml: image: registry.videolan.org/dav1d-debian-unstable:20251231153127 +tests/checkasm/ext/.gitlab-ci.yml: image: registry.videolan.org/vlc-debian-llvm-msvcrt:20250305204125 +tests/checkasm/ext/.gitlab-ci.yml: image: registry.videolan.org/dav1d-debian-bookworm-aarch64:20250215002814 +tests/checkasm/ext/.gitlab-ci.yml: image: registry.videolan.org/dav1d-debian-bookworm-armv7:20250215014239 +tests/checkasm/ext/.gitlab-ci.yml: image: registry.videolan.org/dav1d-debian-unstable-ppc64le:20250215003029 +tests/checkasm/ext/.gitlab-ci.yml: image: registry.videolan.org/vlc-debian-android:20241118101328 +tests/checkasm/ext/.gitlab-ci.yml: - git remote add upstream https://code.videolan.org/videolan/checkasm.git +tests/checkasm/ext/.gitlab-ci.yml: - git remote add x86inc https://code.videolan.org/videolan/x86inc.asm.git Again, even if you do not call that a hard dependency, it shows that the imported project is structured around external CI and external hosting. I would be happy to see self-tests and wrapper validation added to FFmpegs checkasm. But that is not the same as replacing FFmpeg’s implementation with an imported external one whose surrounding documentation, CI, and contribution paths point elsewhere. > > So far, ffmpeg's checkasm hasn't had any selftests at all. (And it also > means that e.g. the x86_64 checked_call wrapper can fail to fill the upper > half of 32 bit parameters with garbage, which can lead to bugs going > undetected.) > > The standalone checkasm does have selftests now, to make sure that the > wrapper actually does what is expected, etc. And since the standalone I agree those tests are useful and worth having. > checkasm has been developed on Videolan gitlab, it has integration for using > that CI for running the selftests on a number of configurations. > > It also has a set of Github Actions CI scripts for testing building with > MSVC (which isn't available in the videolan gitlab CI). > > > We need to be able to run the full relevant test coverage ourselves, > > under FFmpeg-controlled infrastructure > > I don't see why you're in any position to make any demands at all here? Someone submited a pull request. Discussing what needs to be changed is the normal path of a pull request. [...] > And as far as I understand, you have a major role in that > company, so why are you complaining here about what your company does, > instead of raising that issue inside your company? I have raised it multiple times internally. And this PR surpised me, it was not what i expected to see. But back to the subject, About this PR At minimum, I think this would need: * FFmpeg-hosted project entry points and documentation * FFmpeg-controlled bug reporting and contribution paths * a clear and reviewed attribution/relicensing story * a clear plan for how the self-tests are run under FFmpeg-controlled infrastructure * If a upstream remains that is not FFmpeg, then a clear plan how changes from it and local changes can be merged. Bascially git merge must work Without that, I do not think this is ready to merge. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Arguing that you don’t care about the right to privacy because you have nothing to hide is no different than saying you don’t care about free speech because you have nothing to say - Edward Snowden
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
