| Message ID | 20210114104035.302968-6-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, 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@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 ? > + > + 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")) { > + 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()); > + > + if (!key.compare("ro.product.model")) > + cameraModel_ = val; > + if (!key.compare("ro.product.manufacturer")) > + cameraMake_ = val; > + } > } > > 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_; > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, Thank you for the patch. 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@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. 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_; > >
Hi Laurent, On Fri, Jan 15, 2021 at 08:55:38PM +0200, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > 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@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. > > 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
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@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 > > 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_; > > > >
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@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
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@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_; > > > > > >
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"; + + 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")) { + 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()); + + if (!key.compare("ro.product.model")) + cameraModel_ = val; + if (!key.compare("ro.product.manufacturer")) + cameraMake_ = val; + } } 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_;
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@ideasonboard.com> --- src/android/camera_device.cpp | 30 ++++++++++++++++++++++++++++++ src/android/camera_device.h | 5 +++++ 2 files changed, 35 insertions(+)