-
Notifications
You must be signed in to change notification settings - Fork 54
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
Play audio in cutscenes #288
Conversation
c84fb24
to
de53b06
Compare
de53b06
to
2289311
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dethrace-labs
Can you please review and/or take over? Thanks.
src/smackw32/smackw32.c
Outdated
/* FIXME: get rid of bodge term */ | ||
if (now < user->smack_last_frame_time + smack->MSPerFrame - 5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this to avoid stuttering.
When sleeping too long, the next audio frame is not ready soon enough.
src/smackw32/smackw32.c
Outdated
int SmackSoundUseDirectSound(void* dd) { | ||
// TODO: do some miniaudio init | ||
// FIXME: is this a more appropriate place to init miniaudio? What is type of argument? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the arguent of SmackSoundUseDirectSound
was NULL
, I did the audio initialization in SmackOpen
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good. The NULL
argument is an optional pointer to a DIRECTSOUND
instance, (NULL
apparently means "create a new one")
https://github.com/TheNitesWhoSay/lawine/blob/master/lawine/misc/smack.c#L34
@@ -43,7 +43,7 @@ typedef struct SmackTag { | |||
unsigned long addr32; | |||
|
|||
// added by dethrace | |||
void* smk_handle; | |||
void* user; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved all user data to a private struct, that is also allocated.
src/smackw32/smackw32.c
Outdated
|
||
uint32_t smack_last_frame_time; | ||
extern ma_engine miniaudio_engine; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This re-uses miniaudio_engine
, so the separation between smackw32 and S3 is not perfect.
Sure, thanks for bringing it up to latest |
@@ -0,0 +1,290 @@ | |||
#define SDL_MAIN_HANDLED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the long run, it would be nice to convert this tool to using smackw32 + brender.
Perhaps worth a fixme :)
I still experience stuttering with this. I started to experiment by trying to feed the audio 1 frame in front of the video so we always have the next audio page to consume before we run out, but its not quite working yet |
Hello @dethrace-labs Congratulations for this impressive work. I rewrote your smackw32 files to add sound playback support to videos using the supplied miniaudio backend. I started with @madebr's code, figured out the problem (sort of) with sound cracklings/stuttering, and moved it to smackw32 where I think it belongs. There were three problems. One of them was that your frame duration measurement was incorrect. The time measurement shall be done exactly once per loop, not twice (begin / end). Because of this little mistake, the video playback speed wasn't exactly on par with the audio consumption rate, leading to frequent sound buffer starvation, causing arbitrary seek/restarts producing audible pops. The second problem, as you guessed, was that more than one audio page needs to be precached in miniaudio's linked list to avoid sound starvation at the end of the first page. The third problem, once the others were solved, appeared in so that even playing the videos with audio without any stuttering and both audio and video playing at their designated speeds, a time shift could be observed between audio and video that turned out to be simply proportional to the video length. It was very noticeable in the intro video (the longest), when the roadsign shortly spins at the end. Around 25 pages of audio needed to be precached (and the sound playback start delayed by this amount) for audio and video to become accurately synced. On the other hand, on shorter videos only 1 or 2 audio pages needed to be precached to achieve good sync. It's as if the player code was supposed to precache a certain amount of audio pages that's directly proportional to the length of the video. By trial and error I figured out that the value (total number of frames / 100) would give the exact number of pages to precache for each video. Don't ask me why it works this way... perhaps it's by design ? As I have no write access on this repo, here are the files if you want to review them. The only files changed are smackw32.c and smackw32.h. There are a few comments and I tried to adopt your coding style. I hope you find this contribution useful: https://www.pmbaty.com/smackw32.zip |
Feel free to create a new PR with your code. I will happily close this PR. Or you can create a PR on my fork to the |
Thanks @PierreMarieBaty for your analysis and code changes! As @madebr said, are you able to send the changes as a pull request? |
I’m sorry, I don’t use git - except when I’m forced to. My SCM of choice is SVN (don’t feed the troll) :-)
|
Co-Authored-By: PierreMarie Baty <PierreMarieBaty@users.noreply.github.com>
I copied |
Interesting. Which SMK file did you test, what's your system? I tested on Win7/x64 and macOS with a French version of the Carmageddon CD. The results looked good to me. How noticeable is it? I'd like to be able to reproduce the problem.
But as I said, this precache delay was mostly guessed out and discovered by trial and error. It is certainly possible that a better mechanism exists to trigger audio and video start at the right time, that's yet to be discovered.
|
I run a x64 Fedora 38 Linux system. The video I'm seeing time synchronization issues with is When I compare the video played through dethrace and through minismackerplay, the first one is not synchronized. |
Okay. I compiled your minismackerplay (should have started with that),
saw that it does no caching and optional conversion, and could see that
all videos seem to play correctly with it. Which means the
minismackerplay code must do it right.
So I added that optional check for whether resampling is needed in
smackw32.c, disabled soundpage caching, and that seemed to work just as
well : no sound pops, no sound starve (except when e.g. I resize the
game window and the SDL loop exceptionally exceeds the frame duration -
an arbitrary sync is still needed in this case. BTW minismackerplay has
the same problem). Anyway, this tends to indicate that sample conversion
in miniaudio from and to the same sample rates (?) produce output that's
shorter than the input.
It didn't fix the sound delay in the intro video MIX_INTR.SMK though, so
I checked further. I saw that dethrace uses SDL_GetTicks() and
minismackerplay uses SDL_GetPerformanceCounter(). Having minismackerplay
use SDL_GetTicks() too didn't help, so I made them both display the
sound page sizes, and I could spot the discrepancy.
minismackerplay (right):
frame #0 sound converter framesIn 23154 framesOut 46308
frame #1 sound converter framesIn 1102 framesOut 2204
frame #2 sound converter framesIn 1102 framesOut 2204
frame #3 sound converter framesIn 1102 framesOut 2204
frame #4 sound converter framesIn 1102 framesOut 2204
...
dethrace (wrong):
frame #0 sound converter framesIn 1102 framesOut 2204
frame #1 sound converter framesIn 1102 framesOut 2204
frame #2 sound converter framesIn 1102 framesOut 2204
frame #3 sound converter framesIn 1102 framesOut 2204
frame #4 sound converter framesIn 1102 framesOut 2204
...
Which seemed to prove 2 things:
1. When things are done right, the first audio page is much bigger than
the other ones, and thus lasts much longer.
2. The sound "precaching" effect is simply necessary because dethrace
skips that frame and directly proceeds to audio frame #1 as if it was
frame #0.
After some more investigation, the reason for this turned out because
smk_enable_audio() was called *after* smk_first() in dethrace, whereas
in libsmackerplay smk_get_audio() is called before it.
By reordering things in SmackOpen() I could solve the problem. There was
no need to precache anything after all. As a sidenote, having a big
first audio page sounds like a sensible conscious choice by the RAD SMK
encoder to ensure the sound won't starve at video init on low-end
machines.
|
I updated my smackw32 files (.c and .h). Same URL:
https://www.pmbaty.com/smackw32.zip
Let me know if it sounds good this time.
|
Thank you! I love it when a bunch of complicated work goes in and the end result is a simple small change :). I'll make a PR with the changes. |
@PierreMarieBaty I can't access the zip file - could you check your website, or share the zip somewhere else? |
Weird, I can still download it. |
My server is located in Russia, that could be the reason. Your ISP might
have a firewall or something blocking russian hosts maybe?
… Message ID: ***@***.***>
|
Closing this and will reopen shortly |
tools/minismackerplay.c
using libsmacker, miniaudio and SDL2. This shows the overhead/timing/sleeps/... of dethrace should be improved