[libcamera-devel] [PATCH 1/8] android: camera_device: Add CameraStream::Type
Hirokazu Honda
hiroh at chromium.org
Fri Sep 11 06:26:54 CEST 2020
On Thu, Sep 10, 2020 at 8:16 PM Niklas Söderlund
<niklas.soderlund at ragnatech.se> wrote:
>
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-09-09 17:54:50 +0200, Jacopo Mondi wrote:
> > 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.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/android/camera_device.cpp | 14 ++++--
> > src/android/camera_device.h | 87 ++++++++++++++++++++++++++++++++++-
> > 2 files changed, 96 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 50923df22a8f..3c58523e528e 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -168,9 +168,11 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
> > }
> > }
> >
> > -CameraStream::CameraStream(PixelFormat format, Size size,
> > +CameraStream::CameraStream(PixelFormat format, Size size, Type type,
> > unsigned int index, Encoder *encoder)
> > - : format_(format), size_(size), index_(index), encoder_(encoder)
> > +
> > + : format_(format), size_(size), type_(type), index_(index),
> > + encoder_(encoder)
> > {
> > }
> >
> > @@ -1215,12 +1217,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 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. */
> > @@ -1238,6 +1242,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > LOG(HAL, Info)
> > << "Android JPEG stream mapped on stream " << i;
> >
> > + type = CameraStream::Type::Mapped;
> > index = i;
> > break;
> > }
> > @@ -1262,6 +1267,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;
> > }
> > @@ -1280,7 +1286,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_device.h b/src/android/camera_device.h
> > index 912c59aeb5a8..9dea7c42bdb5 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -30,17 +30,102 @@ class CameraMetadata;
> > 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
> > + * Android framework.
> > + *
> > + * Direct:
> > + *
> > + * The Android stream is directly mapped onto a libcamera stream: frames
> > + * 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 |
> > + * +-----+ +------+
> > + *
> > + *
> > + * Internal:
> > + *
> > + * Data for the Android stream are 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 |--->| B |
> > + * +-----+ +-----+ +----+
> > + * | | ^
> > + * V V |
> > + * +-----+ +------+ |
> > + * | B | | FB |-----+
> > + * +-----+ +------+
> > + * ^ |
> > + * |-------Processing------|
> > + *
> > + *
> > + * Mapped:
> > + *
> > + * Data for the Android stream are 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
> > + * 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)
> > + */
> > + enum Type {
> > + Direct,
> > + Internal,
> > + Mapped,
> > + };
>
nit: enum class
> I really like the diagrams!
>
> I'm scratching my head trying to think of other names that are more self
> explanatory from a HAL point-of-view. What about Direct, Processed and
> CopyProcessed ?
>
> I don't want to block this over bike-shedding and we can always argue
> over names on-top so,
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> > +
> > 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_; }
> > unsigned int index() const { return index_; }
> > Encoder *encoder() const { return encoder_.get(); }
> > + Type type() const { return type_; }
> >
huge nit: Put type() after size() to follow the order of arguments and
variable declarations.
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > 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
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> 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