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

Fix packet synchronization bug #1806

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

daniel-riehm
Copy link
Collaborator

This bug got missed in #1802 due to a typo in one of the unit tests causing it to run on the wrong video file. Run on the correct file, the unit test fails, alerting us that copying both the video and audio streams causes a desync between them. The bug may be fixed by adjusting how we adjust timestamps in accordance with start_timestamp - instead of having FFmpeg shift all packets forward, then manually shifting back the ones that are direct from the original video (and therefore already take start_timestamp into account), we tell FFmpeg to do nothing and manually shift forward packets that aren't from the original video (i.e. only video packets encoded by us). Alternately, we could have added backward shifting to the copied video packets and kept the rest as is, but this results in code duplication and is probably less clear anyway.

Copy link
Collaborator

@borovik135 borovik135 left a comment

Choose a reason for hiding this comment

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

there are a couple of comments regarding the changes

arrows/ffmpeg/ffmpeg_video_output.cxx Outdated Show resolved Hide resolved
arrows/ffmpeg/ffmpeg_video_settings.h Show resolved Hide resolved
@daniel-riehm daniel-riehm force-pushed the dev/ts-offset-packet-sync branch 2 times, most recently from ceec1dc to a0e7017 Compare November 9, 2023 19:29
Copy link
Collaborator

@borovik135 borovik135 left a comment

Choose a reason for hiding this comment

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

Changes look OK.

@daniel-riehm daniel-riehm merged commit 5729bc8 into Kitware:master Nov 10, 2023
5 checks passed
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