| Message ID | 20191204132106.21582-10-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Wed, Dec 04, 2019 at 02:21:05PM +0100, Jacopo Mondi wrote: > Construct two example static metadata to be reported to the Android > framework using the properties reported by the Camera. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 29 +++++++++++++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 065e0292e186..674867d313ac 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -7,6 +7,9 @@ > > #include "camera_device.h" > > +#include <libcamera/controls.h> > +#include <libcamera/property_ids.h> > + > #include "log.h" > #include "utils.h" > > @@ -97,6 +100,8 @@ camera_metadata_t *CameraDevice::getStaticMetadata() > if (staticMetadata_) > return staticMetadata_->get(); > > + const ControlList &properties = camera_->properties(); > + > /* > * The here reported metadata are enough to implement a basic capture > * example application, but a real camera implementation will require > @@ -261,9 +266,15 @@ camera_metadata_t *CameraDevice::getStaticMetadata() > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE, > &exposureTimeRange, 2); > > + /* > + * Android reports orientation as clockwise correction, while Linux > + * uses counter-clockwise. What Linux does isn't relevant I think, what matters here is what libcamera does. Android defines the rotation as "Clockwise angle through which the output image needs to be rotated to be upright on the device screen in its native orientation." I have to say I have a hard time parsing the description of the libcamera rotation property: "Camera mounting rotation expressed as counterclockwise rotation degrees towards the axis perpendicular to the sensor surface and directed away from it." I can't figure out what this means :-S Is it just me ? > + */ > int32_t orientation = 0; > - staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, > - &orientation, 1); > + if (properties.contains(properties::Rotation)) > + orientation = (360 - properties.get(properties::Rotation)) This could be made slightly more efficient if you used .find() to avoid the double lookup. Same below. > + % 360; > + staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1); > > std::vector<int32_t> testPatterModes = { > ANDROID_SENSOR_TEST_PATTERN_MODE_OFF, > @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata() > lensApertures.size()); > > uint8_t lensFacing = ANDROID_LENS_FACING_FRONT; > + if (properties.contains(properties::Location)) { > + int32_t location = properties.get(properties::Location); > + switch (location) { > + case CAMERA_LOCATION_FRONT: > + lensFacing = ANDROID_LENS_FACING_FRONT; > + break; > + case CAMERA_LOCATION_BACK: > + lensFacing = ANDROID_LENS_FACING_BACK; > + break; > + case CAMERA_LOCATION_EXTERNAL: > + lensFacing = ANDROID_LENS_FACING_EXTERNAL; > + break; > + } > + } > staticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1); > > std::vector<float> lensFocalLenghts = {
Hi Laurent, On Wed, Dec 04, 2019 at 06:32:35PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Dec 04, 2019 at 02:21:05PM +0100, Jacopo Mondi wrote: > > Construct two example static metadata to be reported to the Android > > framework using the properties reported by the Camera. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 29 +++++++++++++++++++++++++++-- > > 1 file changed, 27 insertions(+), 2 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 065e0292e186..674867d313ac 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -7,6 +7,9 @@ > > > > #include "camera_device.h" > > > > +#include <libcamera/controls.h> > > +#include <libcamera/property_ids.h> > > + > > #include "log.h" > > #include "utils.h" > > > > @@ -97,6 +100,8 @@ camera_metadata_t *CameraDevice::getStaticMetadata() > > if (staticMetadata_) > > return staticMetadata_->get(); > > > > + const ControlList &properties = camera_->properties(); > > + > > /* > > * The here reported metadata are enough to implement a basic capture > > * example application, but a real camera implementation will require > > @@ -261,9 +266,15 @@ camera_metadata_t *CameraDevice::getStaticMetadata() > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE, > > &exposureTimeRange, 2); > > > > + /* > > + * Android reports orientation as clockwise correction, while Linux > > + * uses counter-clockwise. > > What Linux does isn't relevant I think, what matters here is what > libcamera does. Libcamera does what Linux does, might that be because the same person did both ends ? Do we want to introduce a different handling of rotation in libcamera ? > > Android defines the rotation as > > "Clockwise angle through which the output image needs to be rotated to > be upright on the device screen in its native orientation." > > I have to say I have a hard time parsing the description of the > libcamera rotation property: > > "Camera mounting rotation expressed as counterclockwise rotation degrees > towards the axis perpendicular to the sensor surface and directed away > from it." That's the mounting rotation description, almost copied from the dt-bindings property description. Might it be better to make the libcamera rotation about the correction and not the mounting rotation ? In that case the below [(360 - rotation) % 360] would be performed by the CameraSensor class and not here.. > > I can't figure out what this means :-S Is it just me ? Didn't we have this currently in review in linux-media as a dt-property ? :) > > > + */ > > int32_t orientation = 0; > > - staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, > > - &orientation, 1); > > + if (properties.contains(properties::Rotation)) > > + orientation = (360 - properties.get(properties::Rotation)) > > This could be made slightly more efficient if you used .find() to avoid > the double lookup. Same below. Indeed, I'll fix Thanks j > > > + % 360; > > + staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1); > > > > std::vector<int32_t> testPatterModes = { > > ANDROID_SENSOR_TEST_PATTERN_MODE_OFF, > > @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata() > > lensApertures.size()); > > > > uint8_t lensFacing = ANDROID_LENS_FACING_FRONT; > > + if (properties.contains(properties::Location)) { > > + int32_t location = properties.get(properties::Location); > > + switch (location) { > > + case CAMERA_LOCATION_FRONT: > > + lensFacing = ANDROID_LENS_FACING_FRONT; > > + break; > > + case CAMERA_LOCATION_BACK: > > + lensFacing = ANDROID_LENS_FACING_BACK; > > + break; > > + case CAMERA_LOCATION_EXTERNAL: > > + lensFacing = ANDROID_LENS_FACING_EXTERNAL; > > + break; > > + } > > + } > > staticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1); > > > > std::vector<float> lensFocalLenghts = { > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Dec 05, 2019 at 10:45:50AM +0100, Jacopo Mondi wrote: > On Wed, Dec 04, 2019 at 06:32:35PM +0200, Laurent Pinchart wrote: > > On Wed, Dec 04, 2019 at 02:21:05PM +0100, Jacopo Mondi wrote: > > > Construct two example static metadata to be reported to the Android > > > framework using the properties reported by the Camera. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/android/camera_device.cpp | 29 +++++++++++++++++++++++++++-- > > > 1 file changed, 27 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 065e0292e186..674867d313ac 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -7,6 +7,9 @@ > > > > > > #include "camera_device.h" > > > > > > +#include <libcamera/controls.h> > > > +#include <libcamera/property_ids.h> > > > + > > > #include "log.h" > > > #include "utils.h" > > > > > > @@ -97,6 +100,8 @@ camera_metadata_t *CameraDevice::getStaticMetadata() > > > if (staticMetadata_) > > > return staticMetadata_->get(); > > > > > > + const ControlList &properties = camera_->properties(); > > > + > > > /* > > > * The here reported metadata are enough to implement a basic capture > > > * example application, but a real camera implementation will require > > > @@ -261,9 +266,15 @@ camera_metadata_t *CameraDevice::getStaticMetadata() > > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE, > > > &exposureTimeRange, 2); > > > > > > + /* > > > + * Android reports orientation as clockwise correction, while Linux > > > + * uses counter-clockwise. > > > > What Linux does isn't relevant I think, what matters here is what > > libcamera does. > > Libcamera does what Linux does, might that be because the same person > did both ends ? Do we want to introduce a different handling of > rotation in libcamera ? I'm not saying we should, I'm just saying that the HAL implementation, which translates from libcamera to Android, shouldn't mention Linux as such, especially given that Linux means V4L2 in this context and we may in the future support other APIs too. The fact that the libcamera rotation control uses the same semantics as V4L2 isn't good or bad in this context, but is relevant to the lower layer, not this layer. > > Android defines the rotation as > > > > "Clockwise angle through which the output image needs to be rotated to > > be upright on the device screen in its native orientation." > > > > I have to say I have a hard time parsing the description of the > > libcamera rotation property: > > > > "Camera mounting rotation expressed as counterclockwise rotation degrees > > towards the axis perpendicular to the sensor surface and directed away > > from it." > > That's the mounting rotation description, almost copied from the > dt-bindings property description. Might it be better to make the > libcamera rotation about the correction and not the mounting rotation > ? In that case the below [(360 - rotation) % 360] would be performed > by the CameraSensor class and not here.. Not necessarily, we don't need to align the libcamera and Android controls perfectly, but at the very least I need to understand what the libcamera definition of the rotation control means :-) > > I can't figure out what this means :-S Is it just me ? > > Didn't we have this currently in review in linux-media as a > dt-property ? :) I'll review the series and bring this up there. > > > + */ > > > int32_t orientation = 0; > > > - staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, > > > - &orientation, 1); > > > + if (properties.contains(properties::Rotation)) > > > + orientation = (360 - properties.get(properties::Rotation)) > > > > This could be made slightly more efficient if you used .find() to avoid > > the double lookup. Same below. > > Indeed, I'll fix > > > > + % 360; > > > + staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1); > > > > > > std::vector<int32_t> testPatterModes = { > > > ANDROID_SENSOR_TEST_PATTERN_MODE_OFF, > > > @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata() > > > lensApertures.size()); > > > > > > uint8_t lensFacing = ANDROID_LENS_FACING_FRONT; > > > + if (properties.contains(properties::Location)) { > > > + int32_t location = properties.get(properties::Location); > > > + switch (location) { > > > + case CAMERA_LOCATION_FRONT: > > > + lensFacing = ANDROID_LENS_FACING_FRONT; > > > + break; > > > + case CAMERA_LOCATION_BACK: > > > + lensFacing = ANDROID_LENS_FACING_BACK; > > > + break; > > > + case CAMERA_LOCATION_EXTERNAL: > > > + lensFacing = ANDROID_LENS_FACING_EXTERNAL; > > > + break; > > > + } > > > + } > > > staticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1); > > > > > > std::vector<float> lensFocalLenghts = {
Hi Laurent, On Wed, Dec 04, 2019 at 06:32:35PM +0200, Laurent Pinchart wrote: [snip] > > + */ > > int32_t orientation = 0; > > - staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, > > - &orientation, 1); > > + if (properties.contains(properties::Rotation)) > > + orientation = (360 - properties.get(properties::Rotation)) > > This could be made slightly more efficient if you used .find() to avoid > the double lookup. Same below. > I actually overlooked this one. We don't have a ControlList::find() operation, or at least we have one (const and non const versions) but it's rightfully private. The find() we have do not return an iterator, as one would expect from a find() on an std::map, but they return the ControlValue associated with the numerical id of a ControlId, allowing the insertion of a new item in the non-const version (that's why they're rightfully private). The only current pattern to 1) check if a control is on the control list and 2) get its value is the contains()+get() sequence implemented in thi patch, which, as you noticed, performs a double lookup. I see a few ways to avoid this: 1) - rename the current find() operations - provide public find() operations which return an iterator - Explicitly call get<T> on iterator::second (which is a ControlValue) auto it &ctrl = list.find(Control<>); if (ctrl != list.end()) int32_t value = ctrl.second.get<int32_t>(); 2) - make ControlList::get<>() return an iterator - again explictly call get<T>() on the ControlValue returned as iterator::second const auto &it = list.get(Control<>); if (it != list.end) int32_t value = it.second.get<int32_t>(); I'm not particularly thrilled by none of the two to be honest > > + % 360; > > + staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1); > > > > std::vector<int32_t> testPatterModes = { > > ANDROID_SENSOR_TEST_PATTERN_MODE_OFF, > > @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata() > > lensApertures.size()); > > > > uint8_t lensFacing = ANDROID_LENS_FACING_FRONT; > > + if (properties.contains(properties::Location)) { > > + int32_t location = properties.get(properties::Location); > > + switch (location) { > > + case CAMERA_LOCATION_FRONT: > > + lensFacing = ANDROID_LENS_FACING_FRONT; > > + break; > > + case CAMERA_LOCATION_BACK: > > + lensFacing = ANDROID_LENS_FACING_BACK; > > + break; > > + case CAMERA_LOCATION_EXTERNAL: > > + lensFacing = ANDROID_LENS_FACING_EXTERNAL; > > + break; > > + } > > + } > > staticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1); > > > > std::vector<float> lensFocalLenghts = { > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Dec 05, 2019 at 03:40:17PM +0100, Jacopo Mondi wrote: > On Wed, Dec 04, 2019 at 06:32:35PM +0200, Laurent Pinchart wrote: > > [snip] > > > > + */ > > > int32_t orientation = 0; > > > - staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, > > > - &orientation, 1); > > > + if (properties.contains(properties::Rotation)) > > > + orientation = (360 - properties.get(properties::Rotation)) > > > > This could be made slightly more efficient if you used .find() to avoid > > the double lookup. Same below. > > > > I actually overlooked this one. We don't have a ControlList::find() > operation, or at least we have one (const and non const versions) but > it's rightfully private. > > The find() we have do not return an iterator, as one would expect from > a find() on an std::map, but they return the ControlValue associated > with the numerical id of a ControlId, allowing the insertion of a new > item in the non-const version (that's why they're rightfully private). > > The only current pattern to 1) check if a control is on the control > list and 2) get its value is the contains()+get() sequence implemented > in thi patch, which, as you noticed, performs a double lookup. > > I see a few ways to avoid this: > > 1) > - rename the current find() operations > - provide public find() operations which return an iterator > - Explicitly call get<T> on iterator::second (which is a > ControlValue) > > auto it &ctrl = list.find(Control<>); > if (ctrl != list.end()) > int32_t value = ctrl.second.get<int32_t>(); > > 2) > - make ControlList::get<>() return an iterator > - again explictly call get<T>() on the ControlValue returned as > iterator::second > > const auto &it = list.get(Control<>); > if (it != list.end) > int32_t value = it.second.get<int32_t>(); > > I'm not particularly thrilled by none of the two to be honest I think we should fix this, but let's leave it as-is for now. > > > + % 360; > > > + staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1); > > > > > > std::vector<int32_t> testPatterModes = { > > > ANDROID_SENSOR_TEST_PATTERN_MODE_OFF, > > > @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata() > > > lensApertures.size()); > > > > > > uint8_t lensFacing = ANDROID_LENS_FACING_FRONT; > > > + if (properties.contains(properties::Location)) { > > > + int32_t location = properties.get(properties::Location); > > > + switch (location) { > > > + case CAMERA_LOCATION_FRONT: > > > + lensFacing = ANDROID_LENS_FACING_FRONT; > > > + break; > > > + case CAMERA_LOCATION_BACK: > > > + lensFacing = ANDROID_LENS_FACING_BACK; > > > + break; > > > + case CAMERA_LOCATION_EXTERNAL: > > > + lensFacing = ANDROID_LENS_FACING_EXTERNAL; > > > + break; > > > + } > > > + } > > > staticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1); > > > > > > std::vector<float> lensFocalLenghts = {
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 065e0292e186..674867d313ac 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -7,6 +7,9 @@ #include "camera_device.h" +#include <libcamera/controls.h> +#include <libcamera/property_ids.h> + #include "log.h" #include "utils.h" @@ -97,6 +100,8 @@ camera_metadata_t *CameraDevice::getStaticMetadata() if (staticMetadata_) return staticMetadata_->get(); + const ControlList &properties = camera_->properties(); + /* * The here reported metadata are enough to implement a basic capture * example application, but a real camera implementation will require @@ -261,9 +266,15 @@ camera_metadata_t *CameraDevice::getStaticMetadata() staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE, &exposureTimeRange, 2); + /* + * Android reports orientation as clockwise correction, while Linux + * uses counter-clockwise. + */ int32_t orientation = 0; - staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, - &orientation, 1); + if (properties.contains(properties::Rotation)) + orientation = (360 - properties.get(properties::Rotation)) + % 360; + staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1); std::vector<int32_t> testPatterModes = { ANDROID_SENSOR_TEST_PATTERN_MODE_OFF, @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata() lensApertures.size()); uint8_t lensFacing = ANDROID_LENS_FACING_FRONT; + if (properties.contains(properties::Location)) { + int32_t location = properties.get(properties::Location); + switch (location) { + case CAMERA_LOCATION_FRONT: + lensFacing = ANDROID_LENS_FACING_FRONT; + break; + case CAMERA_LOCATION_BACK: + lensFacing = ANDROID_LENS_FACING_BACK; + break; + case CAMERA_LOCATION_EXTERNAL: + lensFacing = ANDROID_LENS_FACING_EXTERNAL; + break; + } + } staticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1); std::vector<float> lensFocalLenghts = {
Construct two example static metadata to be reported to the Android framework using the properties reported by the Camera. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)