Opened 11 years ago
Closed 11 years ago
#11531 closed Patch - Bug Fix (Fixed)
PULL REQUETS: fix bug in mythtranscodes clean-cutting mechanism
Reported by: | Owned by: | Karl Egly | |
---|---|---|---|
Priority: | minor | Milestone: | 0.26.1 |
Component: | MythTV - Mythtranscode | Version: | Unspecified |
Severity: | medium | Keywords: | |
Cc: | Ticket locked: | no |
Description
One of the clean-cut methods interacted badly with mythtranscodes synchronisation algorithm and would sometimes cause frames from cut segments to be kept. This commit fixes that bug.
Here are pull requests for master and fixes/0.26 https://github.com/MythTV/mythtv/pull/46 https://github.com/MythTV/mythtv/pull/47
Change History (7)
comment:1 Changed 11 years ago by
comment:2 Changed 11 years ago by
The logic is difficult, but the patch is correct. Mythtranscode is asking the cutter whether it can drop a frame for the sake of keeping sync. The cutter was in the unpatched version returning true (meaning inhibit) which would stop mythtranscode dropping the frame, thus putting sync out, but internally the cutter would decrease audioFramesToCut so that it would cut less audio to restore sync.
In the new case. The cutter returns false (meaning don't inhibit). That allows mythtranscode to drop the frame so that it keeps sync. The cutter now doesn't need to alter its internal state for the sake of keeping sync, but it does want to record that there is one less frame to drop. Hence it must decrease both audioFramesToCut and videoFramesToCut.
I have to admit, although I wrote the cutter, I understand it only for fleeting moments. :-)
comment:3 Changed 11 years ago by
Here's another way to look at it. For each method, define as follows:
rtv --- the return value of the method
apv --- audioFramesPerVideoFrames
delta_v --- the change in videoFramesToCut made by the method
delta_a --- the change in audioFramesToCut made by the method
actual_v --- the actual number of video frames the method causes mythtranscode to cut
that it otherwise wouldn't have (can be negative)
actual_a --- the actual number of audio frames the method causes mythtranscode to cut
that it otherwise wouldn't have (can be negative)
delta_v and delta_a can be determined just by looking at what the method does to the corresponding variable. To determine actual_v and actual_a, one has to use the effect the method's return value has on mythtranscode.
To correctly keep sync, every method must satisfy
(delta_v + actual_v) - apv * (delta_a + actual_a) = 0
Here are actual_v and actual_a for each method
actual_v = rtv ? 1 : 0 actual_a = 0
InhibitUseAudioFrames?(frames)
actual_v = 0 actual_a = rtv ? frames : 0
actual_v = rtv ? -1 : 0 actual_a = 0
That should allow the invariant to be checked for every method, but also it can be used to see that my patch correctly keeps sync because it alters InhibitDummyFrame? in a particular case, so that rtv goes from true to false, which increases actual_v by 1 (from -1 to 0), decreases delta_v by 1, and leaves the effect on delta_a and actual_a as it was before. That clearly maintains the invariant.
comment:6 Changed 11 years ago by
Milestone: | unknown → 0.26.1 |
---|---|
Owner: | set to Karl Egly |
Status: | new → accepted |
comment:7 Changed 11 years ago by
Resolution: | → Fixed |
---|---|
Status: | accepted → closed |
From reading the patch it appears as if we end up dropping audio *and* video instead of audio *or* video in the new code. How does this variant look?