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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

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

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

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.

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