| Message ID | 20200925150743.1822226-8-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for this work. 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@ragnatech.se> > Reviewed-by: Kieran Bingham <kieran.bingham@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. Apart for that: Reviewed-by: Umang Jain <email@uajain.com> > + } > + > TIFFSetField(tif, TIFFTAG_SOFTWARE, "qcam"); > TIFFSetField(tif, TIFFTAG_ORIENTATION, ORIENTATION_TOPLEFT); >
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@ragnatech.se> > > Reviewed-by: Kieran Bingham <kieran.bingham@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@ideasonboard.com> > Apart for that: > Reviewed-by: Umang Jain <email@uajain.com> > > > + } > > + > > TIFFSetField(tif, TIFFTAG_SOFTWARE, "qcam"); > > TIFFSetField(tif, TIFFTAG_ORIENTATION, ORIENTATION_TOPLEFT); > >
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()); + } + TIFFSetField(tif, TIFFTAG_SOFTWARE, "qcam"); TIFFSetField(tif, TIFFTAG_ORIENTATION, ORIENTATION_TOPLEFT);