-
Notifications
You must be signed in to change notification settings - Fork 237
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
Save-to-webm: use timestamp from samplebuilder #130
base: master
Are you sure you want to change the base?
Conversation
If/when this gets merged I can do the same for the Twitch example. |
3dc7e3a
to
4f95b29
Compare
4f95b29
to
83158f2
Compare
Tweaked to use |
This avoids desyncing by accumulating drift.
83158f2
to
5bab82e
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.
Basic idea looks good.
I think PacketTimestamp overflow should be handled.
ac7d2b9
to
051d328
Compare
In the event that the packet timestamp wraps past uint32max, cast to int64 and add uint32_max so that the delta (and thus the webm timestamp) stays positive.
051d328
to
aca9a01
Compare
Hopefully something like aca9a01 is what you had in mind, I would squash before merging I just wanted the change to accommodate overflow to be clear. |
Instead we use the sample.Duration (as before) for the first packet only, subsequent packets will be computed as a delta.
523f40f
to
c827071
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.
I'm busy for next two weeks later, so please ask anyone else for reviewing if need to hurry.
As it's an example, there is an option to abort the program when reaching the point it can't handle timestamp correctly, and keep the example code simple.
timestampSinceStart := int64(sample.PacketTimestamp) - int64(s.startAudioOffset) | ||
// handle range where PacketTimestamp has wrapped past uint32 by operating in int64 range until the timestamp has caught up to the offset | ||
if timestampSinceStart < 0 { | ||
timestampSinceStart = int64(sample.PacketTimestamp+math.MaxUint32) - int64(s.startAudioOffset) | ||
} | ||
ts = timestampSinceStart / 48 // convert from RTPTime to sample time |
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.
Does it work after the second overflow of packet timestamp?
Sorry for the radio silence, I finally circled back to this and I'm tempted to drop it as in my own application, it is not fixing the problem it should be. |
This avoids desyncing by accumulating drift.
Description
In my own application I've been able to force audio and video to get badly out of sync by interfering with the source (causing dips in FPS etc., I don't fully grok the desync, but I can reproduce it reliably), which would cause the mkv written to disk to be badly out of sync. This change uses the sambuilder packet timestamp directly - the starting offset instead of accumulating durations.
Some caveats: