Closed Bug 819835 Opened 7 years ago Closed 7 years ago

[Camera] Date & time data in EXIF is not correct

Categories

(Firefox OS Graveyard :: Gaia::Camera, defect, P4)

All
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18+ fixed)

RESOLVED FIXED
B2G C4 (2jan on)
blocking-b2g -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed

People

(Reporter: kanru, Assigned: mikeh)

Details

(Whiteboard: softblock, [EU_TPE_TRIAGED])

Attachments

(1 file, 3 obsolete files)

All my photos taken by my dogfooding phone(unagi) have

 Date and Time (Origi|2002:12:08 12:00:00
 Date and Time (Digit|2002:12:08 12:00:00

in the EXIF data.
blocking-basecamp: ? → -
Priority: -- → P4
Whiteboard: softblock
dale, I think this is yours--updatePosition() needs to stuff in the position.timestamp field (in seconds from January 1, 1970) and optionally the altitude (in metres).  It looks like a simple change--if you're too busy, I can make it.
Assignee: nobody → dale
Yeh I am good with the one, cheers
Component: General → Gaia::Camera
QA Contact: jhammink
Just to update, the camera now specifies the timestamp (of the geolocation fix) in the options, the exif data needs to be seperately specified probably inside the CameraControl code, leaving it assigned to me though
dale, did you still want to tackle this one?  If not, I can look at it.
TEF? tracking-b2g18?
many users will rely on EXIF information to keep track of photo taken dates and organize photos. All photos show 2012/12/08 12:00 isn't useful
blocking-b2g: --- → tef?
tracking-b2g18: --- → ?
I agree with Joe's nomination. Photo geeks and organizations depend on proper date taken in the EXIF. Normal people not so much initially when they get their first digital camera or cameraphone; after they have taken thousands of photos they do care because they have to organize their photos!
Triage: TEF-, tracking-b2g18+, but won't block ship due to this. Really would like to take a patch though
Assignee: dale → mhabicher
blocking-b2g: tef? → -
blocking-basecamp: - → ---
Whiteboard: softblock → softblock, [EU_TPE_TRIAGED]
Ref. KEY_GPS_TIMESTAMP vs. KEY_EXIF_DATETIME.  According to CameraParameters.h, the former is "UTC in seconds since January 1, 1970"; according to Camera.java, so is the latter.  Easy enough.
daleharvey, what format would you like a 'dateTime' property in?  It turns out the documentation in Camera.java is wrong, and KEY_EXIF_DATETIME just writes a 19-character string (ostensibly of the format "YYYY:MM:DD HH:MM:SS") into the corresponding EXIF header field.

Date and Time (Origi|9221120237041090560
Date and Time (Digit|2002:12:08 12:00:00

I'm still not sure how to get the "Digitized" field filled in, but it looks like the same format.
Flags: needinfo?(dale)
FYI, it looks like the EXIF header stores the GPS_TIMESTAMP (passed as a double) in a hybrid format, where the date portion is stored as "YYYY:MM:DD", but the time portion is stored as an EXIF RATIONAL value (i.e. as separate numerators and denominators).
Currently this takes a timestamp in seconds from UTC January 1, 1970, and stores it as a local time string in the EXIF header.  If this timestamp is not specified, the GPS timestamp is used in its place.

This may change, depending on feedback from Gaia team.
Cheers for picking this up mike, Looks good, will give it a test asap (it doesnt merge against b2g18, should central be ok?) 

I dont think we should fallback to GPS timestamp, there is no reason for the app to not provide a timestamp and seems like it would hide possible bugs if the timestamp for autopopulated with seperate data.
Flags: needinfo?(dale)
sotaro, can you look this over, with particular attention to the proper use of localtime_r() and strftime()?
Attachment #703570 - Attachment is obsolete: true
Attachment #703932 - Flags: review?(sotaro.ikeda.g)
Removing errno/strerror calls from strftime() handler; according to [1], they aren't set.

1. http://pubs.opengroup.org/onlinepubs/009695399/functions/strftime.html
Attachment #703932 - Attachment is obsolete: true
Attachment #703932 - Flags: review?(sotaro.ikeda.g)
Attachment #703934 - Flags: review?(sotaro.ikeda.g)
(In reply to Mike Habicher [:mikeh] from comment #9)
> daleharvey, what format would you like a 'dateTime' property in?  It turns
> out the documentation in Camera.java is wrong, and KEY_EXIF_DATETIME just
> writes a 19-character string (ostensibly of the format "YYYY:MM:DD
> HH:MM:SS") into the corresponding EXIF header field.
> 

It's strange, normal android source code do not have such function.
It might be qcom dependent function and parameter.
(In reply to Sotaro Ikeda [:sotaro] from comment #15)
> (In reply to Mike Habicher [:mikeh] from comment #9)
> It's strange, normal android source code do not have such function.
> It might be qcom dependent function and parameter.

I understand that KEY_EXIF_DATETIME is added by qcom. qcom's API is inconsistent to KEY_GPS_TIMESTAMP. Then changed the parameter consistent to gps timestamp. Other non-qcom hw might not support KEY_EXIF_DATETIME.
Comment on attachment 703934 [details] [diff] [review]
add support for specifying the EXIF DateTime field (v3)

No problem about how to use of localtime_r() and strftime().
Attachment #703934 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 703934 [details] [diff] [review]
add support for specifying the EXIF DateTime field (v3)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): with setting dateTime, the EXIF header in the image always shows "2002:12:08 12:00:00", even if the GPS timestamp is set.
User impact if declined: users may not be able to sort images based on EXIF data
Testing completed: set .dateTime option when taking photo to X seconds from epoch UTC, verified that time string that appears in the EXIF header corresponds to equivalent local time (based on the current timezone settings of the DUT).
Risk to taking this patch (and alternatives if risky): very low
String or UUID changes made by this patch: none
Attachment #703934 - Flags: approval-mozilla-b2g18?
Also, for Testing completed: I also verified that the code handles 0 and >32-bit values.  The former case generates a time string around epoch (1969, actually, due to time zone differences); the latter overflows the bionic representation of time_t, which triggers an error in the logcat with no setting applied to the EXIF data.
This patch breaks B2G Arm opt, B2G Arm debug, and B2G Panda opt, because the 'KEY_EXIF_DATETIME' symbol doesn't exist on those platforms.

Nevermind.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/19b3551b0058

A fix does not exist for this problem that can be cleanly applied to unagi and !unagi.
Changed KEY_EXIF_DATETIME to raw "exif-datetime" string to not break !unagi builds.

[Approval Request Comment]
See comment 18.
Attachment #703934 - Attachment is obsolete: true
Attachment #703934 - Flags: approval-mozilla-b2g18?
Attachment #704089 - Flags: review+
Attachment #704089 - Flags: approval-mozilla-b2g18?
Created bug 832494 to track eventual removal of the raw string.
Comment on attachment 704089 [details] [diff] [review]
add support for specifying the EXIF DateTime field (v4), r=sotaro

Mike told me that he thinks taking this patch is worth it and not risky.  He's "tested the change with a variety of inputs, and it's solid ... this change doesn't fix any crashes, so whether or not it makes it to tef+ is less critical."
Comment on attachment 704089 [details] [diff] [review]
add support for specifying the EXIF DateTime field (v4), r=sotaro

low risk, high win on user experience so we'll take this on branches, please land to tip of mozilla-b2g18 before 1/25 as per https://wiki.mozilla.org/Release_Management/B2G_Landing
Attachment #704089 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b32e9647d6b5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Over to Gaia for the field-stuffing.
Target Milestone: --- → B2G C4 (2jan on)
You need to log in before you can comment on or make changes to this bug.