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

Use ringbuffer as a real PSP instead the huge buffer. #2284

Merged
merged 7 commits into from Jun 26, 2013

Conversation

@oioitff
Copy link
Contributor

oioitff commented Jun 15, 2013

It may break some previous fixes, but it should be more like a real PSP, and it seems to work fine. I'm not sure if it broke some other games, maybe need more tests.
And the savestate for mpeg video part is still uncompleted, but at least mpeg audio part has been done.

@unknownbrackets
unknownbrackets reviewed Jun 15, 2013
View changes
Core/HW/MediaEngine.cpp Outdated
break;

#ifdef USE_FFMPEG
// Don't seek, just return the full size.
// Returning this means FFmpeg won't think frames are truncated if we don't have them yet.
case AVSEEK_SIZE:
return mpeg->m_streamSize;
return 0xFFFFF;

This comment has been minimized.

Copy link
@unknownbrackets

unknownbrackets Jun 15, 2013

Collaborator

Hmm, I'm not sure about this. We still know the full streamSize right? I think we should still return it.

Also, we should probably just return -1 if we can't do SEEK_END...

-[Unknown]

This comment has been minimized.

Copy link
@oioitff

oioitff Jun 15, 2013

Author Contributor

Right now _MpegSeekbuffer is just fake. Maybe the streamSize should be unlimited, I guessed. For an example, in Wipeout Pulse, the video playback is unlimited.

This comment has been minimized.

Copy link
@unknownbrackets

unknownbrackets Jun 22, 2013

Collaborator

Should we just remove the function, then?

-[Unknown]

return false;

u8* tempbuf = (u8*)av_malloc(m_bufSize);

m_pFormatCtx = avformat_alloc_context();
m_pIOContext = avio_alloc_context(tempbuf, m_bufSize, 0, (void*)this, _MpegReadbuffer, NULL, _MpegSeekbuffer);
m_pIOContext = avio_alloc_context(tempbuf, m_bufSize, 0, (void*)this, _MpegReadbuffer, NULL, 0);

This comment has been minimized.

Copy link
@unknownbrackets

unknownbrackets Jun 15, 2013

Collaborator

Oh, but you removed seeking entirely? I guess that's probably right, but I wonder about the corruption...

-[Unknown]

This comment has been minimized.

Copy link
@oioitff

oioitff Jun 15, 2013

Author Contributor

It seems to be no use right now, so I just remove it. But it's probably necessary for implementing savestate.

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 15, 2013

I think this is a good direction in general but we should probably wait until after 0.8.

-[Unknown]

@hrydgard

This comment has been minimized.

Copy link
Owner

hrydgard commented Jun 15, 2013

Indeed, this is a good direction. I will release 0.8 next week, and I'll merge this immediately after.

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 15, 2013

Yep, that's all right :)

@raven02

This comment has been minimized.

Copy link
Contributor

raven02 commented Jun 16, 2013

Just tested this commit with bunch of games and all seems to be working fine and no broken and major issues . Only minor issues is the corrupted early key frame (removal of mpeg->m_streamSize;?)

Dai2Ji Super Robot Taisen Z Saisehen
Dainiji_Super_Robot_Taisen_Z_Hakaihen
Dissidia 012 Duodecim Final Fantasy
God of War Chains of Olympus
3rd_birthday
Castlevania The Dracula X Chronicles
Final Fantasy Type-0
Final Fantasy VII Crisis Core
God Eater Burst
Gundam AGE Universe
Gundam Vs Gundam Next Plus
Kingdom Hearts Birth by Sleep
Sol_Trigger
Wipeout Pulse
Sword_Art_Online
Project Diva 2 extend

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 16, 2013

Yep, this pull request would break some previous fixes for early key frames, thought that issue is still unsolved now. We need to find a right way to fix that issue later :)

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 16, 2013

If we can't seek, the only way is to demux properly, and only send full frames, I think.

-[Unknown]

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 16, 2013

Maybe there is still a better way, and is possible to solve the savestate too.
The main problem for mpeg is that, ffmpeg need first video frame to init the decoder. After that, ffmpeg could just load each frame and then decode one frame.
(Edit: When ffmpeg is trying to init, it would try to load all data as it could, and think that is all data it need to decode. After all data decoded, it would begin to load a frame and then decode a frame.)

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 16, 2013

I think sceMpeg on a PSP may return NO_DATA for the first few calls while it buffers, we may end up doing that.

If we can determine how much data we need to init the decoder properly, we could just savestate that and re-init afterward.

-[Unknown]

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 16, 2013

Hmm, I just test out that ffmpeg just need 4096 bytes to init the decoder ( actually should be "mpegoffset + 2048" bytes frame infos).
And it seems the early key frames issue is also solved if we just use first "mpegoffset + 2048" bytes to init the decoder :)

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 16, 2013

Ah, that makes sense, because then it can't read a partial frame probably.

-[Unknown]

@raven02

This comment has been minimized.

Copy link
Contributor

raven02 commented Jun 16, 2013

Good to hear .Just wonder what would be the exact change ? i can then test out here in those problematic games .

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 16, 2013

I have not written a correct fix for it now. But here is a fake fix for testing if you wanted:

bool MediaEngine::openContext() {
    ...
    if (m_pFormatCtx || !m_pdata)
        return false;
+   int mpegoffset = bswap32(*(int*)(m_pdata->bufQueue + 8));
+   int dataend = m_pdata->end;
+   m_pdata->end = mpegoffset + 2048;
    ...
    m_isVideoEnd = false;
    m_isAudioEnd = false;
+   m_pdata->end = dataend;
    ...
}
@raven02

This comment has been minimized.

Copy link
Contributor

raven02 commented Jun 16, 2013

Thanks @oioitff .Some testing results from my list of early key frame issue.

Sol Trigger
2

Three Kindgdom of Romance 8
1

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 16, 2013

@raven02 Thanks. So maybe it's better to use first video frame to init decoder than use first 2048 bytes.

@raven02

This comment has been minimized.

Copy link
Contributor

raven02 commented Jun 16, 2013

Yep .Looking forward to test your proper fix.

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 17, 2013

By the way, God Eater's demo seems to require that the first sceMpegGetAvcAu() returns ERROR_MPEG_NO_DATA. It hits a BREAK right now, but this quick hack:

    static int j = 0;
    if (++j < 2) {
        return ERROR_MPEG_NO_DATA;
    }

At the very top makes it run fine. Possibly sceMpegGetAvcAu() should only return 0 when the ringbuffer has a full frame in it?

-[Unknown]

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 17, 2013

Hmm, that means a real PSP just needs more data to start decoding. Before that, it should return ERROR_MPEG_NO_DATA .

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 17, 2013

That makes sense because AFAICT, Patapon 3 (and I think probably Burnout) just keeps feeding in packets, even after the video ends. It's manually looping the video probably seeking back to the start once it hits the end. So basically the real firmware probably doesn't have a "video end reached", just runs out of packets.

-[Unknown]

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 17, 2013

Yep, actually right now I can not see any games using sceMpeg that hit "video end reached" and then end the video. And sceMpeg doesn't provide any loop functions, it just try to decode all the input data.

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 17, 2013

FWIW, here are a list of games which might be affected by that problem / improved by this:
http://report.ppsspp.org/temp/recent/kind/178
(although some may be for other reasons.)

-[Unknown]

@hrydgard

This comment has been minimized.

Copy link
Owner

hrydgard commented Jun 17, 2013

Thinking I might merge this anyway. Have you found any bad side effects yet?

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 17, 2013

I think right now it brings back the corrupted frames, at least.... no? I haven't tested it myself yet.

-[Unknown]

@solarmystic

This comment has been minimized.

Copy link
Contributor

solarmystic commented Jun 17, 2013

@hrydgard

Corrupted frames would return and have to be fixed again.

@hrydgard

This comment has been minimized.

Copy link
Owner

hrydgard commented Jun 17, 2013

Ok I'll wait.

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 17, 2013

Savestate for mpeg is working now.
Not sure if I was missing something.

@raven02

This comment has been minimized.

Copy link
Contributor

raven02 commented Jun 18, 2013

@oioitff , i have to move the following DoState from private to public in order to compile successfully . not too sure if it is correct .

void DoState(PointerWrap &p) {
    p.Do(m_index);
    p.Do(m_len);
    p.Do(m_audioChannel);
    p.Do(m_readSize);
    if (m_buf)
        p.DoArray(m_buf, m_len);
    p.DoClass(m_audioStream);
}

If yes , compliled one caused movie in Sol Trigger shown below and stop for around 5 seconds before moving on again

1

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 18, 2013

@raven02 Thanks. I notice some bugs and fix it. Does it work well now?

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 18, 2013

Thanks, maybe it's better to open a new issue for that.

@thedax

This comment has been minimized.

Copy link
Collaborator

thedax commented Jun 18, 2013

@oioitff : Here's a debug log for Diva 2nd on the latest master: http://www.mediafire.com/download/e89h1bd00ws2n62/ppsspp-diva-latestmaster.7z

Maybe it'll help resolve the keyframes issue.

@unknownbrackets
unknownbrackets reviewed Jun 22, 2013
View changes
Core/HLE/scePsmf.cpp Outdated
}
else
{
INFO_LOG(HLE, "scePsmfPlayerSetPsmfOffset(%08x, %s, %i): invalid psmf player", psmfPlayer, filename, offset);

This comment has been minimized.

Copy link
@unknownbrackets

unknownbrackets Jun 22, 2013

Collaborator

*ERROR_LOG, no?

-[Unknown]

This comment has been minimized.

Copy link
@oioitff

oioitff Jun 23, 2013

Author Contributor

Oh, yeah, ERROR_LOG is better.

p.Do(filesize);

p.Do(status);
p.Do(psmfPlayerAvcAu);

This comment has been minimized.

Copy link
@unknownbrackets

unknownbrackets Jun 22, 2013

Collaborator

Savestates seem to work great, but is there any need to save tempbuf? Just asking.

-[Unknown]

This comment has been minimized.

Copy link
@oioitff

oioitff Jun 23, 2013

Author Contributor

Hmm, tempbuf doesn't save any data, it's just a temp bufffer for loading mpeg data and then sending them to MediaEngine. So I think there is no need to save tempbuf.

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 22, 2013

This works great, and for me, I didn't notice any corruption. I tried all the games I could think of which have been picky about video handling.

It also fixed Patapon 3 as expected, and savestates work perfect - nice.

The only problem is, it breaks Wipeout Pure again... exactly the same as before.

-[Unknown]

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 22, 2013

In Wipeout Pure, m_pdata->getRemainSize() eventually returns 513 packets. The ringbuffer size is 512 packets, so even with the - 2048, this ends up returning 512 packets. The game clearly doesn't expect that, so it so it locks the video up.

Using - 4096 does actually work, but that feels like a hack. Before I'd attempted to make it based on actual rendered frames, which worked better.

Unfortunately, - 4096 breaks Ys 6 and probably other games, so it's not the solution.

-[Unknown]

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 23, 2013

m_pdata->getRemainSize() returns the remain space in Ringbuffer. Right now the difference from PPSSPP and a real PSP is that:
In PPSSPP, after the data in Ringbuffer is loaded (no need to be decoded), the space was free.
In a real PSP, after the data in Ringbuffer is decoded, the space was free.
Hmm, so maybe we could try to get more infos from ffmpeg?

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 23, 2013

@unknownbrackets Does Wipeout Pure work fine now?

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 23, 2013

Unfortunately, that doesn't make Wipeout Pure any happier...

-[Unknown]

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 23, 2013

Hmm, I wonder how much data is in the ringbuffer when that game called openContext?

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 23, 2013

First video:
Stream offset: 2048, Stream size: 0x41000
First timestamp: 90000, Last timestamp: 870780
addStreamData: +65536, popped 1, remain: 985088

Note that it plays this video a bit, pauses it... let's me select something, and then continues playing it. Later into the video, it just stops cold. This video I can press a button to skip, but it was playing all the way before.

Next video which gets almost all the way, then stops I think:
Stream offset: 2048, Stream size: 0x940000
First timestamp: 90000, Last timestamp: 8642544
addStreamData: +65536, popped 1, remain: 985088

And then the unskippable video:
Stream offset: 2048, Stream size: 0xC000
First timestamp: 90000, Last timestamp: 234144
addStreamData: +49152, popped 1, remain: 1001472

IIRC from before (could be wrong), it basically reads the entire video (it's very short) in all at once. It's probably hanging because it knows there are more frames to render, and the ringbuffer says it's empty, but there ain't no more data left in the file.

-[Unknown]

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 23, 2013

Hmm, for the third video, I think we could just lock the last data till all frames decoded. So how about the first video?

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 23, 2013

FWIW, when I tried to replicate on PSP firmware with the video from Wipeout (the third one), it went like this:

[x] sceMpegRingbufferAvailableSize: 512
[x] mpeg_callback called: 00000000, 24, 00001234
[x] sceMpegRingbufferPut: 00000018
[x] sceMpegRingbufferAvailableSize: 488

Right away sceMpegAvcDecode returned 0 (not sure of the rules here yet..) Anyway, here are some excerpts:

[x] sceMpegGetAvcAu: 00000000 (00000001)
[x] Au (interesting): presented=0/90000, decoded=0/86997, es=AVC, size=160
[r] sceMpegAvcDecode: 00000000 (00000200, 00000001)
[x] sceMpegRingbufferAvailableSize: 488
...
(frame 15 - note that the first few frames are mostly white iirc.)
[x] sceMpegRingbufferAvailableSize: 491
(frame 20)
[x] sceMpegRingbufferAvailableSize: 492
(frame 27)
[x] sceMpegRingbufferAvailableSize: 493
(frame 29)
[x] sceMpegRingbufferAvailableSize: 494
(frame 30)
[x] sceMpegRingbufferAvailableSize: 495
(frame 31)
[x] sceMpegRingbufferAvailableSize: 497
(frame 32)
[x] sceMpegRingbufferAvailableSize: 498
(frame 34)
[x] sceMpegRingbufferAvailableSize: 500
(frame 37)
[x] sceMpegRingbufferAvailableSize: 501
(frame 41)
[x] sceMpegRingbufferAvailableSize: 503
(frame 43)
[x] sceMpegRingbufferAvailableSize: 504
(frame 46)
[x] sceMpegRingbufferAvailableSize: 506
(frame 48)
[x] sceMpegRingbufferAvailableSize: 512
[r] sceMpegAvcDecode: 80628002

And that was the end. Note that this seemed to involve I/B/P frames:
http://vsr.informatik.tu-chemnitz.de/~jan/MPEG/HTML/mpeg_tech.html

The basic idea is that B frames need I and P frames to be decoded properly (they're basically half way in between the two, from my very limited understanding.) So the bitstream (the file) has I and P first, and then a bunch of Bs. The decoder has to use the P frame to decode the Bs, but not render it yet - it gets reordered to the end, before the next I frame probably.

One very likely possibility is that while they P frame is in a particular packet, that packet cannot be consumed (it still needs the packet to later present the P frame, and even to decode future B frames in later packets.) So, probably that's why the final jump from 506 - 512, there's probably a bunch of B frames in there or something.

But I'm not really an expert on video so my understanding of the above is a little sketchy.

-[Unknown]

@oioitff

This comment has been minimized.

Copy link
Contributor Author

oioitff commented Jun 26, 2013

Hmm, does it still break anything now?

@unknownbrackets

This comment has been minimized.

Copy link
Collaborator

unknownbrackets commented Jun 26, 2013

Both Wipeout Pure and Patapon 3 seem happy with this. Don't have time to test others, but looks good.

-[Unknown]

@hrydgard

This comment has been minimized.

Copy link
Owner

hrydgard commented Jun 26, 2013

Okay, I'll merge after I've pushed 0.8.1 tonight with crashfixes.

@thedax

This comment has been minimized.

Copy link
Collaborator

thedax commented Jun 26, 2013

I tested ~15 games or so and none of them seemed to have cutscene issues(including Project Diva 2nd, which used to have corrupted keyframes with this being applied, and is finally happy with this, as is Peace Walker), so it looks like we finally have a home run here. Impressive how far it's come in a week + 3 days.

@hrydgard

This comment has been minimized.

Copy link
Owner

hrydgard commented Jun 26, 2013

Let's go! :)

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