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

CoreAudio::HLE: Add FFmpeg/WMF AAC decoder #4508

Merged
merged 35 commits into from Feb 14, 2019

Conversation

Projects
None yet
@B3n30
Copy link
Contributor

B3n30 commented Dec 15, 2018

Due to the DSP LLE and the research from @wwylele I was able to reconstruct the messages used in pipe3 to decode Audio. The encoded and decoded samples are transferred via DMA. So far this was only tested with XY and resolves all the softlocks but still keeps a good speed.

How to get the codec

Since AAC needs a license to distribute the compiled codec, we do not add the codec to the build.
This means for:

  • Windows 7-10: No need to do anything
  • OSX: ffmpeg uses the already with OSX shipped codec, so on OSX it works out of the box
  • Linux: Linux users have to build Citra together with installed libavtools (including the AAC codec) to enable the decoder. I will look into how to enable ffmpeg with flatpak
  • Android: @liushuyu will look into how to use androids built in codecs for that

If the decoder is missing:

The decoder will still respond with proper responds but since no data gets decoded this will result in missing audio. Sotflocks shouldn't happen anymore though

Future plans:

  • For Linux dynamic library loading could also be an option, but it's unclear how this could work with flatpacks
  • The decoding happens async. This should get implemented in the same way.
  • There are other codecs (mp3, ac3) supported by the DSP chip/firmware. Implementing this should be easy as soon as the first decoder is merged.

Merry Christmas 🎁 😁


This change is Reviewable

@zhaowenlan1779
Copy link
Member

zhaowenlan1779 left a comment

mostly looks okay to me. will check again soon.

return true;
}

#endif // _Win32

This comment has been minimized.

Copy link
@zhaowenlan1779

zhaowenlan1779 Dec 15, 2018

Member

wrong comment

Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.h Outdated
Show resolved Hide resolved src/audio_core/CMakeLists.txt Outdated
Show resolved Hide resolved src/audio_core/CMakeLists.txt Outdated
Show resolved Hide resolved CMakeLists.txt Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.h Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.h Outdated
Show resolved Hide resolved src/audio_core/hle/aac_decoder.cpp Outdated
Show resolved Hide resolved src/audio_core/hle/aac_decoder.cpp Outdated
Show resolved Hide resolved src/audio_core/hle/aac_decoder.cpp Outdated
Show resolved Hide resolved src/audio_core/hle/aac_decoder.cpp Outdated
Show resolved Hide resolved src/audio_core/hle/aac_decoder.cpp Outdated

@B3n30 B3n30 force-pushed the B3n30:dsp_aac branch from 2d798da to 07c6424 Dec 15, 2018

@benderscruffy

This comment has been minimized.

Copy link

benderscruffy commented Dec 16, 2018

Nintendo 3DS Guide - Louvre (Europe)
crashes out Citra when you choose to listen to the tour
[ 41.987516] Debug audio_core/hle/aac_decoder.cpp:operator():189: Assertion Failed!

@Dragios

This comment has been minimized.

Copy link
Contributor

Dragios commented Dec 16, 2018

There is sound popping issue with the AAC decoder on Pokémon X/Y it seems.

Show resolved Hide resolved src/audio_core/hle/aac_decoder.cpp Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.h Outdated
Show resolved Hide resolved src/audio_core/hle/decoder.h Outdated
Show resolved Hide resolved src/audio_core/hle/decoder.cpp Outdated
Show resolved Hide resolved src/audio_core/hle/decoder.cpp Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.h Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.h Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.h Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.h Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.h Outdated
@B3n30

This comment has been minimized.

Copy link
Contributor Author

B3n30 commented Dec 16, 2018

@benderscruffy Can you please try again?

@B3n30

This comment has been minimized.

Copy link
Contributor Author

B3n30 commented Dec 16, 2018

@Danman3412 Yes I intentionally coded it that way that even if the dlls are missing the decoder would still return a valid response. This means the softlocks are fixed regardless of the dlls.

The only issue you observe with missing dlls is that the audio is missing. But since the "Fake response" is based of the response from XY other games might still be broken due to missing dlls. The line in question here is: https://github.com/citra-emu/citra/pull/4508/files#diff-0f90696414f137a30c20616f52bdc1aaR151

@Disciple3

This comment has been minimized.

Copy link

Disciple3 commented Dec 16, 2018

This patch fixes crash #3443 in Fire Emblem If, but voices in treehouse mini-game have wrong pitch. Switching audio to lle mode makes the issue go away.

@MojoJojoDojo

This comment has been minimized.

Copy link

MojoJojoDojo commented Dec 16, 2018

On Rhythm Heaven Megamix the various minigames are too fast even though the game reports 60 FPS.
Both the music and the gameplay as well.

template <typename T>
struct FuncDL {
FuncDL() = default;
FuncDL(void* dll, const char* name) {

This comment has been minimized.

Copy link
@lioncash

lioncash Dec 16, 2018

Member

dll can still be an HMODULE like it initially was.

Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.cpp Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.cpp Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.cpp
Show resolved Hide resolved src/audio_core/hle/decoder.cpp Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.cpp Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.cpp Outdated
Show resolved Hide resolved src/audio_core/hle/ffmpeg_dl.h Outdated
typedef HMODULE pointer;
void operator()(HMODULE h) {
using pointer = HMODULE;
void operator()(HMODULE h) const {

This comment has been minimized.

Copy link
@ghost

ghost Dec 17, 2018

you can use pointer

This comment has been minimized.

Copy link
@B3n30

B3n30 Dec 17, 2018

Author Contributor

HMODULE is preferred here

@Zombie-Feynman

This comment has been minimized.

Copy link

Zombie-Feynman commented Dec 17, 2018

On my system (Gentoo Linux, ffmpeg-3.3.6, fdk-aac-0.1.5), this fails to find the codec because avcodec_register_all is not called before avcodec_find_decoder. Relevant packages used are ffmpeg-3.3.6 and fdk-aac-0.1.5.

A very quick-and-dirty patch fixes this, though I'm sure someone more familiar with Citra's code can figure out a better place to put the missing line:

--- src/audio_core/hle/ffmpeg_dl.h.old  2018-12-17 23:14:16.618582043 +0100
+++ src/audio_core/hle/ffmpeg_dl.h      2018-12-17 23:15:28.316260697 +0100
@@ -72,7 +72,8 @@
 const auto av_parser_parse2_dl = &av_parser_parse2;
 const auto av_parser_close_dl = &av_parser_close;
 
-bool InitFFmpegDL() {
+inline bool InitFFmpegDL() {
+    avcodec_register_all();
     return true;
 }
@zhaowenlan1779

This comment has been minimized.

Copy link
Member

zhaowenlan1779 commented Dec 18, 2018

@Zombie-Feynman
That is actually an old function that has been deprecated in newer FFmpeg versions.
Probably should check the version of the library in the code.
Actually should just use #if here, as this is on linux anyway.

@saibotlive

This comment has been minimized.

Copy link

saibotlive commented Dec 18, 2018

While playing Fire Emblem Echoes with the latest canary builds and using HLE, the game freezes whenever you try to talk to a character both in battle or outside battle. I have attached a screenshot and log file below.

screenshot 2018-12-18 at 17 01 23

citra_log.txt

@B3n30

This comment has been minimized.

Copy link
Contributor Author

B3n30 commented Dec 18, 2018

@saibotlive You probably didn't include the dlls according to https://github.com/citra-emu/citra/wiki/HLE-AAC-Decoder

@saibotlive

This comment has been minimized.

Copy link

saibotlive commented Dec 18, 2018

@B3n30 I am on macOS Mojave, which doesn't need ffmpeg dll files.

@B3n30

This comment has been minimized.

Copy link
Contributor Author

B3n30 commented Dec 18, 2018

on macOS it is known that our current build doesn't include al dylibs necessary. There is already a PR #4517 open that should fix that.

@saibotlive

This comment has been minimized.

Copy link

saibotlive commented Dec 18, 2018

@B3n30 that's great to hear. Is there a way I can checkout the open PR branch and build on my machine?

@B3n30

This comment has been minimized.

Copy link
Contributor Author

B3n30 commented Dec 18, 2018

this PR isn't a place to discuss this. Maybe you join discord for such questions

@B3n30 B3n30 force-pushed the B3n30:dsp_aac branch from 89ca060 to ab1f47e Feb 9, 2019


// last two bytes of MF AAC decoder user data
// see https://docs.microsoft.com/en-us/windows/desktop/medfound/aac-decoder#example-media-types
u16 MFGetAACTag(const ADTSData input);

This comment has been minimized.

Copy link
@wwylele

wwylele Feb 9, 2019

Member

This const in function signature has no effect. Either remove it, or make it const reference const ADTSData&

MFCoInit();
}

WMFDecoder::Impl::~Impl() = default;

This comment has been minimized.

Copy link
@wwylele

wwylele Feb 9, 2019

Member

The destructor should symmetrically do MFDeInit as opposite to the constructor


// the following was taken from ffmpeg version of the decoder
f32 val_f32;
for (size_t i = 0; i < output_buffer->size();) {

This comment has been minimized.

Copy link
@wwylele

wwylele Feb 9, 2019

Member

std::size_t

return;
}
LPSTR err;
FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_ALLOCATE_BUFFER |

This comment has been minimized.

Copy link
@wwylele

wwylele Feb 9, 2019

Member
  • You should call LocalFree after this function as you use FORMAT_MESSAGE_ALLOCATE_BUFFER
  • This is a windows API with variant string type. You should use one of the following combinations:
    • Use LPCTSTR / TCHAR* for err and use FormatMessage
    • Use LPCSTR / char* for err, and use FormatMessageA
    • Use LPCWSTR / wchar_t* for err, and use FormatMessageW
return nullptr;
}
CoTaskMemFree(activate);
return std::move(transform);

This comment has been minimized.

Copy link
@wwylele

wwylele Feb 9, 2019

Member

No need to std::move here. return itself has move semantics.

}

bool SelectInputMediaType(IMFTransform* transform, int in_stream_id, const ADTSData& adts,
UINT8* user_data, UINT32 user_data_len, GUID audio_format) {

This comment has been minimized.

Copy link
@wwylele

wwylele Feb 9, 2019

Member

user_data should be const UINT8*

return false;
}

std::optional<ADTSMeta> DetectMediaType(char* buffer, size_t len) {

This comment has been minimized.

Copy link
@wwylele

wwylele Feb 9, 2019

Member

std::size_t

bool SelectInputMediaType(IMFTransform* transform, int in_stream_id, const ADTSData& adts,
UINT8* user_data, UINT32 user_data_len,
GUID audio_format = MFAudioFormat_AAC);
std::optional<ADTSMeta> DetectMediaType(char* buffer, size_t len);

This comment has been minimized.

Copy link
@wwylele

wwylele Feb 9, 2019

Member

std::size_t

if (!dll_util) {
DWORD error_message_id = GetLastError();
LPSTR message_buffer = nullptr;
size_t size =

This comment has been minimized.

Copy link
@wwylele

wwylele Feb 9, 2019

Member

std::size_t

if (!dll_codec) {
DWORD error_message_id = GetLastError();
LPSTR message_buffer = nullptr;
size_t size =

This comment has been minimized.

Copy link
@wwylele

wwylele Feb 9, 2019

Member

std::size_t

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Feb 9, 2019

Mostly LGTM. Just some more nit above

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Feb 14, 2019

Merging this bulk PR first. Please do follow up PR for fixing nits

@wwylele wwylele merged commit 1f38c53 into citra-emu:master Feb 14, 2019

2 of 4 checks passed

ci/bitrise/4ccd8e5720f0d13b/pr Failed - citra
Details
code-review/reviewable 27 files, 76 discussions left (B3n30, lioncash, liushuyu, MerryMage, valentinvanelslande, wwylele, zhaowenlan1779)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@B3n30 B3n30 deleted the B3n30:dsp_aac branch Apr 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.