[Video] Improve user experience when playing movies/episodes from Bluray ISO/BDMV. #24720

78andyp · 2024-02-15T21:26:06Z

This is another attempt at #24277 using @rmrector's advice about the VFS.

Complements #24304.

When a movie/episode is played from an ISO/BDMV an additional file is added files table and this is linked to the movies or episodes entry.

I've also fixed the way Play from Here works (or didn't work) for ISOs/BDMVs - by properly removing duplicates.

It doesn't require any database changes (unlike the first attempt).

Description

Motivation and context

How has this been tested?

Locally

What is the effect on users?

Base
Having just refreshed from scraper - no details shown and duration from scraper.

Before
Having just watched episode 1 - the details for both Episodes 1 and 2 (both on first ISO) have changed.

After
Having just watched episode 1 - only episode 1 has changed

Then watching episode 2 - only 2 has changed and 1 is preserved.

Screenshots (if appropriate):

Types of change

Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
Student submission (PR was done for educational purposes and will be treated as such)
None of the above (please explain below)

Checklist:

My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

neo1973

I only checked the code style and didn't check the actual functionality.

neo1973 · 2024-02-24T12:43:50Z

xbmc/FileItem.cpp

+  else
+    return GetDynPath();


Suggested change

else

return GetDynPath();

return GetDynPath();

Remove the else so that the function returns a value on every path.

neo1973 · 2024-02-24T12:46:01Z

xbmc/FileItem.h

@@ -142,6 +142,8 @@ class CFileItem :
  const std::string &GetDynPath() const;
  void SetDynPath(const std::string &path);

+  const std::string GetBlurayPath() const;


Suggested change

const std::string GetBlurayPath() const;

std::string GetBlurayPath() const;

Please don't return by const value, see http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-return-const

neo1973 · 2024-02-24T13:04:10Z

xbmc/windows/GUIMediaWindow.cpp

+                                    const CFileItem& j = *i.get();
+                                    return j.IsZIP() || j.IsRAR() || j.m_bIsFolder;


Suggested change

const CFileItem& j = *i.get();

return j.IsZIP() || j.IsRAR() || j.m_bIsFolder;

return i->IsZIP() || i->IsRAR() || i->m_bIsFolder;

neo1973 · 2024-02-24T13:04:59Z

xbmc/windows/GUIMediaWindow.cpp

+                      return i.get()->GetVideoInfoTag()->m_basePath ==
+                             j.get()->GetVideoInfoTag()->m_basePath;


Suggested change

return i.get()->GetVideoInfoTag()->m_basePath ==

j.get()->GetVideoInfoTag()->m_basePath;

return i->GetVideoInfoTag()->m_basePath ==

j->GetVideoInfoTag()->m_basePath;

neo1973 · 2024-02-24T13:05:31Z

xbmc/windows/GUIMediaWindow.cpp

+                                                   return i.get()->GetVideoInfoTag()->m_basePath ==
+                                                          item.get()->GetVideoInfoTag()->m_basePath;


Suggested change

return i.get()->GetVideoInfoTag()->m_basePath ==

item.get()->GetVideoInfoTag()->m_basePath;

return i->GetVideoInfoTag()->m_basePath ==

item->GetVideoInfoTag()->m_basePath;

78andyp · 2024-02-24T14:55:15Z

I only checked the code style and didn't check the actual functionality.

Thanks for the feedback.

neo1973 · 2024-04-26T03:30:23Z

Please squash the rebase fixes, apply the formatting changes and I'll merge this 🙂

…ray ISO/BDMV.

78andyp · 2024-04-26T13:48:50Z

Please squash the rebase fixes, apply the formatting changes and I'll merge this 🙂

Brilliant, thanks. All done.

Any chance we could get #24794 (folder stacks) in as well?? As that would provide a good base for me to finish off #24304 - as that will need tweaks once both the first two are in (especially for DVDs - the vast majority of the work for that is already done in #24997) as that would then mean those using discs have almost the same seamless experience as those using files. The only remaining work would be around movie versions (ie. handling two versions on one disc - which would need some work with @ksooo and @CrystalP).

Edit - by as welll I mean after this one and I've reconfirmed that they play together nicely.

Thanks
Andy

ksooo · 2024-04-27T19:51:09Z

Sorry that I missed to review this PR before it was merged. I really would like to ask @78andyp to take a look at my comments here 54a63d0 and to proof whether some of them can be addressed in a follow-up PR. Thanks.

78andyp · 2024-04-27T20:03:55Z

Sorry that I missed to review this PR before it was merged. I really would like to ask @78andyp to take a look at my comments here 54a63d0 and to proof whether some of them can be addressed in a follow-up PR. Thanks.

More than happy to. Let me just finish on folder stacks as I've got my head around that (and I'll ask you to review that when done) and I'll come back to this. Thanks for your comments.

[Video] Fix for #25097 and requested updates from #24720

CrystalP · 2024-05-04T23:38:41Z

xbmc/video/VideoDatabase.cpp

+    std::string sql = PrepareSQL("UPDATE movie SET c22='%s', idFile=%i WHERE idMovie=%i",
+                                 fileAndPath.c_str(), idFile, idMovie);
+    m_pDS->exec(sql);
+    sql = PrepareSQL("UPDATE videoversion SET idFile=%i WHERE idMedia=%i AND media_type='movie'",


Sorry for noticing this late and after a fixup commit was just merged.
This line is wrong, it will update all versions/extra of the movie and break them. videoversion.idFile is the only link between a parent movie and its additional versions/extras.
This also breaks version artwork, which uses idFile as key in the art table.
What is the need to change the idFile?
While this function may do the correct thing for a standalone/parent movie, it won't work if you try to apply this to an additional version or an extra.

idFile is changed when the initial path to an ISO/BDMV is resolved to a bluray:// url by user selection. Off the top of my head I think I kept the original so it is not re-added when the directory changes and library is refreshed.

Actually that line doesn't work if there is more than one version - is triggers and exceptoin - idFile is a primary key so can't appear twice.

BeginTransaction(); std::string sql = PrepareSQL("UPDATE movie SET c22='%s', idFile=%i WHERE idMovie=%i", fileAndPath.c_str(), idFile, idMovie); m_pDS->exec(sql); sql = PrepareSQL( "UPDATE videoversion SET idFile=%i WHERE idFile=%i AND idMedia=%i AND media_type='movie'", idFile, oldIdFile, idMovie); m_pDS->exec(sql); CommitTransaction(); sql = PrepareSQL( "UPDATE art SET media_id=%i WHERE media_id=%i AND media_type='videoversion'", idFile, oldIdFile); m_pDS->exec(sql); CommitTransaction();

Seems to do the trick - using two bluray ISOs as versions - playing each (so there is then an additional file entry for each as bluray://) - versions seem OK and independent still and artwork seems fine in the info dialog.

That logic is OK to update the default version of the movie (though the videoversion and art update would be better located in a new SetFileForVideoAsset function). There is an extra CommitTransaction() to remove line 10.

I don't see how it could work fine for the non-default version of a movie. Let's say you have uhd bd iso of a movie as default version and the bd iso as secondary version, and you play the bd.
Modifying the movie record with the bd info would be wrong. Changing idFile changes the default version of the movie > not correct, Changing c22 (path?) seems wrong too - an official file structure hasn't been decided yet but it's unlikely that all versions of a movie will have to live in the same folder.

(to be verified, not related to version). Another potential problem with the idFile change could come from the library scanner / info refresh creating an additional copy of the movie in the library since there is no movie record matching the idFile of the file found by scan anymore.

Thanks.
I hadn't realised that additional versions were removed from the movies database.
I have a couple of ideas and will generate another PR soon.

…).

…fix 2 for xbmc#24720).

78andyp marked this pull request as draft February 15, 2024 21:26

78andyp force-pushed the playlist branch 2 times, most recently from 7186c23 to ae6c6dc Compare February 21, 2024 21:28

78andyp changed the title ~~[WIP][v22][Video] Improve user experience when playing movies/episodes from Bluray ISO/BDMV.~~ [Video] Improve user experience when playing movies/episodes from Bluray ISO/BDMV. Feb 21, 2024

78andyp marked this pull request as ready for review February 21, 2024 21:31

neo1973 added Type: Improvement non-breaking change which improves existing functionality v22 "P" labels Feb 24, 2024

neo1973 added this to the "P" 22.0 Alpha 1 milestone Feb 24, 2024

neo1973 requested changes Feb 24, 2024

View reviewed changes

78andyp force-pushed the playlist branch from ae6c6dc to 657432c Compare February 24, 2024 14:54

78andyp mentioned this pull request Mar 2, 2024

[Video][Stacks] Fix folder stacks. #24794

Closed

14 tasks

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Mar 18, 2024

78andyp force-pushed the playlist branch from 657432c to a684e10 Compare March 18, 2024 13:34

jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Mar 18, 2024

78andyp mentioned this pull request Apr 14, 2024

[Video] Improve episode display/selection for multi-episode discs. #24997

Closed

14 tasks

jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Apr 20, 2024

78andyp force-pushed the playlist branch from a684e10 to c84505a Compare April 22, 2024 23:36

jenkins4kodi removed the Rebase needed PR that does not apply/merge cleanly to current base branch label Apr 23, 2024

[Video] Improve user experience when playing movies/episodes from Blu…

54a63d0

…ray ISO/BDMV.

78andyp force-pushed the playlist branch from a941df9 to 54a63d0 Compare April 26, 2024 13:43

neo1973 approved these changes Apr 26, 2024

View reviewed changes

neo1973 merged commit 4ee6643 into xbmc:master Apr 26, 2024
2 checks passed

smp79 mentioned this pull request Apr 30, 2024

Starting file playback with keyboard Enter button is broken in master #25097

Closed

7 tasks

78andyp added a commit to 78andyp/xbmc that referenced this pull request May 2, 2024

[Video] Fix for xbmc#25097 and requested updates from xbmc#24720.

4317318

78andyp added a commit to 78andyp/xbmc that referenced this pull request May 3, 2024

[Video] Fix for xbmc#25097 and requested updates from xbmc#24720.

3dba96a

78andyp added a commit to 78andyp/xbmc that referenced this pull request May 3, 2024

[Video] Fix for xbmc#25097 and requested updates from xbmc#24720.

178aa38

78andyp added a commit to 78andyp/xbmc that referenced this pull request May 3, 2024

[Video] Fix for xbmc#25097 and requested updates from xbmc#24720.

7a69d0d

enen92 added a commit that referenced this pull request May 4, 2024

Merge pull request #25099 from 78andyp/playlist_fix

db1f89b

[Video] Fix for #25097 and requested updates from #24720

CrystalP reviewed May 4, 2024

View reviewed changes

78andyp deleted the playlist branch May 5, 2024 19:59

78andyp added a commit to 78andyp/xbmc that referenced this pull request May 5, 2024

[Video] Fix for videoversions and bluray:// paths (fix 2 for xbmc#24720)

33432af

78andyp mentioned this pull request May 5, 2024

[Video] Fix for videoversions (and other fixes) and bluray:// paths (fix 2 for #24720). #25140

Open

14 tasks

78andyp added a commit to 78andyp/xbmc that referenced this pull request May 7, 2024

[Video] Fix for videoversions and bluray:// paths (fix 2 for xbmc#24720…

766cdad

…).

78andyp added a commit to 78andyp/xbmc that referenced this pull request May 7, 2024

[Video] Fix for videoversions and bluray:// paths (fix 2 for xbmc#24720…

f5333ec

…).

graysky2 pushed a commit to graysky2/xbmc that referenced this pull request May 16, 2024

[Video] Fix for xbmc#25097 and requested updates from xbmc#24720.

baee50d

78andyp added a commit to 78andyp/xbmc that referenced this pull request May 27, 2024

[Video] Fix for videoversions (and other fixes) and bluray:// paths (…

c606034

…fix 2 for xbmc#24720).

78andyp added a commit to 78andyp/xbmc that referenced this pull request May 27, 2024

[Video] Fix for videoversions (and other fixes) and bluray:// paths (…

807b160

…fix 2 for xbmc#24720).

78andyp added a commit to 78andyp/xbmc that referenced this pull request May 28, 2024

[Video] Fix for videoversions (and other fixes) and bluray:// paths (…

825a172

…fix 2 for xbmc#24720).

78andyp added a commit to 78andyp/xbmc that referenced this pull request Jun 18, 2024

[Video] Fix for videoversions (and other fixes) and bluray:// paths (…

306781a

…fix 2 for xbmc#24720).

78andyp added a commit to 78andyp/xbmc that referenced this pull request Jun 18, 2024

[Video] Fix for videoversions (and other fixes) and bluray:// paths (…

eb71b2d

…fix 2 for xbmc#24720).

78andyp added a commit to 78andyp/xbmc that referenced this pull request Jun 25, 2024

[Video] Fix for videoversions (and other fixes) and bluray:// paths (…

1efc5f5

…fix 2 for xbmc#24720).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Video] Improve user experience when playing movies/episodes from Bluray ISO/BDMV. #24720

[Video] Improve user experience when playing movies/episodes from Bluray ISO/BDMV. #24720

78andyp commented Feb 15, 2024 •

edited

Loading

neo1973 left a comment

neo1973 Feb 24, 2024

78andyp Feb 24, 2024

neo1973 Feb 24, 2024

78andyp Feb 24, 2024

neo1973 Feb 24, 2024

78andyp Feb 24, 2024

neo1973 Feb 24, 2024

78andyp Feb 24, 2024

neo1973 Feb 24, 2024

78andyp Feb 24, 2024

78andyp commented Feb 24, 2024

neo1973 commented Apr 26, 2024

78andyp commented Apr 26, 2024 •

edited

Loading

ksooo commented Apr 27, 2024 •

edited

Loading

78andyp commented Apr 27, 2024

CrystalP May 4, 2024

78andyp May 4, 2024

78andyp May 5, 2024 •

edited

Loading

CrystalP May 5, 2024

78andyp May 5, 2024

	const std::string GetBlurayPath() const;
	std::string GetBlurayPath() const;

		const CFileItem& j = *i.get();
		return j.IsZIP() \|\| j.IsRAR() \|\| j.m_bIsFolder;

	const CFileItem& j = *i.get();
	return j.IsZIP() \|\| j.IsRAR() \|\| j.m_bIsFolder;
	return i->IsZIP() \|\| i->IsRAR() \|\| i->m_bIsFolder;

		return i.get()->GetVideoInfoTag()->m_basePath ==
		j.get()->GetVideoInfoTag()->m_basePath;

		return i.get()->GetVideoInfoTag()->m_basePath ==
		item.get()->GetVideoInfoTag()->m_basePath;

[Video] Improve user experience when playing movies/episodes from Bluray ISO/BDMV. #24720

[Video] Improve user experience when playing movies/episodes from Bluray ISO/BDMV. #24720

Conversation

78andyp commented Feb 15, 2024 • edited Loading

Description

Motivation and context

How has this been tested?

What is the effect on users?

Screenshots (if appropriate):

Types of change

Checklist:

neo1973 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

78andyp commented Feb 24, 2024

neo1973 commented Apr 26, 2024

78andyp commented Apr 26, 2024 • edited Loading

ksooo commented Apr 27, 2024 • edited Loading

78andyp commented Apr 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

78andyp May 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

78andyp commented Feb 15, 2024 •

edited

Loading

78andyp commented Apr 26, 2024 •

edited

Loading

ksooo commented Apr 27, 2024 •

edited

Loading

78andyp May 5, 2024 •

edited

Loading