lamikr / rocm_sdk_builder

Other
113 stars 8 forks source link

Wip/rocm sdk builder 612 bg74 #81

Open lamikr opened 1 week ago

lamikr commented 1 week ago

pytorch audio build against disto version of ffmpeg

jeroen-mostert commented 1 week ago

It seems things are built twice, once through package_pytorch_audio_rocm.sh and again through package_pytorch_audio_rocm_wheel.sh. The FFMPEG_ROOT should be added to the latter as well.

lamikr commented 6 days ago

Yes, I noticed the same. Actually the package version can be removed totally as setup.py install does also the wheel file. But as I commented on bug 74, I was able to reproduce the problem on ubuntu 24.04 and this did not fix it. So if you make a patch based on your findings, I could apply that instead and abandon this one.

I can go throught the package script remove, it's on couple of other projects also. (Thought on those it does not cause full rebuild)

jeroen-mostert commented 6 days ago

Sorry for making you go through this when it wasn't a fix (another great reminder for me to always do root cause analysis before doing unrelated things that only fix things accidentally :stuck_out_tongue: ) but this code still has some value in case building against the system's FFmpeg does become more useful or necessary (like the FFmpeg 7 case) so don't completely purge the branch, at least.

lamikr commented 5 days ago

Yes, I can keep this here. I was also actually thinking that maybe it would be possible to add a patch for supporting the ffmpgeg7 load. I have not looked the code yet, but if it's already supports 4,5 and 6, maybe the addition of support for ffmpeg7 would not be that hard.

jeroen-mostert commented 5 days ago

It shouldn't be too much work, but it's more than just changing one or two lines of code to rename identifiers, which is why I didn't attempt a patch when the compile failed. Some things have shifted around to other levels. For someone who is actually familiar with FFmpeg I expect it to be quite manageable, since the torio lib basically only does some basic decoding and encoding.

lamikr commented 2 days ago

So is the situation now that the latest pytorch audio does not build for Manjaro or Archlinux because it only has ffmpeg7?

jeroen-mostert commented 2 days ago

Correct (at least for Manjaro unstable, but Arch should be the same). Worse, pytorch-vision also does not build, and this one's more annoying (for me that is) because it appears to have no simple way to override where FFmpeg is located at all. It will use whatever FFmpeg it finds in the includes, and if that's FFmpeg 7, it will use that and break.

Update: for Arch, there is now an ffmpeg6.1 package in the AUR. This builds on Manjaro as well and installs the necessary headers in /usr/include/ffmpeg6.1 (and the libs in /usr/lib/ffmpeg6.1, as well as /usr/lib for the major version symlinks). This means patches are still necessary to at the very least pick up these headers (the libs should work on their own). Also, the use of the AUR on Manjaro is not generally encouraged (and is not enabled by default), so support for FFmpeg 7 would still be superior to having to install a legacy package. Even so, it's better than nothing.

jeroen-mostert commented 2 days ago

Duh, I'm a dum-dum. I forgot that the nice volunteers at Arch build their own versions of this stuff as well, so things like https://gitlab.archlinux.org/archlinux/packaging/packages/torchvision/-/blob/main/torchvision-ffmpeg7.patch?ref_type=heads exist. Neat! (torchaudio has not been similarly patched that I could find, alas.)

jeroen-mostert commented 1 day ago

I set up #90 for pytorch-vision. For pytorch-audio, on Arch and Manjaro you currently have to live with the pain of installing ffmpeg6.1 yourself. We cannot/should not automate this from install-deps.sh as it requires building things from source, at best we can detect this situation for now (pacman -Q "ffmpeg<7" will error if no suitable package is installed), and possibly disable building FFmpeg support entirely in that case. It would then be up to the user to get the AUR package on the system.

Patching in support for FFmpeg 7 is quite a bit more involved, especially doing it "properly" (with runtime loading support). I've been working on this for a bit but there are very many more changes required than the simple patch for pytorch-vision, due to the fundamental changes in how channel layouts now work (they used to be uint64_t but are now a more complex struct). FFmpeg 6 kept around deprecated interfaces for this, but FFmpeg 7 removes them. Those postponed chickens are now coming home to roost.

jeroen-mostert commented 15 hours ago

Quick update: I have a patch in the works for pytorch-audio to build against FFmpeg 7. The current status is that it is still failing a handful of unit tests that do not fail on vanilla upstream, but otherwise it's looking promising.