[libcamera-devel] [PATCH v3 8/8] android: camera_device: Add stream mapping log

[Kieran Bingham]

Hi Jacopo,

On 22/09/2020 10:47, Jacopo Mondi wrote:
> To ease following how Android stream gets mapped onto libcamera ones
> add a (quite verbose) printout before queueing a request to libcamera.
> 
> The output looks like:
>  0 -  (320x240)[0x00000022] -> (320x240)[NV12] (direct)
>  1 -  (640x480)[0x00000021] -> (640x480)[NV12] (internal)

Excellent, - that's definitely helpful.

> 
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 1b35c72a3de6..316d8293843f 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1474,9 +1474,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	Request *request =
>  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
>  
> +	LOG(HAL, Debug) << "Queueing Request to libcamera with "
> +			<< descriptor->numBuffers << " streams";

This doesn't feel right.  You're counting the number of android buffers
to state the number streams being 'queued' to libcamera.

In the event that one libcamera stream is shared (i.e. say a JPEG
encode) to generate two android streams, this would be wrong.



>  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> +		camera3_stream *camera3Stream = camera3Buffers[i].stream;
>  		CameraStream *cameraStream =
> -			static_cast<CameraStream *>(camera3Buffers[i].stream->priv);
> +			static_cast<CameraStream *>(camera3Stream->priv);
>  		const StreamConfiguration &config = config_->at(cameraStream->index());
>  		Stream *stream = config.stream();
>  
> @@ -1487,6 +1490,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		descriptor->buffers[i].stream = camera3Buffers[i].stream;
>  		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
>  
> +		std::stringstream ss;
> +		ss << i << " - ("
> +		   << camera3Stream->width << "x" << camera3Stream->height << ")"
> +		   << "[" << utils::hex(camera3Stream->format) << "] -> "

We should really add a string mapping for this format ... 21 ... 20 ...
they're not great for following what's been chosen.

Still, that's it's own patch outright and separate to this I guess.


> +		   << "(" << config.size.toString() << ")["
> +		   << config.pixelFormat.toString() << "]";
> +
>  		/*
>  		 * Inspect the camera stream type, create buffers opportunely
>  		 * and add them to the Request if required.
> @@ -1498,6 +1508,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			 * Mapped streams don't need buffers added to the
>  			 * Request.
>  			 */
> +			LOG(HAL, Debug) << ss.str() << " (mapped)";
>  			continue;
>  
>  		case CameraStream::Type::Direct:
> @@ -1509,6 +1520,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			 */
>  			buffer = createFrameBuffer(*camera3Buffers[i].buffer);
>  			descriptor->frameBuffers.emplace_back(buffer);
> +			LOG(HAL, Debug) << ss.str() << " (direct)";
>  			break;
>  
>  		case CameraStream::Type::Internal:
> @@ -1522,6 +1534,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			 * allocator once it has been processed.
>  			 */
>  			buffer = getBuffer(stream);
> +			LOG(HAL, Debug) << ss.str() << " (internal)";
>  			break;
>  		}
>  		if (!buffer) {
> 

-- 
Regards
--
Kieran