Removes incorrect ADPCM encoder's block size requirement (to fix MG #6701) #6
Conversation
…Game issue #6701)
|
It seems the fix here will also allow MonoGame Issue #8433 (a DesktopGL audio looping issue) to be fixed as well. |
|
Why are we not just changing adpcmenc.c itself? Changing the line like this in code seems hacky. |
Because the file is part of the FFmpeg repo which we don't have direct access to modify or to submit PRs for. Various members of the MonoGame community have filed issues over the last 7 years with them so that they would fix it on their side. Unfortunately that hasn't worked. We can now fix this on our side though, thanks to this new MonoGame.Tool.FFmpeg build task.
That's because it is. I did contact @AristurtleDev before making this PR to check, and he pointed to this line where he was doing something similar, so I made the comment match that. |
|
Why not make a fork of ffmpeg? |
The only thing I know is that this tool repo was developed as a result of this issue & bounty: |
In the original bounty MonoGame/MonoGame#8241, this FFMpeg library was to be setup similar to the MonoGame.Library.Freetype repository. Using that as the base example, the external dependencies, in this case FFMpeg, would be added as a submodule from the original source and not a fork. It does feel a little hacky though doing it this way. Though my adjustment and @squarebananas adjustment are both well documented in comments as to why they are being done, it may be worth considering having forks of these dependencies in the MonoGame org on GitHub and make the changes directly to the source. Especially if more and more get changed in the future. Would need @harry-cpp to probably weigh in on this though. This means maintaining a separate fork within the MonoGame org. |
|
The idea is simple, for any code change worth making here, it is probably worth submitting upstream. Thats why there is no need to keep our fork around. Now that being said, right now we keep a couple of custom changes around, but that is only temporary, and going forward we won't be allowing for PRs to modify the code of submodules, instead requiring modification of upstream projects (we might include temporary code fixes until the upstream project makes a new release of course). This patch is perfect example of it, it shouldn't be made here but instead made on the ffmpeg repo. The only issue is figuring out how to submit patches for some of these ancient projects :D |
|
The ffmpeg repo doesn't accept PRs on github. @squarebananas has opened a bug on their own system, 3 years ago. https://trac.ffmpeg.org/ticket/9397#ticket It has not been resolved. I'm not sure we can get a change pushed to the ffmpeg repo any time soon. So either we have to use the in-code fix or create a fork of ffmpeg. |
Open source projects have their own rules for submitting patches and in case of ffmpeg its to submit a patch through the mailing list, see: https://ffmpeg.org/developer.html#Submitting-patches-1
An issue has been open but no patch has been submitted upstream. Its like opening an issue on MonoGame and fixing the problem in your own fork and wondering why the MonoGame issue is still open, unless someone goes and submits a patch upstream, yea, it will stay open. |
The patch that was suppose to fix XAudio2 is here, but it didn't work due to a simple mistake on their side.
To clarify I only commented on the issue when the FFmpeg maintainer of |
|
Note that this issue is important because of MonoGame/MonoGame#8589 I am just anxious to wait for who knows how long for the ffmpeg people to get this merged when this fixes an important issue. We could create a fork then just link the submodule to that fork. Personally this feels like the best option since it is immediate and avoids the hacky fix. |
Currently FFmpeg produces incompatible reduced quality audio (MGCB medium & low settings) for XAudio2 playback. Basically the samples per block should be a power of 2, however FFmpeg incorrectly forces this requirement on the block size instead). This PR removes the encoder's power of 2 check. A one line change will also be required to the Content Pipeline project afterwards.
Further details can be found here:
MonoGame/MonoGame#6701
https://trac.ffmpeg.org/ticket/9397#ticket
Please note I have only tested this on Windows at the moment.