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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Sep 25, 2020 at 11:39:19AM +0200, Jacopo Mondi wrote:
> On Thu, Sep 24, 2020 at 05:29:44PM +0100, Kieran Bingham wrote:
> > On 24/09/2020 17:26, Jacopo Mondi wrote:
> > > On Thu, Sep 24, 2020 at 05:10:23PM +0100, Kieran Bingham wrote:
> > >> On 22/09/2020 10:47, Jacopo Mondi wrote:
> > >>> To ease following how Android stream gets mapped onto libcamera ones

s/stream gets/streams get/
s/onto/to/

> > >>> 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.
> 
> uff
> 
> What I meant is that android has queued a request to the libcamera HAL
> layer with #n streams of the reported format and size and the reported
> mapping mode.

I also think the message is slightly confusing. How about "Queuing
Request to libcamera for 3 HAL streams" ? That's a simple change and
would make sure it can't be understood as queuing a request with 3
libcamera streams.

The change is good, it really helps understanding what happens.

> > >>>  	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() << "]";

The output would be a bit different, but you could use config.toString()
to simplify this.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > >>> +
> > >>>  		/*
> > >>>  		 * 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,

Laurent Pinchart