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

[Laurent Pinchart]

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.

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

> 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