Fix T74111: Animation Playback Delayed With Time Remapping And AV-Sync
ClosedPublic
Actions

Authored by Sebastian Parborg (zeddb) on Mar 26 2020, 7:10 PM.

Details

Reviewers

Richard Antalik (ISS)
Sergey Sharybin (sergey)
Sybren A. Stüvel (sybren)

Commits

rB9ddbb03861fb: Fix T74111: Animation Playback Delayed With Time Remapping And AV-Sync

Summary

When setting the current playback time in BKE_sound_play_scene we didn't account for the frame length. So the current frame/time would be wrong when we asked the audio playback what time it was. This would lead to playback being offset when using time remapping and AV sync.

Hopefully this doesn't break stuff horribly... Didn't manage to spot anything obvious with some quick tests I did.

Diff Detail

Event Timeline

Sebastian Parborg (zeddb) created this revision.Mar 26 2020, 7:10 PM

Sebastian Parborg (zeddb) mentioned this in T74111: Animation Playback Delayed With Time Remapping And AV-Sync.

Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)Mar 26 2020, 7:14 PM

Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)

I think this is our best option if we don't change sound pitch. So I will accept, but still would like to know Sergey's opinion.

I have tested this, and it has still problem with quite extreme time remapping values (if you start with playhead at 0.999, it still has to catch up).
You can fix this edge case by adding SUBFRA to playback start time calculation.
const float cur_time = (float)((double)((CFRA + SUBFRA) / scene->r.framelen) / FPS);

This revision is now accepted and ready to land.Mar 27 2020, 8:14 PM

Sybren A. Stüvel (sybren) added a subscriber: Sybren A. Stüvel (sybren).Mar 30 2020, 4:14 PM

Sybren A. Stüvel (sybren) added inline comments.

source/blender/blenkernel/intern/sound.c
553–558	Please keep such changes out of a functional patch, and just commit them.
767	Use `FRA2TIME(CFRA/scene->r.framelen)` here, instead of dividing by `FPS`. If the readability isn't convincing enough, maybe the performance gain wins you over (it removes a division).

Updated the patch.

Noticed that an other function was also calculating the current time, so I created a small helper function so that no one else overlooks this (like I did at first).

Sebastian Parborg (zeddb) marked an inline comment as done.Apr 6 2020, 1:03 PM

Sybren A. Stüvel (sybren) added inline comments.Apr 6 2020, 1:12 PM

source/blender/blenkernel/intern/sound.c
754	Hmmm this does keep me wondering why this is doing something different than just `FRA2TIME(frame)` (and the opposite, why `FRA2TIME` is not dividing by `framelen`). There is no documenting comment at all at `RenderData::framelen`, so it's really unclear to me when a division by `framelen` is appropriate and when it is not. That is a bigger issue, though, so here I think it suffices to add a comment to explain why it is necessary in this particular use case.