fixed: Stream flags would show for non-video files/items (fixes #14176) #2486

Merged
merged 8 commits into from Apr 9, 2013

3 participants

@arnova
Team Kodi member

No description provided.

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Mar 22, 2013
xbmc/video/VideoThumbLoader.cpp
@@ -200,7 +200,11 @@ bool CVideoThumbLoader::LoadItem(CFileItem* pItem)
m_database->Open();
- if (pItem->HasVideoInfoTag() && !pItem->GetVideoInfoTag()->HasStreamDetails() && pItem->IsVideo())
+ if (pItem->HasVideoInfoTag() && !pItem->GetVideoInfoTag()->HasStreamDetails() &&
+ (pItem->GetVideoInfoTag()->m_type.empty() ||
+ pItem->GetVideoInfoTag()->m_type == "movie" ||
+ pItem->GetVideoInfoTag()->m_type == "episode" ||
+ pItem->GetVideoInfoTag()->m_type == "musicvideo"))
{
@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Mar 22, 2013

I think in this block we can rule out folders with a not a folder check? Basically any files will do, right?

What checks for stream details for files not in the library that have stream details extracted already but don't have an info tag set? Are they taken care of somewhere else? Or do you never get a video file without an info tag in here?

This doesn't apply to the block below however.

@arnova
Team Kodi member
arnova added a line comment Mar 23, 2013

Yup, just files will do: updated with !m_bIsFolder check.

Afaik as soon as an item gets stream details extracted (thumbs and/or flags) it gets automatically added to the library. I've tested this myself and it seems to work fine.

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

Compiled with this patch

Only thing it did was reset all my Files view modes to List instead of mediainfo

Library was unscaved from view reset but still shows issues on tv shows and movie sets in various views. SO unfortunately unless Im missing something, no joy.

uNi

@arnova
Team Kodi member

@uNiversaI : I think I understand why, the logic at the end of loaditem has always been flawed (which I just noticed now). I'll fix it up which should take care of it.

@arnova
Team Kodi member

@uNiversaI / @jmarshallnz : Updated PR with another fix, please let me know whether it's ok.

@un1versal

Ok presuming that only that last patch is applied. I can confirm that the rogue flag being pulled for non file items, does indeed fix the issue.

Thank you @arnova for addressing this large annoyance and @jm for queuing this under the radar.

thanks again, gratitude.

@un1versal

It also would be worth mentioning that with aeed179 it also works for extracting the listing SD/HD (blue/green) flag for multiepisodes showname S01E01E02.extension type scenario, as previously the second episode was listed without that flag until playback was initiated/stopped.
It also worked if file was not in library and previously had been, haven't tested if file was not in library at any point and haven't test stacked/non stacked files, but can test all those scenarios out.

If testing is welcome for this last bit let me know what commits to pull and compile as its gotten somewhat confusing now which is the patch(s) to pull/compile, presume its's 2c48b89 and 4c063c7.

Please provide instructions and I will test it and confirm its those two last patches needed only.

Thanks again.

@arnova
Team Kodi member

@jmarshallnz : I updated the PR. It also includes a fixed for the automatic extraction of flags/thumbs which didn't previously work for items without a videoinfotag. Let me know whether it's ok now.

@jmarshallnz
Team Kodi member

I think some of the first and third commits could be squashed, right?

@arnova
Team Kodi member

@jmarshallnz : Updated and squashed (as much as seemed logical). Let me know whether there's anything missing and/or needs reshuffling/squashing.

@arnova
Team Kodi member

@jmarshallnz: Ping. Only thing I'm not 100% sure about is the approach I chose in IsVideo(bool). alternatively we could also use the infotag's filename to determine whether it's a video file...

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Apr 5, 2013
xbmc/video/VideoDatabase.cpp
+ if (fileId < 0)
+ return false;
+
+ return GetStreamDetails(*item.GetVideoInfoTag());
+}
@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Apr 5, 2013

I think this could be improved slightly as follows:

if (fileId < 0)
  fileId = GetFileId(item);

if (fileId < 0)
  return false;

// have a file id, get stream details if available (creates tag either way)
item.GetVideoInfoTag()->m_iFileId = fileId;
return GetStreamDetails(*item.GetVideoInfoTag());
@arnova
Team Kodi member
arnova added a line comment Apr 5, 2013

Why would we want to do a GetFileId(item) if the its VideoInfoTag may already have a fileid, seems like waisting a db call...?

@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Apr 5, 2013

Thus the if statement?

@arnova
Team Kodi member
arnova added a line comment Apr 5, 2013

If you mean by "thus":

if (fileId >= 0)
item.GetVideoInfoTag()->m_iFileId = fileId;

Then yes...

@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Apr 5, 2013

Not sure what you are referring to. I mean exactly the code I wrote in place of the block that I commented on. If you already have the fileId from the infotag, then fileId >= 0 so the if (fileId < 0) blocks aren't executed, thus no db lookup other than the streamdetails call.

@arnova
Team Kodi member
arnova added a line comment Apr 5, 2013

Oh crap, I missed the piece of code that gets the FileId from the infotag before that block. Sorry: still early here in Holland.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Apr 5, 2013
xbmc/video/VideoThumbLoader.cpp
{
+ // Only extract details when we either don't have an infotag or no streamdetails already
@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Apr 5, 2013

not sure the comment here adds anything other than confusion. Maybe

// no tag or no details set, so extract them.

@arnova
Team Kodi member
arnova added a line comment Apr 5, 2013

Agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Apr 5, 2013
xbmc/video/VideoThumbLoader.cpp
@@ -200,7 +201,7 @@ bool CVideoThumbLoader::LoadItem(CFileItem* pItem)
m_database->Open();
- if (pItem->HasVideoInfoTag() && !pItem->GetVideoInfoTag()->HasStreamDetails() && pItem->IsVideo())
+ if ((pItem->m_bIsFolder || pItem->IsVideo(true)) && (!pItem->HasVideoInfoTag() || !pItem->GetVideoInfoTag()->HasStreamDetails() ) )
@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Apr 5, 2013

this conditional either is wrong or needs some explanation. I would think we should be able to go with ignoring the video type here, as that should be taken care of in the database function anyway (no fileid can be derived -> no details)

@arnova
Team Kodi member
arnova added a line comment Apr 5, 2013

Well the idea was again that we want to prevent useles db calls and thus at least make sure it's an item that could possibly have streamdetails (videofile or folder), right?

@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Apr 5, 2013

Sure, but you allow all folder items anyway, most of which won't have a fileid.

@arnova
Team Kodi member
arnova added a line comment Apr 5, 2013

That's true. Well I'm all for preventing db calls where possible (especially beneficial for eg. ARM), so if you really want no longer want to check the file-type here, I'll change it.

@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Apr 5, 2013

I agree with the principle, but it's proving tricky to get a nice conditional. :) I think the only folders allowed are those with a fileid already available in the videoinfotag, so maybe something like:

if (!HasVideoInfoTag() || !GetVideoInfoTag()->HasStreamDetails()) // no stream details
{
  if ((HasVideoInfoTag() && GetVideoInfoTag()->m_iFileId >= 0) // file (or maybe folder) is in the database
  || (IsVideo() && !m_bIsFolder)) // some other video file for which we haven't yet got any database details
    // grab them there streamdetails from the db.
}

Something similar may then work for the extraction stuff below maybe?

@arnova
Team Kodi member
arnova added a line comment Apr 5, 2013

Yeah at first sight that seems to make sense. I'll let it run through my mind a few times to make sure it's the best approach

@arnova
Team Kodi member
arnova added a line comment Apr 5, 2013

I'm not entirely sure about the folder thing, although I can't think of a scenario right now. I do think this seems slightly better:

if (!HasVideoInfoTag() || !GetVideoInfoTag()->HasStreamDetails()) // no stream details
{
if ((HasVideoInfoTag() && GetVideoInfoTag()->m_iFileId >= 0) // file (or maybe folder) is in the database
|| (IsVideo(true)) // some other video file for which we haven't yet got any database details
// grab them there streamdetails from the db.
}

Agreed?

@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Apr 5, 2013

IsVideo(true) just does !m_bIsFolder && IsVideo() pretty much - only difference is we only go with the infotag if m_type is not set or set to movie, episode or musicvideo. Given that the only other times m_type is set is for folder items from the database, this seems not required.

You'll notice I've subtly got around all uses of IsVideo(true) :p

@arnova
Team Kodi member
arnova added a line comment Apr 6, 2013

Well, honestly I don't see the point of doing it the way you propose if we might as well use IsVideo(true) for that. But I'm fine with it either way, as I just like to wrap this PR up ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Apr 5, 2013
xbmc/FileItem.cpp
+ if (filesOnly)
+ {
+ if (m_bIsFolder)
+ return false;
+
+ if (HasVideoInfoTag() &&
+ (GetVideoInfoTag()->m_type == "movie" ||
+ GetVideoInfoTag()->m_type == "episode" ||
+ GetVideoInfoTag()->m_type == "musicvideo") )
+ return true;
+ }
+ else
+ {
+ if (HasVideoInfoTag()) return true;
+ if (HasMusicInfoTag()) return false;
+ if (HasPictureInfoTag()) return false;
@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Apr 5, 2013

These two should be done in both branches (move out as per PVR)

@arnova
Team Kodi member
arnova added a line comment Apr 5, 2013

Ah yes, makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Apr 5, 2013
xbmc/video/VideoThumbLoader.cpp
@@ -244,10 +249,10 @@ bool CVideoThumbLoader::LoadItem(CFileItem* pItem)
SetArt(*pItem, artwork);
}
- // thumbnails are special-cased due to auto-generation
- if (!pItem->m_bIsFolder && pItem->IsVideo())
+ // We can only extract flags/thumbs for file-like items
+ if (pItem->IsVideo(true))
@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Apr 5, 2013

I'm not sure if this actually makes any difference. Does anything return true for the original conditional but false for the new one?

@arnova
Team Kodi member
arnova added a line comment Apr 5, 2013

Yup: This is where a very old bug was creeping. We would always try to extract thumbs/flags for m_type==tvshow here due to the IsVideo() returning true because of HasVideoInfoTag()...

@jmarshallnz
Team Kodi member
jmarshallnz added a line comment Apr 5, 2013

Yes, but !pItem->m_bIsFolder is false for tvshows, right?

@arnova
Team Kodi member
arnova added a line comment Apr 5, 2013

Hmm yes (at least I like to believe it does). Oh I recall now where the actual problem I mentioned was: it was in the block below that handles the flag-extraction. If you want this back to the way it was, I'll change it back, I'm fine with it either way.

@arnova
Team Kodi member
arnova added a line comment Apr 6, 2013

Same goes for the one here: Either IsVideo(true) or "IsVideo() && !m_bIsFolder" is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arnova
Team Kodi member

@jmarshallnz : All updated/squashed. As you can see I've kept the 2 uses of IsVideo(true) as I really believe those are much safer/accurate to determine whether those blocks should be run. Especially if the surrounding code is ever going to be changed (again). Relying on the fact that hitting the HasVideoInfoTag() in IsVideo() won't happen due to other factors in the equation isn't going to do it for me, and I really don't want this to bite us/me in the tail again...

@jmarshallnz
Team Kodi member

The reason I prefer (!pItem->m_bIsFolder && pItem->IsVideo()) is because it's obvious from reading the code what the conditions are. Now it's masked behind IsVideo(true) so needs a comment so you know what it means. Self-documenting code seems better to me, particularly when it's simple.

The tricky bit here was that some folders can have streamdetails previously assigned. This is taken care of by the check for m_iFileId, so the tricky bit isn't even affected by this conditional.

The simple bit is determining when to generate stream details or auto-thumbs. In this case you need only make sure you don't have a folder and that it's a video. Nice and simple to understand.

@arnova
Team Kodi member

@jmarshallnz: If you put it that way I agree. I updated the PR, I do wonder whether you want to get rid of the commit that implements IsVideo(bool) now?

@jmarshallnz
Team Kodi member

I think so, yeah. maybe replace it with a bit of doxy (on IsVideo, IsMusic, IsPicture) about the potential issue?

@arnova
Team Kodi member

@jmarshallnz: Removed the commit for IsVideo(bool) + added doxygen.

@jmarshallnz
Team Kodi member

Looks good with that minor change - merge away once done.

@arnova arnova merged commit d1d4334 into xbmc:master Apr 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment