[libcamera-devel] [PATCH v2 10/12] android: camera_device: Generate ResultMetadata earlier

[Jacopo Mondi]

Hi Kieran,

On Tue, Aug 04, 2020 at 12:10:45PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 04/08/2020 12:08, Jacopo Mondi wrote:
> > Hi Kieran
> >
> > On Mon, Aug 03, 2020 at 05:18:14PM +0100, Kieran Bingham wrote:
> >> Generate the ResultMetadata before performing JPEG compression so that
> >> JPEG specific metadata can be added to the metadata when it has been
> >> processed.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  src/android/camera_device.cpp | 25 ++++++++++++++-----------
> >>  1 file changed, 14 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index ae52a5ca8b86..e23ab055d012 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -1138,6 +1138,8 @@ void CameraDevice::requestComplete(Request *request)
> >>  	const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
> >>  	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
> >>  	std::unique_ptr<CameraMetadata> resultMetadata;
> >> +	Camera3RequestDescriptor *descriptor =
> >> +		reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
> >>
> >>  	if (request->status() != Request::RequestComplete) {
> >>  		LOG(HAL, Error) << "Request not successfully completed: "
> >> @@ -1145,9 +1147,18 @@ void CameraDevice::requestComplete(Request *request)
> >>  		status = CAMERA3_BUFFER_STATUS_ERROR;
> >>  	}
> >>
> >> +	/*
> >> +	 * \todo The timestamp used for the metadata is currently always taken
> >> +	 * from the first buffer (which may be the first stream) in the Request.
> >> +	 * It might be appropriate to return a 'correct' (as determined by
> >> +	 * pipeline handlers) timestamp in the Request itself.
> >> +	 */
> >> +	FrameBuffer *buffer = buffers.begin()->second;
> >> +	resultMetadata = getResultMetadata(descriptor->frameNumber,
> >> +					   buffer->metadata().timestamp);
> >> +
> >>  	/* Prepare to call back the Android camera stack. */
> >> -	Camera3RequestDescriptor *descriptor =
> >> -		reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
> >> +
> >
> > One empty line is enough
> >
> >>
> >>  	camera3_capture_result_t captureResult = {};
> >>  	captureResult.frame_number = descriptor->frameNumber;
> >> @@ -1160,21 +1171,13 @@ void CameraDevice::requestComplete(Request *request)
> >>  	captureResult.output_buffers =
> >>  		const_cast<const camera3_stream_buffer_t *>(descriptor->buffers);
> >>
> >> -	/*
> >> -	 * \todo The timestamp used for the metadata is currently always taken
> >> -	 * from the first buffer (which may be the first stream) in the Request.
> >> -	 * It might be appropriate to return a 'correct' (as determined by
> >> -	 * pipeline handlers) timestamp in the Request itself.
> >> -	 */
> >> -	FrameBuffer *buffer = buffers.begin()->second;
> >>
> >>  	if (status == CAMERA3_BUFFER_STATUS_OK) {
> >>  		notifyShutter(descriptor->frameNumber,
> >>  			      buffer->metadata().timestamp);
> >>
> >>  		captureResult.partial_result = 1;
> >> -		resultMetadata = getResultMetadata(descriptor->frameNumber,
> >> -						   buffer->metadata().timestamp);
> >> +
> >
> > I would drop this one too
> >
> > Overall I think this is fine so far, as we generate metadata
> > statically, but going forward if we have to access the Controls
> > returned by a completed Request, we shall make sure it completed
> > successfully. Can we record that with a \todo entry ?
> >
>
> I'm not sure I fully understand your comment here, do you mean the
> libcamera request should be completed successfully before updating the
> resultMetaData?

What I meant to say is that before this change the metadata buffer was
generated in the
	if (status == CAMERA3_BUFFER_STATUS_OK)
case, while now, if I got this right, it's generated unconditionally.
When we'll assemble it inspecting the ControlList associated with the
just completed Request, if it has not completed succesfully, we might
try to access an invalid control list or a control list with unset
values. If that's correct, I think it has to be recorded.

Thanks
  j

>
> It will still only be assigned to the captureResult.result if the
>  status == CAMERA3_BUFFER_STATUS_OK
>
> check passes still...
>
> You'll see how this gets used in 12/12 of course.
>
>
> > That apart
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks,
>
>
> >
> > Thanks
> >   j
> >
> >>  		captureResult.result = resultMetadata->get();
> >>  	}
> >>
> >> --
> >> 2.25.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran