Skip to content

Commit

Permalink
Fix video playback in LocoRoco 2. scePsmfPlayerGetInfo had a couple m…
Browse files Browse the repository at this point in the history 
…ore parameters.

Fixes #7887.
  • Loading branch information
@hrydgard
hrydgard committed Feb 17, 2017
1 parent a362af7 commit e9d5eb6
Showing 1 changed file with 29 additions and 6 deletions.
35 changes: 29 additions & 6 deletions Core/HLE/scePsmf.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "Core/HLE/FunctionWrappers.h"
#include "Core/HLE/scePsmf.h"
#include "Core/HLE/sceMpeg.h"
#include "Core/HLE/sceKernelMemory.h"
#include "Core/HW/MediaEngine.h"
#include "GPU/GPUInterface.h"
#include "GPU/GPUState.h"
Expand Down Expand Up @@ -206,7 +207,7 @@ class Psmf {
class PsmfPlayer {
public:
// For savestates only.
PsmfPlayer() : filehandle(0), finishThread(nullptr) {
PsmfPlayer() : filehandle(0), finishThread(nullptr), videoWidth(480), videoHeight(272) {
mediaengine = new MediaEngine();
}
PsmfPlayer(const PsmfPlayerCreateData *data);
Expand Down Expand Up @@ -260,6 +261,9 @@ class PsmfPlayer {
int warmUp;
s64 seekDestTimeStamp;

int videoWidth;
int videoHeight;

SceMpegAu psmfPlayerAtracAu;
SceMpegAu psmfPlayerAvcAu;
PsmfPlayerStatus status;
Expand Down Expand Up @@ -411,7 +415,7 @@ PsmfPlayer::PsmfPlayer(const PsmfPlayerCreateData *data) {
playSpeed = 1;
totalDurationTimestamp = 0;
status = PSMF_PLAYER_STATUS_INIT;
mediaengine = new MediaEngine;
mediaengine = new MediaEngine();
finishThread = nullptr;
filehandle = 0;
fileoffset = 0;
Expand Down Expand Up @@ -480,7 +484,7 @@ void Psmf::DoState(PointerWrap &p) {
}

void PsmfPlayer::DoState(PointerWrap &p) {
auto s = p.Section("PsmfPlayer", 1, 7);
auto s = p.Section("PsmfPlayer", 1, 8);
if (!s)
return;

Expand Down Expand Up @@ -549,6 +553,11 @@ void PsmfPlayer::DoState(PointerWrap &p) {
} else {
finishThread = NULL;
}

if (s >= 8) {
p.Do(videoWidth);
p.Do(videoHeight);
}
}

bool Psmf::setStreamNum(u32 psmfStruct, int num, bool updateCached) {
Expand Down Expand Up @@ -1217,6 +1226,9 @@ static int _PsmfPlayerSetPsmfOffset(u32 psmfPlayer, const char *filename, int of

psmfplayer->totalVideoStreams = 0;
psmfplayer->totalAudioStreams = 0;
psmfplayer->videoWidth = buf[142] * 16;
psmfplayer->videoHeight = buf[143] * 16;

psmfplayer->playerVersion = PSMF_PLAYER_VERSION_FULL;
for (u16 i = 0; i < numStreams; i++) {
const u8 *currentStreamAddr = buf + 0x82 + i * 16;
Expand Down Expand Up @@ -1725,8 +1737,7 @@ static u32 scePsmfPlayerGetCurrentPts(u32 psmfPlayer, u32 currentPtsAddr)
return 0;
}

static u32 scePsmfPlayerGetPsmfInfo(u32 psmfPlayer, u32 psmfInfoAddr)
{
static u32 scePsmfPlayerGetPsmfInfo(u32 psmfPlayer, u32 psmfInfoAddr, u32 widthAddr, u32 heightAddr) {
auto info = PSPPointer<PsmfInfo>::Create(psmfInfoAddr);
if (!Memory::IsValidAddress(psmfPlayer) || !info.IsValid()) {
ERROR_LOG(ME, "scePsmfPlayerGetPsmfInfo(%08x, %08x): invalid addresses", psmfPlayer, psmfInfoAddr);
Expand All @@ -1753,6 +1764,18 @@ static u32 scePsmfPlayerGetPsmfInfo(u32 psmfPlayer, u32 psmfInfoAddr)
info->numPCMStreams = 0;
info->playerVersion = psmfplayer->playerVersion;

if (psmfPlayerLibVersion >= 0x03090510) {
// LocoRoco 2 depends on these for sizing its video output. Without this, its height is zero
// and nothing is drawn.
// Can't ask mediaengine for width/height here, it's too early, so we grabbed it from the
// header in scePsmfPlayerSetPsmf.
if (Memory::IsValidAddress(widthAddr) && psmfplayer->videoWidth) {
Memory::Write_U32(psmfplayer->videoWidth, widthAddr);
}
if (Memory::IsValidAddress(heightAddr) && psmfplayer->videoHeight) {
Memory::Write_U32(psmfplayer->videoHeight, heightAddr);
}
}
return 0;
}

Expand Down Expand Up @@ -2123,7 +2146,7 @@ const HLEFunction scePsmfPlayer[] =
{0XA3D81169, &WrapU_UII<scePsmfPlayerChangePlayMode>, "scePsmfPlayerChangePlayMode", 'x', "xii"},
{0XB8D10C56, &WrapU_U<scePsmfPlayerSelectAudio>, "scePsmfPlayerSelectAudio", 'x', "x" },
{0XB9848A74, &WrapI_UU<scePsmfPlayerGetAudioData>, "scePsmfPlayerGetAudioData", 'i', "xx" },
{0XDF089680, &WrapU_UU<scePsmfPlayerGetPsmfInfo>, "scePsmfPlayerGetPsmfInfo", 'x', "xx" },
{0XDF089680, &WrapU_UUUU<scePsmfPlayerGetPsmfInfo>, "scePsmfPlayerGetPsmfInfo", 'x', "xxxx" },
{0XE792CD94, &WrapI_U<scePsmfPlayerReleasePsmf>, "scePsmfPlayerReleasePsmf", 'i', "x" },
{0XF3EFAA91, &WrapU_UUU<scePsmfPlayerGetCurrentPlayMode>, "scePsmfPlayerGetCurrentPlayMode", 'x', "xxx"},
{0XF8EF08A6, &WrapI_U<scePsmfPlayerGetCurrentStatus>, "scePsmfPlayerGetCurrentStatus", 'i', "x" },
Expand Down

5 comments on commit e9d5eb6

@unknownbrackets
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes Growlanser hit invalid memory accesses.

I added this:
hleLogError(ME, 0, "TEST %08x %08x", psmfPlayerLibVersion, psmfplayer->playerVersion);

And see this:

00000000=scePsmfPlayerGetPsmfInfo(096645e0, 096645a8, 00000000, 09ba3a48): TEST 05050010 00000001

A few ideas:

  1. Maybe it only has three parameters, and it just happens that a3 == a2 + 4 in LocoRoco 2?
  2. Maybe certain playerVersions don't do this somehow?
  3. Maybe it only null checks width, or it null checks both?
  4. Maybe the behavior stopped in some later library version?

Can you share the values for LocoRoco 2?

-[Unknown]

@hrydgard
Copy link
Owner Author

@hrydgard hrydgard commented on e9d5eb6 Apr 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might very well be right , it might only have 2 parameters. Here are the values in LocoRoco2:

psmfPlayer = 0x09fbf2ec
psmfInfoAddr = 0x09fff610
widthAddr = 0x09fff624

So it could very well be explained by psmfInfo having a few extra fields (possibly in this and later library versions) and widthAddr just happening to point to the size part of it.

@hrydgard
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, that doesn't work because heightAddr is 0x09fbf3ac which is closer to psmfPlayer which seems unlikely to be part of the same structure? Or actually, it might be that the app computed the address in psmfPlayer from which to read the height before making the call, and it just happened to be in a parameter-looking register. Hm.

@hrydgard
Copy link
Owner Author

@hrydgard hrydgard commented on e9d5eb6 Apr 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following works, of course:

if (psmfPlayerLibVersion >= 0x03090510) {
	Memory::Write_U32(psmfplayer->videoHeight, psmfPlayer + 0xC0);
}

But in that case, if the value is just part of the psmfPlayer struct it probably has nothing to do with GetPsmfInfo, just an accident that it was reading it soon after - and we should do that memory write earlier, like when opening the video.

@unknownbrackets
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. From memory, I think the psmfPlayer reference is a pointer to a struct - and currently we just have it point to itself (in case they copy that pointer.) So maybe it's reaching into that struct...

I did a quick test against 03080010 and 05000010, and didn't reproduce either behavior:

  • Extra parameters were ignored and not written to.
  • Extra members of the struct were not populated.

However, it might be that the first field is treated as the struct size or something, I didn't spend much time trying things. I didn't have 03090510 to test against, but see a few games use that version:
http://report.ppsspp.org/logs/kind/589

-[Unknown]

Please sign in to comment.