[libcamera-devel] [PATCH 04/15] android: camera_stream: Construct with Android stream
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 7 02:57:58 CEST 2020
Hello,
On Tue, Oct 06, 2020 at 01:21:00PM +0100, Kieran Bingham wrote:
> On 06/10/2020 12:24, Jacopo Mondi wrote:
> > On Mon, Oct 05, 2020 at 05:31:31PM +0100, Kieran Bingham wrote:
> >> On 05/10/2020 11:46, Laurent Pinchart wrote:
> >>> From: Jacopo Mondi <jacopo at jmondi.org>
> >>>
> >>> A CameraStream maps a stream requested by Android to a
> >>> libcamera::StreamConfiguration. It shall then be constructed with
> >>> those information clearly specified.
> >>>> Change the CameraStream constructor to accept an Android
> >>> camera3_stream_t and a libcamera::StreamConfiguration to copy the
> >>> format and size information in the class members as no reference can be
> >>> taken to the StreamConfiguration instances as they're subject to
> >>> relocations.
> >>
> >> I'm not sure we need to say all that, but could have just been "Pass the
> >> android camera3_stream_t, and a libcamera::StreamConfiguration to
> >> identify the source and destination parameters of this stream.
> >>
> >>> Pass to the CameraStream constructor a pointer to the CameraDevice class
> >>> and make the CameraConfiguration accessible. It will be used to retrieve
> >>> the StreamConfiguration associated with the CameraStream.
> >>
> >> Pass a CameraDevice pointer to the CameraStream constructor to allow
> >> retrieval of the StreamConfiguration associated with the CameraStream.
> >
> > Thanks, much better!
> >
> >>> Also change the format on which the CameraDevice performs checks to
> >>> decide if post-processing is required, as the libcamera facing format is
> >>> not meaningful anymore, but the Android required format should be used
> >>> instead.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >>> ---
> >>> src/android/camera_device.cpp | 11 +++++------
> >>> src/android/camera_device.h | 4 ++++
> >>> src/android/camera_stream.cpp | 14 ++++++++++++--
> >>> src/android/camera_stream.h | 18 ++++++++++++++++--
> >>> 4 files changed, 37 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >>> index 9c9a5cfa3c2f..2c4dd4dee28c 100644
> >>> --- a/src/android/camera_device.cpp
> >>> +++ b/src/android/camera_device.cpp
> >>> @@ -1216,8 +1216,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>>
> >>> config_->addConfiguration(streamConfiguration);
> >>> unsigned int index = config_->size() - 1;
> >>> - streams_.emplace_back(format, size, CameraStream::Type::Direct,
> >>> - index);
> >>> + streams_.emplace_back(this, stream, streamConfiguration,
> >>> + CameraStream::Type::Direct, index);
> >>> stream->priv = static_cast<void *>(&streams_.back());
> >>> }
> >>>
> >>> @@ -1272,8 +1272,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >>> }
> >>>
> >>> StreamConfiguration &cfg = config_->at(index);
> >>> -
> >>> - streams_.emplace_back(formats::MJPEG, cfg.size, type, index);
> >>> + streams_.emplace_back(this, jpegStream, cfg, type, index);
> >>> jpegStream->priv = static_cast<void *>(&streams_.back());
> >>> }
> >>>
> >>> @@ -1405,7 +1404,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >>> descriptor->buffers[i].buffer = camera3Buffers[i].buffer;
> >>>
> >>> /* Software streams are handled after hardware streams complete. */
> >>> - if (cameraStream->format() == formats::MJPEG)
> >>> + if (cameraStream->outputFormat() == HAL_PIXEL_FORMAT_BLOB)
> >>
> >> I understand the premise for this change, it's just so unfortunate that
> >> the android format is so ... well ... unrepresentative :-)
> >
> > More than this, I'm thinking of doing what I'll do with the libcamera
> > StreamConfiguration
> > if (cameraStream->androidStream().format == ... )
> >
> > And not provide a confusing CameraStream::outputFormat() helper at
> > all.
>
> Great, that looks good!
>
> >> I haven't gone to check yet, but I wonder if there is a better boolean
> >> logic to check ...
> >>
> >> if (format != NV12) perhaps ?
> >
> > We can't bet to have only NV12 and MJPEG
>
> No, indeed - my premise was that we will (later) need to make the
> decision somewhere about what a software stream is.
>
> I.e. - if the Android stream is an NV12, and we can only generate a
> YUYV, we will need a software(or opengl :D) format convertor (to support
> UVC).
>
> So there is going to be a more complex requirement here to decide what a
> software stream is, which will really be
Should we start calling them post-processed streams (we can bikeshed the
name), as the post-processing may be handled by hardware (hardware
scaler or format converter, JPEG encoder, GPU, ...) ?
> "input format != output format"...
>
> Anyway, this is fine for now.
>
> The internal buffer handling will in fact make it much easier to look at
> software format conversion anyway.
>
> >> I guess it depends on what other combinations we'll see through here.
> >>
> >> But no need to change this now
> >>
> >>> continue;
> >>>
> >>> /*
> >>> @@ -1469,7 +1468,7 @@ void CameraDevice::requestComplete(Request *request)
> >>> CameraStream *cameraStream =
> >>> static_cast<CameraStream *>(descriptor->buffers[i].stream->priv);
> >>>
> >>> - if (cameraStream->format() != formats::MJPEG)
> >>> + if (cameraStream->outputFormat() != HAL_PIXEL_FORMAT_BLOB)
> >>> continue;
> >>>
> >>> Encoder *encoder = cameraStream->encoder();
> >>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >>> index 52923ec979a7..4e326fa3e1fb 100644
> >>> --- a/src/android/camera_device.h
> >>> +++ b/src/android/camera_device.h
> >>> @@ -43,6 +43,10 @@ public:
> >>> unsigned int id() const { return id_; }
> >>> camera3_device_t *camera3Device() { return &camera3Device_; }
> >>> const libcamera::Camera *camera() const { return camera_.get(); }
> >>> + libcamera::CameraConfiguration *cameraConfiguration() const
> >>> + {
> >>> + return config_.get();
> >>> + }
> >>>
> >>> int facing() const { return facing_; }
> >>> int orientation() const { return orientation_; }
> >>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> >>> index 5b2b625c563b..5c1f1d7da416 100644
> >>> --- a/src/android/camera_stream.cpp
> >>> +++ b/src/android/camera_stream.cpp
> >>> @@ -7,13 +7,23 @@
> >>>
> >>> #include "camera_stream.h"
> >>>
> >>> +#include "camera_device.h"
> >>> #include "jpeg/encoder_libjpeg.h"
> >>>
> >>> using namespace libcamera;
> >>>
> >>> -CameraStream::CameraStream(PixelFormat format, Size size, Type type, unsigned int index)
> >>> - : format_(format), size_(size), type_(type), index_(index), encoder(nullptr)
> >>> +CameraStream::CameraStream(CameraDevice *cameraDev,
> >>> + camera3_stream_t *androidStream,
> >>> + const libcamera::StreamConfiguration &cfg,
> >>> + Type type, unsigned int index)
> >>> + : cameraDevice_(cameraDev), androidStream_(androidStream), type_(type),
> >>> + index_(index), encoder_(nullptr)
> >>> {
> >>> + config_ = cameraDevice_->cameraConfiguration();
> >>> +
> >>> + format_ = cfg.pixelFormat;
> >>> + size_ = cfg.size;
> >>> +
> >>> if (type_ == Type::Internal || type_ == Type::Mapped)
> >>> encoder_.reset(new EncoderLibJpeg);
> >>> }
> >>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> >>> index d0dc40d81151..2d71a50c78a4 100644
> >>> --- a/src/android/camera_stream.h
> >>> +++ b/src/android/camera_stream.h
> >>> @@ -9,11 +9,16 @@
> >>>
> >>> #include <memory>
> >>>
> >>> +#include <hardware/camera3.h>
> >>> +
> >>> +#include <libcamera/camera.h>
> >>> #include <libcamera/geometry.h>
> >>> #include <libcamera/pixel_format.h>
> >>>
> >>> #include "jpeg/encoder.h"
> >>>
> >>> +class CameraDevice;
> >>> +
> >>> class CameraStream
> >>> {
> >>> public:
> >>> @@ -99,9 +104,12 @@ public:
> >>> Internal,
> >>> Mapped,
> >>> };
> >>> - CameraStream(libcamera::PixelFormat format, libcamera::Size size,
> >>> + CameraStream(CameraDevice *cameraDev,
> >>
> >> I would have just used 'dev', or 'device', I don't think it conflicts
> >> with anything else in this function does it ?
> >>
> >> or as the class private member it goes into is cameraDevice_, it could
> >> have been cameraDevice ;-) - But it's not important either.
> >
> > I thought about shortening the name too, but as it goes to
> > cameraDevice_ I kept it long.
> >
> > And as Laurent said, if we could unify the naming schemes (ie.
> > everything android prefixed with camera3 and everything libcamera
> > prefixed with.... ) it would be nice
We could also use hal as a prefix for the android side to keep it
shorter.
> >
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > Thanks
> >
> >>> + camera3_stream_t *androidStream,
> >>> + const libcamera::StreamConfiguration &cfg,
> >>> Type type, unsigned int index);
> >>>
> >>> + int outputFormat() const { return androidStream_->format; }
> >>> const libcamera::PixelFormat &format() const { return format_; }
> >>> const libcamera::Size &size() const { return size_; }
> >>> Type type() const { return type_; }
> >>> @@ -111,9 +119,15 @@ public:
> >>> int configure(const libcamera::StreamConfiguration &cfg);
> >>>
> >>> private:
> >>> + CameraDevice *cameraDevice_;
> >>> + libcamera::CameraConfiguration *config_;
> >>> + camera3_stream_t *androidStream_;
> >>> + Type type_;
> >>> +
> >>> + /* Libcamera facing format and sizes. */
> >>> 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list