mxl-sink: Fix wrong write video/audio offset#430
Conversation
SimaoFonseca2
force-pushed
the
fix/network-stream-write
branch
from
c63de18 to
cd51cad
Compare
mlefebvre1
left a comment
mlefebvre1
left a comment
There was a problem hiding this comment.
Thank you for this fix!
I've added some minor comments.
Unfortunately, I did not have time to test this new version yet, but I should have time soon.
| trace!("Audio sleeping: {:#?}", Duration::from_nanos(pts - mxl_now)); | ||
| let (lock, cvar) = &*engine.sleep_flag; | ||
|
|
||
| let shutdown_guard = lock.lock().map_err(|_| gst::FlowError::Error)?; |
There was a problem hiding this comment.
you can just declare this one as mutable and delete the assignment below
| .destroy() | ||
| .map_err(|_| gst::error_msg!(gst::CoreError::Failed, ["Failed to destroy flow"]))? | ||
| }; | ||
| let audio = state.audio.take().ok_or(gst::error_msg!( |
There was a problem hiding this comment.
Could be simplified by using if let Some(audio) = state.audio instead. Same applies to video.
| pub index: u64, | ||
| } | ||
|
|
||
| const LATENCY_CUSHION: u64 = 5_000_000; |
There was a problem hiding this comment.
Both video and audio define the same latency cushion shouldn't that be shared?
There was a problem hiding this comment.
I think it makes sense to keep the ability to tune the latency cushion for audio and video independently if needed. Also, having different latency cushions shouldn’t introduce A/V desync, since synchronization is still driven by the PTS.
| pub sleep_flag: Arc<(Mutex<bool>, Condvar)>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone)] |
There was a problem hiding this comment.
Clone could be removed, it's only used once, but not be needed. You probably don't want the channel to be cloned anyway.
|
|
||
| let mut shutdown_guard = shutdown_guard; | ||
|
|
||
| while !*shutdown_guard && pts > mxl_now { |
There was a problem hiding this comment.
Unless I'm missing something, I think this loop is useless. On first iteration you either return because of a shutdown notification or because of a timeout. So you basically never loop. (Also applies to video)
Thanks, @mlefebvre1! @SimaoPFonseca will get back to you as soon as he has some time. |
Signed-off-by: Simao Fonseca <simao.fonseca@bisect.pt>
SimaoFonseca2
force-pushed
the
fix/network-stream-write
branch
from
cd51cad to
b256f36
Compare
mlefebvre1
left a comment
mlefebvre1
left a comment
There was a problem hiding this comment.
I've done some spot checks and it worked well.
Thanks, @mlefebvre1! |
vt-tv
added
the
backport/v1.0
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release/v1.0
git worktree add -d .worktree/backport/430/-/release/v1.0 origin/release/v1.0
cd .worktree/backport/430/-/release/v1.0
git switch --create backport/430/-/release/v1.0
git cherry-pick -x 20b243207d24c9f8358b401ffe664183f963ca55 |
@SimaoFonseca2 Can you please check this? Thanks |
5 participants
This PR changes the mxl-sink write algorithm so that writes to the circular buffer are delayed until just before their presentation time.
This avoids overflowing the buffer in case the video and audio streams are received at significantly different times, e.g. as occurs in MPEG TS.