[libcamera-devel] [RFC PATCH 4/6] android: camera_device: Support MJPEG stream construction
Jacopo Mondi
jacopo at jmondi.org
Tue Jul 28 18:58:21 CEST 2020
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 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);
> +
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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list