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

[Niklas Söderlund]

Hello Laurent,

On 2020-06-12 11:00:21 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> 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.

- 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 
  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.

- 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.

  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.

> 
> > > > > > +	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

-- 
Regards,
Niklas Söderlund