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

[Jacopo Mondi]

Hi Laurent,

On Mon, Jan 18, 2021 at 11:28:42AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> 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 ?

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 ?

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