Modify
Warning Please read the Ticket HowTo before creating or commenting on a ticket. Failure to do so may cause your ticket to be rejected or result in a slower response.

Opened 11 months ago

Closed 11 months ago

#11531 closed Patch - Bug Fix (Fixed)

PULL REQUETS: fix bug in mythtranscodes clean-cutting mechanism

Reported by: mythtv@… Owned by: dekarl
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

Attachments (0)

Change History (7)

comment:1 Changed 11 months ago by dekarl@…

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?

// amount to drop is sufficient then we can drop less
// audio rather than drop a frame
// But if it's a frame we are supposed to drop anyway, still do so,
// and record that we have
if (videoFramesToCut > 0)
{
    videoFramesToCut--;
    return false;
}
else
{
    audioFramesToCut -= (int64_t)(audioFramesPerVideoFrame + 0.5);
    return true;
}

comment:2 Changed 11 months ago by mythtv@…

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 months ago by mythtv@…

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

InhibitUseVideoFrame?

actual_v = rtv ? 1 : 0
actual_a = 0

InhibitUseAudioFrames?(frames)

actual_v = 0
actual_a = rtv ? frames : 0

InhibitDummyFrame?

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:4 Changed 11 months ago by Paul Gardiner <paul@…>

In 3e5a0008306d18fe5237167b04b99b2af14c846c/mythtv:

Fix bug in mythtranscodes clean-cutting mechanism

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

Refs #11531

comment:5 Changed 11 months ago by Paul Gardiner <paul@…>

In 7150d64b6cf4d16ca0a0fd31d872ad2ba8dd0589/mythtv:

Fix bug in mythtranscodes clean-cutting mechanism

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

Refs #11531

comment:6 Changed 11 months ago by dekarl

  • Milestone changed from unknown to 0.26.1
  • Owner set to dekarl
  • Status changed from new to accepted

comment:7 Changed 11 months ago by dekarl

  • Resolution set to Fixed
  • Status changed from accepted to closed

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'new'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.