Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fixes for bug 14119 #2294

Merged
merged 1 commit into from

3 participants

@smfontes

These are the changes required to fix defect 14119 which I opened a few days ago. The problem was that all of the $INFO[slideshow.*] variables were blank when used in SlideShow.xml.
There were two problems.
The first problem was an incorrect condition in an if statement in GUIInfoManager.cpp. The condition tried to use a pointer only if the pointer was NOT set. It should have tried to use the pointer only if it WAS set. I fixed the condition.

The second problem was that when metadata was explicitly set using PictureInfoTag.SetInfo(), it would cause all future calls to load the metadata from the picture file to be ignored. I created a new private variable to distinguish between setting metadata with SetInfo(), and setting metadata by loading it from the picture file. I used the variable to allow loading of metadata from the picture file even after some metadata had been set using SetInfo(). The metadata provided by SetInfo() is used if it does not appear in the file. If the data exists in the file, it overrides that provided by SetInfo().

The only two pieces of metadata accepted by PictureInfoTag.SetInfo() are "exiftime" and "resolution". If SetInfo() is called with any value other than those two, nothing happens. I think this is to handle the case where pictures were created using a scanner rather than a camera. An addon can get the info from the file attributes, then call SetInfo to make sure the picture has at least those two useful pieces of metadata.

@smfontes

Sorry ulion - I didn't refresh the page between my comments, so did not see your reply to take out the first part of the condition.

@ulion
Collaborator

I checked ListItem.cpp, in it:

          else
          {
            const CStdString& exifkey = key;
            if (!exifkey.Left(5).Equals("exif:") || exifkey.length() < 6) continue;
            int info = CPictureInfoTag::TranslateString(exifkey.Mid(5));
            item->GetPictureInfoTag()->SetInfo(info, value);
            pictureTagLoaded = true;
          }
        }
        if (pictureTagLoaded)
          item->GetPictureInfoTag()->SetLoaded(true);

you see, the SetLoaded(true) is called, so finally if the pictureinfotag was set by here, it still will not call Load() in SetCurrentSlide().

@smfontes

Right - that is handled in the second file of my fix. I changed SetLoaded() in PictureInfoTag.cpp to set a different variable called m_isExternallyLoaded, to distinguish between the two cases. If metadata is loaded from a file, m_isLoaded is set to true. That is the only case where m_isLoaded is set to true. If metadata is set by a call to SetInfo() then m_isExternallyLoaded is set to true. This allows some data to be set externally, but does not stop additional data to be loaded from the file. I then modified Load() in PictureInfoTag.cpp with logic to handle using both types of metadata.

xbmc/pictures/PictureInfoTag.cpp
@@ -605,6 +629,6 @@ void CPictureInfoTag::SetInfo(int info, const CStdString& value)
void CPictureInfoTag::SetLoaded(bool loaded)
{
- m_isLoaded = loaded;
+ m_isExternallyLoaded = loaded;
}
@ulion Collaborator
ulion added a note

it's a little weird, SetLoaded(true) but Loaded() still return false. you should consider whether the logic where calling SetLoaded(true) is correct, if it's wrong there, then fix there (e.g. not call SetLoaded(true)), rather than change this place into unsync result about SetLoaded() and Loaded().

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

You are right, calling SetLoaded(true) then Loaded() returns false is weird. But there is really no reason to expose the internal loaded state to be modified externally. Whether data has been loaded from the file or by a call to SetInfo is information that can be maintained by the class itself. We should not allow an external call to change the internal state indicators in a possibly inconsistent way. I removed the ability to call SetLoaded. The class maintains its state consistently. Callers do not have to worry about how they should change the state after they make calls.

@smfontes

Scanner is not part of xbmc. But MyPicturesDatabase doesn't handle files with no exif info any differently than xbmc.
I just tested the MyPicturesDatabase code, and it just passes in info that it already found in the file. So right now we don't need to restore it. We can leave that for a future enhancement. So I will undo all of my changes from the Load() function, and this fix will still solve both problems. The Load() function will not have any diffs from current code.

@ulion
Collaborator

then just only put code for fixing problem 1 & 2 here, others keep in your repo, let's clean it up and merge it.

two more question:
1. does the Load() function set picture width and height to the info tag if the file contains no exif / iptc ?
2. it seems only jpeg file has exif? at least for png file it has not, then will it's info is empty after Load() if previous set by others.

@smfontes

All cleaned up and ready to merge. I've tested it, and everything works. Slideshows display the right info when launched from the context menu, from the left side menu, and recursive slideshows work under picture sources. Slideshows and recursive slideshows launched from the MyPicturesDatabase addon also display the correct info.

Answers to your questions (as far as I can tell)
For number 1, here are the comments in the jpeg parsing routine:

// This module gathers information about a digital image file. This includes:
//   - File name and path
//   - File size
//   - Resolution (if available)
//   - IPTC information (if available)
//   - EXIF information (if available)

It says it gets the resolution "if available", but I can't figure out from the code when it will get it and when it won't. I'd have to do a lot more research into the actual format of a jpeg file.

For question 2. Any info set by SetInfo will be overwritten by the Load. So if Load has empty info, the SetInfo stuff will be lost. The MyPicturesDatabase addon calls SetInfo with time and resolution it extracts from the file, so Load will get the exact same info. I do not know whether any other picture addons use SetInfo()

@smfontes

Thank you for all your help ulion. You pointed me to places that needed more investigation, and you made me simplify things as much as possible. I think we have come up with a good solution.

@ulion
Collaborator
@smfontes

The current pr has no effect on HOW info is loaded. Whatever support was available for png/gif/bmp before this pr, it will still work exactly the same way after this pr. This pr does not add any additional function for png/gif/bmp, but it also does not remove any existing function.

All this pr does is keep track of one extra state variable, so we can (load info externally AND load info from file).
Without this pr, we could only (load info externally OR load info from file).

@smfontes

I just found this ticket with similar intent to fix this problem. http://trac.xbmc.org/ticket/10519. I think we need to add code in the load function as I originally had.

bool CPictureInfoTag::Load(const CStdString &path)
{
  ExifInfo_t savedExifInfo;
  m_isLoaded = false;

  DllLibExif exifDll;
  if (path.IsEmpty() || !exifDll.Load())
    return false;

  /* Its possible that the datetime and resolution were already set externally */
  /* Retrieve them, then - if they are not loaded from the file - put them back in */
  if (m_isExternallyLoaded) savedExifInfo = m_exifInfo;

  if (exifDll.process_jpeg(path.c_str(), &m_exifInfo, &m_iptcInfo))
    m_isLoaded = true;

  /* If set info does not exist in file, add it back in */
  if (m_isExternallyLoaded){
    if (m_exifInfo.DateTime == "") strcpy(m_exifInfo.DateTime,savedExifInfo.DateTime);
    if (m_exifInfo.Height == 0) m_exifInfo.Height = savedExifInfo.Height;
    if (m_exifInfo.Width == 0) m_exifInfo.Width = savedExifInfo.Width;
  }

  return m_isLoaded;
}

Can you get jmarshall to comment on this pr?

@ulion
Collaborator

@jmarshallnz , we seems reverted your old commit, please help here.

xbmc/pictures/PictureInfoTag.cpp
@@ -30,7 +30,8 @@ void CPictureInfoTag::Reset()
{
memset(&m_exifInfo, 0, sizeof(m_exifInfo));
memset(&m_iptcInfo, 0, sizeof(m_iptcInfo));
- m_isLoaded = false;
+ m_isLoaded = false; // Internal state initially shows no metadata has been successfully loaded from the picture file
+ m_isInfoSetExternally = false; // Internal state initially shows no metadata has been set by a call to SetInfo
@jmarshallnz Owner

Remove the comments - it's already present in the header.

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

Looks fine to me once the comments are cleaned up a bit. It essentially takes care of the other ticket in a different (better) way.

@ulion
Collaborator

I think we still has png/gif/bmp file no info issue, but it should fix the Load() method to load these info from picture file anyway, currently it's jpeg only.
current pr is also ok to me.

@ulion ulion merged commit 406f32d into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 1, 2013
  1. fixes for 14119

    sfontes authored
This page is out of date. Refresh to see the latest.
View
2  xbmc/GUIInfoManager.cpp
@@ -5012,7 +5012,7 @@ void CGUIInfoManager::SetCurrentSlide(CFileItem &item)
{
if (m_currentSlide->GetPath() != item.GetPath())
{
- if (!item.HasPictureInfoTag() && !item.GetPictureInfoTag()->Loaded())
+ if (!item.GetPictureInfoTag()->Loaded()) // If picture metadata has not been loaded yet, load it now
item.GetPictureInfoTag()->Load(item.GetPath());
*m_currentSlide = item;
}
View
4 xbmc/interfaces/legacy/ListItem.cpp
@@ -442,7 +442,6 @@ namespace XBMCAddon
}
else if (strcmpi(type,"pictures") == 0)
{
- bool pictureTagLoaded = false;
for (Dictionary::const_iterator it = infoLabels.begin(); it != infoLabels.end(); it++)
{
CStdString key = it->first;
@@ -468,11 +467,8 @@ namespace XBMCAddon
if (!exifkey.Left(5).Equals("exif:") || exifkey.length() < 6) continue;
int info = CPictureInfoTag::TranslateString(exifkey.Mid(5));
item->GetPictureInfoTag()->SetInfo(info, value);
- pictureTagLoaded = true;
}
}
- if (pictureTagLoaded)
- item->GetPictureInfoTag()->SetLoaded(true);
}
} // end ListItem::setInfo
View
14 xbmc/pictures/PictureInfoTag.cpp
@@ -31,6 +31,7 @@ void CPictureInfoTag::Reset()
memset(&m_exifInfo, 0, sizeof(m_exifInfo));
memset(&m_iptcInfo, 0, sizeof(m_iptcInfo));
m_isLoaded = false;
+ m_isInfoSetExternally = false;
}
const CPictureInfoTag& CPictureInfoTag::operator=(const CPictureInfoTag& right)
@@ -39,6 +40,7 @@ const CPictureInfoTag& CPictureInfoTag::operator=(const CPictureInfoTag& right)
memcpy(&m_exifInfo, &right.m_exifInfo, sizeof(m_exifInfo));
memcpy(&m_iptcInfo, &right.m_iptcInfo, sizeof(m_iptcInfo));
m_isLoaded = right.m_isLoaded;
+ m_isInfoSetExternally = right.m_isInfoSetExternally;
return *this;
}
@@ -61,6 +63,7 @@ void CPictureInfoTag::Archive(CArchive& ar)
if (ar.IsStoring())
{
ar << m_isLoaded;
+ ar << m_isInfoSetExternally;
ar << m_exifInfo.ApertureFNumber;
ar << CStdString(m_exifInfo.CameraMake);
ar << CStdString(m_exifInfo.CameraModel);
@@ -124,6 +127,7 @@ void CPictureInfoTag::Archive(CArchive& ar)
else
{
ar >> m_isLoaded;
+ ar >> m_isInfoSetExternally;
ar >> m_exifInfo.ApertureFNumber;
GetStringFromArchive(ar, m_exifInfo.CameraMake, sizeof(m_exifInfo.CameraMake));
GetStringFromArchive(ar, m_exifInfo.CameraModel, sizeof(m_exifInfo.CameraModel));
@@ -267,7 +271,7 @@ void CPictureInfoTag::GetStringFromArchive(CArchive &ar, char *string, size_t le
const CStdString CPictureInfoTag::GetInfo(int info) const
{
- if (!m_isLoaded)
+ if (!m_isLoaded && !m_isInfoSetExternally) // If no metadata has been loaded from the picture file or set with SetInfo(), just return
return "";
CStdString value;
@@ -590,21 +594,17 @@ void CPictureInfoTag::SetInfo(int info, const CStdString& value)
{
m_exifInfo.Width = atoi(dimension[0].c_str());
m_exifInfo.Height = atoi(dimension[1].c_str());
+ m_isInfoSetExternally = true; // Set the internal state to show metadata has been set by call to SetInfo
}
break;
}
case SLIDE_EXIF_DATE_TIME:
{
strcpy(m_exifInfo.DateTime, value.c_str());
+ m_isInfoSetExternally = true; // Set the internal state to show metadata has been set by call to SetInfo
break;
}
default:
break;
}
}
-
-void CPictureInfoTag::SetLoaded(bool loaded)
-{
- m_isLoaded = loaded;
-}
-
View
4 xbmc/pictures/PictureInfoTag.h
@@ -100,12 +100,12 @@ class CPictureInfoTag : public IArchivable, public ISerializable, public ISortab
static int TranslateString(const CStdString &info);
void SetInfo(int info, const CStdString& value);
- void SetLoaded(bool loaded = true);
private:
void GetStringFromArchive(CArchive &ar, char *string, size_t length);
ExifInfo_t m_exifInfo;
IPTCInfo_t m_iptcInfo;
- bool m_isLoaded;
+ bool m_isLoaded; // Set to true if metadata has been loaded from the picture file successfully
+ bool m_isInfoSetExternally; // Set to true if metadata has been set by an external call to SetInfo
};
Something went wrong with that request. Please try again.