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

[Jacopo Mondi]

Hi Kieran,

On Thu, Sep 24, 2020 at 05:29:44PM +0100, Kieran Bingham wrote:
> 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.

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.

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