Is the EXIF data in the media file itself, or the thumbnail, or is it exposed through the API, and just not parsed?

The latter would be the only way for us to actually add that data, otherwise we'd need to download the full file before being able to add that metadata and that would be expensive.

It is exposed through the API for each photo: http://www.flickr.com/services/api/flickr.photos.getExif.html

Looks like it would need a resolve method to be added, as the call is per-picture.

Example output from the call:
https://api.flickr.com/services/rest/?method=flickr.photos.getExif&api_key=0ebbcdb22a92c15cabfad443da873428&photo_id=21804899078&format=rest&api_sig=3f55cd6e7ad6d21292be3e973b81ed4d

Created attachment 312727 [details] [review]
WIP: flickr: Populate GrlMediaImage with EXIF metadata

FIXME:
- EXIF XML parsing
- hash table -> grilo keys conversion

Created attachment 320078 [details] [review]
Populate flickr exif metadata

This is my attempt at finishing implementing support to flickr's exif metadata.

Note that it depends on the other attached patch on grilo to work.

Created attachment 320079 [details] [review]
Set boolean metadata key on grilo

This patch is needed for setting the flash-used value on the flickr plugin.

Review of attachment 320079 [details] [review]:

The commit message needs to:
- use a prefix in the subject as other commits to the same file did
- explain in the main body what those changes actually do

The subject of the commit message is also pretty unexplanatory.

A better one might be:
core: Fix warning when setting a related boolean key

<show the warning, explain how this fixed it>

Review of attachment 320078 [details] [review]:

Once you've fixed the changes below, please merge this patch into mine, and assign yourself authorship. I've done very little in this bug except get the ball rolling.

::: src/flickr/gflickr.c
@@ +210,3 @@
+    if (content != NULL) {
+      g_hash_table_insert (photo,
+                           g_strconcat ("EXIF_", (gchar *) prop, NULL),

You should do this in a separate call, it's a pretty ugly thing to do in C.

@@ +211,3 @@
+      g_hash_table_insert (photo,
+                           g_strconcat ("EXIF_", (gchar *) prop, NULL),
+                           (gchar *) content);

The content here will be freed. You might want to make sure all the items you put into the hash table are copied.

@@ +343,3 @@
+  doc = xmlReadMemory (xml_result, xmlStrlen ((xmlChar*) xml_result), NULL,
+                       NULL, XML_PARSE_RECOVER | XML_PARSE_NOBLANKS);
+  node = xmlDocGetRootElement (doc);

What if doc == NULL here?

@@ +604,3 @@
+  g_return_if_fail (G_IS_FLICKR (f));
+
+  gchar *params[2];

Declarations at the top of the function.

@@ +609,3 @@
+  params[1] = g_strdup_printf ("method=%s", FLICKR_PHOTOS_GETEXIF_METHOD);
+
+  gchar *request = create_url (f, params, 2);

Again with the declaration.

@@ +613,3 @@
+  free_params (params, 2);
+
+  GFlickrData *gfd = g_slice_new (GFlickrData);

Declaration.

::: src/flickr/grl-flickr.c
@@ +561,3 @@
+    }
+    else if (g_strcmp0 (key, "EXIF_Flash") == 0) {
+      gboolean flash = g_str_has_prefix (value, "Fired") ||

For all those possible values, please make sure to link to the documentation that defines them.

@@ +584,3 @@
+      if (g_strcmp0 (value, "Horizontal (normal)") == 0 ||
+          g_strcmp0 (value, "Mirror horizontal") == 0 ||
+          g_strcmp0 (value, "Mirro vertical") == 0)

Typo?

Comment on attachment 312727 [details] [review]
WIP: flickr: Populate GrlMediaImage with EXIF metadata

Marking as obsolete.

Created attachment 320433 [details] [review]
Fix setting related key of type boolean

New version addressing the requested fixes.

Created attachment 320434 [details] [review]
Improve error message for non-handled related key.

This patch makes it easier to know which related key is not currently being handled.

(In reply to Bastien Nocera from comment #9)
> Review of attachment 320078 [details] [review] [review]:
> 
> Once you've fixed the changes below, please merge this patch into mine, and
> assign yourself authorship. I've done very little in this bug except get the
> ball rolling.
> 
> ::: src/flickr/gflickr.c
> @@ +210,3 @@
> +    if (content != NULL) {
> +      g_hash_table_insert (photo,
> +                           g_strconcat ("EXIF_", (gchar *) prop, NULL),
> 
> You should do this in a separate call, it's a pretty ugly thing to do in C.

Here I was just repeating what had been done on line 173. But ok, I'll change it to a separate call.

(In reply to Bastien Nocera from comment #9)
> Review of attachment 320078 [details] [review] [review]:
> @@ +211,3 @@
> +      g_hash_table_insert (photo,
> +                           g_strconcat ("EXIF_", (gchar *) prop, NULL),
> +                           (gchar *) content);
> 
> The content here will be freed. You might want to make sure all the items
> you put into the hash table are copied.

None of the results from xml*Get* in this file are copied, they are directly inserted into the hash table. So isn't this problem also present in the rest of the code?

> 
> @@ +343,3 @@
> +  doc = xmlReadMemory (xml_result, xmlStrlen ((xmlChar*) xml_result), NULL,
> +                       NULL, XML_PARSE_RECOVER | XML_PARSE_NOBLANKS);
> +  node = xmlDocGetRootElement (doc);
> 
> What if doc == NULL here?

Same as above. This case is not handled anywhere in the code.

> ::: src/flickr/grl-flickr.c
> @@ +561,3 @@
> +    }
> +    else if (g_strcmp0 (key, "EXIF_Flash") == 0) {
> +      gboolean flash = g_str_has_prefix (value, "Fired") ||
> 
> For all those possible values, please make sure to link to the documentation
> that defines them.

Does [1] serve as documentation?

[1] http://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/EXIF.html

(In reply to Bastien Nocera from comment #9)
> @@ +604,3 @@
> +  g_return_if_fail (G_IS_FLICKR (f));
> +
> +  gchar *params[2];
> 
> Declarations at the top of the function.
> 
> @@ +609,3 @@
> +  params[1] = g_strdup_printf ("method=%s", FLICKR_PHOTOS_GETEXIF_METHOD);
> +
> +  gchar *request = create_url (f, params, 2);
> 
> Again with the declaration.
> 
> @@ +613,3 @@
> +  free_params (params, 2);
> +
> +  GFlickrData *gfd = g_slice_new (GFlickrData);
> 
> Declaration.

Again, I was just following the style of other parts of the code. See, e.g., g_flickr_photos_getInfo just above.

Created attachment 322658 [details] [review]
Check that doc is valid

Fix checking for valid doc in the whole file.

Created attachment 322659 [details] [review]
style changes

Make sure the coding style is consistent throughout the whole file.

Created attachment 322660 [details] [review]
Populate flickr exif metadata

New version addressing raised points.

Review of attachment 322658 [details] [review]:

Looks reasonable apart from that. In your commit subject though, focus on the user-visible changes in the patch, not the programmatic changes. Something like:
flickr: Fix possible crashes with invalid network responses

By checking whether we fail to parse the XML sent back by the server.
For example, if we were behind a captive portal, we'd try to parse
garbage as though it came from the Flickr servers.

::: src/flickr/gflickr.c
@@ +277,3 @@
                        NULL, XML_PARSE_RECOVER | XML_PARSE_NOBLANKS);
+  if (!doc) {
+    goto free_resources;

Is the callback never called if we fail to parse the XML then?

Please also check whether the other functions have the same problem.

@@ -284,2 @@
     node = node->xmlChildrenNode;
-

Please try not to include coding style changes in patches.

Review of attachment 322659 [details] [review]:

> - Don't mix declaration and code.

Good.

> - Separate g_strconcat in its own line.
> - Check return of xml*Get functions.

That's fine, but that's a functional change, please split it up.

> - Don't mix xmlChar and gchar inside hash_table

That would also be easier to read if it was a separate commit.

The code itself looks fine though.

Review of attachment 322660 [details] [review]:

Can you split the gflickr.[ch] changes from the grl-flickr.c ones?

::: src/flickr/gflickr.c
@@ +208,3 @@
+    /* Try to get the pretty value for the property */
+    xmlNodePtr child = node->xmlChildrenNode;
+    if (child == NULL) { // empty metadata field?

No C++-style comments please. The style of the comment above is fine though.

@@ +397,3 @@
+  data->hashtable_cb (data->flickr, photo, data->user_data);
+
+ free_resources:

This has the same problem as the earlier patches in that it never calls the callback on error, leaving the operation dangling.

::: src/flickr/grl-flickr.c
@@ +575,3 @@
+      gchar **operands = g_strsplit (value, "/", 2);
+      if (operands != NULL && operands[0] != NULL && operands[1] != NULL) {
+        gdouble time = g_ascii_strtod (operands[0], NULL) / g_ascii_strtod (operands[1], NULL);

You could very well crash here with a division by zero.

@@ +584,3 @@
+    else if (g_strcmp0 (key, "EXIF_ISO") == 0) {
+      relkeys = grl_related_keys_new_with_keys (GRL_METADATA_KEY_ISO_SPEED,
+                                                g_ascii_strtod (value, NULL),

You should check the value of g_ascii_strtod() here, 0 isn't a valid ISO.

Rafael, will you update your patches?

(In reply to Bastien Nocera from comment #22)
> Rafael, will you update your patches?

Give me a couple of days and I will submit the new version. I still don't have the tests though.

Created attachment 329803 [details] [review]
flickr: don't mix declaration and code

Created attachment 329804 [details] [review]
flickr: don't mix xmlChar and gchar

Created attachment 329805 [details] [review]
flickr: Check return of xml*Get functions

Created attachment 329806 [details] [review]
flickr: fix variable leaking when response != 'ok'

Created attachment 329807 [details] [review]
flickr: move g_strconcat to its own line

Created attachment 329808 [details] [review]
flickr:  Fix possible crashes with invalid xmls

Review of attachment 320433 [details] [review]:

Yep.

Review of attachment 320434 [details] [review]:

s/warning message/warning/
s/non-handled/unhandled/

Looks fine otherwise.

Review of attachment 329803 [details] [review]:

Otherwise looks fine.

::: src/flickr/gflickr.c
@@ +110,3 @@
               const gchar *oauth_token_secret)
 {
+  GFlickr *f = NULL;

No need to set it to NULL;

@@ +512,2 @@
   gchar *params[2];
+  gchar *request = NULL;

Ditto

@@ +512,3 @@
   gchar *params[2];
+  gchar *request = NULL;
+  GFlickrData *gfd = NULL;

Ditto.

@@ +543,3 @@
 {
+  gchar *params[8];
+  gchar *request = NULL;

etc.

Review of attachment 329804 [details] [review]:

Correct if a little inefficient. Marking as needs-work as I'd like to see a new version of the commit message.

> Mixing both types can be potentially bad.

Explain why though (they use different allocators is the answer).

Review of attachment 329805 [details] [review]:

Looks good.

::: src/flickr/gflickr.c
@@ +174,3 @@
   for (attr = node->properties; attr != NULL; attr = attr->next) {
     xmlChar *prop = xmlGetProp (node, attr->name);
+    if (!prop) {

no braces for one-line conditionals.

Review of attachment 329806 [details] [review]:

Yep.

Review of attachment 329807 [details] [review]:

::: src/flickr/gflickr.c
@@ +176,2 @@
   for (attr = node->properties; attr != NULL; attr = attr->next) {
+    gchar *key = NULL;

No need to initialise to NULL;

Review of attachment 329808 [details] [review]:

Looks fine otherwise.

::: src/flickr/gflickr.c
@@ +505,3 @@
 
+  data->hashtable_cb (data->flickr, token, data->user_data);
+  if (token) {

No braces for one-line conditionals.

Created attachment 329854 [details] [review]
flickr: don't mix xmlChar and gchar (v2)

Is this one better? Instead of copying the string, I changed the de-allocator in the hash table to xmlFree.

If it is, I will rebase the other patches on top of this one.

Review of attachment 329854 [details] [review]:

That's betterm yes.

Created attachment 329858 [details] [review]
flickr: don't mix declaration and code (v2)

Created attachment 329859 [details] [review]
flickr: Check return of xml*Get functions (v2)

Created attachment 329860 [details] [review]
flickr: move g_strconcat to its own line

Created attachment 329861 [details] [review]
flickr: fix possible crashes with invalid xmls (v2)

Created attachment 329963 [details] [review]
flickr: add parsing of EXIF data

Created attachment 329964 [details] [review]
grl-flickr: add EXIF tags to grilo keys

Review of attachment 329858 [details] [review]:

Looks good.

Review of attachment 329859 [details] [review]:

Looks good.

Review of attachment 329860 [details] [review]:

Looks good.

Review of attachment 329861 [details] [review]:

Yep.

Review of attachment 329963 [details] [review]:

Yes.

Review of attachment 329964 [details] [review]:

Getting closer to the end game :)

::: src/flickr/grl-flickr.c
@@ +574,3 @@
+                                                used,
+                                                NULL);
+      /* ExposureTime is in the format "%d/%d" seconds */

Put that inside the if statement, below the line "} else if ..."

@@ +584,3 @@
+        continue;
+
+      if (endptr == value || *endptr != '/')

|| *(endptr + 1) == '\0'

@@ +601,3 @@
+      errno = 0;
+      iso = g_ascii_strtod (value, NULL);
+      if (errno != ERANGE)

You need to check for "fabs(iso) == HUGE_VAL" as well.

Created attachment 330017 [details] [review]
grl-flickr: add EXIF tags to grilo keys (v2)

Review of attachment 330017 [details] [review]:

Looks good!

All those patches look fine to commit, after you've done some testing. The next step could be to add some tests for all this, as we want to avoid regressions in the future.

Rafael, did you test those patches?

I did in the sense that I checked manually if they worked, but I didn't write tests for them. I started to do so, but then got distracted with other tasks.

(In reply to Rafael Fonseca from comment #56)
> I did in the sense that I checked manually if they worked, but I didn't
> write tests for them. I started to do so, but then got distracted with other
> tasks.

That's fine. I don't think it would cause more problems. I think that the next step would be to port this to lua anyway, as per:
https://bugzilla.gnome.org/show_bug.cgi?id=712617