mirror of
https://github.com/hrydgard/ppsspp.git
synced 2026-04-24 21:56:10 +03:00
[GH-ISSUE #3805] Grand Theft Auto Chinatown Wars (ULES01347):- Game crashes at the opening plane sequence since v0.9.1-842-g00f8ae5 (Atrac3+ issue) #1565
Labels
No labels
Atrac3+
Audio
CPU emulation
D3D11
D3D9 (removed)
Depth / Z
Feature Request
Font Atlas
GE emulation
Guardband / Range Culling
HLE/Kernel
I/O
Input/Controller
MP3
Multithreading
Needs hardware testing
Networking/adhoc/infrastructure
No Feedback / Outdated?
OpenGL
PGF / sceFont
PSMF / MPEG
Platform-specific (Android)
Platform-specific (Windows)
Platform-specific (iOS)
PowerVR GPU
SDL2
Saving issue
User Interface
Vulkan
arm64jit
armjit
armv6
x86jit
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/ppsspp#1565
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @solarmystic on GitHub (Sep 16, 2013).
Original GitHub issue: https://github.com/hrydgard/ppsspp/issues/3805
Issue is as stated in the title.
First singular responsible build/commit found via bisecting is v0.9.1-833-gcf8a3e4
github.com/hrydgard/ppsspp@cf8a3e4df1which was merged into master with v0.9.1-842-g00f8ae5github.com/hrydgard/ppsspp@00f8ae5f2dhttps://github.com/hrydgard/ppsspp/pull/3772Last unaffected revision is the one right before the commit was merged into master, v0.9.1-841-g26f935b
github.com/hrydgard/ppsspp@26f935bbe5Problem reproduction:-
cf8a3e4df1command). Make sure you've obtained the at3+ audio plugin and have it enabled in the Audio settings of the emulator.Windows Error Report:-
Stack Trace from affected 64bit debug build:-
Stack Trace from affected 32bit debug build:-
Logfile from an affected 64bit debug build until it crashes:-
http://www.mediafire.com/download/9g0wbo629ngccuq/ppssppDebug64.7z
Logfile from an affected 32bit debug build until it crashes:-
http://www.mediafire.com/download/623e117assjgycz/ppssppDebug32.7z
Temporary solution:-
Play through that entire section without the atrac3+ plugin enabled. Game will not crash if the plugin is either disabled or missing from your ppsspp directory. Game will lack BGM if you do that though.
@hrydgard commented on GitHub (Sep 16, 2013):
I bet something causes it to try to decode invalid data as atrac3. The plugin is very intolerant to that unfortunately.
@unknownbrackets commented on GitHub (Sep 16, 2013):
As far as I can tell from tests, this function basically asks the question, "where do I need to seek to in the file to start at this sample offset?"
The previous code said "the end." Always. Ys Seven got super confused by this answer, it's well above the range it expected to get. Now it returns the proper offset into the file where that sample starts. This number matches what the PSP outputs for the test files I tried.
I changed a few other things:
It could be another syscall not handling errors properly, leading to bad data passed to this one or other ones in a way the game did not anticipate. Or, maybe for some types of atrac files these outputs are totally different (this is why I hate the atrac/mpeg/sas stuff.) I also noticed that the values of writeable bytes / min write bytes were wrong before (and they're still wrong.) Maybe with the correct file offset, games are now noticing this flaw.
Or it could even just be a bug in this atrac plugin thing.
-[Unknown]
@sum2012 commented on GitHub (Sep 17, 2013):
Does ppsspp have a check this as JPCSP ?
@unknownbrackets commented on GitHub (Sep 17, 2013):
Definitely that mask business is not correct, per the audio/atrac/ids test there are a lot less and they are affected by sceAtracReinit().
sceAtracGetNextDecodePosition() does not return a proper error with a bad id.
-[Unknown]
@dbz400 commented on GitHub (Sep 17, 2013):
Humm GTA seems to be much more easier to reproduce .
@dbz400 commented on GitHub (Sep 17, 2013):
Alright .Everytime it crashed , sample is 0
@dbz400 commented on GitHub (Sep 17, 2013):
This is the log after i change this
36:30:069 idle0 I[SCEGE]: GLES\Framebuffer.cpp:1248 Decimating FBO for 04000000 (64 x 64 x 0), age 6
36:30:080 mix sound th I[ME]: HLE\sceAtrac.cpp:986 sceAtracReleaseAtracID(0)
36:30:089 mix sound th I[ME]: HLE\sceAtrac.cpp:1292 0=sceAtracSetHalfwayBufferAndGetID(08bfec80, 00001000, 00040000)
36:30:089 mix sound th W[ME]: HLE\sceAtrac.cpp:1173 This is an atrac3+ stereo audio
36:30:089 mix sound th W[ME]: HLE\sceAtrac.cpp:764 sceAtracGetBufferInfoForResetting(0, 0, 09fcf238): invalid sample position
36:30:090 mix sound th I[ME]: HLE\sceAtrac.cpp:992 sceAtracResetPlayPosition(0, 0, 0, 0)
36:30:090 mix sound th I[ME]: HLE\sceAtrac.cpp:1307 sceAtracSetLoopNum(0, -1)
36:30:128 threadmain I[SCEGE]: GLES\Framebuffer.cpp:603 Creating FBO for 04000000 : 64 x 64 x 0
36:30:131 threadmain I[SCEGE]: GLES\Framebuffer.cpp:603 Creating FBO for 04044000 : 64 x 64 x 0
36:42:145 idle0 I[SCEGE]: GLES\Framebuffer.cpp:1275 Destroying FBO for 00178000 : 480 x 272 x 3
@unknownbrackets commented on GitHub (Sep 17, 2013):
What is Sampleoffset in that case?
I don't think 0 is an invalid sample offset at all. But maybe getDecodePosBySample() is giving an incorrect number for sample offset 0 in some files.
-[Unknown]
@dbz400 commented on GitHub (Sep 17, 2013):
Sampleoffset is 96
@unknownbrackets commented on GitHub (Sep 17, 2013):
What happens if you force it to 0?
-[Unknown]
@dbz400 commented on GitHub (Sep 17, 2013):
int Sampleoffset = 0; ?
@unknownbrackets commented on GitHub (Sep 17, 2013):
Yeah. Maybe when sample is 0, the offset should be 0 also.
-[Unknown]
@sum2012 commented on GitHub (Sep 17, 2013):
My Stack Trace is difficult
@dbz400 commented on GitHub (Sep 17, 2013):
Humm still crash
@sum2012 commented on GitHub (Sep 17, 2013):
I have tried this code.But still crash in release mode.(debug mode no crash)
sceAtrac3plus.cpp
bool Decode(Context context, void* inbuf, int inbytes, int outbytes, void outbuf) {
...
void* buf;
__try {
int ret = frame_decoder(context, inbuf, inbytes, &channels, &buf);
if (ret != 0) {
*outbytes = 0;
return false;
}
}
__except( EXCEPTION_EXECUTE_HANDLER )
{
return false;
}
@sum2012 commented on GitHub (Sep 17, 2013):
This is JPCSP trace log of this setting
sceAtracGetNextDecodePosition 0xE23E3A35 2
https://gist.github.com/sum2012/e8634c5443f8b007ba92
Seem PSP do not return error in sceAtracGetNextDecodePosition
@unknownbrackets commented on GitHub (Sep 18, 2013):
And sceAtracGetBufferInfoForResetting never returns an error either, right? (might need to trace sceAtracGetBufferInfoForReseting also.)
https://code.google.com/p/jpcsp/source/browse/trunk/src/jpcsp/HLE/modules150/sceAtrac3plus.java#556
JPCSP seems to put the same value in writable bytes as in min write bytes, which is definitely not what the PSP actually does. But it also seems to use the sample offset.
AFAICT JPCSP never returns an error either. Does this game work in JPCSP and play music fine? I realize it does not use the plugin thing.
Does it help to set atrac->currentSample = 0 or anything? Maybe it needs to reset the packets it's already sent to the decoder when this function is called, or somewhere else? Maybe it's crashing because of a mix of old packets + new packets after seeking?
-[Unknown]
@dbz400 commented on GitHub (Sep 18, 2013):
I will try this out shortly to set atrac->currentSample = 0
@sum2012 commented on GitHub (Sep 18, 2013):
Yes,sceAtracGetBufferInfoForReseting never returns an error in JPCSP trace log.
https://gist.github.com/anonymous/3f0bc7c3ee5130c4ab05
edit:JPCSP play the music fine
@dbz400 commented on GitHub (Sep 18, 2013):
@unknownbrackets , tried atrac->currentSample = 0 or atrac->currentSample = sample , also crash .
@solarmystic commented on GitHub (Sep 18, 2013):
This is related to the issue so I'll post it here instead of opening another redundant issue report page.
Apparently Grand Theft Auto: Vice City Stories (ULUS10160) also has the same exact issue as this game, with the same reponsible original commit
github.com/hrydgard/ppsspp@cf8a3e4df1I discovered it purely by accident when testing the game for this particular @DanyalZia pull request https://github.com/hrydgard/ppsspp/pull/3822
Problem reproduction:-
Same as GTA Chinatown Stories, start a New Game with the atrac3+ codec enabled in any 32 or 64bit build from v0.9.1-842-g00f8ae5 onwards with default settings, skip through the opening cutscenes and the game will crash, taking down the emulator with it.
Disabling the atrac3+ plugin will mitigate the issue for now.
Stack Trace from 32bit debug build :-
Stack Trace from 64bit debug build :-
Logfile from an affected 64bit debug build:-
http://www.mediafire.com/?w27q4xjn4i6nzdk
Logfile from an affected 32bit debug build:-
http://www.mediafire.com/?dcvg710v3mxz9e1
@sum2012 commented on GitHub (Sep 18, 2013):
Stack Trace from from 32 bit (of Grand Theft Auto Chinatown Wars ) of at3plusdecoder.dll
@raven02 Window 32 bit debug at3plusdecoder.dll
you can test more to debug
https://docs.google.com/file/d/0B3OaSdeV0L8kNnEwaldyQlFOODA/edit?usp=sharing
@dbz400 commented on GitHub (Sep 19, 2013):
I'm wondering if Memory::Write_U32(1, outposAddr); helps when invalid ID detected. (I cannot test it right now unfornaturely)
In sceAtracGetNextDecodePosition()
@sum2012 commented on GitHub (Sep 19, 2013):
@raven02 no help
@dbz400 commented on GitHub (Sep 19, 2013):
Ooops . Thanks for testing
@dbz400 commented on GitHub (Sep 20, 2013):
When crash , i got more detail on the values of Sampleoffset ,sample , minWritebytes
@dbz400 commented on GitHub (Sep 20, 2013):
14:40:130 I[HLE]: HLE\sceAtrac.cpp:770 Sampleoffset = 29743376, sample = 108776854 , minWritebytes = 29741328
14:40:181 I[HLE]: HLE\sceAtrac.cpp:770 Sampleoffset = 18855296, sample = 68957323 , minWritebytes = 18853248
14:53:949 I[HLE]: HLE\sceAtrac.cpp:770 Sampleoffset = 96, sample = 0 , minWritebytes = 0
@unknownbrackets commented on GitHub (Sep 20, 2013):
Hmm, I think that value of minWritebytes is crazy.
What is atrac->first.writableBytes in this case?
I'm not really sure I understand these values, but it sounds like it's running against a very long atrac3+ file with a relatively small buffer, so that's probably what we should test against.
-[Unknown]
@dbz400 commented on GitHub (Sep 20, 2013):
This is the values before crash
33:32:950 mix sound th W[ME]: HLE\sceAtrac.cpp:1174 This is an atrac3+ stereo audio
33:32:950 mix sound th I[HLE]: HLE\sceAtrac.cpp:772 atrac->first.filesize = 845976 , atrac->first.size = 4096, atrac->atracBufSize = 262144, atrac->first.writableBytes = 262144
@dbz400 commented on GitHub (Sep 20, 2013):
I just tried to revert this line and seems to be it crashes on this line previously
from
to
@dbz400 commented on GitHub (Sep 20, 2013):
Therefore , we are passing 96 to bufferInfo->first.filePos now while previously we are passing 4096 to bufferInfo->first.filePos.
bufferInfo->first.filePos = atrac->first.fileoffset;
INFO_LOG(HLE,"atrac->first.fileoffset = %i , Sampleoffset = %i", atrac->first.fileoffset , Sampleoffset);
@unknownbrackets commented on GitHub (Sep 20, 2013):
Well,
atrac->first.fileoffsetis for sure wrong, at least in the common cases I tested. Just to clarify, are you saying that 4096 also crashes, or does not crash?-[Unknown]
@dbz400 commented on GitHub (Sep 20, 2013):
4096 doeesn't crash . I also think bufferInfo->first.filePos = atrac->first.fileoffset; is wrong.
@solarmystic commented on GitHub (Sep 22, 2013):
Apparently Class of Heroes also has the exact same issue, according to mr.chya from the ppsspp forums, which I verified by asking him to do some further testing.
http://forums.ppsspp.org/showthread.php?tid=419&pid=49712#pid49712
http://forums.ppsspp.org/showthread.php?tid=419&pid=49782#pid49782
Logfile courtesy of mr.chya from the forums:-
http://forums.ppsspp.org/attachment.php?aid=8640
@dbz400 commented on GitHub (Sep 24, 2013):
For this issue .I'm wondering if we can use a workaround with comment here at least temporarily avoid this issue as there are few games already affected by this bug .
@unknownbrackets commented on GitHub (Sep 24, 2013):
Wait, is sample negative? It's 0, right?
I expect other games to use sample=0 in very normal ways and probably wig out if it returns an error and fails to update the buffer properly.
-[Unknown]
@dbz400 commented on GitHub (Sep 24, 2013):
Should be 0 .I just put < in case there is sample indeed return negative though i'm not sure any games will do .
@unknownbrackets commented on GitHub (Sep 24, 2013):
Well, for example in the audio/atrac/reseting test (which you can run in ppsspp without even a psp):
O 1: write=p+0x0, writable=00005a84, min=00000000, file=00000060
E 1: write=p+0x0, writable=00005998, min=000002f0, file=00000060
So, in that case, it's expecting minWriteBytes to be 0x2f0, and writable is off by a bit (too high by 0xEC.) I'm not sure why. But it does use 0x60 (96) for 0. What happens if you use minWriteBytes = 0x2f0? There's probably more to it, but I don't have the game to reproduce the crash...
-[Unknown]
@dbz400 commented on GitHub (Sep 24, 2013):
Humm do you mean simply change the following to see if it still crash ?
to
@unknownbrackets commented on GitHub (Sep 24, 2013):
Yes. Just want to see what variables might be involved.
-[Unknown]
@dbz400 commented on GitHub (Sep 24, 2013):
Alright .I will test it soon .
@dbz400 commented on GitHub (Sep 24, 2013):
I don't have the testing env this moment .However , if 0x2f0 didn't crash , that's mean
Sampleoffset - (int)atrac->first.size return negtaive value then .
14:53:949 I[HLE]: HLE\sceAtrac.cpp:770 Sampleoffset = 96, sample = 0 , minWritebytes = 0
@dbz400 commented on GitHub (Sep 24, 2013):
0X2f0 is 752 . If (int)atrac->first.size = 848 then make sense (assume it is (int)atrac->first.size - Sampleoffset) = 848(atrac->first.size) - 96(SampleOffset) = 752(minWritebytes)
@dbz400 commented on GitHub (Sep 24, 2013):
Humm no luck .Tried minWritebytes = 0x2f0 , still crash
@sum2012 commented on GitHub (Sep 24, 2013):
@raven02 I try your hack code but don't work ?
@dbz400 commented on GitHub (Sep 24, 2013):
Humm it should work .Let me try again .
@dbz400 commented on GitHub (Sep 24, 2013):
Just tried .It is okay and no crash.
@sum2012 commented on GitHub (Sep 24, 2013):
@raven02
Copy your code inside
u32 sceAtracGetBufferInfoForResetting(int atracID, int sample, u32 bufferInfoAddr) ?
@dbz400 commented on GitHub (Sep 24, 2013):
Yes, right after
@sum2012 commented on GitHub (Sep 24, 2013):
Not sure don't work ...
Maybe I am using Win32 build and you are using Win 64 build ?
@sum2012 commented on GitHub (Sep 24, 2013):
@raven02
I just another try.Can you try this code hack work or not ? (I work)
if (inbytes > 0 && inbytes == atrac->atracBytesPerFrame) change to
if (inbytes > 0 && inbytes == atrac->atracBytesPerFrame && SamplesNum < finish) {
@dbz400 commented on GitHub (Sep 24, 2013):
I'm using 32bit build as well .Will try your code.
@dbz400 commented on GitHub (Sep 24, 2013):
It works well and no crash . @unknownbrackets , what do you think ?
@dbz400 commented on GitHub (Sep 24, 2013):
The following one seems to be the function called to crash
Atrac3plus_Decoder::Decode(atrac->decoder_context, atrac->data_buf + atrac->decodePos, inbytes, &decodebytes, buf);
int ret = frame_decoder(context, inbuf, inbytes, &channels, &buf);
@dbz400 commented on GitHub (Sep 24, 2013):
@sum2012 , i think it just somehow skipped the decoding at all as there should be conversation among those soliders as well as background music.
@sum2012 commented on GitHub (Sep 24, 2013):
@raven02 So that I call this one is a hack code.
@unknownbrackets commented on GitHub (Sep 25, 2013):
Does it work fine in JPCSP still? I noticed some of the results from the tests I did were implemented in JPCSP:
http://code.google.com/p/jpcsp/source/detail?r=3392
(again I realize it uses a different decoder...)
-[Unknown]
@dbz400 commented on GitHub (Sep 25, 2013):
Yep, JPCSP seems to be changed the logic for BufferInfoForResetting. Trying to figure out hw to translate them to here
@unknownbrackets commented on GitHub (Sep 28, 2013):
Seems to be affecting Tales of the World Radiant Mythology 3 as well, most likely.
http://forums.ppsspp.org/showthread.php?tid=1807&pid=50652#pid50652
-[Unknown]
@sum2012 commented on GitHub (Sep 28, 2013):
I think that should fix\work arround this before major release
@dbz400 commented on GitHub (Oct 9, 2013):
I think it would be good to know if the new atrac3+ ffmpeg implementation did same crash here.
@solarmystic commented on GitHub (Oct 17, 2013):
This issue should be labelled with the atrac3+ label.
@solarmystic commented on GitHub (Oct 19, 2013):
This game no longer crashes upon starting a New Game after https://github.com/hrydgard/ppsspp/pull/4221 was merged into master.
However, it cannot be considered fixed yet, as the BGM just stops playing instead at that segment instead of crashing, so a bug is still present.
Notably, the crash related to atrac3+ when loading a savegame in GTA Vice City Stories https://github.com/hrydgard/ppsspp/issues/3805#issuecomment-24659414 also no longer occurs after the maxim stuff got merged to master as well, consider that one fixed.
@solarmystic commented on GitHub (Oct 19, 2013):
Logfile is also spamming these lines repeatedly too:
@hrydgard commented on GitHub (Oct 19, 2013):
probably another case of the game playing junk data, confusing instead of crashing the new Atrac3+ code.
@solarmystic commented on GitHub (Nov 12, 2013):
Finally properly fixed with https://github.com/hrydgard/ppsspp/pull/4483 with the
github.com/hrydgard/ppsspp@225f8efbb9commit provided by @papelThe game no longer crashes at the aforementioned spot, and the BGM doesn't cut off either, it continues to play throughout the entire sequence.
https://github.com/hrydgard/ppsspp/issues/3805#issuecomment-24659414 in GTA VCS is also properly fixed, with game now playing the appropriate dialogue during that opening scene, with no crashes.
Big thanks to @papel for the responsible commit that fixed it.
Closed.
@sum2012 commented on GitHub (Nov 12, 2013):
It is a good news :)
@unknownbrackets commented on GitHub (Nov 12, 2013):
Just to say, I'm pretty certain I can create a test that shows this is not "properly" fixed. It may be fine for now, but that doesn't mean it's what the PSP does, but I don't like things that aren't at all what the PSP does being called "proper." The only proper fix is to make it work the way the PSP observably does. An unknown quantity (possibly zero, probably less than before) of games could depend on correct behavior.
-[Unknown]
@hrydgard commented on GitHub (Nov 12, 2013):
Agree, but was the previous behaviour matching the PSP in all cases? If not we are choosing between two hacks, and if we're at that stage, might as well choose the one that fixes the most games. Ideally of course we should match the PSP's behaviour, we need a better testsuite.
@unknownbrackets commented on GitHub (Nov 12, 2013):
Again, I'm just saying it's not a "proper" fix. It may be a "good hack."
-[Unknown]
@hrydgard commented on GitHub (Nov 12, 2013):
Right, yeah.
@ydamigos commented on GitHub (Nov 12, 2013):
@unknownbrackets and @hrydgard if you own a PSP which is the best tool to examine PSP's behaviour? pspautotests, jpcsptrace?
@unknownbrackets commented on GitHub (Nov 12, 2013):
The combination of the two. However, pspautotests is probably better for testing how something should work. JpcspTrace is better for seeing how it's being used.
-[Unknown]
@ydamigos commented on GitHub (Nov 12, 2013):
I'll give it a try to find out how sceAtracGetBufferInfoForResetting should work. First thing to find documentation of these tools.
@unknownbrackets commented on GitHub (Nov 12, 2013):
http://forums.ppsspp.org/showthread.php?tid=2822&pid=56026#pid56026
https://github.com/hrydgard/pspautotests#readme
https://github.com/hrydgard/pspautotests/blob/master/tests/audio/atrac/resetting.expected
-[Unknown]
@ydamigos commented on GitHub (Nov 12, 2013):
@unknownbrackets thank you!!!
@solarmystic commented on GitHub (Nov 12, 2013):
Sorry for the improper use of "proper", I should've clarified what I meant.
What I meant by "proper" was that the game just exhibits the correct behaviour since the commit instead of being muted like before, not that the fix itself is "proper".