[libcamera-devel] [PATCH 5/6] android: camera_device: Load make and model from platform settings

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jan 18, 2021 at 10:43:23AM +0100, Jacopo Mondi wrote:
> On Mon, Jan 18, 2021 at 11:28:42AM +0200, Laurent Pinchart wrote:
> > On Mon, Jan 18, 2021 at 10:21:23AM +0100, Jacopo Mondi wrote:
> > > On Fri, Jan 15, 2021 at 08:55:38PM +0200, Laurent Pinchart wrote:
> > > > On Fri, Jan 15, 2021 at 03:24:52PM +0100, Jacopo Mondi wrote:
> > > > > On Thu, Jan 14, 2021 at 07:40:34PM +0900, Paul Elder wrote:
> > > > > > In ChromeOS the camera make and model is saved in
> > > > > > /var/cache/camera/camera.prop. Load and save these values at
> > > > > > construction time, if available.
> > > > > >
> > > > > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > > > > ---
> > > > > >  src/android/camera_device.cpp | 30 ++++++++++++++++++++++++++++++
> > > > > >  src/android/camera_device.h   |  5 +++++
> > > > > >  2 files changed, 35 insertions(+)
> > > > > >
> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > > index d2a8e876..ed47c7cd 100644
> > > > > > --- a/src/android/camera_device.cpp
> > > > > > +++ b/src/android/camera_device.cpp
> > > > > > @@ -18,6 +18,7 @@
> > > > > >  #include <libcamera/formats.h>
> > > > > >  #include <libcamera/property_ids.h>
> > > > > >
> > > > > > +#include "libcamera/internal/file.h"
> > > > > >  #include "libcamera/internal/formats.h"
> > > > > >  #include "libcamera/internal/log.h"
> > > > > >  #include "libcamera/internal/utils.h"
> > > > > > @@ -344,6 +345,35 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer
> > > > > >  	 *  streamConfiguration.
> > > > > >  	 */
> > > > > >  	maxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */
> > > > > > +
> > > > > > +	cameraMake_ = "libcamera";
> > > > > > +	cameraModel_ = "cameraModel";
> > > > >
> > > > > You know, library-wide support for configuration files should land
> > > > > somewhen soon. I would rather hardcode the above values with a \todo
> > > > > entry.
> > > > >
> > > > > Or does this make any test un-happy ?
> > > >
> > > > Chrome OS expects the value to be taken from
> > > > /var/cache/camera/camera.prop, so I think it makes sense to do so on
> > > > that platform.
> > >
> > > I'm not debating this, but this patch accesses the file, while I think
> > > the library-wide configuration file helper should be used.
> >
> > But we'll use JSON for our configuration files, while this file is in a
> > format specific to Chrome OS :-S
> 
> Don't want to pester you with this discussion, but, what does it mean
> "Chrome OS expects" ? Is this a system-wide requirement ? Or is it a
> requirement of the ChromeOS HAL implementations ?

/var/cache/camera/camera.prop is a file created by Chrome OS, not by us.

> In either cases, can't we let ChromeOS components that has this
> requirement (outside of the HAL) maintain their dependency on said
> file, while our HAL can encode information as it likes long as they're
> consistent between the two files ?

Won't storing the same information in two places cause more issues than
it will solve ? If Chrome OS provides an official means, through this
file, to access the camera maker and model string, I think this can be
treated as platform integration and be supported with platform-specific
code. For Android we'll have to find a different method, which could be
a libcamera-specific configuration file if Android doesn't provide any
API to get the same information, but I would be surprised if this was
the case.

> > > > For Android we need something else, possibly a custom configuration
> > > > file, or an Android-specific file or API. I'd record this in a todo.
> > > >
> > > > > > +
> > > > > > +	File prop("/var/cache/camera/camera.prop");
> > > > > > +	if (!prop.open(File::ReadOnly))
> > > > > > +		return;
> > > > > > +
> > > > > > +	size_t fileSize = prop.size();
> > > > > > +	if (fileSize <= 0)
> > > > > > +		return;
> > > > > > +
> > > > > > +	std::vector<uint8_t> buf(fileSize);
> > > > > > +	if (prop.read(buf) < 0)
> > > > > > +		return;
> > > > > > +
> > > > > > +	std::string fileContents(buf.begin(), buf.end());
> > > > > > +	for (const auto &line : utils::split(fileContents, "\n")) {
> > > >
> > > > This is a bit inefficient, as utils::split() makes a copy of
> > > > fileContents, which itself makes a copy of buf. The configuration file
> > > > isn't expected to be large so it may be OK, even if it's not great.
> > > > Another option would be to use std::ifstream() and std::getline(), which
> > > > should be fairly simple.
> > > >
> > > > > > +		off_t delimPos = line.find("=");
> > > > > > +		if (delimPos == std::string::npos)
> > > > > > +			continue;
> > > > > > +		std::string key = line.substr(0, delimPos);
> > > > > > +		std::string val = line.substr(delimPos + 1, line.size());
> > > >
> > > > You can drop the second argument to substr().
> > > >
> > > > > > +
> > > > > > +		if (!key.compare("ro.product.model"))
> > > > > > +			cameraModel_ = val;
> > > > > > +		if (!key.compare("ro.product.manufacturer"))
> > > > > > +			cameraMake_ = val;
> > > > > > +	}
> > > >
> > > > Should we move all this to a separate private function to avoid
> > > > cluttering the constructor ?
> > > >
> > > > > >  }
> > > > > >
> > > > > >  CameraDevice::~CameraDevice()
> > > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > > > > index 0874c80f..a285d0a1 100644
> > > > > > --- a/src/android/camera_device.h
> > > > > > +++ b/src/android/camera_device.h
> > > > > > @@ -56,6 +56,8 @@ public:
> > > > > >  		return config_.get();
> > > > > >  	}
> > > > > >
> > > > > > +	const std::string &cameraMake() const { return cameraMake_; }
> > > > > > +	const std::string &cameraModel() const { return cameraModel_; }
> > > > > >  	int facing() const { return facing_; }
> > > > > >  	int orientation() const { return orientation_; }
> > > > > >  	unsigned int maxJpegBufferSize() const { return maxJpegBufferSize_; }
> > > > > > @@ -127,6 +129,9 @@ private:
> > > > > >  	std::map<int, libcamera::PixelFormat> formatsMap_;
> > > > > >  	std::vector<CameraStream> streams_;
> > > > > >
> > > > > > +	std::string cameraMake_;
> > > > > > +	std::string cameraModel_;
> > > > > > +
> > > > > >  	int facing_;
> > > > > >  	int orientation_;
> > > > > >

-- 
Regards,

Laurent Pinchart