(No comment was entered for this change.)

I claim this task.

Welcome aboard! What is your name?

If you have any questions, just ask.

Please test all patches thoroughly before submission.

This task has a 7 day time limit.


- Mike

Hello Mike,
Can I find you on irc if I have any questions?
-Natan

No, I'm not normally on IRC. Ask here.

- Mike

Ok. I'm working on installing the svn version of gthumb with jhbuild now. Do I also
need a svn version of exif or exiv?

You should have exiv2 0.16 to get the XMP support. You can use the 0.16 release or
svn for that.

Any version of libexif is OK.

jhbuild is good, but gthumb will normally build just fine on its own if you have a
recent distro like Fedora 8. So either way is fine.

- Mike

Here is a sample image that I like to use, because it has lots of metadata.

	s_gps.jpg 43.6 KB   View   Download

Ubuntu has exiv2 0.15, so I'll install a more recent version and then recompile gthumb.

Where is exiv2's svn located?

According to http://www.exiv2.org/download.html:

svn checkout svn://dev.robotbattle.com/exiv2/trunk .

However, it times out on me... maybe it's down.

You can start work using 0.15 for now, if you have to. But it should be checked with
0.16 before the task is completed.

- Mike

Mike,
Exactly when does file_data_load_exif_data get called?
Just to figure out exactly what's happening when, I added a printf to the bottom of
the file_data_load_exif_data function in libgthumb/file-data.c and it never seems to
be called.

When you sort your images based on the exif date.

- Mike

Thanks. I see the output now.

Is it possible to run gthumb from the source directory?

I usually run "./autogen.sh --prefix=/usr" so that the binaries will be installed in
the normal places.

You can install the compiled binaries to a different directory if you want, using
--prefix=/what/ever.

I haven't tried any other approaches.

- Mike

What should I do if there is no exif metadata for a file?

From my understanding, I should set fd->exif_data_loaded to TRUE, fd->exif_metadata
to NULL, and fd->exif_time to (time_t) 0.

Is that all correct?

Yes, that's correct.

Name it fd->metadata instead of fd->exif_metadata, however (because it may contain
exif, iptc, and xmp metadata).

I think I misunderstood your compiling question: no, gthumb can not be compiled to
the source directory and run without "make install". I don't know the details of why
- probably because some parts are loaded as libraries.

- Mike

Ok.

Do I need to load the xmp and iptc metadata also? Right now I'm using the following:
fd->exif_metadata = update_metadata (NULL, fd->path, fd->mime_type);

Should I do be doing something else also?

I'm not sure where "Xmp.exif.DateTimeOriginal" is supposed to come from.

update_metadata reads all three types of metadata (exif, iptc, xmp), so you do not
need to do anything special. Most photos will just have exif data.

If you view s_gps.jpg with an xmp sidecar in the same folder (see the attached
sidecar file), gthumb should show an Xmp.exif.DateTimeOriginal in the XMP (Sidecar)
category. Most images do not have this tag, of course.

- Mike

s_gps.xmp 5.1 KB   Download

Hmm, you said:

fd->exif_metadata = update_metadata (NULL, fd->path, fd->mime_type);

That doesn't look right, it should be 

fd->exif_metadata = update_metadata (fd->exif_metadata, fd->path, fd->mime_type);

- Mike

I added on an extra argument to update_metadata for the GList of metadata. Is that a
bad idea?

I don't understand what you mean. The first argument to update_metadata should be a
pointer to the empty GList. See how it is used in
src/gth-exif-data-viewer.c:gth_exif_data_viewer_update. update_metadata returns a
pointer to the new location of the metadata GList, after all the metadata has been added.

You shouldn't need any additional arguments.

- Mike

Sorry, ignore my last comment.

What I meant to say is that I'd like to add on a parameter to get_metadata_time so
that the function signature would look like:
time_t
get_metadata_time (const char *mime_type, 
		   const char *uri,
		   const GList *md);
Is that acceptable, or is that function also called externally?

About your original comment, isn't fd->exif_metadata undefined when the function is
first called?

I wont be able to wrok on this much more today. I can work for another few minutes
now, and then I'll try to finish this tomorrow.

1) Hmm, get_metadata_time is used in a couple of other places - let's leave it
unchanged, and add a new function:

time_t read_metadata_time (FileData *file)

that just reads the time from the metadata GList. I will purge get_metadata_time later.

2) You will need to add "GList *metadata = NULL" in file_data_new to initialize the
GList. fd->metadata will be NULL when gth_exif_data_viewer_update calls
read_metadata_time - so your original call would have been OK (that is, comment 20
can be ignored.) However, I think it's better coding style to use fd->metadata rather
than hard-coding NULL.

- Mike

I changed get_metadata_time in all of the places it appears in libgthumb and gthumb
before I saw your comment. Shall I change it back and add a new function like you
recommended, or should I just leave it as is?

If it works, you can leave it as is. :-)

- Mike

Ok. :)

I'm still not sure how I can tell whether a given piece of metadata is Exif of Xmp.
The GthMetadata struct contains a GthMetadata category, but I don't see an enum for
anything like Xmp.photoshop.

Thanks in advance,
Natan

Even with s_gps.xmp in the same directory, the xmp information doesn't seem to show
up in the metadata GList. Any ideas?

I'm still using exiv 2 0.15. Do you think that could be the cause?

Do you have exiv2 0.16 or 0.15 installed? You need 0.16 for xmp support. Run "exiv2
-V" to be sure.

The "display_name" for the date tags will actually be stored as:

DateTimeOriginal
exif.DateTimeOriginal
DateTimeDigitized
exif.DateTimeDigitized
DateTime
exif.DateTime
photoshop.DateCreated

which might be causing some confusion. That is, the metadata reader strips off part
of the full tag name depending on the metadata type.

You do not actually need to know whether the type is exif or xmp. You just need to
match the above display names.

Make sense?

- Mike

That makes sense. I was actually going to do something like the following:

const char ** date_tags_names = {
	"DateTimeOriginal",
	"DateTimeOriginal",
	"DateTimeDigitized",
	"DateTimeDigitized"
	"DateTime",
	"DateTime",
	"DateCreated"
	};

gint
find_date_metadata (gconstpointer a,
			gconstpointer b)
{
	if (a->display_name == b->display_name && a->category == b->category) return 0;
	return 1;
}

time_t
get_metadata_time (const char *mime_type, 
		   const char *uri,
		   const GList *md)
{
	...

	if (mime_type_is (mime_type, "image/jpeg"))
	{
		if (md == NULL)
			md = update_metadata (NULL, uri, mime_type);
		
		int i;
		gboolean found = FALSE; 
		
		for (i = 0; i < 7 && found == FALSE; i ++;)
		{
			GthMetadata *search_data = g_malloc(sizeof GthMetadata);
			search_data->display_name = DATE_TAG_NAMES[i];
			search_data->category = DATE_TAG_CATEGORIES[i];
			
			GthMetadata *result = g_list_find_custom (md, search_data, find_date_metadata);
			if (result != NULL) found = TRUE;
			g_free (search_data);
		}
		if (found)
		{
			// Turn result->value into a time_t
			// and return it
		}
		
		else return (time_t) 0
	}

	...

}

I should be able to get rid of DATE_TAG_CATEGORIES[] and change around the
comparisson function to only compare the tag names.
Does that make any sense to you?

Is there a much more efficient way to do this that I'm overlooking?

Oops. Ignore the first line of code. It should be this:

const char ** DATE_TAG_NAMES = {
								"DateTimeOriginal",
								"DateTimeOriginal",
								"DateTimeDigitized",
								"DateTimeDigitized"
								"DateTime",
								"DateTime",
								"DateCreated"
								};
const int DATE_TAG_CATEGORIES[] = {
								GTH_METADATA_CATEGORY_EXIF_CAMERA,
								GTH_METADATA_CATEGORY_EXIF_CAMERA,
								GTH_METADATA_CATEGORY_EXIF_CAMERA,
								GTH_METADATA_CATEGORY_EXIF_CAMERA,
								GTH_METADATA_CATEGORY_EXIF_CAMERA,
								GTH_METADATA_CATEGORY_EXIF_CAMERA,
								GTH_METADATA_CATEGORY_EXIF_CAMERA
								};

As you say, you don't need the category comparisons.

Also, "7" should not be hard-coded (use "sizeof" or some other automatic method).

I actually haven't done much GList searching, so I'm not sure what is most efficient
offhand. I don't have a better method to suggest.

- Mike

Is libexif part of libexiv2? I'm trying to remove all of Ubuntu's libexiv2 packages
before I build it myself from source.

No, it isn't.

- Mike

I'm getting this error when running jhbuild run gthumb:

gthumb: error while loading shared libraries: libexiv2.so.2: cannot open shared
object file: No such file or directory

I installed gthumb to the jhbuild install directory, and gthumb was installed to the
usual binary location. 

Any ideas? I'm thinking of heading off for bed as soon as this is done.

Err... sorry about my last comment (again). What I meant to say was that I installed
gthumb to the jhbuild install directory and exiv2 to the usual location.

I fixed it anyway. ldconfig did the trick.

I've attached a patch.

There's still one possible issue. Should get_metadata_time free the memory for the
GList *md if md was originaly passed into the function as NULL. In other words, if
get_metadata_time has to call update_metadata, should it later free the returned GList?

md.diff 8.4 KB   Download

I have one quick question. In the FileData struct, should metadata be pointer to a
glist or a pointer to a pointer to a glist? It seems to segfault if I make it the former.

A new patch that shouldn't segfault is attached.

md.diff 8.4 KB   Download

I've attached a patch. It deals with all of the issues mentioned in my previous comments.

I tried editing the function file_data_dup so that it copies the GList of metadata
when making a copy of a FileData struct. I don't think I did it correctly because
gthumb segfaults whenever it attempts to run my code. :-D
I commented out that line in the patch, but I'd appreciate it if you could look it
over and tell me what I'm doing wrong. Asides from that, I think this is pretty much
done.

Thanks,
Natan

md.diff 10.1 KB   Download

Pterjan in #gnome-hackers pointed out that the code to copy the GList wasn't working
because the function dup_metadata_entry was only changing the local pointer to the
old GthMetadata.

I've changed it around and it works now. The patch is attached and (hopefully) this
*should* be the final change.

md.diff 10.1 KB   Download

Looking good, but some fixes are needed:

1) The current patch generates a LOT of compiler warnings. Don't ignore them!

1a) Most of the warning are due to insufficient use of the "const" qualifier. The
mime_type and uri parameters for most of the affected functions should be "const". If
they are "const" in some functions but not in others, you'll get warnings like this:

dlg-change-date.c: In function ‘exif_time_available’:
dlg-change-date.c:93: warning: passing argument 1 of ‘get_metadata_time’ discards
qualifiers from pointer target type

You'll need to clean that up by adding more "const" qualifiers in *.c and *.h files.

1b) dlg-photo-importer.c: In function 'save_image':
dlg-photo-importer.c:1338: error: too few arguments to function 'get_metadata_time'

Obvious error.

2) Simplify:

    if ( strcmp (a->display_name, b) == 0) return 0;
        return 1;

to 
    return strcmp (a->display_name, b);


- Mike

Should the GList and the mime_type parameters be constants? The function may change
both of them.

The GList is definitely not const.

For mime_type, it should be all-const or all-not-const. Some calling functions might
define it as const, but I haven't checked. If they have, define the mime_type
parameter as const and make a local copy.

They may be several ways to fix it - just as long as everything is consistent...

- Mike

I just did a quick google, and I think that I was getting confused with the
difference between a constant pointer and a pointer to a constant string.

Anyway, is it possible to cast a non constant to a constant?

You can pass a non-const as an argument to a function where the parameter has been
marked as const - that just means the function promises not to modify it.

You can not do the reverse - pass a const to a function where the parameter is not
marked as const. That means the function may modify the value, but the calling code
does not want that to happen. The compiler should complain.

The casting usually occurs in the function definition or in a constant variable
definition. I don't recall seeing it elsewhere.

I might be wrong on some details...

- Mike

Ok.

I think everything should be fixed now. Tell me if I missed anything.

md.diff 11.5 KB   Download

Much better!

However, you wrapped the call to gth_read_gstreamer and made a non-const copy of the
const uri.

The best solution is to leave the call to gth_read_gstreamer as it was, but declare
the uri parameter of gth_read_gstreamer to be const. gth_read_gstreamer has no need
to modify uri.

I have corrected that, and committed the patch to trunk:
http://svn.gnome.org/viewvc/gthumb?rev=2202&view=rev

Thank you!

Marking as completed.

- Mike

Some follow-up:

+const char *DATE_TAG_NAMES[] = {
+"DateTimeOriginal",
+"exif.DateTimeOriginal",
+"DateTimeDigitized",
+"exif.DateTimeDigitized"
+"DateTime",
+"exif.DateTime",
+"photoshop.DateCreated"
+};

was missing a comma after "exif.DateTimeDigitized", which led to this incorrect usage:

int len = (sizeof (DATE_TAG_NAMES) / sizeof (DATE_TAG_NAMES[0])) + 1;

The "+ 1" made no sense - that was a hack to account for the missing comma.

Also, sizeof(arr)/sizeof(arr[0]) can be replaced by the G_N_ELEMENTS (arr) macro.

I have made the corrections, this is just feedback for educational purposes. :-)

- Mike

Thanks Mike.

I'm surprised that I missed a comma, but thanks for catching it.