-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
[big refactoring] Audio latency fix for Android. Support to preload effects on Android now. #15875
Conversation
1113e6a
to
a03be61
Compare
I see the linter errors:
Please do the follow to fixes:
|
@xpol I developed this functionality on Android Studio by a simple demo. I think we need to change the rule of |
Currently:
As result, using style as But libraries like:
Introducing But anyway, it is only a choice of flavor, you and your team are the keeper of keys at cocos2d-x. |
@xpol , thanks for adding this useful static analysis tool. For developing an independent module, including a header file( with cocos2d-x path coupled ) in the same directory as I suggest to keep current tool That's my opinion. :) @xpol @ricardoquesada @minggo @zilongshanren What's your opinions? |
@dumganhar I am not sure if it is able to modify linter to just satisfy your requirements. But i think we should keep it like this if:
|
@minggo , I looked at the |
@dumganhar Only have one search path is good:
#include "ui/xxx.h" Then i know i uses xxx UI module. I think it is good. |
@minggo , if it's in another module. yep, it should include the module prefix. BTW, current directory is the default include path for all compliers. Not extra search paths is added. |
@dumganhar Yep, if the .cpp and .h file in the same directory, it is reasonable not to include full path. But as i said, it should not add additional search path. |
for example we have the follow #include "a.h" // (1) include the audio/a.h
#include "b.h" // (2) include the audio/b.h
#include "./b.h" // (3) include the audio/b.h
#include "android/c.h" // (4) include the audio/android/c.h
#include "../ui/d.h" // (5) include the ui/d.h and the follow is #include "b.h" // (6) include the audio/b.h
#include "./b.h" // (7) include the audio/b.h
#include "android/c.h" // (8) include the audio/android/c.h
#include "../ui/d.h" // (9) include the ui/d.h The line 1~8. which is allowed which is not allowed? |
@xpol |
I'm thinking about if current directory is supported for header search path. e.g. audio/a.cpp #include "audio/a.h" // allowed
#include "a.h" // allowed
// a sub folder like `audio/android/b.(h | cpp)`, could a.cpp be included like:
#include "android/b.h" ?? |
if (_pcmMetaData != nullptr) | ||
{ | ||
free(_pcmMetaData); | ||
_pcmMetaData = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_pcmMetaData
won't be read after the destructor, you can skip this assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not skip this assignment since AudioDecoder::start
may be interrupted and return false with _pcmMetaData != null
, and the destructor should handle it.
@dumganhar I configured cocos2d-x as follows:
My configuration
My game experiment FPS variation in the range 30-60 fps, without audio the game is more enjoyable with FPS in range 45-60. Tell me if you want I make some other tests. |
@Drakon-Cocos , thanks for testing. |
@dumganhar I saw in the log various lines of kind: UPDATE: |
Hi @Drakon-Cocos , bool AudioPlayerProvider::isSmallFile(long fileSize)
{
//TODO: If file size is smaller than 30k, we think it's a small file. This value should be set by evelopers.
return fileSize < 30000;
} I looked at the log, you have effect files that bigger than 60k, so try to change its value bigger and test. |
@dumganhar |
Do you mind to send me an apk of your game? |
@dumganhar |
@dumganhar I tried two kind of preload (in my Layer init):
However the first play of each sound effect continue to take a time between 30-60 ms for the first play! UPDATE1: UPDATE2 : |
@Drakon-Cocos , thank you for testing. Preload does work on my device. I don't know why it works badly on your android device. :( |
a49585f
to
54675b6
Compare
@xpol , updated, made |
@dumganhar
Here the full log: |
@Drakon-Cocos , could you add more logs in AudioDecoder::~AudioDecoder()
{
ALOGV("~AudioDecoder() %p", this);
SL_DESTROY_OBJ(_playObj);
ALOGV("After destroying SL play object");
if (_assetFd > 0)
{
ALOGV("Closing assetFd: %d", _assetFd);
::close(_assetFd);
_assetFd = 0;
}
ALOGV("before free _pcmData"); // add this
free(_pcmData);
ALOGV("after free _pcmData"); // add this
} I wanna know the exact line which cause the crash. |
@dumganhar When I did:
|
Another Issue, but I don't know if it's related to this PR. COCOS Version 3.3 with this PR merged. The game worked right at most time, but if it hits the specified mp3 files, then crash occur. attachment is the mp3 file, which will cause crash. |
@Drakon-Cocos, could you help to find the exact line which causes the crash since I don't have a device to reproduce it. Thanks. @ryanxia2016 , thanks for your feedback. For a glance, it seems to be a PCM decoding issue of OpenSLES. Anyway, I will check it. |
@ryanxia2016 , it seems like a bug of decoding small files by OpenSLES API. |
@dumganhar or, we should ensure all the sound file's size > xxxx bytes? |
@ryanxia2016 , basically, it shouldn't crash while dealing with small file. |
@ryanxia2016 , I created an issue (#16192). |
@dumganhar
Here the log with grep on main thread id: any clue? EDIT: I start to think that could be a rendering bug in |
I'm also getting a crash but with different error:
|
@dumganhar I transplant the new audioengine to my cocos2dx 2.1.2 version, and i tested in my project found that in Huawei Nexus 6 (android 6.0.1) performance is low, play one effect(MP3 ) that not preload consumed about 500ms, but i tested the same effect in Samsang GALAXY S6 (Android 5.1.1)that consumed only 5ms, it is very different。 |
@superman-t , please attach the log. |
@dumganhar Playing the effect file after preloading it only consumed 5ms. I found the preload in GLThread? |
@superman-t , it's in a sub thread. You could refer to AudioPlayerProvider.cpp: _threadPool->pushTask([this, audioFilePath](int tid) {
ALOGV("AudioPlayerProvider::preloadEffect: (%s)", audioFilePath.c_str());
PcmData d;
AudioDecoder decoder(_engineItf, audioFilePath, _bufferSizeInFrames, _deviceSampleRate);
bool ret = decoder.start(_fdGetterCallback);
if (ret) Putting decoding to thread pool for executing. Preload is needed before entering game scene. Otherwise, AudioEngine::play2d will wait AudioEngine::preload to finish, the GLThread will be blocked. Currently, we use OpenSLES API for decoding, I also found that its performance isn't really great. Another probable improvement is to |
@dumganhar Tks,sometimes there are many effects, the scene can't preload all the effects, so it is only play the effect not preloading.The sub thread locks the GLThread, I know the reason why play2d consume many times. it must decode the effect into memory Thank you for doing this work. I will solve my problem using preload. |
@dumganhar
|
@Drakon-Cocos there are few more additional files changed outside audio directory. Check out "Files changed" to ensure you have everything. |
@piotrros in few days Cocos2d-x 3.13 will be released. This release will include also this PR. |
@piotrros , about |
@Drakon-Cocos I'd like to use 3.13 or even 3.12, but I'm stuck on 3.11, because 3.12 and above breaks cocos studio compatibility and there's no alternative for now. @dumganhar Well yeah I forgot to post them:
|
@piotrros , it seems that there was an OpenSLES error while decoding audio to PCM buffer.
Did it happen only on Nexus 6p Android 7.0? How about other devices? |
Today for testing I only have Nexus 6P and old lenovo tablet with android 4.4.4, which I mentioned before. That's strange that it can't decode .ogg files, because as for as I know it should be the most compatible audio format. That's why I use it. |
I've uploaded modified cocos2d-x 3.11.1 here: https://drive.google.com/file/d/0ByXjTjdhFfLAZXhWdW5qRTdWYnM/view?usp=sharing It includes this new audio engine as well as performance bugfix from 3.12 and few minor changes. |
Ogg should be supported by Android. I don't know why decoder doesn't love it.... |
Hello, Do you have any idea of how to fix this ? Thanks |
No news on this issue ? @dumganhar @piotrros |
No news.. I just went back to the old cocos2d::experimental::AudioEngine and it's working fine. |
3.16 audio effect with preload plays with delay both for AudioEngine & SimpleAudioEngine. Tested on emulator & LG G5 Android 7. Ndk 16b, 13b, 11c. Target & compile sdk 19, min. sdk 15. Tried wav, mp3, ogg. |
PLEASE DONT MERGE BEFORE DEVELOPERS TEST IT. AND MORE COMMENTS NEED TO BE ADDED.
This Pull Request has done the following things:
Please help to test it on the Android devices which have low performance. Thanks :)