[libcamera-devel] [PATCH] qcam: dng_writer: Record creation time in the EXIF directory

[Laurent Pinchart]

Hi Niklas,

On Fri, Jun 12, 2020 at 12:35:15PM +0200, Niklas Söderlund wrote:
> On 2020-06-12 11:00:21 +0300, Laurent Pinchart wrote:
> > On Thu, Jun 11, 2020 at 10:16:05PM +0200, Niklas Söderlund wrote:
> >> On 2020-06-11 21:46:57 +0300, Laurent Pinchart wrote:
> >>> On Thu, Jun 11, 2020 at 08:37:25PM +0200, Niklas Söderlund wrote:
> >>>> On 2020-06-11 05:47:52 +0300, Laurent Pinchart wrote:
> >>>>> On Thu, Jun 11, 2020 at 03:46:14AM +0200, Niklas Söderlund wrote:
> >>>>>> If the EXIF directory is empty due to no metadata being available tools
> >>>>>> such as tiffinfo complains that the directory is malformed.
> >>>>>> 
> >>>>>>     TIFFFetchDirectory: Sanity check on directory count failed, this is probably not a valid IFD offset.
> >>>>>>     TIFFReadCustomDirectory: Failed to read custom directory at offset 0.
> >>>>>> 
> >>>>>> Always record the creation time in the EXIF directory instead of adding
> >>>>>> complexity to skip creating the EXIF directory if there is no metadata
> >>>>>> to record. This ensures there are at least one entry in the EXIF
> >>>>>> directory and that makes tiffinfo happy.
> >>>>>> 
> >>>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >>>>>> ---
> >>>>>>  src/qcam/dng_writer.cpp | 11 +++++++++++
> >>>>>>  1 file changed, 11 insertions(+)
> >>>>>> 
> >>>>>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> >>>>>> index 330b169af25b7ac7..c1c3833740d96630 100644
> >>>>>> --- a/src/qcam/dng_writer.cpp
> >>>>>> +++ b/src/qcam/dng_writer.cpp
> >>>>>> @@ -426,6 +426,17 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> >>>>>>  	/* Create a new IFD for the EXIF data and fill it. */
> >>>>>>  	TIFFCreateEXIFDirectory(tif);
> >>>>>>  
> >>>>>> +	/* Store creation time. */
> >>>>>> +	time_t rawtime;
> >>>>>> +	struct tm *timeinfo;
> >>>>>> +	char strTime[20];
> >>>>>> +
> >>>>>> +	time(&rawtime);
> >>>>> 
> >>>>> Could we use the timestamp from the buffer instead ?
> >>>> 
> >>>> We can't as the timestamp in the buffer is copied from the v4l2 buffer 
> >>>> and not guaranteed to be an offset from the epoch.
> >>> 
> >>> So how do we fix that ? :-) It's not a prerequisite for this patch, but
> >>> it seems to me that the problem needs to be addressed. We should also
> >>> add a \todo here to mention that the timestamp should come from the
> >>> buffer.
> >> 
> >> I'm not sure we should attempt to fix it in user-space. I think it's 
> >> more value for applications to have as accurate timestamps as possible 
> >> to compare between captured buffers then it's to be able to record in a 
> >> file on disk the exact nano second compared to the epoch it was 
> >> captured, unless we manage to get libcamera running on ATLAS at CERN...  
> >> :-)
> >> 
> >> So if we are to fix this I think we need to solve it in the kernel 
> >> interface so that all buffer timestamps are indeed an epoch number.
> > 
> > I agree with the first part of your statement, the kernel should give
> > userspace high-precision timestamps, not a calendar date. I'm however
> > puzzled by why you think we couldn't rely on those timestamps here, and
> > convert them to the time representation we need. This involves figuring
> > out the delta between the two, and maybe in the end we will lose so much
> > precision there that using the high-precision timestamp wouldn't be
> > worth it in the first place.
> 
> I'm sorry if I was unclear. What I tried to say was that
> 
> - The timestamps from V4L2 may not be in relation to any calendar date 
>   and can only be treated as a counter where the only meaning one can 
>   extract from it is comparing the stamp from two buffers from the same 
>   source to figure out the delta time.

They're not related to a calendar date, but they're related to one of
the system clocks supported by clock_gettime(). The timestamp isn't
meant only to compare buffers from the same source, it's very important
to be able to synchronize it with timestamps from other sources (motion
sensors come into mind, there's also audio and display that need to be
taken into account).

> - I think it's a bad idea for libcamera to insert a calendar timestamp 
>   into the FrameBuffer object once it get holds of the V4L2 buffer. I 

That I don't dispute, I was proposing doing so in qcam, not in the
libcamera core.

>   think we should do it the way it's done today, copy the timestamp from 
>   the V4L2 buffer. I think it's more value for applications to be able 
>   to calculate a high precision delta time between two buffers then to 
>   have a calender timestamp with high jitter as we would have if we 
>   create it in user-space.

Userspace doesn't imply a large jitter. The calendar time should be
derived from the high precision timestamp provided in the buffer.

> - If we want to use a timestamp to produce a calender time here and we 
>   want the source of the timestamp to come from the FrameBuffer it's my 
>   opinion we need to fix the kernel API so that the high precision 
>   timestamp created by the kernel for the V4L2 buffer is in addition to 
>   a high precision also guaranteed to be relatable to calendar time.

I don't see why that should be a job for V4L2. We are provided with all
the information we need, as far as I can tell.

>   If we can't do that I think producing a calendar time in the DNG 
>   writer is just as accurate as doing so in the libcamera core and 
>   storing it in an additional field in FrameBuffer.

I wasn't proposing doing it in the libcamera core :-) And I agree it may
not be worth it anyway, given the precision we store here (1s).

> >>>>>> +	timeinfo = localtime(&rawtime);
> >>>>>> +	strftime(strTime, 20, "%Y:%m:%d %H:%M:%S", timeinfo);
> >>>>> 
> >>>>> How are time zones supposed to be handled ?
> >>>> 
> >>>> The documentation states noting about timezones so I think the best 
> >>>> option is the use the local timezone of the device running qcam.
> >>> 
> >>> How about the OffsetTime, OffsetTimeOriginal and OffsetTimeDigitized
> >>> tags ?
> >> 
> >> I looked at those, but they are not present in my libtiff (4.1.0), there 
> >> are no #defines for the tags. If I use the numeric value from the EXIF 
> >> documentation I endup with warnings creating the DNG file.
> >> 
> >>     TIFFSetField: /home/chrx/test.dng: Unknown tag 36880.
> >>     TIFFSetField: /home/chrx/test.dng: Unknown tag 36881.
> >>     TIFFSetField: /home/chrx/test.dng: Unknown tag 36882.
> >> 
> >> I'm not sure if they are recorded to the output DNG file since the 
> >> warnings are printed, but nothing shows up when I run tiffinfo on the  
> >> dng file that still gets created.
> > 
> > libtiff has an API to write custom tags, but it's awkward to use. It
> > involves creating a custom directory with TIFFCreateCustomDirectory(),
> > and passing a table of tag metadata to that function. As we use
> > TIFFCreateEXIFDirectory() here, we would have to reimplement a large
> > part of that, which isn't practical. The fact that
> > TIFFCreateCustomDirectory() takes a TIFFFieldArray pointer, with
> > 
> > typedef struct _TIFFFieldArray TIFFFieldArray;
> > 
> > and the structure _TIFFFieldArray not being defined in public headers
> > makes me think this isn't a widely used API...
> > 
> > Support for the timezone offsets has been added in libtiff, I suppose it
> > will make it to the next release. Can you record a \todo here ?
> 
> I will add an \todo to add OffsetTimeOriginal and OffsetTimeDigitized 
> tags here once libtiff catches up.
> 
> >> So I can't find a good option to record the timezone information in the 
> >> DNG. So I think we have two options reccord the time in the local 
> >> timezone or GMT. My preference would be to store it in the local 
> >> timezone but I have no strong feelings, let me know what you think.
> > 
> > The local time is best I think, that seems to be in line with what most
> > cameras do.
> > 
> >>>>>> +
> >>>>>> +	TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);
> >>>>> 
> >>>>> Should we also set EXIFTAG_DATETIMEORIGINAL to the same value ?
> >>>> 
> >>>> We should will send a v2.
> >>>> 
> >>>>>> +
> >>>>>>  	if (metadata.contains(controls::AnalogueGain)) {
> >>>>>>  		float gain = metadata.get(controls::AnalogueGain);
> >>>>>>  		uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);

-- 
Regards,

Laurent Pinchart