[libcamera-devel,RFC,4/6] android: camera_device: Support MJPEG stream construction

8913 diff mbox series

MJPEG streams must be created referencing a libcamera stream.
This stream may already be provided by the request configuration,
in which case the existing stream is utilised.

If no compatible stream is available to encode, a new stream is requested
from the libcamera configuration.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/android/camera_device.cpp | 158 +++++++++++++++++++++++++++++++++-
 src/android/camera_device.h   |   8 ++
 2 files changed, 162 insertions(+), 4 deletions(-)

Hi Kieran,
   sorry I got here later than expected

On Tue, Jul 21, 2020 at 11:01:24PM +0100, Kieran Bingham wrote:
> MJPEG streams must be created referencing a libcamera stream.
> This stream may already be provided by the request configuration,
> in which case the existing stream is utilised.
>
> If no compatible stream is available to encode, a new stream is requested
> from the libcamera configuration.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 158 +++++++++++++++++++++++++++++++++-
>  src/android/camera_device.h   |   8 ++
>  2 files changed, 162 insertions(+), 4 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 56652ac57676..7323d4e58f68 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -8,6 +8,7 @@
>  #include "camera_device.h"
>  #include "camera_ops.h"
>
> +#include <sys/mman.h>
>  #include <tuple>
>  #include <vector>
>
> @@ -21,6 +22,8 @@
>  #include "camera_metadata.h"
>  #include "system/graphics.h"
>
> +#include "jpeg/compressor_jpeg.h"
> +
>  using namespace libcamera;
>
>  namespace {
> @@ -1004,6 +1007,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	 */
>  	unsigned int streamIndex = 0;
>
> +	/* First handle all non-MJPEG streams */
>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>  		camera3_stream_t *stream = stream_list->streams[i];
>
> @@ -1019,6 +1023,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		if (!format.isValid())
>  			return -EINVAL;
>
> +		stream->priv = static_cast<void *>(&streams_[i]);

Dumb question, you called reserve on the stream_ vector, and that
reserves space for the container, but shouldn't you call push_back()
to increase its size and make sure all its internal counters are
correct. From the STL documentation:

reserve() does not change the size of the vector.

> +		streams_[i].format = format;
> +		streams_[i].size = Size(stream->width, stream->height);
> +

That's maybe me, and I know I like longer names which are not always
good, but a stream_ vector of CameraStreams, where we already have
daa types of Camera3... type adds confusion to a class which already
relies on the names "streams" and "camera" a bit too much...

What about
struct StreamMap {
        int streamIndex;
	libcamera::PixelFormat format;
	libcamera::Size size;
	CompressorJPEG *jpeg;
};

or something similar, and a

        std::vector<StreamMap> streamMaps_;

I know they are not std::map, but they map Android streams to
libcamera streams, and conveying that could be nice to read.

> +		/* Defer handling of MJPEG streams until all others are known. */
> +		if (format == formats::MJPEG) {
> +			LOG(HAL, Info) << "Handling MJPEG stream";
> +
> +			streams_[i].index = -1;
> +			continue;
> +		}
> +
>  		StreamConfiguration streamConfiguration;
>
>  		streamConfiguration.size.width = stream->width;
> @@ -1028,7 +1044,61 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		config_->addConfiguration(streamConfiguration);
>
>  		streams_[i].index = streamIndex++;

I had this question if you could get rid of index and store a pointer
to the StreamConfiguration. Since I recall I already asked that, as an
exercize I tried getting rid of it and I failed, so no need to ask :)
But I leave here my exercize for reference, expecially for the
part that actually emplaces in the vector the CameraStream.

        unsigned int streamIndex = 0;
	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
		camera3_stream_t *stream = stream_list->streams[i];
		PixelFormat format = toPixelFormat(stream->format);

		LOG(HAL, Info) << "Stream #" << i
			       << ", direction: " << stream->stream_type
			       << ", width: " << stream->width
			       << ", height: " << stream->height
			       << ", format: " << utils::hex(stream->format)
			       << " (" << format.toString() << ")";

		if (!format.isValid())
			return -EINVAL;

                streams_.emplace_back(-1, format,
                                      { stream->width, stream->height }, nullptr);
                stream->priv = static_cast<void *>(&streams_[i]);

		/* Defer handling of MJPEG streams until all others are known. */
		if (format == formats::MJPEG)
			continue;

		StreamConfiguration streamConfiguration;
		streamConfiguration.size.width = stream->width;
		streamConfiguration.size.height = stream->height;
		streamConfiguration.pixelFormat = format;
		config_->addConfiguration(streamConfiguration);

		streams_[i].index = streamIndex++;
         }

> -		stream->priv = static_cast<void *>(&streams_[i]);
> +	}
> +
> +	/* Now handle MJPEG streams, adding a new stream if required. */
> +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> +		camera3_stream_t *stream = stream_list->streams[i];
> +		CameraStream *cameraStream = &streams_[i];

Here I would avoid doing this. Another thing being named as a
combination of the terms 'camera' and 'stream' is confusing. Can you
use streams[i] directly and avoid a confusing indirection ? We are
clearly running out of terms here :)

> +
> +		/* Only process MJPEG streams */
> +		if (cameraStream->format != formats::MJPEG)
> +			continue;
> +
> +		bool match = false;
> +
> +		/* Search for a compatible stream */
> +		for (unsigned int j = 0; j < config_->size(); j++) {
> +			StreamConfiguration &cfg = config_->at(j);
> +
> +			/*
> +			 * The PixelFormat must also be compatible with the
> +			 * encoder.
> +			 */
> +			if (cfg.size == cameraStream->size) {
> +				    LOG(HAL, Info)
> +					<< "Stream " << i
> +					<< " using libcamera stream "
> +					<< j;
> +
> +				    match = true;
> +				    cameraStream->index = j;
> +			}
> +		}
> +
> +		/*
> +		 * Without a compatible match for JPEG encoding we must
> +		 * introduce a new stream to satisfy the request requirements.
> +		 */
> +		if (!match) {
> +			StreamConfiguration streamConfiguration;
> +
> +			/*
> +			 * \todo: The pixelFormat should be a 'best-fit' choice
> +			 * and may require a validation cycle. This is not yet
> +			 * handled, and should be considered as part of any
> +			 * stream configuration reworks.
> +			 */
> +			streamConfiguration.size.width = stream->width;
> +			streamConfiguration.size.height = stream->height;
> +			streamConfiguration.pixelFormat = formats::NV12;
> +
> +			LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
> +				       << " for MJPEG support";
> +
> +			config_->addConfiguration(streamConfiguration);
> +			streams_[i].index = streamIndex++;
> +		}
>  	}
>
>  	switch (config_->validate()) {
> @@ -1059,6 +1129,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>
>  		/* Use the bufferCount confirmed by the validation process. */
>  		stream->max_buffers = cfg.bufferCount;
> +
> +		/*
> +		 * Construct a software compressor for MJPEG streams from the
> +		 * chosen libcamera source stream.
> +		 */
> +		if (cameraStream->format == formats::MJPEG) {
> +			cameraStream->jpeg = new CompressorJPEG();
> +			cameraStream->jpeg->configure(cfg);
> +		} else {
> +			/* Either construct this correctly, or use a better interface */
> +			cameraStream->jpeg = nullptr;

I would be tempted to ask if we could create a JPEGCameraStream which
derives from CameraStream, store pointers to CameraStream base class
in stream_ and create the correct derived depending on the format to
avoid cameraStream->jpeg = nullptr.

But I suspect this is used as a flag somewhere in the next patches.

> +		}
>  	}
>
>  	/*
> @@ -1112,6 +1194,9 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
>
>  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>  {
> +	LOG(HAL, Error) << "Received request " << camera3Request->frame_number
> +			<< " with " << camera3Request->num_output_buffers << " buffers";
> +
>  	if (!camera3Request->num_output_buffers) {
>  		LOG(HAL, Error) << "No output buffers provided";
>  		return -EINVAL;
> @@ -1158,6 +1243,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		descriptor->buffers[i].stream = camera3Buffers[i].stream;
>  		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
>
> +		/* Software streams are handled after hardware streams complete. */
> +		if (cameraStream->format == formats::MJPEG)
> +			continue;
> +
>  		/*
>  		 * Create a libcamera buffer using the dmabuf descriptors of
>  		 * the camera3Buffer for each stream. The FrameBuffer is
> @@ -1190,11 +1279,40 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	return 0;
>  }
>
> +static CompressedImage mapAndroidBlobBuffer(const buffer_handle_t camera3buffer)
> +{
> +	CompressedImage img;
> +
> +	/* ANDROID_JPEG_MAX_SIZE */
> +	unsigned int length = int32_t{13 << 20};
> +
> +	/* Take only the first plane */
> +	void *memory = mmap(NULL, length, PROT_READ|PROT_WRITE, MAP_SHARED,
> +			    camera3buffer->data[0], 0);
> +
> +	img.length = length;
> +	img.data = static_cast<unsigned char*>(memory);
> +
> +	return img;
> +}
> +
> +static void unmapAndroidBlobBuffer(CompressedImage *img)
> +{
> +	munmap(img->data, img->length);
> +	img->data = nullptr;
> +	img->length = 0;
> +}
> +
>  void CameraDevice::requestComplete(Request *request)
>  {
>  	const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
>  	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
>  	std::unique_ptr<CameraMetadata> resultMetadata;
> +	Camera3RequestDescriptor *descriptor =
> +		reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
> +
> +
> +	LOG(HAL, Error) << "Request completed:...";

Why move this up ?

>
>  	if (request->status() != Request::RequestComplete) {
>  		LOG(HAL, Error) << "Request not succesfully completed: "
> @@ -1202,10 +1320,41 @@ void CameraDevice::requestComplete(Request *request)
>  		status = CAMERA3_BUFFER_STATUS_ERROR;
>  	}
>
> -	/* Prepare to call back the Android camera stack. */
> -	Camera3RequestDescriptor *descriptor =
> -		reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
> +	/* Handle any JPEG compression */
> +	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> +		CameraStream *cameraStream =
> +			static_cast<CameraStream *>(descriptor->buffers[i].stream->priv);
> +		Compressor *compressor = cameraStream->jpeg;
>
> +		/* Only handle streams with compression enabled. */
> +		if (!compressor)
> +			continue;

Ah yes, it is used as flag here. Maybe the idea of subclassing is an
overkill.

> +
> +		/* \todo: Optimise this dance routine, just to get the stream/buffer ... */
> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
> +		Stream *stream = streamConfiguration->stream();
> +		FrameBuffer *buffer = request->findBuffer(stream);
> +		if (!buffer) {
> +			LOG(HAL, Error) << "Failed to find a source stream buffer";
> +			continue;
> +		}
> +
> +		CompressedImage output = mapAndroidBlobBuffer(*descriptor->buffers[i].buffer);
> +		if (output.data == MAP_FAILED) {
> +			LOG(HAL, Error) << "Failed to mmap android blob buffer of length " << output.length;
> +			continue;
> +		}
> +
> +		int ret = compressor->compress(buffer, &output);
> +		if (ret) {
> +			LOG(HAL, Error) << "Failed to compress stream image";
> +			status = CAMERA3_BUFFER_STATUS_ERROR;
> +		}
> +
> +		unmapAndroidBlobBuffer(&output);
> +	}
> +
> +	/* Prepare to call back the Android camera stack. */
>  	camera3_capture_result_t captureResult = {};
>  	captureResult.frame_number = descriptor->frameNumber;
>  	captureResult.num_output_buffers = descriptor->numBuffers;
> @@ -1246,6 +1395,7 @@ void CameraDevice::requestComplete(Request *request)
>  			    descriptor->buffers[0].stream);
>  	}
>
> +
>  	callbacks_->process_capture_result(callbacks_, &captureResult);
>
>  	delete descriptor;
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 5b8b9c3e26e2..1973adaa2b4b 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -23,6 +23,8 @@
>  #include "libcamera/internal/log.h"
>  #include "libcamera/internal/message.h"
>
> +#include "jpeg/compressor_jpeg.h"
> +
>  class CameraMetadata;
>
>  struct CameraStream {
> @@ -32,6 +34,12 @@ struct CameraStream {
>  	 * one or more streams to the Android framework.
>  	 */
>  	unsigned int index;
> +
> +	libcamera::PixelFormat format;
> +	libcamera::Size size;
> +
> +	/* Make sure this gets destructed correctly */
> +	CompressorJPEG *jpeg;

Looking at this now, it seems all it serves is actually for JPEG
compression. Maybe convey that in the name. I know I've suggested map,
but that's really a too generic name, and my head got twisted a few
times when trying to keep track of all the 'streams' we have in this
class.

>  };
>
>  class CameraDevice : protected libcamera::Loggable
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On 28/07/2020 17:58, Jacopo Mondi wrote:
> Hi Kieran,
>    sorry I got here later than expected

No worries, thanks for your time.


> On Tue, Jul 21, 2020 at 11:01:24PM +0100, Kieran Bingham wrote:
>> MJPEG streams must be created referencing a libcamera stream.
>> This stream may already be provided by the request configuration,
>> in which case the existing stream is utilised.
>>
>> If no compatible stream is available to encode, a new stream is requested
>> from the libcamera configuration.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/android/camera_device.cpp | 158 +++++++++++++++++++++++++++++++++-
>>  src/android/camera_device.h   |   8 ++
>>  2 files changed, 162 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 56652ac57676..7323d4e58f68 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -8,6 +8,7 @@
>>  #include "camera_device.h"
>>  #include "camera_ops.h"
>>
>> +#include <sys/mman.h>
>>  #include <tuple>
>>  #include <vector>
>>
>> @@ -21,6 +22,8 @@
>>  #include "camera_metadata.h"
>>  #include "system/graphics.h"
>>
>> +#include "jpeg/compressor_jpeg.h"
>> +
>>  using namespace libcamera;
>>
>>  namespace {
>> @@ -1004,6 +1007,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  	 */
>>  	unsigned int streamIndex = 0;
>>
>> +	/* First handle all non-MJPEG streams */
>>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>>  		camera3_stream_t *stream = stream_list->streams[i];
>>
>> @@ -1019,6 +1023,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  		if (!format.isValid())
>>  			return -EINVAL;
>>
>> +		stream->priv = static_cast<void *>(&streams_[i]);
> 
> Dumb question, you called reserve on the stream_ vector, and that
> reserves space for the container, but shouldn't you call push_back()
> to increase its size and make sure all its internal counters are
> correct. From the STL documentation:
> 
> reserve() does not change the size of the vector.
> 
>> +		streams_[i].format = format;
>> +		streams_[i].size = Size(stream->width, stream->height);
>> +


Hrm, I guess by reserving the space I can write to it with the array
index, but indeed, I didn't think as to whether it has automatically
increased the size to the max of the accessed index, so it probably is
incorrect.

I wanted to use the indexes directly, because we are skipping mjpeg
streams in this loop iteration, so using push_back would break the indices.

Maybe I just need to rethink this, (or see if it's still necessary).


~~~~~~

So, I need to be able to map from the buffers in a given request, back
to the streams which handle that buffer.

The index here is important currently. I'll have to see if it's really
required, or if I was just using it for convenience.



> That's maybe me, and I know I like longer names which are not always
> good, but a stream_ vector of CameraStreams, where we already have
> daa types of Camera3... type adds confusion to a class which already
> relies on the names "streams" and "camera" a bit too much...
> 
> What about
> struct StreamMap {
>         int streamIndex;
> 	libcamera::PixelFormat format;
> 	libcamera::Size size;
> 	CompressorJPEG *jpeg;
> };
> 
> or something similar, and a
> 
>         std::vector<StreamMap> streamMaps_;
> 
> I know they are not std::map, but they map Android streams to
> libcamera streams, and conveying that could be nice to read.

Well, in fact it could be a map from camera3_stream_t, to our internal
representation.

Although, I'm not sure if the map gives a benefit yet, as I store the
pointer in the camera3_stream_t->priv which is a faster access than
looking up in a map, so I guess it's only if the reverse is needed ...

Hrm...

In fact, if I store the pointer, maybe I don't need the index to match
anyway, as the access route is there (until I see I need to go
backwards, which could be handled by a reverse pointer anyway).


>> +		/* Defer handling of MJPEG streams until all others are known. */
>> +		if (format == formats::MJPEG) {
>> +			LOG(HAL, Info) << "Handling MJPEG stream";
>> +
>> +			streams_[i].index = -1;
>> +			continue;
>> +		}
>> +
>>  		StreamConfiguration streamConfiguration;
>>
>>  		streamConfiguration.size.width = stream->width;
>> @@ -1028,7 +1044,61 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  		config_->addConfiguration(streamConfiguration);
>>
>>  		streams_[i].index = streamIndex++;
> 
> I had this question if you could get rid of index and store a pointer
> to the StreamConfiguration. Since I recall I already asked that, as an
> exercize I tried getting rid of it and I failed, so no need to ask :)
> But I leave here my exercize for reference, expecially for the
> part that actually emplaces in the vector the CameraStream.

We can't store a pointer to the StreamConfiguration, because they change
when they are added, and the address isn't guaranteed to stay constant.



>         unsigned int streamIndex = 0;
> 	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> 		camera3_stream_t *stream = stream_list->streams[i];
> 		PixelFormat format = toPixelFormat(stream->format);
> 
> 		LOG(HAL, Info) << "Stream #" << i
> 			       << ", direction: " << stream->stream_type
> 			       << ", width: " << stream->width
> 			       << ", height: " << stream->height
> 			       << ", format: " << utils::hex(stream->format)
> 			       << " (" << format.toString() << ")";
> 
> 		if (!format.isValid())
> 			return -EINVAL;
> 
>                 streams_.emplace_back(-1, format,
>                                       { stream->width, stream->height }, nullptr);
>                 stream->priv = static_cast<void *>(&streams_[i]);
> 
> 		/* Defer handling of MJPEG streams until all others are known. */
> 		if (format == formats::MJPEG)
> 			continue;
> 
> 		StreamConfiguration streamConfiguration;
> 		streamConfiguration.size.width = stream->width;
> 		streamConfiguration.size.height = stream->height;
> 		streamConfiguration.pixelFormat = format;
> 		config_->addConfiguration(streamConfiguration);
> 
> 		streams_[i].index = streamIndex++;
>          }
> 
>> -		stream->priv = static_cast<void *>(&streams_[i]);
>> +	}
>> +
>> +	/* Now handle MJPEG streams, adding a new stream if required. */
>> +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>> +		camera3_stream_t *stream = stream_list->streams[i];
>> +		CameraStream *cameraStream = &streams_[i];
> 
> Here I would avoid doing this. Another thing being named as a
> combination of the terms 'camera' and 'stream' is confusing. Can you
> use streams[i] directly and avoid a confusing indirection ? We are
> clearly running out of terms here :)

Well the point was the CameraStream was supposed to be 'our'
representation of the 'stream'.

Almost makes me want to continue down the path that all streams get
given a CameraStream, which has the camera3_stream_t stored in it at
creation, so that we only then iterate 'our' streams, and reference the
android type when needed.

Maybe that would be clearer ...

>> +
>> +		/* Only process MJPEG streams */
>> +		if (cameraStream->format != formats::MJPEG)
>> +			continue;
>> +
>> +		bool match = false;
>> +
>> +		/* Search for a compatible stream */
>> +		for (unsigned int j = 0; j < config_->size(); j++) {
>> +			StreamConfiguration &cfg = config_->at(j);
>> +
>> +			/*
>> +			 * The PixelFormat must also be compatible with the
>> +			 * encoder.
>> +			 */
>> +			if (cfg.size == cameraStream->size) {
>> +				    LOG(HAL, Info)
>> +					<< "Stream " << i
>> +					<< " using libcamera stream "
>> +					<< j;
>> +
>> +				    match = true;
>> +				    cameraStream->index = j;
>> +			}
>> +		}
>> +
>> +		/*
>> +		 * Without a compatible match for JPEG encoding we must
>> +		 * introduce a new stream to satisfy the request requirements.
>> +		 */
>> +		if (!match) {
>> +			StreamConfiguration streamConfiguration;
>> +
>> +			/*
>> +			 * \todo: The pixelFormat should be a 'best-fit' choice
>> +			 * and may require a validation cycle. This is not yet
>> +			 * handled, and should be considered as part of any
>> +			 * stream configuration reworks.
>> +			 */
>> +			streamConfiguration.size.width = stream->width;
>> +			streamConfiguration.size.height = stream->height;
>> +			streamConfiguration.pixelFormat = formats::NV12;
>> +
>> +			LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
>> +				       << " for MJPEG support";
>> +
>> +			config_->addConfiguration(streamConfiguration);
>> +			streams_[i].index = streamIndex++;

Hrm, seems I forgot all about the fact that if I add my own libcamera
stream here, I'm giong to be responsible for allocating buffers for that
stream, and queueing them appropriately when the request is queued! Ugh.


>> +		}
>>  	}
>>
>>  	switch (config_->validate()) {
>> @@ -1059,6 +1129,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>
>>  		/* Use the bufferCount confirmed by the validation process. */
>>  		stream->max_buffers = cfg.bufferCount;
>> +
>> +		/*
>> +		 * Construct a software compressor for MJPEG streams from the
>> +		 * chosen libcamera source stream.
>> +		 */
>> +		if (cameraStream->format == formats::MJPEG) {
>> +			cameraStream->jpeg = new CompressorJPEG();
>> +			cameraStream->jpeg->configure(cfg);
>> +		} else {
>> +			/* Either construct this correctly, or use a better interface */
>> +			cameraStream->jpeg = nullptr;
> 
> I would be tempted to ask if we could create a JPEGCameraStream which
> derives from CameraStream, store pointers to CameraStream base class
> in stream_ and create the correct derived depending on the format to
> avoid cameraStream->jpeg = nullptr.
> 
> But I suspect this is used as a flag somewhere in the next patches.

Yes, this bit needs rework (hence the comment) but is used as a flag.

I'm not fond of using the pointer as a flag, but it's sort of convenient
I guess as it also needs to be stored anyway.


> 
>> +		}
>>  	}
>>
>>  	/*
>> @@ -1112,6 +1194,9 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
>>
>>  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>>  {
>> +	LOG(HAL, Error) << "Received request " << camera3Request->frame_number
>> +			<< " with " << camera3Request->num_output_buffers << " buffers";
>> +
>>  	if (!camera3Request->num_output_buffers) {
>>  		LOG(HAL, Error) << "No output buffers provided";
>>  		return -EINVAL;
>> @@ -1158,6 +1243,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>  		descriptor->buffers[i].stream = camera3Buffers[i].stream;
>>  		descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
>>
>> +		/* Software streams are handled after hardware streams complete. */
>> +		if (cameraStream->format == formats::MJPEG)
>> +			continue;
>> +
>>  		/*
>>  		 * Create a libcamera buffer using the dmabuf descriptors of
>>  		 * the camera3Buffer for each stream. The FrameBuffer is
>> @@ -1190,11 +1279,40 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>  	return 0;
>>  }
>>
>> +static CompressedImage mapAndroidBlobBuffer(const buffer_handle_t camera3buffer)
>> +{
>> +	CompressedImage img;
>> +
>> +	/* ANDROID_JPEG_MAX_SIZE */
>> +	unsigned int length = int32_t{13 << 20};
>> +
>> +	/* Take only the first plane */
>> +	void *memory = mmap(NULL, length, PROT_READ|PROT_WRITE, MAP_SHARED,
>> +			    camera3buffer->data[0], 0);
>> +
>> +	img.length = length;
>> +	img.data = static_cast<unsigned char*>(memory);
>> +
>> +	return img;
>> +}
>> +
>> +static void unmapAndroidBlobBuffer(CompressedImage *img)
>> +{
>> +	munmap(img->data, img->length);
>> +	img->data = nullptr;
>> +	img->length = 0;
>> +}
>> +
>>  void CameraDevice::requestComplete(Request *request)
>>  {
>>  	const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
>>  	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
>>  	std::unique_ptr<CameraMetadata> resultMetadata;
>> +	Camera3RequestDescriptor *descriptor =
>> +		reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
>> +
>> +
>> +	LOG(HAL, Error) << "Request completed:...";
> 
> Why move this up ?


Because I need to add fields to the resultMetaData, so I had to move
that up earlier. Looks like the two corresponding changes got put in
separate patches. I've split that so it's clear why it moves.



> 
>>
>>  	if (request->status() != Request::RequestComplete) {
>>  		LOG(HAL, Error) << "Request not succesfully completed: "
>> @@ -1202,10 +1320,41 @@ void CameraDevice::requestComplete(Request *request)
>>  		status = CAMERA3_BUFFER_STATUS_ERROR;
>>  	}
>>
>> -	/* Prepare to call back the Android camera stack. */
>> -	Camera3RequestDescriptor *descriptor =
>> -		reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
>> +	/* Handle any JPEG compression */
>> +	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
>> +		CameraStream *cameraStream =
>> +			static_cast<CameraStream *>(descriptor->buffers[i].stream->priv);
>> +		Compressor *compressor = cameraStream->jpeg;
>>
>> +		/* Only handle streams with compression enabled. */
>> +		if (!compressor)
>> +			continue;
> 
> Ah yes, it is used as flag here. Maybe the idea of subclassing is an
> overkill.
> 
>> +
>> +		/* \todo: Optimise this dance routine, just to get the stream/buffer ... */
>> +		StreamConfiguration *streamConfiguration = &config_->at(cameraStream->index);
>> +		Stream *stream = streamConfiguration->stream();
>> +		FrameBuffer *buffer = request->findBuffer(stream);
>> +		if (!buffer) {
>> +			LOG(HAL, Error) << "Failed to find a source stream buffer";
>> +			continue;
>> +		}
>> +
>> +		CompressedImage output = mapAndroidBlobBuffer(*descriptor->buffers[i].buffer);
>> +		if (output.data == MAP_FAILED) {
>> +			LOG(HAL, Error) << "Failed to mmap android blob buffer of length " << output.length;
>> +			continue;
>> +		}
>> +
>> +		int ret = compressor->compress(buffer, &output);
>> +		if (ret) {
>> +			LOG(HAL, Error) << "Failed to compress stream image";
>> +			status = CAMERA3_BUFFER_STATUS_ERROR;
>> +		}
>> +
>> +		unmapAndroidBlobBuffer(&output);
>> +	}
>> +
>> +	/* Prepare to call back the Android camera stack. */
>>  	camera3_capture_result_t captureResult = {};
>>  	captureResult.frame_number = descriptor->frameNumber;
>>  	captureResult.num_output_buffers = descriptor->numBuffers;
>> @@ -1246,6 +1395,7 @@ void CameraDevice::requestComplete(Request *request)
>>  			    descriptor->buffers[0].stream);
>>  	}
>>
>> +
>>  	callbacks_->process_capture_result(callbacks_, &captureResult);
>>
>>  	delete descriptor;
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index 5b8b9c3e26e2..1973adaa2b4b 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -23,6 +23,8 @@
>>  #include "libcamera/internal/log.h"
>>  #include "libcamera/internal/message.h"
>>
>> +#include "jpeg/compressor_jpeg.h"
>> +
>>  class CameraMetadata;
>>
>>  struct CameraStream {
>> @@ -32,6 +34,12 @@ struct CameraStream {
>>  	 * one or more streams to the Android framework.
>>  	 */
>>  	unsigned int index;
>> +
>> +	libcamera::PixelFormat format;
>> +	libcamera::Size size;
>> +
>> +	/* Make sure this gets destructed correctly */
>> +	CompressorJPEG *jpeg;
> 
> Looking at this now, it seems all it serves is actually for JPEG
> compression. Maybe convey that in the name. I know I've suggested map,
> but that's really a too generic name, and my head got twisted a few
> times when trying to keep track of all the 'streams' we have in this
> class.

I guess so, except I think I was envisaging that this object would
represent both JPEG streams and 'pure' libcamera streams.

But I maybe I can rename it to make it as JPEGStream to make it clearer
for now. I'll have to check if I'm actually using it for non-jpeg
streams though. I suspect I'm not (or not essentially).


>>  };
>>
>>  class CameraDevice : protected libcamera::Loggable
>> --
>> 2.25.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel