| Message ID | 20190817105937.29353-6-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Sat, Aug 17, 2019 at 12:59:37PM +0200, Jacopo Mondi wrote: > Retrieve the camera sensor pixel array sizes and the active pixel sizes > using the V4L2 selection APIs. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/camera_sensor.cpp | 13 +++++++++++++ > src/libcamera/include/camera_sensor.h | 2 ++ > 2 files changed, 15 insertions(+) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 2703d10c719e..e6b01c242328 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -114,6 +114,19 @@ int CameraSensor::init() > return -EINVAL; > } > > + /* Retrieve and store the sensor pixel array and active area sizes. */ Nit-picking, the active area is a rectangle, not a size unlike the pixel array. > + ret = subdev_->getCropBounds(0, &activeAreaSize_); > + if (ret) > + return ret; > + > + Rectangle rect = {}; I think you can skip the initialisation of rect. > + ret = subdev_->getNativeSize(0, &rect); > + if (ret) > + return ret; > + > + pixelArraySize_.width = rect.w; > + pixelArraySize_.height = rect.h; > + > /* Enumerate and cache media bus codes and sizes. */ > const ImageFormats formats = subdev_->formats(0); > if (formats.isEmpty()) { > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > index a237a1684605..f6b184bf2838 100644 > --- a/src/libcamera/include/camera_sensor.h > +++ b/src/libcamera/include/camera_sensor.h > @@ -58,6 +58,8 @@ private: > std::vector<Size> sizes_; > > CameraLocation location_; > + Size pixelArraySize_; > + Rectangle activeAreaSize_; Should you provide get accessors for these (with documentation :-)) ? Do we also need to expose the sensor physical size ? > }; > > } /* namespace libcamera */
Hi Laurent, On Sat, Aug 17, 2019 at 07:28:37PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Sat, Aug 17, 2019 at 12:59:37PM +0200, Jacopo Mondi wrote: > > Retrieve the camera sensor pixel array sizes and the active pixel sizes > > using the V4L2 selection APIs. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/camera_sensor.cpp | 13 +++++++++++++ > > src/libcamera/include/camera_sensor.h | 2 ++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index 2703d10c719e..e6b01c242328 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -114,6 +114,19 @@ int CameraSensor::init() > > return -EINVAL; > > } > > > > + /* Retrieve and store the sensor pixel array and active area sizes. */ > > Nit-picking, the active area is a rectangle, not a size unlike the pixel > array. > > > + ret = subdev_->getCropBounds(0, &activeAreaSize_); > > + if (ret) > > + return ret; > > + > > + Rectangle rect = {}; > > I think you can skip the initialisation of rect. > > > + ret = subdev_->getNativeSize(0, &rect); > > + if (ret) > > + return ret; > > + > > + pixelArraySize_.width = rect.w; > > + pixelArraySize_.height = rect.h; > > + > > /* Enumerate and cache media bus codes and sizes. */ > > const ImageFormats formats = subdev_->formats(0); > > if (formats.isEmpty()) { > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > index a237a1684605..f6b184bf2838 100644 > > --- a/src/libcamera/include/camera_sensor.h > > +++ b/src/libcamera/include/camera_sensor.h > > @@ -58,6 +58,8 @@ private: > > std::vector<Size> sizes_; > > > > CameraLocation location_; > > + Size pixelArraySize_; > > + Rectangle activeAreaSize_; > > Should you provide get accessors for these (with documentation :-)) ? > That, or expose them as properties, that are gathered together with the Camera reported ones in a single list of Camera properties. I would prefer going for properties directly instead of going through accessors and build the properties list from there. > Do we also need to expose the sensor physical size ? > For android HAL integration purposes, yes: https://jmondi.org/android_metadata_tags/ddocs.html#static_android.sensor.info.physicalSize as well as rotation: https://jmondi.org/android_metadata_tags/docs.html#static_android.sensor.orientation and sensitiviy range: https://jmondi.org/android_metadata_tags/docs.html#static_android.sensor.info.sensitivityRange With these we would complete support for static metadata tags marked as "BC" required to support the LEGACY hardware level. Goging forward there are much more sensor/lens related properties that will need to be retrieved, in particular to support the RAW mode. But that's for later. > > }; > > > > } /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Aug 19, 2019 at 09:45:51AM +0200, Jacopo Mondi wrote: > On Sat, Aug 17, 2019 at 07:28:37PM +0300, Laurent Pinchart wrote: > > On Sat, Aug 17, 2019 at 12:59:37PM +0200, Jacopo Mondi wrote: > > > Retrieve the camera sensor pixel array sizes and the active pixel sizes > > > using the V4L2 selection APIs. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/camera_sensor.cpp | 13 +++++++++++++ > > > src/libcamera/include/camera_sensor.h | 2 ++ > > > 2 files changed, 15 insertions(+) > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > index 2703d10c719e..e6b01c242328 100644 > > > --- a/src/libcamera/camera_sensor.cpp > > > +++ b/src/libcamera/camera_sensor.cpp > > > @@ -114,6 +114,19 @@ int CameraSensor::init() > > > return -EINVAL; > > > } > > > > > > + /* Retrieve and store the sensor pixel array and active area sizes. */ > > > > Nit-picking, the active area is a rectangle, not a size unlike the pixel > > array. > > > > > + ret = subdev_->getCropBounds(0, &activeAreaSize_); > > > + if (ret) > > > + return ret; > > > + > > > + Rectangle rect = {}; > > > > I think you can skip the initialisation of rect. > > > > > + ret = subdev_->getNativeSize(0, &rect); > > > + if (ret) > > > + return ret; > > > + > > > + pixelArraySize_.width = rect.w; > > > + pixelArraySize_.height = rect.h; > > > + > > > /* Enumerate and cache media bus codes and sizes. */ > > > const ImageFormats formats = subdev_->formats(0); > > > if (formats.isEmpty()) { > > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h > > > index a237a1684605..f6b184bf2838 100644 > > > --- a/src/libcamera/include/camera_sensor.h > > > +++ b/src/libcamera/include/camera_sensor.h > > > @@ -58,6 +58,8 @@ private: > > > std::vector<Size> sizes_; > > > > > > CameraLocation location_; > > > + Size pixelArraySize_; > > > + Rectangle activeAreaSize_; > > > > Should you provide get accessors for these (with documentation :-)) ? > > That, or expose them as properties, that are gathered together with > the Camera reported ones in a single list of Camera properties. I > would prefer going for properties directly instead of going through > accessors and build the properties list from there. I'm not entirely sure about that. I think at least the active area would be useful for pippeline handlers to know, and going through properties may not be the best API for that. Furthermore, I wonder if pipeline handlers wouldn't need to mangle data from the camera sensor before exposing them as controls. I suppose we'll figure it out as we move forward. > > Do we also need to expose the sensor physical size ? > > For android HAL integration purposes, yes: > https://jmondi.org/android_metadata_tags/ddocs.html#static_android.sensor.info.physicalSize > as well as rotation: > https://jmondi.org/android_metadata_tags/docs.html#static_android.sensor.orientation > and sensitiviy range: > https://jmondi.org/android_metadata_tags/docs.html#static_android.sensor.info.sensitivityRange > > With these we would complete support for static metadata tags marked as "BC" > required to support the LEGACY hardware level. > > Goging forward there are much more sensor/lens related properties that > will need to be retrieved, in particular to support the RAW mode. > > But that's for later. Sure :-) > > > }; > > > > > > } /* namespace libcamera */
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 2703d10c719e..e6b01c242328 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -114,6 +114,19 @@ int CameraSensor::init() return -EINVAL; } + /* Retrieve and store the sensor pixel array and active area sizes. */ + ret = subdev_->getCropBounds(0, &activeAreaSize_); + if (ret) + return ret; + + Rectangle rect = {}; + ret = subdev_->getNativeSize(0, &rect); + if (ret) + return ret; + + pixelArraySize_.width = rect.w; + pixelArraySize_.height = rect.h; + /* Enumerate and cache media bus codes and sizes. */ const ImageFormats formats = subdev_->formats(0); if (formats.isEmpty()) { diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h index a237a1684605..f6b184bf2838 100644 --- a/src/libcamera/include/camera_sensor.h +++ b/src/libcamera/include/camera_sensor.h @@ -58,6 +58,8 @@ private: std::vector<Size> sizes_; CameraLocation location_; + Size pixelArraySize_; + Rectangle activeAreaSize_; }; } /* namespace libcamera */
Retrieve the camera sensor pixel array sizes and the active pixel sizes using the V4L2 selection APIs. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/camera_sensor.cpp | 13 +++++++++++++ src/libcamera/include/camera_sensor.h | 2 ++ 2 files changed, 15 insertions(+)