[libcamera-devel] [PATCH 02/15] android: camera_stream: Add CameraStream::Type
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Oct 5 17:50:55 CEST 2020
Hi Jacopo,
On 05/10/2020 12:21, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Oct 05, 2020 at 01:46:36PM +0300, Laurent Pinchart wrote:
>> From: Jacopo Mondi <jacopo at jmondi.org>
>>
>> Define the CameraStream::Type enumeration and assign it to
>> each CameraStream instance at construction time.
>>
>> The CameraStream type will be used to decide if memory needs to be
>> allocated on its behalf or if the stream is backed by memory externally
>> allocated by the Android framework.
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> 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 | 8 +++-
>> src/android/camera_stream.cpp | 4 +-
>> src/android/camera_stream.h | 86 ++++++++++++++++++++++++++++++++++-
>> 3 files changed, 93 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index bbc692fe109f..0600ebc81c64 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1216,12 +1216,14 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>
>> config_->addConfiguration(streamConfiguration);
>> unsigned int index = config_->size() - 1;
>> - streams_.emplace_back(format, size, index);
>> + streams_.emplace_back(format, size, CameraStream::Type::Direct,
>> + index);
>> stream->priv = static_cast<void *>(&streams_.back());
>> }
>>
>> /* Now handle the MJPEG streams, adding a new stream if required. */
>> if (jpegStream) {
>> + CameraStream::Type type;
>> int index = -1;
>>
>> /* Search for a compatible stream in the non-JPEG ones. */
>> @@ -1239,6 +1241,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>> LOG(HAL, Info)
>> << "Android JPEG stream mapped to libcamera stream " << i;
>>
>> + type = CameraStream::Type::Mapped;
>> index = i;
>> break;
>> }
>> @@ -1263,6 +1266,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>> LOG(HAL, Info) << "Adding " << streamConfiguration.toString()
>> << " for MJPEG support";
>>
>> + type = CameraStream::Type::Internal;
>> config_->addConfiguration(streamConfiguration);
>> index = config_->size() - 1;
>> }
>> @@ -1281,7 +1285,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>> return ret;
>> }
>>
>> - streams_.emplace_back(formats::MJPEG, cfg.size, index, encoder);
>> + streams_.emplace_back(formats::MJPEG, cfg.size, type, index, encoder);
>> jpegStream->priv = static_cast<void *>(&streams_.back());
>> }
>>
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index 01c62978ca3a..585bf2b68f4f 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -10,7 +10,7 @@
>> using namespace libcamera;
>>
>> CameraStream::CameraStream(PixelFormat format, Size size,
>> - unsigned int index, Encoder *encoder)
>> - : format_(format), size_(size), index_(index), encoder_(encoder)
>> + Type type, unsigned int index, Encoder *encoder)
>> + : format_(format), size_(size), type_(type), index_(index), encoder_(encoder)
>> {
>> }
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index 10dece7beb69..07c714e3a365 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -17,17 +17,101 @@
>> class CameraStream
>> {
>> public:
>> + /*
>> + * Enumeration of CameraStream types.
>> + *
>> + * A camera stream associates an Android stream to a libcamera stream.
>> + * This enumeration describes how the two streams are associated and how
>> + * and where data produced from libcamera are delivered to the
s/are/is/
>> + * Android framework.
>> + *
>> + * Direct:
>> + *
>> + * The Android stream is directly mapped onto a libcamera stream: frames
s/: f/. F/ ?
Or maybe it was supposed to be s/:/;/ ?
>> + * are delivered by the library directly in the memory location
>> + * specified by the Android stream (buffer_handle_t->data) and provided
>> + * to the framework as they are. The Android stream characteristics are
>> + * directly translated to the libcamera stream configuration.
>> + *
>> + * +-----+ +-----+
>> + * | A | | L |
>> + * +-----+ +-----+
>> + * | |
>> + * V V
>> + * +-----+ +------+
>> + * | B |<---------------| FB |
>> + * +-----+ +------+
>> + *
Maybe we should expand the initials/acronyms somewhere?
A: Android
B: Android Buffer
L: libcamera
FB: libcamera FrameBuffer ?
(Assuming my assumptions are correct :-D )
In fact, reading down - FB is actually an FB wrapping B.
Maybe we could express that 'wrapping' in ascii art - but I expect it
might be painful too - so don't worry about it.
>> + *
>> + * Internal:
>> + *
>> + * Data for the Android stream is produced by processing a libcamera
>> + * stream created by the HAL for that purpose. The libcamera stream
>> + * needs to be supplied with intermediate buffers where the library
>> + * delivers frames to be processed and then provided to the framework.
>> + * The libcamera stream configuration is not a direct translation of the
>> + * Android stream characteristics, but it describes the format and size
>> + * required for the processing procedure to produce frames in the
>> + * Android required format.
>> + *
>> + * +-----+ +-----+
>> + * | A | | L |
>> + * +-----+ +-----+
>> + * | |
>> + * V V
>> + * +-----+ +------+
>> + * | B | | FB |
>> + * +-----+ +------+
>> + * ^ |
>> + * |-------Processing------|
>> + *
>> + *
>> + * Mapped:
>> + *
>> + * Data for the Android stream is produced by processing a libcamera
>> + * stream associated with another CameraStream. Mapped camera streams do
>> + * not need any memory to be reserved for them as they process data
>> + * produced by libcamera for a different stream whose format and size
>> + * is compatible with the processing procedure requirements to produce
>
> s/is/are/
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
>> + * frames in the Android required format.
>> + *
>> + * +-----+ +-----+ +-----+
>> + * | A | | A' | | L |
>> + * +-----+ +-----+ +-----+
>> + * | | |
>> + * V V V
>> + * +-----+ +-----+ +------+
>> + * | B | | B' |<---------| FB |
>> + * +-----+ +-----+ +------+
>> + * ^ |
>> + * |--Processing--|
>> + *
>> + *
>> + * --------------------------------------------------------------------
>> + * A = Android stream
>> + * L = libcamera stream
>> + * B = memory buffer
>> + * FB = libcamera FrameBuffer
>> + * "Processing" = Frame processing procedure (Encoding, scaling etc)
>> + */
Aha - here it is.
I wonder if this should be first so we see the definitions before
they're used.
But it's not far to follow down, so it's kind of footnotes - so I don't
object to keeping this here.
You've already got my tag - and you can keep it of course ;-)
>> + enum class Type {
>> + Direct,
>> + Internal,
>> + Mapped,
>> + };
>> CameraStream(libcamera::PixelFormat format, libcamera::Size size,
>> - unsigned int index, Encoder *encoder = nullptr);
>> + Type type, unsigned int index, Encoder *encoder = nullptr);
>>
>> const libcamera::PixelFormat &format() const { return format_; }
>> const libcamera::Size &size() const { return size_; }
>> + Type type() const { return type_; }
>> unsigned int index() const { return index_; }
>> Encoder *encoder() const { return encoder_.get(); }
>>
>> private:
>> libcamera::PixelFormat format_;
>> libcamera::Size size_;
>> + Type type_;
>> /*
>> * The index of the libcamera StreamConfiguration as added during
>> * configureStreams(). A single libcamera Stream may be used to deliver
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list