Skip to content

DIRECTOR: Correct for invalid loop bounds in D4 #5651

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

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

threefins
Copy link
Contributor

In some D4 version 400 (file version 0x45B) files, sound resource loop bounds are invalid but their cast member flags are set to loop. Examples are "Barbie and her Magical Dreamhouse" and "Necrobius" (Windows demo). Invalid is any situation where the following does not hold: loop_start < loop_end <= size

In the original environment, these resources are handled by disabling the looping flag and resetting the loop bounds on the sound resources to 0 -> sample size.

This commit checks for invalid loop bounds and applies roughly the same fix. This fixes the invalid sound looping behaviours present in the aforementioned titles.

@@ -100,6 +100,16 @@ void SoundCastMember::load() {
// In older versions, always loop sounds that contain a loop start and end.
_looping = audio->hasLoopBounds();
}
else if (_cast->_version == kFileVer400) {
Copy link
Member

Choose a reason for hiding this comment

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

This does not conform to our https://wiki.scummvm.org/index.php?title=Code_Formatting_Conventions.

Also, I would do it a bit differently:

if (!audio->hasValidLoopBounds()) {
   if (_cast->_version == kFileVer400) {
      debugC(2, kDebugLoading, "Cor....
      ...
    } else {
      warning("SoundCastMember::load(): invalid loop bounds detected");
    }
}

e.g. the idea is to catch the same situation if it happens in other Director versions, so we could test with the original and see what it does. Perhaps, the condition is >= kFileVer400.

If you have possibility to have this movie loaded in the later versions of Director, you could figure it out now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - I 'll rework it how you suggest

e.g. the idea is to catch the same situation if it happens in other Director versions, so we could test with the original and see what it does. Perhaps, the condition is >= kFileVer400.

Importing into Windows D4, then re-saving fixes invalid loop bounds :

  • sets loop flag to 16 (as in disabling looping)
  • Adds valid loop bounds (0->sample length) into the sound resources

It feels to me like it's a migration away from using the sound resource loop bounds to determine looping. Maybe even stop using the soundmanager format for sound resources in later Director Versions ?

I looked through the D4 demos also and found that out of the 200 or so demos and ~11,000 embedded sound cast members, every sound cast member in kFileVer404 movies had valid loop points (start < end <= length). This suggests to me the issue does not need to be corrected at kFileVer404. As such, would it be better to leave it as the existing warning for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@threefins threefins force-pushed the fix_v400_audio_loops branch from a6134b0 to 05f8893 Compare February 8, 2024 22:36
Copy link
Member

@sev- sev- left a comment

Choose a reason for hiding this comment

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

Thank you for the changes. But for whatever reason, there are quite a lot of incorrect code formatting. Please, refer to https://wiki.scummvm.org/index.php?title=Code_Formatting_Conventions

In some D4 version 400 (file version 0x45B) files, sound resource loop bounds are
invalid but their cast member flags are set to loop. Examples are
"Barbie and her Magical Dreamhouse" and "Necrobius" (Windows demo).
Invalid is any situation where the following does not hold:
`loop_start < loop_end <= size`

In the original environment, these resources are handled by disabling
the looping flag and resetting the loop bounds on the sound resources to
0 -> sample size.

This commit checks for invalid loop bounds and applies roughly the same
fix. This fixes the invalid sound looping behaviours present in the
aforementioned titles.
@threefins threefins force-pushed the fix_v400_audio_loops branch from 05f8893 to 7441380 Compare February 13, 2024 06:40
@threefins threefins requested a review from sev- February 13, 2024 06:42
@sev-
Copy link
Member

sev- commented Feb 27, 2024

Thank you

@sev- sev- merged commit 0e6c4f3 into scummvm:master Feb 27, 2024
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