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

[Kieran Bingham]

On 24/09/2020 17:26, Jacopo Mondi wrote:
> Hi Kieran
> 
> On Thu, Sep 24, 2020 at 05:10:23PM +0100, Kieran Bingham wrote:
>> 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.
> 
> 'android buffers' is actually the number of output streams requested
> by android
> 
>>
>> In the event that one libcamera stream is shared (i.e. say a JPEG
>> encode) to generate two android streams, this would be wrong.
> 
> Isn't this intended ? I do expect something like
> 
> Queueing Request to libcamera with 3 streams
>  0 - (4160x3104)[0x00000023] -> (4160x3104)[NV12] (direct)
>  1 - (4160x3104)[0x00000023] -> (4160x3104)[NV12] (external)
>  2 - (4160x3104)[0x00000023] -> (4160x3104)[NV12] (mapped)
> 
> Anyway, we cycle on descriptor->numBuffers, and that's what was
> intended to be print out..


It's confusing because - how many streams are there in the libcamera
request... (I don't believe it's 3?)

Maybe you could fix it by saying instead:

"Queuing Request to libcamera to satisfy 3 streams"...
But the request going to libcamera - does not have 3 streams still - so
it just sounds odd.

--
K

> 
>>
>>
>>
>>>  	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

-- 
Regards
--
Kieran