Bug in CheckAutoSceneSkip (EDL/comskip processing) ?
#1
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) it misses about 1 in 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 and 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 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.
Reply
#2
Thank you very much for looking into this. Highly appreciated.
Looks like you found an issue. Good work. Do you want to submit a fix for it on github?
Reply
#3
If you wouldn't mind submitting something yourself, I think that would be best. As I said, I'm not a c/c++ programmer and someone (you?) would have to spend as much, if not more time checking what I did, and it might be better if it were done right the first time.

FWIW, here is what I would likely do ...

I would separate out the storing of m_lastQueryTime from the function CEdl::InCut, and create a new function to do that.

So I would remove the line "m_lastQueryTime = iSeek;" from CEdl::InCut

and put it in a new function in Edl.cpp that would only be called from CVideoPlayer::CheckAutoSceneSkip

(and now here is where what I would do might diverge from what anyone else would do)

Because the value "m_lastQueryTime" is now not really associated with the time that CEdl::InCut was queried, I would change the name to something that reflected where its being called from.

So this is what my function would look like ,,,
Code:
bool CEdl::CheckASSTime(const int iCheckASS, Cut *pCut)
{
    // "CheckASS" .... "to make it funny"

    m_lastCheckASSTime = iCheckASS;  

  return true;       // not sure bool is the right datatype, but since it was cut&paste from CEdl::InCut, thats what it is.
}

Smile
Reply
#4
OK, so I have coded some changes and have been testing for the last few days and everything looks good. (I removed the "joke" comment, and changed the new function name and its arguments and return value so that its more sensible.)

It also fixes the behavior so that when skipping forward into a commercial break it will then skip the commercial, which I think is the correct thing, but it wan't doing it because of this bug.

The code changes are to Edl.h, Edl.cpp and VideoPlayer.cpp

Now I don't have a GitHub account, and have never used GitHub except for making clone copies.

So, can I either

1) give you the changes I coded - there really isn't much ... its less than a dozen line changes.

or

b) find some instructions for using GitHub to submit this fix and nothing more - I have absolutely no need or desire to understand how GitHub works or the differences between forks,branches, tags, push, pulls, merges, commits etc etc ... quite frankly it all gives me a headache ... all I want to do is get this fix into Kodi.
Reply
#5
Thanks very much! Do you prefer 1) or b)? strange numbering btw. Smile
In case of 1) please post the diff somewhere.
Reply
#6
Output from ...
diff -Naur git gitNew > diff.txt

File in folder git = files from a "git clone", and folder gitNew = those files with my changes applied

Code:
diff -Naur git/Edl.cpp gitNew/Edl.cpp
--- git/Edl.cpp    2017-06-03 21:03:32.834999573 -0400
+++ gitNew/Edl.cpp    2017-06-03 21:43:05.624781200 -0400
@@ -47,7 +47,7 @@
   m_vecCuts.clear();
   m_vecSceneMarkers.clear();
   m_iTotalCutTime = 0;
-  m_lastQueryTime = 0;
+  m_lastCheckASSTime = 0;
}

bool CEdl::ReadEditDecisionLists(const std::string& strMovie, const float fFrameRate, const int iHeight)
@@ -826,8 +826,6 @@

bool CEdl::InCut(const int iSeek, Cut *pCut)
{
-  m_lastQueryTime = iSeek;
-
   for (int i = 0; i < (int)m_vecCuts.size(); i++)
   {
     if (iSeek < m_vecCuts[i].start) // Early exit if not even up to the cut start time.
@@ -844,9 +842,14 @@
   return false;
}

-int CEdl::GetLastQueryTime() const
+int CEdl::GetLastCheckASSTime() const
+{
+  return m_lastCheckASSTime;
+}
+
+void CEdl::SetLastCheckASSTime(const int iCheckASSTime)
{
-  return m_lastQueryTime;
+  m_lastCheckASSTime = iCheckASSTime;    
}

bool CEdl::GetNearestCut(bool bPlus, const int iSeek, Cut *pCut) const
diff -Naur git/Edl.h gitNew/Edl.h
--- git/Edl.h    2017-06-03 21:03:36.624985433 -0400
+++ gitNew/Edl.h    2017-06-03 21:57:40.782014800 -0400
@@ -55,7 +55,9 @@

   bool InCut(int iSeek, Cut *pCut = NULL);
   bool GetNearestCut(bool bPlus, const int iSeek, Cut *pCut) const;
-  int GetLastQueryTime() const;
+
+  int GetLastCheckASSTime() const;
+  void SetLastCheckASSTime(const int iCheckASSTime);

   bool GetNextSceneMarker(bool bPlus, const int iClock, int *iSceneMarker);

@@ -65,7 +67,7 @@
   int m_iTotalCutTime; // ms
   std::vector<Cut> m_vecCuts;
   std::vector<int> m_vecSceneMarkers;
-  int m_lastQueryTime;
+  int m_lastCheckASSTime;

   bool ReadEdl(const std::string& strMovie, const float fFramesPerSecond);
   bool ReadComskip(const std::string& strMovie, const float fFramesPerSecond);
diff -Naur git/VideoPlayer.cpp gitNew/VideoPlayer.cpp
--- git/VideoPlayer.cpp    2017-06-03 21:03:48.034942878 -0400
+++ gitNew/VideoPlayer.cpp    2017-06-03 21:54:36.553327600 -0400
@@ -2351,8 +2351,10 @@
     return;

   const int64_t clock = GetTime();
-  int lastPos = m_Edl.GetLastQueryTime();
+  int lastPos = m_Edl.GetLastCheckASSTime();

+  m_Edl.SetLastCheckASSTime(clock);
+  
   CEdl::Cut cut;
   if (!m_Edl.InCut(clock, &cut))
     return;

And option numbering is "humorous" ... the sequence would usually continue something like iii) Δ), five) VI) ... etc etc ...
Reply
#7
I opened this PR in bahalf of you https://github.com/xbmc/xbmc/pull/12231
Reply
#8
Looks like this part of the change didn't make it ...
Code:
--- git/Edl.cpp    2017-06-03 21:03:32.834999573 -0400
+++ gitNew/Edl.cpp    2017-06-03 21:43:05.624781200 -0400
@@ -47,7 +47,7 @@
   m_vecCuts.clear();
   m_vecSceneMarkers.clear();
   m_iTotalCutTime = 0;
-  m_lastQueryTime = 0;
+  m_lastCheckASSTime = 0;
}

(Maybe I should get over my irrational dislike of GitHub and submit my own changes ?)
Reply
#9
I have updated the PR. The problem with the posted diff is that "patch" results into errors because after c/p of this diff there is no ' ' at the beginning of some lines.

Could you have another look at the PR before you start studying git Smile
Reply
#10
There is still a problem with your "Pull Request" Sad

So I bit the bullet, signed up and submitted my own PR ...

https://github.com/xbmc/xbmc/pull/12237

Looks like I created 3 commits ... one for each file ... shouldn't be a problem, right?
Reply

Logout Mark Read Team Forum Stats Members Help
Bug in CheckAutoSceneSkip (EDL/comskip processing) ?1