-
Notifications
You must be signed in to change notification settings - Fork 460
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
Migrate to FFmpeg 6.0 #707
Conversation
04bc48a
to
1ce13f6
Compare
Returned |
It seems that this also broke AC3 (it compiles but audio is wrong) and I have no idea why. AC3 works with FFmpeg 4.4.4 witout this patch, but played sound is distorted with FFMpeg 6.0 and this patch. MP2 works fine in both cases. |
@equeim Have you checked logcat? ( I've migrated to ffmpeg 6 some times ago locally and it seems the comment :
is no more true and requires a bigger buffer. Doubling |
Nope, it was wrong option names for SwrContext (and there is no error handling av_opt_set calls so there was nothing in logcat). I think I will change it to use swr_alloc_set_opts2 function which accepts these options as parameters, so that there will be a better chance of discovering this at compile time. |
@Tolriq I tried to encode some audio using ffmpeg with pcm_mulaw encoder and wav container and there wasn't any issues with ffmpeg 6.0 and this patch (without any changes to OUTPUT_BUFFER_SIZE_16BIT). |
The issue is about decoding media I can send you a file in private that shows the issue. |
Sure, I have gmail address with the same user as on here. |
@equeim file sent please do not spread. |
I'm not sure that just doubling OUTPUT_BUFFER_SIZE_16BIT is a good idea. From what I can gather the required size of output buffer depends on numbers of samples in AVFrame structure (
Both seems a non-trivial changes. |
Ho yes doubling is not the proper solution but it reduces the issue a lot without too much side effects or changes that may be refused upstream. Anyway just wanted to inform about the issue as the comment is no more valid I assumed it's ffmpeg change that needs to be taken in account during update. |
@microkatz @rohitjoins Currently there are changes regarding NDK r26 in this PR, I think it would be better to move them to separate PR. The question, do we want to support NDK r26? It dropped support of SDK levels < 21, and AFAIK currently media3 has minSdkVersion 16. I suppose it's possible to add automatic detection of NDK version in ffmpeg build script, but it won't be obvious to users that their minSdkVersion should be bumped to 21 if they are building with r26. |
@Tolriq Actually with your file the issue with output buffer size is present in FFmpeg 4.4 too, so I think I will leave it as is here and make separate PR that adds dynamic reallocation. |
Thank you for your contribution.
While file in particular? Should we throw an WARN log in the build script if someone is building with NDK r26?
yes, and so we mention in the README that our build configuration is tested with NDK r21. |
I've removed it since this is out scope of this PR.
But shouldn't we at least be able to compile with the latest LTS version of SDK (which is r26b right now)? Currently build scripts for native libraries don't work at all with r26. |
These changes are also compatible with FFmpeg 5.1, which is now minimum supported version. Also set -Wl,-Bsymbolic flag via target_link_options command which is more correct.
401ee64
to
6332693
Compare
2db4068
to
7c93959
Compare
Hi @equeim, This PR is merged in the Thank you once again for your contributions. |
Hi @equeim, Upon discussing internally it seems we are not constrained by the NDK support per Gradle version and would be happy to upgrade our README to the latest LTS for NDK which is r26b. |
To do this it will be necessary to replace |
No, we can't do this. May be we can introduce some optional flag which people can enable to use the newer version of NDK. |
I'm thinking of adding another argument to build_ffmpeg.sh (and friends) scripts to pass ABI level (minSdk) from outside instead of hardcoding 16/21. This way those who have minSdk >= 21 will be able to use latest NDK without modifying build scripts. |
PiperOrigin-RevId: 584893190 (cherry picked from commit 45b51d8)
These changes are also compatible with FFmpeg 5.1, which is now minimum supported version.
Also set -Wl,-Bsymbolic flag via target_link_options command which is more correct.