[libcamera-devel] [PATCH v5 7/7] qcam: dng_writer: Record camera model

[Laurent Pinchart]

Hi Niklas and Umang,

On Mon, Sep 28, 2020 at 06:06:49PM +0530, Umang Jain wrote:
> On 9/25/20 8:37 PM, Niklas Söderlund wrote:
> > Record the model property of the camera if available.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >   src/qcam/dng_writer.cpp | 13 ++++++++++---
> >   1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> > index 030d1387118d91e9..f6d004d0407f8641 100644
> > --- a/src/qcam/dng_writer.cpp
> > +++ b/src/qcam/dng_writer.cpp
> > @@ -15,6 +15,7 @@
> >   
> >   #include <libcamera/control_ids.h>
> >   #include <libcamera/formats.h>
> > +#include <libcamera/property_ids.h>
> >   
> >   using namespace libcamera;
> >   
> > @@ -353,6 +354,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> >   		     [[maybe_unused]] const FrameBuffer *buffer,
> >   		     const void *data)
> >   {
> > +	const ControlList &cameraProperties = camera->properties();
> > +
> >   	const auto it = formatInfo.find(config.pixelFormat);
> >   	if (it == formatInfo.cend()) {
> >   		std::cerr << "Unsupported pixel format" << std::endl;
> > @@ -387,9 +390,13 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> >   	TIFFSetField(tif, TIFFTAG_DNGBACKWARDVERSION, version);
> >   	TIFFSetField(tif, TIFFTAG_FILLORDER, FILLORDER_MSB2LSB);
> >   	TIFFSetField(tif, TIFFTAG_MAKE, "libcamera");
> > -	/* \todo Report a real model string instead of id. */
> > -	TIFFSetField(tif, TIFFTAG_MODEL, camera->id().c_str());
> > -	TIFFSetField(tif, TIFFTAG_UNIQUECAMERAMODEL, camera->id().c_str());
> > +
> > +	if (cameraProperties.contains(properties::Model)) {
> > +		std::string model = cameraProperties.get(properties::Model);
> > +		TIFFSetField(tif, TIFFTAG_MODEL, model.c_str());
> > +		TIFFSetField(tif, TIFFTAG_UNIQUECAMERAMODEL, model.c_str());
>
> Looking at TIFFTAG_UNIQUECAMERAMODEL description in the spec, it states 
> that:
> 
> > UniqueCameraModel defines a unique, non-localized name for the camera 
> > model that created the image in the raw file. This name should include 
> > the manufacturer's name to avoid conflicts ...
> > E.g. mentions - "Canon EOS 300D" , "Nikon DIX" , "Sony F828" etc.
> 
> So, I think we can skip setting the UNIQUECAMERAMODEL for now if we 
> can't get the mfg.'s name (I have similiar issue for setting related 
> EXIF tag "setMake()" in src/android/cameraDevice.cpp).  OR atleast 
> introduce a \todo to follow up on the work.

I agree. Following the DNG spec exactly is difficult here, as qcam
doesn't have access to anything ressembling a product name in the first
place.

UniqueCameraModel is supposed to combine the vendor name and the product
name. as the Make tag is set to "libcamera", it would make sense to use
it here too, as the vendor name. We could also use the platform name,
possibly in the form of a pipeline handler name or something similar
(that's a candidate for future improvements, not a blocker for this
series). In any case, I would add a "libcamera" prefix to the
UniqueCameraModel, and add \todo comment. With that addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> Apart for that:
> Reviewed-by: Umang Jain <email at uajain.com>
>
> > +	}
> > +
> >   	TIFFSetField(tif, TIFFTAG_SOFTWARE, "qcam");
> >   	TIFFSetField(tif, TIFFTAG_ORIENTATION, ORIENTATION_TOPLEFT);
> >   

-- 
Regards,

Laurent Pinchart