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

[Jacopo Mondi]

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

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