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

SWORD1/SWORD2: Detect availability of RGB colour support at runtime #6297

Merged
merged 4 commits into from
Mar 2, 2025

Conversation

ccawley2011
Copy link
Member

This is currently a draft because it uncovers a bug in the PSX video decoder - setOutputPixelFormats needs to be called after loadFile() and before the first frame is decoded, however the PSX decoder decodes the first frame within the load function, meaning that it's not possible to override the default format.

@bluegr
Copy link
Member

bluegr commented Dec 8, 2024

@ccawley2011 so what are the actions to be taken here? What's stopping you from proceeding with the bugfix?

From what you said, we should:

  • Remove readNextPacket() from `PSXStreamDecoder::loadStream()'
  • Test PSX videos with this bugfix
  • Test PSX SWORD1/SWORD2 videos with the changes from this PR

So... would you like to proceed with this change and fix the bug you identified within the PSX decoder, instead of leaving this PR as draft?

@ccawley2011
Copy link
Member Author

  • Remove readNextPacket() from `PSXStreamDecoder::loadStream()'

It's not that easy, since readNextPacket() handles reading the initial sector and creating the audio and video tracks that are needed by loadStream().

@bluegr
Copy link
Member

bluegr commented Feb 2, 2025

@ccawley2011 I believe that this patch could solve the bug. I've tested it with the PSX version of BS1:

 video/psx_decoder.cpp | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/video/psx_decoder.cpp b/video/psx_decoder.cpp
index c7f9f4820a2..bb6d2b5c519 100644
--- a/video/psx_decoder.cpp
+++ b/video/psx_decoder.cpp
@@ -189,6 +189,7 @@ void PSXStreamDecoder::readNextPacket() {
 	Common::SeekableReadStream *sector = 0;
 	byte *partialFrame = 0;
 	int sectorsRead = 0;
+	int64 prevPos = _stream->pos();
 
 	while (_stream->pos() < _stream->size()) {
 		sector = readSector();
@@ -211,6 +212,12 @@ void PSXStreamDecoder::readNextPacket() {
 				if (!_videoTrack) {
 					_videoTrack = new PSXVideoTrack(sector, _speed, _frameCount);
 					addTrack(_videoTrack);
+
+					// If no video track is initialized, we are called
+					// by loadStream(). Stop here, and start rendering
+					// the track from the next call.
+					_stream->seek(prevPos);
+					return;
 				}
 
 				sector->seek(28);

bluegr added a commit that referenced this pull request Mar 1, 2025
Tested with the BS1 PSX videos. The PSX decoder now conforms to the
same behavior as the rest of the video decoders. This fixes a bug
uncovered in PR #6297
@bluegr
Copy link
Member

bluegr commented Mar 1, 2025

@ccawley2011: I've committed the patch above for the PSX decoder in 9354d6c.

Please, fix the conflicts in this PR, so that we can proceed with merging it. Thanks!

@ccawley2011 ccawley2011 marked this pull request as ready for review March 1, 2025 20:33
@bluegr
Copy link
Member

bluegr commented Mar 2, 2025

Thanks! Looks good, great work!

Merging

@bluegr bluegr merged commit ad04df7 into scummvm:master Mar 2, 2025
8 checks passed
@ccawley2011 ccawley2011 deleted the sword-codecs branch March 2, 2025 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants