Skip to content

Commit

Permalink
[Video] Improve user experience when playing movies/episodes from Blu…
Browse files Browse the repository at this point in the history
…ray ISO/BDMV.
  • Loading branch information
78andyp committed Apr 26, 2024
1 parent a5fb12b commit 54a63d0
Show file tree
Hide file tree
Showing 18 changed files with 206 additions and 67 deletions.
4 changes: 3 additions & 1 deletion addons/resource.language.en_gb/resources/strings.po
Original file line number Diff line number Diff line change
Expand Up @@ -7289,7 +7289,9 @@ msgctxt "#13423"
msgid "Remember for this path"
msgstr ""

#empty string with id 13424
msgctxt "#13424"

This comment has been minimized.

Copy link
@ksooo

ksooo Apr 27, 2024

Member

@78andyp I know I'm late to the party, but next time, to help our translators, whenever you add a new string, add the files where the string is used and a short verbal description for what the string is used. Look how it is done for other strings in this file and you will see what I mean.

This comment has been minimized.

Copy link
@78andyp

78andyp May 1, 2024

Author Contributor

Done

msgid "Choose playlist"
msgstr ""

#: system/settings/settings.xml
msgctxt "#13425"
Expand Down
25 changes: 24 additions & 1 deletion xbmc/FileItem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1559,6 +1559,9 @@ bool CFileItem::IsSamePath(const CFileItem *item) const
{
if (item->HasProperty("item_start") || HasProperty("item_start"))
return (item->GetProperty("item_start") == GetProperty("item_start"));
// See if we have associated a bluray playlist
if (IsBlurayPlaylist(*this) || IsBlurayPlaylist(*item))
return (GetDynPath() == item->GetDynPath());
return true;
}
if (HasMusicInfoTag() && item->HasMusicInfoTag())
Expand Down Expand Up @@ -1958,6 +1961,22 @@ void CFileItem::SetDynPath(const std::string &path)
m_strDynPath = path;
}

std::string CFileItem::GetBlurayPath() const
{
if (IsBlurayPlaylist(*this))
{
CURL url(GetDynPath());
CURL url2(url.GetHostName()); // strip bluray://
if (url2.IsProtocol("udf"))
// ISO
return url2.GetHostName(); // strip udf://
else if (url.IsProtocol("bluray"))
// BDMV
return url2.Get() + "BDMV/index.bdmv";
}
return GetDynPath();
}

void CFileItem::SetCueDocument(const CCueDocumentPtr& cuePtr)
{
m_cueDocument = cuePtr;
Expand Down Expand Up @@ -2472,7 +2491,11 @@ std::string CFileItem::GetLocalMetadataPath() const
if (m_bIsFolder && !IsFileFolder())
return m_strPath;

std::string parent(URIUtils::GetParentPath(m_strPath));
std::string parent{};
if (IsBlurayPlaylist(*this))
parent = URIUtils::GetParentPath(GetBlurayPath());
else
parent = URIUtils::GetParentPath(m_strPath);
std::string parentFolder(parent);
URIUtils::RemoveSlashAtEnd(parentFolder);
parentFolder = URIUtils::GetFileName(parentFolder);
Expand Down
2 changes: 2 additions & 0 deletions xbmc/FileItem.h
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ class CFileItem :
const std::string &GetDynPath() const;
void SetDynPath(const std::string &path);

std::string GetBlurayPath() const;

This comment has been minimized.

Copy link
@ksooo

ksooo Apr 27, 2024

Member

Not sure this method belongs here, after the recent refactoring done by @notspiff .

This comment has been minimized.

Copy link
@78andyp

78andyp May 1, 2024

Author Contributor

My feeling was path related stuff is still in FileItem - the Isxxxx have been moved out. Happy to put it wherever.


/*! \brief reset class to it's default values as per construction.
Free's all allocated memory.
\sa Initialize
Expand Down
11 changes: 7 additions & 4 deletions xbmc/PlayListPlayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,9 @@ bool CPlayListPlayer::PlayItemIdx(int itemIdx)
return Play();
}

bool CPlayListPlayer::Play(const CFileItemPtr& pItem, const std::string& player)
bool CPlayListPlayer::Play(const CFileItemPtr& pItem,
const std::string& player,
bool forceSelection /* = false */)
{
Id playlistId;
bool isVideo{IsVideo(*pItem)};
Expand Down Expand Up @@ -304,13 +306,14 @@ bool CPlayListPlayer::Play(const CFileItemPtr& pItem, const std::string& player)
SetCurrentPlaylist(playlistId);
Add(playlistId, pItem);

return Play(0, player);
return Play(0, player, false, false, forceSelection);

This comment has been minimized.

Copy link
@ksooo

ksooo Apr 27, 2024

Member

@78andyp this is a perfect example why I hate bool params and magic numbers. What do the two false tell me? What does the 0tell me? Without looking up the declaration of PlayI have no clue what this call is supposed to do exactly. This is just ment to be a hint, because you added another bool param to that method, making it even worse.

This comment has been minimized.

Copy link
@78andyp

78andyp May 1, 2024

Author Contributor

Noted for the future, thanks.

}

bool CPlayListPlayer::Play(int iSong,
const std::string& player,
bool bAutoPlay /* = false */,
bool bPlayPrevious /* = false */)
bool bPlayPrevious /* = false */,
bool forceSelection /* = false */)
{
if (m_iCurrentPlayList == TYPE_NONE)
return false;
Expand Down Expand Up @@ -342,7 +345,7 @@ bool CPlayListPlayer::Play(int iSong,
m_bPlaybackStarted = false;

const auto playAttempt = std::chrono::steady_clock::now();
bool ret = g_application.PlayFile(*item, player, bAutoPlay);
bool ret = g_application.PlayFile(*item, player, bAutoPlay, forceSelection);
if (!ret)
{
CLog::Log(LOGERROR, "Playlist Player: skipping unplayable item: {}, path [{}]", m_iCurrentSong,
Expand Down
7 changes: 5 additions & 2 deletions xbmc/PlayListPlayer.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,19 +55,22 @@ class CPlayListPlayer : public IMsgTargetCallback,
* \brief Creates a new playlist for an item and starts playing it
* \param pItem The item to play.
* \param player The player name.
* \param forceSelection for Blurays, force simple menu to change playlist (default to false)
* \return True if has success, otherwise false.
*/
bool Play(const CFileItemPtr& pItem, const std::string& player);
bool Play(const CFileItemPtr& pItem, const std::string& player, bool forceSelection = false);

This comment has been minimized.

Copy link
@ksooo

ksooo Apr 27, 2024

Member

Default params are not a good design pattern. Most of the time it is just laziness of the developer, because less code needs to be adapted. Not to talk about the beforementioned general problem with bool params.

This comment has been minimized.

Copy link
@78andyp

78andyp May 1, 2024

Author Contributor

Noted for the future, thanks.


/*! \brief Start playing a particular entry in the current playlist
\param index the index of the item to play. This value is modified to ensure it lies within the current playlist.
\param replace whether this item should replace the currently playing item. See CApplication::PlayFile (defaults to false).
\param playPreviousOnFail whether to go back to the previous item if playback fails (default to false)
\param forceSelection for Blurays, force simple menu to change playlist (default to false)
*/
bool Play(int index,
const std::string& player,
bool replace = false,
bool playPreviousOnFail = false);
bool playPreviousOnFail = false,
bool forceSelection = false);

/*! \brief Returns the index of the current item in active playlist.
\return Current item in the active playlist.
Expand Down
9 changes: 6 additions & 3 deletions xbmc/application/Application.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2286,7 +2286,10 @@ bool CApplication::PlayStack(CFileItem& item, bool bRestart)
return PlayFile(selectedStackPart, "", true);
}

bool CApplication::PlayFile(CFileItem item, const std::string& player, bool bRestart)
bool CApplication::PlayFile(CFileItem item,
const std::string& player,
bool bRestart /* = false */,
bool forceSelection /* = false */)
{
// Ensure the MIME type has been retrieved for http:// and shout:// streams
if (item.GetMimeType().empty())
Expand Down Expand Up @@ -2426,7 +2429,7 @@ bool CApplication::PlayFile(CFileItem item, const std::string& player, bool bRes

// a disc image might be Blu-Ray disc
if (!(options.startpercent > 0.0 || options.starttime > 0.0) &&
(IsBDFile(item) || item.IsDiscImage()))
(IsBDFile(item) || item.IsDiscImage() || (IsBlurayPlaylist(item) && forceSelection)))

This comment has been minimized.

Copy link
@ksooo

ksooo Apr 27, 2024

Member

IsBlurayPlaylist(item) && forceSelection- terms concatenated with logical AND should always be evaluated from cheap to expensive, CPU cycle wise, and evaluating a bool variable is definitely cheaper than calling a function and evaluating its return value.

This comment has been minimized.

Copy link
@78andyp

78andyp May 1, 2024

Author Contributor

Fixed, thanks.

{
// No video selection when using external or remote players (they handle it if supported)
const bool isSimpleMenuAllowed = [&]()
Expand All @@ -2444,7 +2447,7 @@ bool CApplication::PlayFile(CFileItem item, const std::string& player, bool bRes
if (isSimpleMenuAllowed)
{
// Check if we must show the simplified bd menu.
if (!CGUIDialogSimpleMenu::ShowPlaySelection(item))
if (!CGUIDialogSimpleMenu::ShowPlaySelection(item, forceSelection))
return true;
}
}
Expand Down
5 changes: 4 additions & 1 deletion xbmc/application/Application.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,10 @@ class CApplication : public IWindowManagerCallback,
PLAYLIST::CPlayList& playlist,
PLAYLIST::Id playlistId,
int track = 0);
bool PlayFile(CFileItem item, const std::string& player, bool bRestart = false);
bool PlayFile(CFileItem item,
const std::string& player,
bool bRestart = false,
bool forceSelection = false);
void StopPlaying();
void Restart(bool bSamePosition = true);
void DelayedPlayerRestart();
Expand Down
1 change: 1 addition & 0 deletions xbmc/dialogs/GUIDialogContextMenu.h
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ enum CONTEXT_BUTTON
CONTEXT_BUTTON_PLAY_NEXT,
CONTEXT_BUTTON_NAVIGATE,
CONTEXT_BUTTON_MANAGE_VIDEOVERSIONS,
CONTEXT_BUTTON_CHOOSE_PLAYLIST,
};

class CContextButtons : public std::vector< std::pair<size_t, std::string> >
Expand Down
17 changes: 13 additions & 4 deletions xbmc/dialogs/GUIDialogSimpleMenu.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
#include "video/VideoFileItemClassify.h"
#include "video/VideoInfoTag.h"

std::string m_savePath;

This comment has been minimized.

Copy link
@ksooo

ksooo Apr 27, 2024

Member

This is not a member variable, but a global variable, thus it should have prefix g_, not m_- despite of this if you need a global variable this is almost ever a hint for a bad design.

BTW: if I see this correctly, this varibale is only used from inside CGUIDialogSimpleMenu, so it easily could be actually a member variable.

This comment has been minimized.

Copy link
@78andyp

78andyp May 1, 2024

Author Contributor

It might be better as a property in the FileItem as that's what it relates to and would save another variable or passing variable.


using namespace KODI;

namespace
Expand All @@ -52,12 +54,18 @@ class CGetDirectoryItems : public IRunnable
};
}


bool CGUIDialogSimpleMenu::ShowPlaySelection(CFileItem& item)
bool CGUIDialogSimpleMenu::ShowPlaySelection(CFileItem& item, bool forceSelection /* = false */)
{
if (CServiceBroker::GetSettingsComponent()->GetSettings()->GetInt(CSettings::SETTING_DISC_PLAYBACK) != BD_PLAYBACK_SIMPLE_MENU)
return true;

m_savePath = "";
if (VIDEO::IsBlurayPlaylist(item) && forceSelection)

This comment has been minimized.

Copy link
@ksooo

ksooo Apr 27, 2024

Member

reverse the order of evaluation

This comment has been minimized.

Copy link
@78andyp

78andyp May 1, 2024

Author Contributor

Done.

{
m_savePath = item.GetDynPath(); // save for screen refresh later
item.SetDynPath(item.GetBlurayPath());
}

if (VIDEO::IsBDFile(item))
{
std::string root = URIUtils::GetParentPath(item.GetDynPath());
Expand Down Expand Up @@ -127,10 +135,11 @@ bool CGUIDialogSimpleMenu::ShowPlaySelection(CFileItem& item, const std::string&

if (item_new->m_bIsFolder == false)
{
std::string original_path = item.GetDynPath();
if (m_savePath.empty()) // If not set above (choose playlist selected)
m_savePath = item.GetDynPath();
item.SetDynPath(item_new->GetDynPath());
item.SetProperty("get_stream_details_from_player", true);
item.SetProperty("original_listitem_url", original_path);
item.SetProperty("original_listitem_url", m_savePath);
return true;
}

Expand Down
2 changes: 1 addition & 1 deletion xbmc/dialogs/GUIDialogSimpleMenu.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class CGUIDialogSimpleMenu
public:

/*! \brief Show dialog allowing selection of wanted playback item */
static bool ShowPlaySelection(CFileItem& item);
static bool ShowPlaySelection(CFileItem& item, bool forceSelection = false);
static bool ShowPlaySelection(CFileItem& item, const std::string& directory);

protected:
Expand Down
26 changes: 21 additions & 5 deletions xbmc/utils/SaveFileStateJob.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,24 @@ void CSaveFileState::DoWork(CFileItem& item,
{
std::string progressTrackingFile = item.GetPath();

if (item.HasVideoInfoTag() && StringUtils::StartsWith(item.GetVideoInfoTag()->m_strFileNameAndPath, "removable://"))
progressTrackingFile = item.GetVideoInfoTag()->m_strFileNameAndPath; // this variable contains removable:// suffixed by disc label+uniqueid or is empty if label not uniquely identified
if (item.HasVideoInfoTag() &&
StringUtils::StartsWith(item.GetVideoInfoTag()->m_strFileNameAndPath, "removable://"))
progressTrackingFile =
item.GetVideoInfoTag()
->m_strFileNameAndPath; // this variable contains removable:// suffixed by disc label+uniqueid or is empty if label not uniquely identified
else if (IsBlurayPlaylist(item) && (item.GetVideoContentType() == VideoDbContentType::MOVIES ||
item.GetVideoContentType() == VideoDbContentType::EPISODES))
progressTrackingFile = item.GetDynPath();
else if (item.HasVideoInfoTag() && IsVideoDb(item))
progressTrackingFile = item.GetVideoInfoTag()->m_strFileNameAndPath; // we need the file url of the video db item to create the bookmark
progressTrackingFile =
item.GetVideoInfoTag()
->m_strFileNameAndPath; // we need the file url of the video db item to create the bookmark
else if (item.HasProperty("original_listitem_url"))
{
// only use original_listitem_url for Python, UPnP and Bluray sources
std::string original = item.GetProperty("original_listitem_url").asString();
if (URIUtils::IsPlugin(original) || URIUtils::IsUPnP(original) || URIUtils::IsBluray(item.GetPath()))
if (URIUtils::IsPlugin(original) || URIUtils::IsUPnP(original) ||
URIUtils::IsBluray(item.GetPath()))
progressTrackingFile = original;
}

Expand Down Expand Up @@ -162,7 +171,14 @@ void CSaveFileState::DoWork(CFileItem& item,
if (!videodatabase.GetStreamDetails(dbItem) ||
dbItem.GetVideoInfoTag()->m_streamDetails != item.GetVideoInfoTag()->m_streamDetails)
{
videodatabase.SetStreamDetailsForFile(item.GetVideoInfoTag()->m_streamDetails, progressTrackingFile);
const int idFile = videodatabase.SetStreamDetailsForFile(
item.GetVideoInfoTag()->m_streamDetails, item.GetDynPath());
if (item.GetVideoContentType() == VideoDbContentType::MOVIES)
videodatabase.SetFileForMovie(item.GetDynPath(), item.GetVideoInfoTag()->m_iDbId,
idFile);
else if (item.GetVideoContentType() == VideoDbContentType::EPISODES)
videodatabase.SetFileForEpisode(item.GetDynPath(), item.GetVideoInfoTag()->m_iDbId,
idFile);
updateListing = true;
}
}
Expand Down
Loading

0 comments on commit 54a63d0

Please sign in to comment.