Streamdetails fixes #2264

Merged
merged 6 commits into from Mar 4, 2013

Projects

None yet

4 participants

@arnova
Member
arnova commented Feb 19, 2013

This PR fixes a few problems concerning stream details:

  • Stream details are now properly loaded/stored for items with incomplete/empty infotags;
  • Fixed racing between DVDPlayer's GetStreamDetails() and CApp:PlayFile() causing the duration/aspect to always be wrong (0) causing it to be stored in the db over and over again and screwing up the streamdetails in the infotag. This would also occasionally cause showing 4:3 in the GUI for video items with totally different AR;
  • As a bonus this puts all the logic in one place (FileSaveState job).
Member
elupus commented Feb 19, 2013

Need to be re based and cleaned from irellevant commits.

Member
arnova commented Feb 20, 2013

@jmarshallnz / @Montellese : Thanks for the input/hints. I've updated the PR, please review.

@elupus: Rebased/cleaned it. Please have a look at the change in CDVDPlayer::GetStreamDetails() as it seems that has always been broken/never properly tested. Only other way I see to fix that is to either init m_State before Process() or move the storing of the streamdetails in CApp to SaveFileState, so they get stored after playback finishes.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Feb 20, 2013
xbmc/Application.cpp
@@ -4129,20 +4129,27 @@ bool CApplication::PlayFile(const CFileItem& item, bool bRestart)
if (!item.IsDVDImage() && !item.IsDVDFile())
{
- CVideoInfoTag *details = m_itemCurrentFile->GetVideoInfoTag();
- // Save information about the stream if we currently have no data
+ CVideoDatabase dbs;
+ dbs.Open();
+ CFileItem item2(item);
+ dbs.GetStreamDetails(item2); // Fetch stream details from the db as they are not guaranteed to be in the tag yet
+ CVideoInfoTag *details = item2.GetVideoInfoTag();
+
jmarshallnz
jmarshallnz Feb 20, 2013 Member

Surely this is equivalent to dbs.GetStreamDetails(item) ? Is it because item is const?

arnova
arnova Feb 21, 2013 Member

item is const, so that doesn't work unless we change the parameter declaration of PlayFile()

Member
arnova commented Feb 21, 2013

@elupus / @jmarshallnz : I also fixed the last remaining issue by moving the storage of the streamdetails from CApp::PlayFile() to the SaveFileState-job. Strangely enough it was already there for DVD stuff but no-one ever bothered to merge it with the stuff in PlayFile(). Let me know what you guys think.

@jmarshallnz jmarshallnz commented on an outdated diff Feb 26, 2013
xbmc/video/VideoThumbLoader.cpp
@@ -212,11 +212,11 @@ bool CVideoThumbLoader::LoadItem(CFileItem* pItem)
{
FillLibraryArt(*pItem);
- if (!pItem->GetVideoInfoTag()->m_type.empty() &&
- pItem->GetVideoInfoTag()->m_type != "movie" &&
- pItem->GetVideoInfoTag()->m_type != "tvshow" &&
- pItem->GetVideoInfoTag()->m_type != "episode" &&
- pItem->GetVideoInfoTag()->m_type != "musicvideo")
+ if (!( (pItem->GetVideoInfoTag()->m_type.empty() && (pItem->IsVideo() || pItem->m_bIsFolder)) ||
+ pItem->GetVideoInfoTag()->m_type == "movie" ||
+ pItem->GetVideoInfoTag()->m_type == "tvshow" ||
+ pItem->GetVideoInfoTag()->m_type == "episode" ||
+ pItem->GetVideoInfoTag()->m_type == "musicvideo" ))
jmarshallnz
jmarshallnz Feb 26, 2013 Member

Don't change code when it's not needed. If there's an issue, then state what it is. There isn't any afaik.

@jmarshallnz jmarshallnz commented on an outdated diff Feb 26, 2013
xbmc/video/VideoThumbLoader.cpp
@@ -201,9 +201,9 @@ bool CVideoThumbLoader::LoadItem(CFileItem* pItem)
m_database->Open();
if (pItem->HasVideoInfoTag() && !pItem->GetVideoInfoTag()->HasStreamDetails() &&
- (pItem->GetVideoInfoTag()->m_type == "movie" || pItem->GetVideoInfoTag()->m_type == "episode" || pItem->GetVideoInfoTag()->m_type == "musicvideo"))
+ ( (pItem->GetVideoInfoTag()->m_type.empty() && pItem->IsVideo()) || pItem->GetVideoInfoTag()->m_type == "movie" || pItem->GetVideoInfoTag()->m_type == "episode" || pItem->GetVideoInfoTag()->m_type == "musicvideo"))
{
jmarshallnz
jmarshallnz Feb 26, 2013 Member

Why not replace the entire condition with pItem->IsVideo() ?

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Feb 26, 2013
xbmc/utils/SaveFileStateJob.h
- updateListing = true;
+ if (m_item.IsDVDImage() || m_item.IsDVDFile())
+ {
+ videodatabase.SetStreamDetailsForFile(m_item.GetVideoInfoTag()->m_streamDetails, progressTrackingFile);
+ updateListing = true;
+ }
+ else
+ {
+ CFileItem dbItem(m_item);
+ videodatabase.GetStreamDetails(dbItem); // Fetch stream details from the db as they are not guaranteed to be in m_item's tag yet
+
+ // In case the db has no streamdetails and/or no duration, store the info in the db
+ if (!dbItem.GetVideoInfoTag()->HasStreamDetails() ||
+ (dbItem.GetVideoInfoTag()->m_streamDetails.GetVideoDuration() <= 0 && m_item.GetVideoInfoTag()->m_streamDetails.GetVideoDuration() > 0))
+ {
+ if (m_item.GetVideoInfoTag()->m_iFileId < 0)
jmarshallnz
jmarshallnz Feb 26, 2013 Member

Do we really need this if? Can't we just always us SetStreamDetailsForFile ?

arnova
arnova Feb 27, 2013 Member

Yup, I think that should work too.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Feb 26, 2013
xbmc/utils/SaveFileStateJob.h
{
- videodatabase.SetStreamDetailsForFile(m_item.GetVideoInfoTag()->m_streamDetails,progressTrackingFile);
- updateListing = true;
+ if (m_item.IsDVDImage() || m_item.IsDVDFile())
+ {
+ videodatabase.SetStreamDetailsForFile(m_item.GetVideoInfoTag()->m_streamDetails, progressTrackingFile);
jmarshallnz
jmarshallnz Feb 26, 2013 Member

Why do we always set it for dvd images but don't always set it for everything else?

Surely we want to either always set it or only set it when not already set regardless of item type?

arnova
arnova Feb 27, 2013 Member

Well this was a remnant of 8877348 . I'll clean it up and properly document why it is the way it is.

@jmarshallnz jmarshallnz commented on the diff Feb 26, 2013
xbmc/cores/dvdplayer/DVDPlayer.cpp
if (result && details.GetStreamCount(CStreamDetail::VIDEO) > 0) // this is more correct (dvds in particular)
{
- GetVideoAspectRatio(((CStreamDetailVideo*)details.GetNthStream(CStreamDetail::VIDEO,0))->m_fAspect);
- ((CStreamDetailVideo*)details.GetNthStream(CStreamDetail::VIDEO,0))->m_iDuration = GetTotalTime() / 1000;
+ float aspect;
+ GetVideoAspectRatio(aspect);
+ if (aspect > 0.0f)
+ ((CStreamDetailVideo*)details.GetNthStream(CStreamDetail::VIDEO,0))->m_fAspect = aspect;
+
+ int64_t duration = GetTotalTime() / 1000;
+ if (duration > 0)
+ ((CStreamDetailVideo*)details.GetNthStream(CStreamDetail::VIDEO,0))->m_iDuration = duration;
jmarshallnz
jmarshallnz Feb 26, 2013 Member

Why is this change needed? Is it to ensure you don't override information already in the details??

arnova
arnova Feb 27, 2013 Member

Unfortunately the discussion I already had with Elupus about this got lost. But the bottomline is that getting duration/aspect from DVDPlayer itself only works if the Process() thread is already running (and UpdatePlayState() got at least called once). This code is for the cases where we call GetStreamDetails() before Process() is running so it falls back to the aspect/duration from the demuxer.

jmarshallnz
jmarshallnz Mar 3, 2013 Member

Perhaps a comment here is due then?

Member

Looks like some squashing is in order to reduce the number of commits.

Member
arnova commented Feb 27, 2013

Will squash as soon as this is ready to be merged.

Member
arnova commented Feb 27, 2013

@jmarshallnz : Updated & squashed PR (and fixed another race in UpdateFileState()). Let me know whether everything is ok now.

arnova added some commits Feb 19, 2013
Member
arnova commented Mar 3, 2013

@jmarshallnz : Is this ok for merge now?

@jmarshallnz jmarshallnz commented on the diff Mar 3, 2013
xbmc/utils/StreamDetails.h
@@ -92,6 +92,8 @@ class CStreamDetails : public IArchivable, public ISerializable
CStreamDetails(const CStreamDetails &that);
~CStreamDetails() { Reset(); };
CStreamDetails& operator=(const CStreamDetails &that);
+ bool operator ==(const CStreamDetails &that) const;
+ bool operator !=(const CStreamDetails &that) const;
jmarshallnz
jmarshallnz Mar 3, 2013 Member

Isn't != compiler generated once == is defined?

arnova added some commits Feb 17, 2013
arnova fixed: Move storing of streamdetails from CApp::PlayFile() to the Sav…
…eFileState-job, where it already partially was (for DVD stuff).

This fixes a few issues:
- Races causing possible invalid streamdetails to be stored in the db;
- Stream details for current file were not stored when item was not in the db yet;
- Check whether the streamdetails in the db need updating by comparing. This fixes also fixes previously invalid/incomplete stored details;
- Also clarify the special case for DVD stuff.
48ad150
arnova fixed: We can only obtain the aspect & duration this way when the Pro…
…cess() thread is running (and UpdatePlayState() has been called at least once) else CApp will race getting the streamdetails on player start and get 0 for ratio/duration. In that case fallback to info from the demuxer
dadc488
arnova changed: When checking for eg. user thumbs just check whether it's ei…
…ther a folder or a video file
3871162
arnova fixed: Instead of checking whether the item is video, check whether w…
…e're playing video else we may obtain invalid info due to racing
3cd2687
Member
arnova commented Mar 4, 2013

@jmarshallnz : Updated everything as suggested, is this ready to go now?

@jmarshallnz jmarshallnz merged commit f9cfcdb into xbmc:master Mar 4, 2013
@davilla
Contributor
davilla commented on dadc488 Mar 5, 2013

@arnova , if you want this in Frodo 12.1 then you need to resolve the conflicts as it requires #2269 and that I cannot bring in.

Member

@davilla: Sure. How do you want this handled?

Contributor

Do a PR against Frodo branch. Then I can pull it in and then we are nicely documented.

Member

Sure np. Didn't know/realise. Btw. 60? I thought DOS decided long ago the default max was 80? :p

Member

@davilla made PR2392 as requested for Frodo branch.

Contributor

thx, will pull it in when I hit the next round of backports

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment