Bug in CheckAutoSceneSkip (EDL/comskip processing) ?

The symptom is that “Commercial skip” sometimes misses the skipping of commercials, and just continues playing the video.

I created a “fake” EDL file that looked like this …

Code:
0009    0100    3
0109    0200    3
0209    0300    3
0309    0400    3

… repeated segments of 9 seconds of “show” followed by 91 seconds of “commercials”.

On a Raspberry Pi 2 running the latest OSMC, when playing an HD file (US broadcast TV recorded by tvheadend) is misses about 1 on 4 commercials and playing an SD file it misses about 1 in 10.

So I dug into the code, and I think I have found a bug. I actually work in IT, and have programmed for many years, but I have never been a c/c++ programmer, so please forgive me if I’m wrong here, or if some of my terminology and or explanation is a little weird … but here is what I think is going on, based on my reading of the code.

In the (main?) loop in VideoPlayer.cpp that starts at line 1357, there are two places that check for EDL skips.

1 – At line 1384, a call to CheckAutoSceneSkip() checks before a chunk of video stream is read, to see if we have entered a commercial and should skip before reading the next chunk of stream.

2 – At line 1625, ProcessPacket() calls ProcessVideoData() which calls CheckSceneSkip() which checks to see if the chunk of video that we are processing is in a commercial an needs to be dropped.

I think that the problem is that both CheckAutoSceneSkip() and CheckSceneSkip() make calls to m_Edl.InCut() which updates the value of m_lastQueryTime within the EDL data structure, but I think that the logical use of lastPos in CheckAutoSceneSkip() is based on it being the last time that m_Edl.InCut was called in that function, but since it is also called from CheckSceneSkip() so it messes up the test at line 2385

For example …

An EDL cut starts at 01:00.00 …

  • Start of main loop at 00:59.99
  • CheckAutoSceneSkip() calls m_Edl.InCut() which sets m_lastQueryTime = 00:59.99
  • Read video stream and time passes to 01:00.01
  • ProcessPacket() calls ProcessVideoData() calls CheckSceneSkip() which sets m_lastQueryTime = 01:00.01
  • (Some tiny imperceptible chunk of video gets dropped?)
  • Loops back top of main loop
  • CheckAutoSceneSkip() calls m_Edl.GetLastQueryTime() which sets lastPos = 01:00.01
  • Test at Line 2385 “if ( lastPos <= cut.start )” is false, so the code to skip the commercial is not executed.