lamikr / rocm_sdk_builder

Other
132 stars 12 forks source link

add support for FFmpeg 7 to pytorch-audio #93

Closed jeroen-mostert closed 2 months ago

jeroen-mostert commented 3 months ago

Exactly what it says on the tin. This obsoletes #81 by being more flexible. This works by building a local copy of FFmpeg 7 for linking purposes (this includes no codecs or filters whatsoever, so it's very quick), just as the code now does for the other versions. A lame and temporary FFMPEG7_SRC variable is used in lieu of the URL of a CDN, as we don't have that.

The code has been changed to support both the pre-FFmpeg 6 uint64_t channel layout as well as the new struct-based layout. I've gone a bit overboard with this because the pytorch-audio interface currently has no support for elaborate channel layouts, it's fine with just supplying a number of channels. There is possibly a smaller patch struggling to get out of this, but this was the cleanest way I saw how to do it.

This was tested using the tests included with torchaudio (a lot), which found and squashed bugs, as well as some basic smoke tests. Note that even in a vanilla build the tests do not all run, the project seems to have racked up some technical debt. I ensured my changes lit up all passing tests. This has been tested with both FFmpeg 7 and FFmpeg 6, but not earlier versions.

I may or may not find the energy to go upstream with this, as I would need to rebase my fixes and I'd really like to move on to other things.

lamikr commented 3 months ago

Hi, I am testing this now. I think I will also need to install Manjaro or arch linux to test bleeding edge changes like this switch ffmpeg update to 7. But with fast reading of patch, I think you should really try to upstream this.

lamikr commented 3 months ago

@jeroen-mostert This part of the patch is suspicious, should it be in else block:

frame->pts = codec_ctx->frame_number + 1?

-        frame->pts = codec_ctx->frame_number + 1;
+#if LIBAVCODEC_VERSION_MAJOR >= 61
+        frame->pts = codec_ctx->frame_num + 1;
+#else        
+        frame->pts = codec_ctx->frame_number;
+#endif        
lamikr commented 2 months ago

@jeroen-mostert I was not sure should I change the frame_number part of the code as you said you find also some bugs in torch audio, so I did not apply this yet. Unfortunately the patch is not broken as I needed to harmonize again the python app build scripts. Change should not be big, build scritp name is just renamed + I added there the SOX build option.

jeroen-mostert commented 2 months ago

@lamikr Good catch. This was just an error, it should indeed just remain frame_number + 1. This is not part of any bug fixes (in fact this commit only contains one, where the check for the version number was just plain incorrect; I've left everything else intact so as to not overburden the already substantial commit).

lamikr commented 2 months ago

Thanks for confirming, I can adapt the patch and merge unless you want to make a new pull req?

jeroen-mostert commented 2 months ago

Go right ahead, I will have limited time to spend on this repo for a while.

lamikr commented 2 months ago

Small changes to be able to merge to latest version in git master.

0001-add-support-for-FFmpeg-7-to-pytorch-audio.patch.txt

lamikr commented 2 months ago

@jeroen-mostert Thanks for the patch, it's now merged to rocm_ask_builder master. I hope you have time to work to send pytorch audio related changes to upstream for review.