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

Optionally build with ffmpeg because of patenting issues #282

Closed
wants to merge 11 commits into from

Conversation

basnijholt
Copy link
Contributor

@basnijholt basnijholt commented Jan 24, 2023

Because of patenting issues in FFmpeg, I cannot install VTK for anything work-related. I imagine many others are also potentially having this issue (although likely without knowing this.)

Here I create a variant that creates an additional variant where FFmpeg is not installed.

This PR leaves the build string of the version with FFmpeg unchanged, however, adds a variant *noffmpeg.

cc @jgukelberger

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@basnijholt
Copy link
Contributor Author

Can anyone review this PR? 😄

@jakirkham
Copy link
Member

Are you aware that there already are separate builds of ffmpeg?

cc @conda-forge/ffmpeg

@hmaarrfk
Copy link
Contributor

Can you confirm that the LGPL build of ffmpeg is not enough for you?

@hmaarrfk
Copy link
Contributor

A few comments:

  1. Really hope that you consider creating your own company (just noticed you work at Microsoft, so maybe "subgroup" ;) ) infrastructure to get this package useful to you while the review process goes on at conda-forge.

  2. A few people (myself included) have gone down this licensing / patenting rabbit hole. The truth is, that FFMPEG is "big" and "grabs attention", but maybe you should consider that it is likely that the same patents that you might be infringing on for using FFMPEG (parallelized image processing), could also be at play with VTK?

  3. I took time to look through the logs, and noticed that ffmpeg libraries (libavcodec and the like) are linked to a single file:

2023-01-09T20:10:29.7256834Z    INFO (vtk,lib/libvtkIOFFMPEG-9.2.so.9.2.5): Needed DSO lib/libavformat.so.59 found in conda-forge::ffmpeg-5.1.2-gpl_h8dda1f0_105
2023-01-09T20:10:29.7339925Z    INFO (vtk,lib/libvtkIOFFMPEG-9.2.so.9.2.5): Needed DSO lib/libavcodec.so.59 found in conda-forge::ffmpeg-5.1.2-gpl_h8dda1f0_105
2023-01-09T20:10:29.7422791Z    INFO (vtk,lib/libvtkIOFFMPEG-9.2.so.9.2.5): Needed DSO lib/libavutil.so.57 found in conda-forge::ffmpeg-5.1.2-gpl_h8dda1f0_105
2023-01-09T20:10:29.7505743Z    INFO (vtk,lib/libvtkIOFFMPEG-9.2.so.9.2.5): Needed DSO lib/libswscale.so.6 found in conda-forge::ffmpeg-5.1.2-gpl_h8dda1f0_105

For conda-forge's benefit (and this is why I suggest you release your own packages on your own channel), I think that the proliferation of "variants" is really difficult to maintain, and to teach users about. As the author of the lgpl/gpl variants of ffmpeg, I'll justify it due to the
"infectiousness" of GPL.
Could it be, that for your needs, you create:

  1. A subpackage for the "base" of VTK.
  2. A subpackage for the library (libvtkIOFFMPEG) vtk-io-ffmpeg
  3. Keep the parent package depending on both?

This generally limits the build time, and keeps discoverability high (its easier to find packages than variants given the anaconda.org/conda tools).

Anyway just a few thoughts for you and other VTK maintainers to consider.

@hmaarrfk
Copy link
Contributor

I also meant to provide an example on how to implement the split package idea.

In the following qt PR, I am suggesting creating a package for an optional feature of the qt package that could help a subset of users

conda-forge/qt-main-feedstock#98

@basnijholt
Copy link
Contributor Author

@jakirkham, yes, I am aware of the GPL and LGPL builds of FFmpeg. Unfortunately, both contain the AV1 and VP9 codec which we cannot install.

@hmaarrfk, thanks a lot for your reply and suggestions. Having thought about the options, it seems like the subpackage idea might be suboptimal. For example, we rely on VTK via PyVista, making the parent VTK package depend on two subpackages (vtk-base and vtk-io-ffmpeg ) means I would then have to convince the PyVista feedstock maintainers to only depend on the vtk-base. There are likely also other packages one might want to install that depend on VTK.

I agree with your discoverability concerns, however, having a variant seems to be the correct way of solving this problem.

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Feb 3, 2023

having a variant seems to be the correct way of solving this problem.

I've found that variants are needed when there is no way to disentangle the libraries. Here it seems like there is hope. For example, I made the GPL/LGPL since FFMPEG didn't seem to allow that split otherwise.

There have been issues in the past in adding too many "globs" on build strings. For example, i still don't think that conda-forge/abseil-cpp-feedstock#46 (comment) is resolved.

I tried to check what links to the the libvtkIOFFMPEG-9.2.so and it doesn't seem like any library has a hard dependency on it within VTK.

ldd *.so > ldd.out.txt

yields ldd.out.txt maybe you can try

rm ${CONDA_PREFIX}/lib/ibvtkIOFFMPEG-9.2.*

on your non-sensitive environments and see how well that works. That should give you information on the viability of the "split" package approach.

I would then have to convince the PyVista feedstock maintainers

This is the challenge of working in a community. This somewhat happened with matplotlib -> matplotlib-base transition (removing the hard qt dependency). It took time, but eventually people got on board. Given you are adept with conda-forge infrastructure, I do think that you can make a successful plea to the package you need in your stack because you can help the maintainers implement the transition.

Truthfully, I don't think conda has a good way to transition packages like debian does

To overcome this in the short term, I strongly recommend making your own channel to move yourself forward on this front. Azure can allow you to build up what you need with little friction from conda-forge forge tooling. Fork -> Add your channel information, and rerender.

Summary

Ultimately, I personally think that using build strings is the wrong approach unless it can be shown that VTK directly depends on the ffmpeg extension once it is compiled it at build time.

The proposed approach permanently doubles the number of builds and maintenance burden.

All said, I really won't stop you and other VTK maintainers from moving this forward if you feel it is important.

PS. I've sat on a similar PR for opencv (conda-forge/opencv-feedstock#206) and (conda-forge/opencv-feedstock#337) for 2-3 years that I just haven't been excited to move on for this reason.

@basnijholt
Copy link
Contributor Author

basnijholt commented Feb 13, 2023

@hmaarrfk, thanks a lot for your detailed message.

We have discussed the options and investigated whether the proposed approach will work or not.

Ultimately, I personally think that using build strings is the wrong approach unless it can be shown that VTK directly depends on the ffmpeg extension once it is compiled it at build time.

Unfortunately, building sub-packages (e.g., vtk-io-ffmpeg) won't work because CMake dynamically generates Python code and when compiled with FFmpeg, the Python code will always import it.

Resulting in:
image

AFAIU this means that this can only be solved by compiling with or without the feature (so build strings).

Given the approval 👍 by one of the recipe maintainers (@Tobias-Fischer) but the hesitance 🤔 from @hmaarrfk, we could perhaps democratically solve this.

Could some (one?) of the other @conda-forge/vtk maintainers weigh in here (which would allow us to make a decision)?

@hmaarrfk
Copy link
Contributor

democratically solve this.

I think you all can do what is best for VTK.

won't work because CMake dynamically generates Python code and when compiled with FFmpeg, the Python code will always import it.

Given you've already done the hard work of finding where the python code is generated. I would consider one last option:

  • Adding a patch to change the import statements to:
with suppress(ImportError): from.vtkIOFFMPEG import *

with

from contextlib import suppress

at the top.

I would then work to submit such a patch (or a version of it) upstream explaining the packaging and slicing challenge of VTK (which I think the smart folk at kitware are well aware of). But given the uniqueness of the conda-forge packaging

tbh, if I thought such a solution were possible for opencv, I would patch it an merge today. You've done great work exploring the limitations of the current VTK build system!

This one liner i learned today from: https://stackoverflow.com/a/52020518

@hmaarrfk
Copy link
Contributor

So for what its worth, i tried to "debug" this recipe with conda debug and i couldn't even create the environment locally. it seemed to look for *=cuda.

I guess that is as far as I have time to go. I understand if it is also as far as you are willing to go too.

@jakirkham
Copy link
Member

Sounds vaguely similar to issue ( conda-forge/nvidia-apex-feedstock#29 ). Given Mamba doesn't have the same problem ( conda-forge/admin-requests#674 (comment) ), wonder if Conda has a bug

@hmaarrfk
Copy link
Contributor

So I did a little more digging this morning, and it seems that the VTK maintainers are likely open to a try/except patch.

See: https://gitlab.kitware.com/vtk/vtk/-/issues/18365#note_1079278

Just wanted to point that out.

@basnijholt
Copy link
Contributor Author

@conda-forge-admin, please rerender

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@basnijholt
Copy link
Contributor Author

superseded by #287

@basnijholt basnijholt closed this Apr 18, 2023
@hmaarrfk
Copy link
Contributor

Thanks for working through all the challenges

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.

4 participants