| Message ID | 20200909155457.153907-2-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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, > + }; 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@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_; } > > 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Thu, Sep 10, 2020 at 8:16 PM Niklas Söderlund <niklas.soderlund@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@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@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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On 09/09/2020 16:54, 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@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 is produced > + * 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 |-----+ > + * +-----+ +------+ I think libcamera always writes to a FrameBuffer right? Shouldn't 'B' be an intermeddiate step between FB and processing? The arrows seem a bit odd otherwise, L points to both B and FB, but 'B' doesn't go anywhere? > + * ^ | > + * |-------Processing------| > + * > + * > + * Mapped: > + * > + * Data for the Android stream are produced by processing a libcamera /stream are produced/stream is produced/ Other than that, this all sounds good. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + * 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, > + }; > + > 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_; } > > 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 >
Hi Kieran, On Fri, Sep 11, 2020 at 02:50:46PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 09/09/2020 16:54, 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@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 is produced > > > + * 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 |-----+ > > + * +-----+ +------+ > > I think libcamera always writes to a FrameBuffer right? Shouldn't 'B' be > an intermeddiate step between FB and processing? Should I swap FB and B here ? I probably makes sense and aligns to the other drawings > > The arrows seem a bit odd otherwise, L points to both B and FB, but 'B' > doesn't go anywhere? > > > + * ^ | > > + * |-------Processing------| > > + * > > + * > > + * Mapped: > > + * > > + * Data for the Android stream are produced by processing a libcamera > > /stream are produced/stream is produced/ > > Other than that, this all sounds good. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Thanks j > > > > + * 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, > > + }; > > + > > 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_; } > > > > 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
Hi Niklas, On Thu, Sep 10, 2020 at 01:16:48PM +0200, Niklas Söderlund 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@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, > > + }; > > 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 ? Thanks, I like "direct" and "processed", not much "CopyProcessed" though... Let me send v2 with the names I have here, we can continue discussing them on that series Thanks j > > 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@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_; } > > > > 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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
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, + }; + 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_; } 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
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@jmondi.org> --- src/android/camera_device.cpp | 14 ++++-- src/android/camera_device.h | 87 ++++++++++++++++++++++++++++++++++- 2 files changed, 96 insertions(+), 5 deletions(-)