| Message ID | 20240226030812.2225757-1-hui.fang@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Fang Hui In subject: android: camera_device: Save capture settings unconditionally On Mon, Feb 26, 2024 at 11:08:12AM +0800, Fang Hui wrote: > If not, it will be deferred to the next frame. This commit message is really too short and does not explain why this is an issue. With your ack I would change it to: ------------------------------------------------------------------------------ As the Android framework sends to the camera device settings incrementally (only the ones that change are updated), the CameraDevice class in the Android camera HAL keeps a copy of the last received settings to be able to apply controls to the libcamera Camera and to populate metadata correctly. When a valid 'camera3Request->settings' is provided, it gets saved to 'lastSettings_' but 'descriptor->settings_' is not initialized until the next frame (assuming it does not contain more settings). Fix this by assigning to 'descriptor->settings_' the last saved settings unconditionally. ------------------------------------------------------------------------------ And now that I wrote the last sentence, I realized lastSettings_ is not re-initialized between streaming sessions. You will likely need: --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1108,6 +1108,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques } if (state_ == State::Stopped) { + lastSettings_ = {}; + ret = camera_->start(); if (ret) { LOG(HAL, Error) << "Failed to start camera"; Which I can again add when applying with your ack. Thanks j > > Signed-off-by: Fang Hui <hui.fang@nxp.com> > --- > src/android/camera_device.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 25cedd44..d45ed1a5 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -952,8 +952,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > */ > if (camera3Request->settings) > lastSettings_ = camera3Request->settings; > - else > - descriptor->settings_ = lastSettings_; > + > + descriptor->settings_ = lastSettings_; > > LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() > << " with " << descriptor->buffers_.size() << " streams"; > -- > 2.25.1 >
Also, as there's a bug assigned to this issue On Mon, Feb 26, 2024 at 11:08:12AM +0800, Fang Hui wrote: > If not, it will be deferred to the next frame. > Bug: https://bugs.libcamera.org/show_bug.cgi?id=210 > Signed-off-by: Fang Hui <hui.fang@nxp.com> > --- > src/android/camera_device.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 25cedd44..d45ed1a5 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -952,8 +952,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > */ > if (camera3Request->settings) > lastSettings_ = camera3Request->settings; > - else > - descriptor->settings_ = lastSettings_; > + > + descriptor->settings_ = lastSettings_; > > LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() > << " with " << descriptor->buffers_.size() << " streams"; > -- > 2.25.1 >
commit 3a5da02954c7ab76a0c939eb1f806c3d529c7ec9 (HEAD -> master)
Author: Fang Hui <hui.fang@nxp.com>
Date: Thu Jan 25 02:11:04 2024 +0800
android: camera_device: Save capture settings unconditionally
As the Android framework sends to the camera device settings
incrementally (only the ones that change are updated), the CameraDevice
class in the Android camera HAL keeps a copy of the last received
settings to be able to apply controls to the libcamera Camera and to
populate metadata correctly.
When a valid 'camera3Request->settings' is provided, it gets saved to
'lastSettings_' but 'descriptor->settings_' is not initialized until
the next frame (assuming it does not contain more settings).
Fix this by assigning to 'descriptor->settings_' the last saved
settings unconditionally.
Bug: https://bugs.libcamera.org/show_bug.cgi?id=210
Signed-off-by: Fang Hui <hui.fang@nxp.com>
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 25cedd44..d2679a97 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -952,8 +952,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
*/
if (camera3Request->settings)
lastSettings_ = camera3Request->settings;
- else
- descriptor->settings_ = lastSettings_;
+
+ descriptor->settings_ = lastSettings_;
LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
<< " with " << descriptor->buffers_.size() << " streams";
@@ -1108,6 +1108,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
}
if (state_ == State::Stopped) {
+ lastSettings_ = {};
+
ret = camera_->start();
if (ret) {
LOG(HAL, Error) << "Failed to start camera";
Hi Jacopo, Thank you for the discussion, On 26/02/24 7:51 pm, Jacopo Mondi wrote: > Hi Fang Hui > > In subject: > > android: camera_device: Save capture settings unconditionally > > On Mon, Feb 26, 2024 at 11:08:12AM +0800, Fang Hui wrote: >> If not, it will be deferred to the next frame. > This commit message is really too short and does not explain why this > is an issue. With your ack I would change it to: > > ------------------------------------------------------------------------------ > As the Android framework sends to the camera device settings > incrementally (only the ones that change are updated), the CameraDevice > class in the Android camera HAL keeps a copy of the last received > settings to be able to apply controls to the libcamera Camera and to > populate metadata correctly. > > When a valid 'camera3Request->settings' is provided, it gets saved to > 'lastSettings_' but 'descriptor->settings_' is not initialized until > the next frame (assuming it does not contain more settings). > > Fix this by assigning to 'descriptor->settings_' the last saved > settings unconditionally. > ------------------------------------------------------------------------------ > > And now that I wrote the last sentence, I realized lastSettings_ is > not re-initialized between streaming sessions. > > You will likely need: > > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1108,6 +1108,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > } > > if (state_ == State::Stopped) { > + lastSettings_ = {}; > + > ret = camera_->start(); > if (ret) { > LOG(HAL, Error) << "Failed to start camera"; > > Which I can again add when applying with your ack. With the original patch hunk below and lastSettings_= {}; and commit message fixed, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > Thanks > j > >> Signed-off-by: Fang Hui <hui.fang@nxp.com> >> --- >> src/android/camera_device.cpp | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 25cedd44..d45ed1a5 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -952,8 +952,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >> */ >> if (camera3Request->settings) >> lastSettings_ = camera3Request->settings; >> - else >> - descriptor->settings_ = lastSettings_; >> + >> + descriptor->settings_ = lastSettings_; >> >> LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() >> << " with " << descriptor->buffers_.size() << " streams"; >> -- >> 2.25.1 >>
Hello On Wed, Feb 28, 2024 at 05:30:25AM +0000, Hui Fang wrote: > commit 3a5da02954c7ab76a0c939eb1f806c3d529c7ec9 (HEAD -> master) > Author: Fang Hui <hui.fang@nxp.com> > Date: Thu Jan 25 02:11:04 2024 +0800 > > android: camera_device: Save capture settings unconditionally > > As the Android framework sends to the camera device settings > incrementally (only the ones that change are updated), the CameraDevice > class in the Android camera HAL keeps a copy of the last received > settings to be able to apply controls to the libcamera Camera and to > populate metadata correctly. > > When a valid 'camera3Request->settings' is provided, it gets saved to > 'lastSettings_' but 'descriptor->settings_' is not initialized until > the next frame (assuming it does not contain more settings). > > Fix this by assigning to 'descriptor->settings_' the last saved > settings unconditionally. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=210 > > Signed-off-by: Fang Hui <hui.fang@nxp.com> Now tested with CTS, no regressions detected Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> I'll merge it! Thanks j > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 25cedd44..d2679a97 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -952,8 +952,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > */ > if (camera3Request->settings) > lastSettings_ = camera3Request->settings; > - else > - descriptor->settings_ = lastSettings_; > + > + descriptor->settings_ = lastSettings_; > > LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() > << " with " << descriptor->buffers_.size() << " streams"; > @@ -1108,6 +1108,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > } > > if (state_ == State::Stopped) { > + lastSettings_ = {}; > + > ret = camera_->start(); > if (ret) { > LOG(HAL, Error) << "Failed to start camera"; > ________________________________ > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > Sent: Monday, February 26, 2024 10:21 PM > To: Hui Fang <hui.fang@nxp.com> > Cc: libcamera-devel@lists.libcamera.org <libcamera-devel@lists.libcamera.org>; biomifang118@gmail.com <biomifang118@gmail.com> > Subject: [EXT] Re: [PATCH] android: camera_device: The first valid settings should be saved > > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button > > > Also, > as there's a bug assigned to this issue > > On Mon, Feb 26, 2024 at 11:08:12AM +0800, Fang Hui wrote: > > If not, it will be deferred to the next frame. > > > > Bug: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.libcamera.org%2Fshow_bug.cgi%3Fid%3D210&data=05%7C02%7Chui.fang%40nxp.com%7Cc9341f6fe9624af769f108dc36d64722%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638445541153242800%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=2JewcBFhgU4K%2BDofrUZLQUduptmwAwDcomPEXwQ3Ybw%3D&reserved=0<https://bugs.libcamera.org/show_bug.cgi?id=210> > > > Signed-off-by: Fang Hui <hui.fang@nxp.com> > > --- > > src/android/camera_device.cpp | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 25cedd44..d45ed1a5 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -952,8 +952,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > */ > > if (camera3Request->settings) > > lastSettings_ = camera3Request->settings; > > - else > > - descriptor->settings_ = lastSettings_; > > + > > + descriptor->settings_ = lastSettings_; > > > > LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() > > << " with " << descriptor->buffers_.size() << " streams"; > > -- > > 2.25.1 > >
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 25cedd44..d45ed1a5 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -952,8 +952,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques */ if (camera3Request->settings) lastSettings_ = camera3Request->settings; - else - descriptor->settings_ = lastSettings_; + + descriptor->settings_ = lastSettings_; LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() << " with " << descriptor->buffers_.size() << " streams";
If not, it will be deferred to the next frame. Signed-off-by: Fang Hui <hui.fang@nxp.com> --- src/android/camera_device.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)