[libcamera-devel] [RFC PATCH 4/6] android: camera_device: Support MJPEG stream construction
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Aug 4 12:55:39 CEST 2020
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 at 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 at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list