| Message ID | 20210121101549.134574-8-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, On Thu, Jan 21, 2021 at 07:15:47PM +0900, Paul Elder wrote: > Use the CameraMetadata wrapper to access android request settings > instead of accessing camera_metadata_t directly. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > New in v2 > --- > src/android/camera_device.cpp | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 16d5b472..49af221b 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1639,12 +1639,10 @@ int CameraDevice::processControls(camera3_capture_request_t *camera3Request, > * \todo As soon as more controls are handled, this part should be > * broke out to a dedicated function. > */ > - const camera_metadata_t *camera3Settings = camera3Request->settings; > + CameraMetadata camera3Settings(camera3Request->settings); I think it is very unfortunate that we clone a full metadata pack just to inspect it. In [8/9] you clone the metadata at processRequest beginning, and you should be able to re-use it here. The settings shall actually be cloned in the Camera3RequestDescriptor constructor, as it can be made to accept the full camera3_capture_request_t. I have an implementation of that which I was waiting to rebase on your CameraMetadata change. What do you think if I break out your [2/9] and handle the conversion here ? It should be rather quick and if we fast-track it you can remove it from your series (I keep thinking you should have broke out the Camera3RequestDescriptor change in 6/9 from the handling of JPEG related metadata) Thanks j > camera_metadata_ro_entry_t entry; > - int ret = find_camera_metadata_ro_entry(camera3Settings, > - ANDROID_SCALER_CROP_REGION, > - &entry); > - if (!ret) { > + int ret = camera3Settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry); > + if (ret) { > const int32_t *data = entry.data.i32; > Rectangle cropRegion = { data[0], data[1], > static_cast<unsigned int>(data[2]), > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Thu, Jan 21, 2021 at 12:36:08PM +0100, Jacopo Mondi wrote: > Hi Paul, > > On Thu, Jan 21, 2021 at 07:15:47PM +0900, Paul Elder wrote: > > Use the CameraMetadata wrapper to access android request settings > > instead of accessing camera_metadata_t directly. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > New in v2 > > --- > > src/android/camera_device.cpp | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 16d5b472..49af221b 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -1639,12 +1639,10 @@ int CameraDevice::processControls(camera3_capture_request_t *camera3Request, > > * \todo As soon as more controls are handled, this part should be > > * broke out to a dedicated function. > > */ > > - const camera_metadata_t *camera3Settings = camera3Request->settings; > > + CameraMetadata camera3Settings(camera3Request->settings); > > I think it is very unfortunate that we clone a full metadata pack just > to inspect it. In [8/9] you clone the metadata at processRequest > beginning, and you should be able to re-use it here. > > The settings shall actually be cloned in the Camera3RequestDescriptor > constructor, as it can be made to accept the full > camera3_capture_request_t. > > I have an implementation of that which I was waiting to rebase on your > CameraMetadata change. What do you think if I break out your [2/9] and > handle the conversion here ? It should be rather quick and if we > fast-track it you can remove it from your series (I keep thinking you > should have broke out the Camera3RequestDescriptor change in 6/9 from Sorry, I mean [8/9] not 6/9 > the handling of JPEG related metadata) > > Thanks > j > > > camera_metadata_ro_entry_t entry; > > - int ret = find_camera_metadata_ro_entry(camera3Settings, > > - ANDROID_SCALER_CROP_REGION, > > - &entry); > > - if (!ret) { > > + int ret = camera3Settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry); > > + if (ret) { > > const int32_t *data = entry.data.i32; > > Rectangle cropRegion = { data[0], data[1], > > static_cast<unsigned int>(data[2]), > > -- > > 2.27.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo and Paul, On Thu, Jan 21, 2021 at 12:36:08PM +0100, Jacopo Mondi wrote: > On Thu, Jan 21, 2021 at 07:15:47PM +0900, Paul Elder wrote: > > Use the CameraMetadata wrapper to access android request settings > > instead of accessing camera_metadata_t directly. This describes what the patch does, but should also tell why. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > New in v2 > > --- > > src/android/camera_device.cpp | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 16d5b472..49af221b 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -1639,12 +1639,10 @@ int CameraDevice::processControls(camera3_capture_request_t *camera3Request, > > * \todo As soon as more controls are handled, this part should be > > * broke out to a dedicated function. > > */ > > - const camera_metadata_t *camera3Settings = camera3Request->settings; > > + CameraMetadata camera3Settings(camera3Request->settings); > > I think it is very unfortunate that we clone a full metadata pack just > to inspect it. In [8/9] you clone the metadata at processRequest > beginning, and you should be able to re-use it here. > > The settings shall actually be cloned in the Camera3RequestDescriptor > constructor, as it can be made to accept the full > camera3_capture_request_t. I like that, it will simplify the constructor of Camera3RequestDescriptor to call it with just Camera3RequestDescriptor(camera_.get(), camera3Request) > I have an implementation of that which I was waiting to rebase on your > CameraMetadata change. What do you think if I break out your [2/9] and > handle the conversion here ? It should be rather quick and if we > fast-track it you can remove it from your series (I keep thinking you > should have broke out the Camera3RequestDescriptor change in 6/9 from > the handling of JPEG related metadata) Sounds good to me ! Thanks Jacopo for helping. > > camera_metadata_ro_entry_t entry; > > - int ret = find_camera_metadata_ro_entry(camera3Settings, > > - ANDROID_SCALER_CROP_REGION, > > - &entry); > > - if (!ret) { > > + int ret = camera3Settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry); > > + if (ret) { > > const int32_t *data = entry.data.i32; > > Rectangle cropRegion = { data[0], data[1], > > static_cast<unsigned int>(data[2]),
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 16d5b472..49af221b 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1639,12 +1639,10 @@ int CameraDevice::processControls(camera3_capture_request_t *camera3Request, * \todo As soon as more controls are handled, this part should be * broke out to a dedicated function. */ - const camera_metadata_t *camera3Settings = camera3Request->settings; + CameraMetadata camera3Settings(camera3Request->settings); camera_metadata_ro_entry_t entry; - int ret = find_camera_metadata_ro_entry(camera3Settings, - ANDROID_SCALER_CROP_REGION, - &entry); - if (!ret) { + int ret = camera3Settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry); + if (ret) { const int32_t *data = entry.data.i32; Rectangle cropRegion = { data[0], data[1], static_cast<unsigned int>(data[2]),
Use the CameraMetadata wrapper to access android request settings instead of accessing camera_metadata_t directly. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- New in v2 --- src/android/camera_device.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)