Merge pull request #25346 from enen92/cleanup_fixme_edl

[EDL][Players] Cleanup FIXMEs

xbmc/SeekHandler.cpp

@@ -311,12 +311,12 @@ bool CSeekHandler::OnAction(const CAction &action)
  }
  case ACTION_NEXT_SCENE:
  {
- appPlayer->SeekScene(true);
+ appPlayer->SeekScene(Direction::FORWARD);
  return true;
  }
  case ACTION_PREV_SCENE:
  {
- appPlayer->SeekScene(false);
+ appPlayer->SeekScene(Direction::BACKWARD);
  return true;
  }
  case ACTION_ANALOG_SEEK_FORWARD:

xbmc/application/ApplicationPlayer.cpp

@@ -327,10 +327,10 @@ bool CApplicationPlayer::CanSeek() const
  return (player && player->CanSeek());
 }
 
-bool CApplicationPlayer::SeekScene(bool bPlus)
+bool CApplicationPlayer::SeekScene(Direction seekDirection)
 {
  std::shared_ptr<IPlayer> player = GetInternal();
- return (player && player->SeekScene(bPlus));
+ return (player && player->SeekScene(seekDirection));
 }
 
 void CApplicationPlayer::SeekTime(int64_t iTime)

xbmc/application/ApplicationPlayer.h

@@ -144,7 +144,7 @@ class CApplicationPlayer : public IApplicationComponent
  void Seek(bool bPlus = true, bool bLargeStep = false, bool bChapterOverride = false);
  int SeekChapter(int iChapter);
  void SeekPercentage(float fPercent = 0);
- bool SeekScene(bool bPlus = true);
+ bool SeekScene(Direction seekDirection);
  void SeekTime(int64_t iTime = 0);
  void SeekTimeRelative(int64_t iTime = 0);
  void SetAudioStream(int iStream);

xbmc/cores/Direction.h

@@ -0,0 +1,18 @@
+/*
+ * Copyright (C) 2024 Team Kodi
+ * This file is part of Kodi - https://kodi.tv
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * See LICENSES/README.md for more information.
+ */
+
+#pragma once
+
+/*!
+ * @brief Specifies/Abstracts a direction
+*/
+enum class Direction : bool
+{
+ FORWARD, /*!< Forward */
+ BACKWARD /*!< Backward */
+};

xbmc/cores/IPlayer.h

@@ -8,6 +8,7 @@
 
 #pragma once
 
+#include "Direction.h"
 #include "IPlayerCallback.h"
 #include "Interface/StreamInfo.h"
 #include "MenuType.h"
@@ -105,7 +106,7 @@ class IPlayer
  virtual bool IsPassthrough() const { return false;}
  virtual bool CanSeek() const { return true; }
  virtual void Seek(bool bPlus = true, bool bLargeStep = false, bool bChapterOverride = false) = 0;
- virtual bool SeekScene(bool bPlus = true) {return false;}
+ virtual bool SeekScene(Direction seekDirection) { return false; }
  virtual void SeekPercentage(float fPercent = 0){}
  virtual float GetCachePercentage() const { return 0; }
  virtual void SetMute(bool bOnOff){}

xbmc/cores/VideoPlayer/Edl.cpp

@@ -43,56 +43,57 @@ void CEdl::Clear()
  m_lastEditTime = -1;
 }
 
-bool CEdl::ReadEditDecisionLists(const CFileItem& fileItem, const float fFramesPerSecond)
+bool CEdl::ReadEditDecisionLists(const CFileItem& fileItem, float fps)
 {
- bool bFound = false;
+ bool found = false;
 
  /*
  * Only check for edit decision lists if the movie is on the local hard drive, or accessed over a
  * network share (even if from a different private network).
  */
- const std::string& strMovie = fileItem.GetDynPath();
- if ((URIUtils::IsHD(strMovie) || URIUtils::IsOnLAN(strMovie, LanCheckMode::ANY_PRIVATE_SUBNET)) &&
- !URIUtils::IsInternetStream(strMovie))
+ const std::string& mediaFilePath = fileItem.GetDynPath();
+ if ((URIUtils::IsHD(mediaFilePath) ||
+ URIUtils::IsOnLAN(mediaFilePath, LanCheckMode::ANY_PRIVATE_SUBNET)) &&
+ !URIUtils::IsInternetStream(mediaFilePath))
  {
  CLog::Log(LOGDEBUG,
  "{} - Checking for edit decision lists (EDL) on local drive or remote share for: {}",
- __FUNCTION__, CURL::GetRedacted(strMovie));
+ __FUNCTION__, CURL::GetRedacted(mediaFilePath));
 
  /*
  * Read any available file format until a valid EDL related file is found.
  */
- if (!bFound)
- bFound = ReadVideoReDo(strMovie);
+ if (!found)
+ found = ReadVideoReDo(mediaFilePath);
 
- if (!bFound)
- bFound = ReadEdl(strMovie, fFramesPerSecond);
+ if (!found)
+ found = ReadEdl(mediaFilePath, fps);
 
- if (!bFound)
- bFound = ReadComskip(strMovie, fFramesPerSecond);
+ if (!found)
+ found = ReadComskip(mediaFilePath, fps);
 
- if (!bFound)
- bFound = ReadBeyondTV(strMovie);
+ if (!found)
+ found = ReadBeyondTV(mediaFilePath);
  }
  else
  {
- bFound = ReadPvr(fileItem);
+ found = ReadPvr(fileItem);
  }
 
- if (bFound)
+ if (found)
  {
  MergeShortCommBreaks();
  AddSceneMarkersAtStartAndEndOfEdits();
  }
 
- return bFound;
+ return found;
 }
 
-bool CEdl::ReadEdl(const std::string& strMovie, const float fFramesPerSecond)
+bool CEdl::ReadEdl(const std::string& mediaFilePath, float fps)
 {
  Clear();
 
- std::string edlFilename(URIUtils::ReplaceExtension(strMovie, ".edl"));
+ const std::string edlFilename(URIUtils::ReplaceExtension(mediaFilePath, ".edl"));
  if (!CFile::Exists(edlFilename))
  return false;
 
@@ -190,10 +191,10 @@ bool CEdl::ReadEdl(const std::string& strMovie, const float fFramesPerSecond)
  }
  else if (strFields[i][0] == '#') // #12345 format for frame number
  {
- if (fFramesPerSecond > 0.0f)
+ if (fps > 0.0f)
  {
- editStartEnd[i] = static_cast<int64_t>(std::atol(strFields[i].substr(1).c_str()) /
- fFramesPerSecond * 1000); // frame number to ms
+ editStartEnd[i] = static_cast<int64_t>(std::atol(strFields[i].substr(1).c_str()) / fps *
+ 1000); // frame number to ms
  }
  else
  {
@@ -281,11 +282,11 @@ bool CEdl::ReadEdl(const std::string& strMovie, const float fFramesPerSecond)
  }
 }
 
-bool CEdl::ReadComskip(const std::string& strMovie, const float fFramesPerSecond)
+bool CEdl::ReadComskip(const std::string& mediaFilePath, float fps)
 {
  Clear();
 
- std::string comskipFilename(URIUtils::ReplaceExtension(strMovie, ".txt"));
+ const std::string comskipFilename(URIUtils::ReplaceExtension(mediaFilePath, ".txt"));
  if (!CFile::Exists(comskipFilename))
  return false;
 
@@ -315,9 +316,9 @@ bool CEdl::ReadComskip(const std::string& strMovie, const float fFramesPerSecond
  /*
  * Not all generated Comskip files have the frame rate information.
  */
- if (fFramesPerSecond > 0.0f)
+ if (fps > 0.0f)
  {
- fFrameRate = fFramesPerSecond;
+ fFrameRate = fps;
  CLog::Log(LOGWARNING,
  "Edl::ReadComskip - Frame rate not in Comskip file. Using detected frames per "
  "second: {:.3f}",
@@ -376,7 +377,7 @@ bool CEdl::ReadComskip(const std::string& strMovie, const float fFramesPerSecond
  }
 }
 
-bool CEdl::ReadVideoReDo(const std::string& strMovie)
+bool CEdl::ReadVideoReDo(const std::string& mediaFilePath)
 {
  /*
  * VideoReDo file is strange. Tags are XML like, but it isn't an XML file.
@@ -385,7 +386,7 @@ bool CEdl::ReadVideoReDo(const std::string& strMovie)
  */
 
  Clear();
- std::string videoReDoFilename(URIUtils::ReplaceExtension(strMovie, ".Vprj"));
+ const std::string videoReDoFilename(URIUtils::ReplaceExtension(mediaFilePath, ".Vprj"));
  if (!CFile::Exists(videoReDoFilename))
  return false;
 
@@ -474,11 +475,12 @@ bool CEdl::ReadVideoReDo(const std::string& strMovie)
  }
 }
 
-bool CEdl::ReadBeyondTV(const std::string& strMovie)
+bool CEdl::ReadBeyondTV(const std::string& mediaFilePath)
 {
  Clear();
 
- std::string beyondTVFilename(URIUtils::ReplaceExtension(strMovie, URIUtils::GetExtension(strMovie) + ".chapters.xml"));
+ const std::string beyondTVFilename(URIUtils::ReplaceExtension(
+ mediaFilePath, URIUtils::GetExtension(mediaFilePath) + ".chapters.xml"));
  if (!CFile::Exists(beyondTVFilename))
  return false;
 
@@ -707,7 +709,7 @@ bool CEdl::AddEdit(const Edit& newEdit)
  return true;
 }
 
-bool CEdl::AddSceneMarker(const int iSceneMarker)
+bool CEdl::AddSceneMarker(int iSceneMarker)
 {
  Edit edit;
 
@@ -882,37 +884,35 @@ EDL::Action CEdl::GetLastEditActionType() const
  return m_lastEditActionType;
 }
 
-bool CEdl::GetNextSceneMarker(bool bPlus, const int iClock, int *iSceneMarker)
+std::optional<int> CEdl::GetNextSceneMarker(Direction direction, int clock)
 {
  if (!HasSceneMarker())
- return false;
+ return std::nullopt;
 
- int iSeek = GetTimeAfterRestoringCuts(iClock);
+ std::optional<int> sceneMarker;
+ const int seekTime = GetTimeAfterRestoringCuts(clock);
 
- int iDiff = 10 * 60 * 60 * 1000; // 10 hours to ms.
- bool bFound = false;
+ int diff = 10 * 60 * 60 * 1000; // 10 hours to ms.
 
- if (bPlus) // Find closest scene forwards
+ if (direction == Direction::FORWARD) // Find closest scene forwards
  {
  for (int i = 0; i < (int)m_vecSceneMarkers.size(); i++)
  {
- if ((m_vecSceneMarkers[i] > iSeek) && ((m_vecSceneMarkers[i] - iSeek) < iDiff))
+ if ((m_vecSceneMarkers[i] > seekTime) && ((m_vecSceneMarkers[i] - seekTime) < diff))
  {
- iDiff = m_vecSceneMarkers[i] - iSeek;
- *iSceneMarker = m_vecSceneMarkers[i];
- bFound = true;
+ diff = m_vecSceneMarkers[i] - seekTime;
+ sceneMarker = m_vecSceneMarkers[i];
  }
  }
  }
- else // Find closest scene backwards
+ else if (direction == Direction::BACKWARD) // Find closest scene backwards
  {
  for (int i = 0; i < (int)m_vecSceneMarkers.size(); i++)
  {
- if ((m_vecSceneMarkers[i] < iSeek) && ((iSeek - m_vecSceneMarkers[i]) < iDiff))
+ if ((m_vecSceneMarkers[i] < seekTime) && ((seekTime - m_vecSceneMarkers[i]) < diff))
  {
- iDiff = iSeek - m_vecSceneMarkers[i];
- *iSceneMarker = m_vecSceneMarkers[i];
- bFound = true;
+ diff = seekTime - m_vecSceneMarkers[i];
+ sceneMarker = m_vecSceneMarkers[i];
  }
  }
  }
@@ -922,16 +922,17 @@ bool CEdl::GetNextSceneMarker(bool bPlus, const int iClock, int *iSceneMarker)
  * picked up when scene markers are added.
  */
  Edit edit;
- if (bFound && InEdit(*iSceneMarker, &edit) && edit.action == Action::CUT)
- *iSceneMarker = edit.end;
+ if (sceneMarker && InEdit(sceneMarker.value(), &edit) && edit.action == Action::CUT)
+ sceneMarker = edit.end;
 
- return bFound;
+ return sceneMarker;
 }
 
-std::string CEdl::MillisecondsToTimeString(const int iMilliseconds)
+std::string CEdl::MillisecondsToTimeString(int milliSeconds)
 {
- std::string strTimeString = StringUtils::SecondsToTimeString((long)(iMilliseconds / 1000), TIME_FORMAT_HH_MM_SS); // milliseconds to seconds
- strTimeString += StringUtils::Format(".{:03}", iMilliseconds % 1000);
+ std::string strTimeString = StringUtils::SecondsToTimeString(
+ static_cast<long>(milliSeconds / 1000), TIME_FORMAT_HH_MM_SS); // milliseconds to seconds
+ strTimeString += StringUtils::Format(".{:03}", milliSeconds % 1000);
  return strTimeString;
 }
 

xbmc/cores/VideoPlayer/Edl.h

@@ -8,8 +8,10 @@
 
 #pragma once
 
+#include "cores/Direction.h"
 #include "cores/EdlEdit.h"
 
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -20,10 +22,7 @@ class CEdl
 public:
  CEdl();
 
- // FIXME: remove const modifier for fFramesPerSecond as it makes no sense as it means nothing
- // for the reader of the interface, but limits the implementation
- // to not modify the parameter on stack
- bool ReadEditDecisionLists(const CFileItem& fileItem, const float fFramesPerSecond);
+ bool ReadEditDecisionLists(const CFileItem& fileItem, float fps);
  void Clear();
 
  /*!
@@ -144,15 +143,15 @@ class CEdl
  */
  EDL::Action GetLastEditActionType() const;
 
- // FIXME: remove const modifier for iClock as it makes no sense as it means nothing
- // for the reader of the interface, but limits the implementation
- // to not modify the parameter on stack
- bool GetNextSceneMarker(bool bPlus, const int iClock, int *iSceneMarker);
+ /*!
+ * @brief Get the next scene marker with respect to the provided clock time
+ * @param direction (the direction of the search - backward or forward)
+ * @param clock the current position of the clock
+ * @return the position of the scenemarker (nullopt if none)
+ */
+ std::optional<int> GetNextSceneMarker(Direction direction, int clock);
 
- // FIXME: remove const modifier as it makes no sense as it means nothing
- // for the reader of the interface, but limits the implementation
- // to not modify the parameter on stack
- static std::string MillisecondsToTimeString(const int iMilliseconds);
+ static std::string MillisecondsToTimeString(int milliSeconds);
 
 private:
  // total cut time (edl cuts) in ms
@@ -170,22 +169,10 @@ class CEdl
  */
  EDL::Action m_lastEditActionType{EDL::EDL_ACTION_NONE};
 
- // FIXME: remove const modifier for fFramesPerSecond as it makes no sense as it means nothing
- // for the reader of the interface, but limits the implementation
- // to not modify the parameter on stack
- bool ReadEdl(const std::string& strMovie, const float fFramesPerSecond);
- // FIXME: remove const modifier for fFramesPerSecond as it makes no sense as it means nothing
- // for the reader of the interface, but limits the implementation
- // to not modify the parameter on stack
- bool ReadComskip(const std::string& strMovie, const float fFramesPerSecond);
- // FIXME: remove const modifier for strMovie as it makes no sense as it means nothing
- // for the reader of the interface, but limits the implementation
- // to not modify the parameter on stack
- bool ReadVideoReDo(const std::string& strMovie);
- // FIXME: remove const modifier for strMovie as it makes no sense as it means nothing
- // for the reader of the interface, but limits the implementation
- // to not modify the parameter on stack
- bool ReadBeyondTV(const std::string& strMovie);
+ bool ReadEdl(const std::string& mediaFilePath, float fps);
+ bool ReadComskip(const std::string& mediaFilePath, float fps);
+ bool ReadVideoReDo(const std::string& mediaFilePath);
+ bool ReadBeyondTV(const std::string& mediaFilePath);
  bool ReadPvr(const CFileItem& fileItem);
 
  /*!
@@ -195,10 +182,7 @@ class CEdl
  */
  bool AddEdit(const EDL::Edit& newEdit);
 
- // FIXME: remove const modifier for strMovie as it makes no sense as it means nothing
- // for the reader of the interface, but limits the implementation
- // to not modify the parameter on stack
- bool AddSceneMarker(const int sceneMarker);
+ bool AddSceneMarker(int sceneMarker);
 
  void MergeShortCommBreaks();