| Message ID | 20200902152236.69770-12-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote: > Make the CameraStream Encoder * a private struct member and require its > initialization at construction time. > > This change dis-allow creating a CameraStream and set the Encoder later, s/dis-allow/disallows/ > which shall not happen now that we create CameraStream once we have all > the required information in place. > > No functional changes intended but this change aims to make the code > more robust enforcing a stricter CameraStream interface. Thinking ahead in the future, do you think this will be a good match when we'll add other post-processing than JPEG encoding (I'm thinking about scaling, rotation and/or format conversion) ? I don't see any particular issue, but a second opinion would be useful. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/camera_device.cpp | 23 ++++++++++++----------- > src/android/camera_device.h | 7 ++++--- > 2 files changed, 16 insertions(+), 14 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 9bcd1d993c17..28d8e081eab0 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -168,14 +168,14 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > } > } > > -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i) > - : format(f), size(s), jpeg(nullptr), index_(i) > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder) > + : format(f), size(s), index_(i), encoder_(encoder) > { > } > > CameraStream::~CameraStream() > { > - delete jpeg; > + delete encoder_; > }; > > /* > @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > PixelFormat format = formats::MJPEG; > Size size = cfg.size; > > - CameraStream &cameraStream = streams_.emplace_back(format, size, index); > - stream->priv = static_cast<void *>(&cameraStream); > - > /* > * Construct a software encoder for MJPEG streams from the > * chosen libcamera source stream. > */ > - cameraStream.jpeg = new EncoderLibJpeg(); > - int ret = cameraStream.jpeg->configure(cfg); > + Encoder *encoder = new EncoderLibJpeg(); > + int ret = encoder->configure(cfg); > if (ret) { > - LOG(HAL, Error) > - << "Failed to configure encoder"; > + LOG(HAL, Error) << "Failed to configure encoder"; > + delete encoder; > return ret; > } > + > + CameraStream &cameraStream = streams_.emplace_back(format, size, > + index, encoder); > + stream->priv = static_cast<void *>(&cameraStream); > } > > switch (config_->validate()) { > @@ -1481,7 +1482,7 @@ void CameraDevice::requestComplete(Request *request) > if (cameraStream->format != formats::MJPEG) > continue; > > - Encoder *encoder = cameraStream->jpeg; > + Encoder *encoder = cameraStream->encoder(); > if (!encoder) { > LOG(HAL, Error) << "Failed to identify encoder"; > continue; > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index f8f237203ce9..3c57ffec265d 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -29,16 +29,16 @@ class CameraMetadata; > > struct CameraStream { > public: > - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i); > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i, > + Encoder *encoder = nullptr); > ~CameraStream(); > > unsigned int index() const { return index_; } > + Encoder *encoder() const { return encoder_; } > > libcamera::PixelFormat format; > libcamera::Size size; > > - Encoder *jpeg; > - > private: > /* > * The index of the libcamera StreamConfiguration as added during > @@ -46,6 +46,7 @@ private: > * one or more streams to the Android framework. > */ > unsigned int index_; > + Encoder *encoder_; > }; > > class CameraDevice : protected libcamera::Loggable
On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote: > > Make the CameraStream Encoder * a private struct member and require its > > initialization at construction time. > > > > This change dis-allow creating a CameraStream and set the Encoder later, > > s/dis-allow/disallows/ > > > which shall not happen now that we create CameraStream once we have all > > the required information in place. > > > > No functional changes intended but this change aims to make the code > > more robust enforcing a stricter CameraStream interface. > > Thinking ahead in the future, do you think this will be a good match > when we'll add other post-processing than JPEG encoding (I'm thinking > about scaling, rotation and/or format conversion) ? I don't see any > particular issue, but a second opinion would be useful. As discussed with Kieran and briefly with Tomasz, I think we'll have to define how to establish which Encoder (or whatever name it will be give) interface derived class to instantiate here. I think this part will need to be greatly expanded, and I'm not yet sure to which direction (I would likely say a factory of some kind, but I think establishing the criteria for selecting which derived class to use is more pressing than the implementation itself). Thanks j > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > src/android/camera_device.cpp | 23 ++++++++++++----------- > > src/android/camera_device.h | 7 ++++--- > > 2 files changed, 16 insertions(+), 14 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 9bcd1d993c17..28d8e081eab0 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -168,14 +168,14 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > > } > > } > > > > -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i) > > - : format(f), size(s), jpeg(nullptr), index_(i) > > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder) > > + : format(f), size(s), index_(i), encoder_(encoder) > > { > > } > > > > CameraStream::~CameraStream() > > { > > - delete jpeg; > > + delete encoder_; > > }; > > > > /* > > @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > PixelFormat format = formats::MJPEG; > > Size size = cfg.size; > > > > - CameraStream &cameraStream = streams_.emplace_back(format, size, index); > > - stream->priv = static_cast<void *>(&cameraStream); > > - > > /* > > * Construct a software encoder for MJPEG streams from the > > * chosen libcamera source stream. > > */ > > - cameraStream.jpeg = new EncoderLibJpeg(); > > - int ret = cameraStream.jpeg->configure(cfg); > > + Encoder *encoder = new EncoderLibJpeg(); > > + int ret = encoder->configure(cfg); > > if (ret) { > > - LOG(HAL, Error) > > - << "Failed to configure encoder"; > > + LOG(HAL, Error) << "Failed to configure encoder"; > > + delete encoder; > > return ret; > > } > > + > > + CameraStream &cameraStream = streams_.emplace_back(format, size, > > + index, encoder); > > + stream->priv = static_cast<void *>(&cameraStream); > > } > > > > switch (config_->validate()) { > > @@ -1481,7 +1482,7 @@ void CameraDevice::requestComplete(Request *request) > > if (cameraStream->format != formats::MJPEG) > > continue; > > > > - Encoder *encoder = cameraStream->jpeg; > > + Encoder *encoder = cameraStream->encoder(); > > if (!encoder) { > > LOG(HAL, Error) << "Failed to identify encoder"; > > continue; > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index f8f237203ce9..3c57ffec265d 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -29,16 +29,16 @@ class CameraMetadata; > > > > struct CameraStream { > > public: > > - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i); > > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i, > > + Encoder *encoder = nullptr); > > ~CameraStream(); > > > > unsigned int index() const { return index_; } > > + Encoder *encoder() const { return encoder_; } > > > > libcamera::PixelFormat format; > > libcamera::Size size; > > > > - Encoder *jpeg; > > - > > private: > > /* > > * The index of the libcamera StreamConfiguration as added during > > @@ -46,6 +46,7 @@ private: > > * one or more streams to the Android framework. > > */ > > unsigned int index_; > > + Encoder *encoder_; > > }; > > > > class CameraDevice : protected libcamera::Loggable > > -- > Regards, > > Laurent Pinchart
On Mon, Sep 7, 2020 at 5:52 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote: > > > Make the CameraStream Encoder * a private struct member and require its > > > initialization at construction time. > > > > > > This change dis-allow creating a CameraStream and set the Encoder later, > > > > s/dis-allow/disallows/ > > > > > which shall not happen now that we create CameraStream once we have all > > > the required information in place. > > > > > > No functional changes intended but this change aims to make the code > > > more robust enforcing a stricter CameraStream interface. > > > > Thinking ahead in the future, do you think this will be a good match > > when we'll add other post-processing than JPEG encoding (I'm thinking > > about scaling, rotation and/or format conversion) ? I don't see any > > particular issue, but a second opinion would be useful. > > As discussed with Kieran and briefly with Tomasz, I think we'll have > to define how to establish which Encoder (or whatever name it will > be give) interface derived class to instantiate here. > > I think this part will need to be greatly expanded, and I'm not yet > sure to which direction (I would likely say a factory of some kind, > but I think establishing the criteria for selecting which derived > class to use is more pressing than the implementation itself). > > Thanks > j > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > src/android/camera_device.cpp | 23 ++++++++++++----------- > > > src/android/camera_device.h | 7 ++++--- > > > 2 files changed, 16 insertions(+), 14 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 9bcd1d993c17..28d8e081eab0 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -168,14 +168,14 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > > > } > > > } > > > > > > -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i) > > > - : format(f), size(s), jpeg(nullptr), index_(i) > > > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder) > > > + : format(f), size(s), index_(i), encoder_(encoder) > > > { > > > } > > > > > > CameraStream::~CameraStream() > > > { > > > - delete jpeg; > > > + delete encoder_; > > > }; > > > > > > /* > > > @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > PixelFormat format = formats::MJPEG; > > > Size size = cfg.size; > > > > > > - CameraStream &cameraStream = streams_.emplace_back(format, size, index); > > > - stream->priv = static_cast<void *>(&cameraStream); > > > - > > > /* > > > * Construct a software encoder for MJPEG streams from the > > > * chosen libcamera source stream. > > > */ > > > - cameraStream.jpeg = new EncoderLibJpeg(); > > > - int ret = cameraStream.jpeg->configure(cfg); > > > + Encoder *encoder = new EncoderLibJpeg(); > > > + int ret = encoder->configure(cfg); > > > if (ret) { > > > - LOG(HAL, Error) > > > - << "Failed to configure encoder"; > > > + LOG(HAL, Error) << "Failed to configure encoder"; > > > + delete encoder; > > > return ret; > > > } > > > + > > > + CameraStream &cameraStream = streams_.emplace_back(format, size, > > > + index, encoder); > > > + stream->priv = static_cast<void *>(&cameraStream); > > > } > > > > > > switch (config_->validate()) { > > > @@ -1481,7 +1482,7 @@ void CameraDevice::requestComplete(Request *request) > > > if (cameraStream->format != formats::MJPEG) > > > continue; > > > > > > - Encoder *encoder = cameraStream->jpeg; > > > + Encoder *encoder = cameraStream->encoder(); > > > if (!encoder) { > > > LOG(HAL, Error) << "Failed to identify encoder"; > > > continue; > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > > index f8f237203ce9..3c57ffec265d 100644 > > > --- a/src/android/camera_device.h > > > +++ b/src/android/camera_device.h > > > @@ -29,16 +29,16 @@ class CameraMetadata; > > > > > > struct CameraStream { > > > public: > > > - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i); > > > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i, > > > + Encoder *encoder = nullptr); > > > ~CameraStream(); > > > > > > unsigned int index() const { return index_; } > > > + Encoder *encoder() const { return encoder_; } > > > > > > libcamera::PixelFormat format; > > > libcamera::Size size; > > > > > > - Encoder *jpeg; > > > - > > > private: > > > /* > > > * The index of the libcamera StreamConfiguration as added during > > > @@ -46,6 +46,7 @@ private: > > > * one or more streams to the Android framework. > > > */ > > > unsigned int index_; > > > + Encoder *encoder_; > > > }; > > > > > > class CameraDevice : protected libcamera::Loggable > > I would like to manage encoder_ with std::unique_ptr<> in CameraStream. > > -- > > Regards, > > > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Tue, Sep 8, 2020 at 2:21 PM Hirokazu Honda <hiroh@chromium.org> wrote: > > On Mon, Sep 7, 2020 at 5:52 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote: > > > Hi Jacopo, > > > > > > Thank you for the patch. > > > > > > On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote: > > > > Make the CameraStream Encoder * a private struct member and require its > > > > initialization at construction time. > > > > > > > > This change dis-allow creating a CameraStream and set the Encoder later, > > > > > > s/dis-allow/disallows/ > > > > > > > which shall not happen now that we create CameraStream once we have all > > > > the required information in place. > > > > > > > > No functional changes intended but this change aims to make the code > > > > more robust enforcing a stricter CameraStream interface. > > > > > > Thinking ahead in the future, do you think this will be a good match > > > when we'll add other post-processing than JPEG encoding (I'm thinking > > > about scaling, rotation and/or format conversion) ? I don't see any > > > particular issue, but a second opinion would be useful. > > > > As discussed with Kieran and briefly with Tomasz, I think we'll have > > to define how to establish which Encoder (or whatever name it will > > be give) interface derived class to instantiate here. > > I am thinking of introducing PostProcessor interface and regard JpegEncoder as one of frame conversions. I would remove the existing Encoder interface. PostProcessor +-- FrameConverter (for format conversion) | +-- JpegEncoder. | | | +-- JpegDecoder. | +-- FrameScaler (for frame scaling and cropping) +-- YUVScaler What do you think? > > I think this part will need to be greatly expanded, and I'm not yet > > sure to which direction (I would likely say a factory of some kind, > > but I think establishing the criteria for selecting which derived > > class to use is more pressing than the implementation itself). > > > > Thanks > > j > > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > --- > > > > src/android/camera_device.cpp | 23 ++++++++++++----------- > > > > src/android/camera_device.h | 7 ++++--- > > > > 2 files changed, 16 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index 9bcd1d993c17..28d8e081eab0 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -168,14 +168,14 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > > > > } > > > > } > > > > > > > > -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i) > > > > - : format(f), size(s), jpeg(nullptr), index_(i) > > > > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder) > > > > + : format(f), size(s), index_(i), encoder_(encoder) > > > > { > > > > } > > > > > > > > CameraStream::~CameraStream() > > > > { > > > > - delete jpeg; > > > > + delete encoder_; > > > > }; > > > > > > > > /* > > > > @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > > PixelFormat format = formats::MJPEG; > > > > Size size = cfg.size; > > > > > > > > - CameraStream &cameraStream = streams_.emplace_back(format, size, index); > > > > - stream->priv = static_cast<void *>(&cameraStream); > > > > - > > > > /* > > > > * Construct a software encoder for MJPEG streams from the > > > > * chosen libcamera source stream. > > > > */ > > > > - cameraStream.jpeg = new EncoderLibJpeg(); > > > > - int ret = cameraStream.jpeg->configure(cfg); > > > > + Encoder *encoder = new EncoderLibJpeg(); > > > > + int ret = encoder->configure(cfg); > > > > if (ret) { > > > > - LOG(HAL, Error) > > > > - << "Failed to configure encoder"; > > > > + LOG(HAL, Error) << "Failed to configure encoder"; > > > > + delete encoder; > > > > return ret; > > > > } > > > > + > > > > + CameraStream &cameraStream = streams_.emplace_back(format, size, > > > > + index, encoder); > > > > + stream->priv = static_cast<void *>(&cameraStream); > > > > } > > > > > > > > switch (config_->validate()) { > > > > @@ -1481,7 +1482,7 @@ void CameraDevice::requestComplete(Request *request) > > > > if (cameraStream->format != formats::MJPEG) > > > > continue; > > > > > > > > - Encoder *encoder = cameraStream->jpeg; > > > > + Encoder *encoder = cameraStream->encoder(); > > > > if (!encoder) { > > > > LOG(HAL, Error) << "Failed to identify encoder"; > > > > continue; > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > > > index f8f237203ce9..3c57ffec265d 100644 > > > > --- a/src/android/camera_device.h > > > > +++ b/src/android/camera_device.h > > > > @@ -29,16 +29,16 @@ class CameraMetadata; > > > > > > > > struct CameraStream { > > > > public: > > > > - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i); > > > > + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i, > > > > + Encoder *encoder = nullptr); > > > > ~CameraStream(); > > > > > > > > unsigned int index() const { return index_; } > > > > + Encoder *encoder() const { return encoder_; } > > > > > > > > libcamera::PixelFormat format; > > > > libcamera::Size size; > > > > > > > > - Encoder *jpeg; > > > > - > > > > private: > > > > /* > > > > * The index of the libcamera StreamConfiguration as added during > > > > @@ -46,6 +46,7 @@ private: > > > > * one or more streams to the Android framework. > > > > */ > > > > unsigned int index_; > > > > + Encoder *encoder_; > > > > }; > > > > > > > > class CameraDevice : protected libcamera::Loggable > > > > > I would like to manage encoder_ with std::unique_ptr<> in CameraStream. > > > > -- > > > Regards, > > > > > > Laurent Pinchart > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro-san, On Tue, Sep 08, 2020 at 02:59:06PM +0900, Hirokazu Honda wrote: > On Tue, Sep 8, 2020 at 2:21 PM Hirokazu Honda <hiroh@chromium.org> wrote: > > On Mon, Sep 7, 2020 at 5:52 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > >> On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote: > >>> Hi Jacopo, > >>> > >>> Thank you for the patch. > >>> > >>> On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote: > >>>> Make the CameraStream Encoder * a private struct member and require its > >>>> initialization at construction time. > >>>> > >>>> This change dis-allow creating a CameraStream and set the Encoder later, > >>> > >>> s/dis-allow/disallows/ > >>> > >>>> which shall not happen now that we create CameraStream once we have all > >>>> the required information in place. > >>>> > >>>> No functional changes intended but this change aims to make the code > >>>> more robust enforcing a stricter CameraStream interface. > >>> > >>> Thinking ahead in the future, do you think this will be a good match > >>> when we'll add other post-processing than JPEG encoding (I'm thinking > >>> about scaling, rotation and/or format conversion) ? I don't see any > >>> particular issue, but a second opinion would be useful. > >> > >> As discussed with Kieran and briefly with Tomasz, I think we'll have > >> to define how to establish which Encoder (or whatever name it will > >> be give) interface derived class to instantiate here. > > I am thinking of introducing PostProcessor interface and regard > JpegEncoder as one of frame conversions. > I would remove the existing Encoder interface. > > PostProcessor > +-- FrameConverter (for format conversion) > | +-- JpegEncoder. > | | > | +-- JpegDecoder. > | > +-- FrameScaler (for frame scaling and cropping) > +-- YUVScaler > > What do you think? I agree, I think all post-processing steps should have the same interface. I'm not sure if the intermediate FrameConverter and FrameScaler classes are needed though. It may be more efficient in some cases to perform both scaling and format conversion in one go (for instance if we use a JPEG hardware encoder that includes a scaler, or if we need to scale and convert from YUV to RGB). > >> I think this part will need to be greatly expanded, and I'm not yet > >> sure to which direction (I would likely say a factory of some kind, > >> but I think establishing the criteria for selecting which derived > >> class to use is more pressing than the implementation itself). > >> > >> Thanks > >> j > >> > >>> > >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> > >>>> --- > >>>> src/android/camera_device.cpp | 23 ++++++++++++----------- > >>>> src/android/camera_device.h | 7 ++++--- > >>>> 2 files changed, 16 insertions(+), 14 deletions(-) > >>>> > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >>>> index 9bcd1d993c17..28d8e081eab0 100644 > >>>> --- a/src/android/camera_device.cpp > >>>> +++ b/src/android/camera_device.cpp > >>>> @@ -168,14 +168,14 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > >>>> } > >>>> } > >>>> > >>>> -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i) > >>>> - : format(f), size(s), jpeg(nullptr), index_(i) > >>>> +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder) > >>>> + : format(f), size(s), index_(i), encoder_(encoder) > >>>> { > >>>> } > >>>> > >>>> CameraStream::~CameraStream() > >>>> { > >>>> - delete jpeg; > >>>> + delete encoder_; > >>>> }; > >>>> > >>>> /* > >>>> @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > >>>> PixelFormat format = formats::MJPEG; > >>>> Size size = cfg.size; > >>>> > >>>> - CameraStream &cameraStream = streams_.emplace_back(format, size, index); > >>>> - stream->priv = static_cast<void *>(&cameraStream); > >>>> - > >>>> /* > >>>> * Construct a software encoder for MJPEG streams from the > >>>> * chosen libcamera source stream. > >>>> */ > >>>> - cameraStream.jpeg = new EncoderLibJpeg(); > >>>> - int ret = cameraStream.jpeg->configure(cfg); > >>>> + Encoder *encoder = new EncoderLibJpeg(); > >>>> + int ret = encoder->configure(cfg); > >>>> if (ret) { > >>>> - LOG(HAL, Error) > >>>> - << "Failed to configure encoder"; > >>>> + LOG(HAL, Error) << "Failed to configure encoder"; > >>>> + delete encoder; > >>>> return ret; > >>>> } > >>>> + > >>>> + CameraStream &cameraStream = streams_.emplace_back(format, size, > >>>> + index, encoder); > >>>> + stream->priv = static_cast<void *>(&cameraStream); > >>>> } > >>>> > >>>> switch (config_->validate()) { > >>>> @@ -1481,7 +1482,7 @@ void CameraDevice::requestComplete(Request *request) > >>>> if (cameraStream->format != formats::MJPEG) > >>>> continue; > >>>> > >>>> - Encoder *encoder = cameraStream->jpeg; > >>>> + Encoder *encoder = cameraStream->encoder(); > >>>> if (!encoder) { > >>>> LOG(HAL, Error) << "Failed to identify encoder"; > >>>> continue; > >>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h > >>>> index f8f237203ce9..3c57ffec265d 100644 > >>>> --- a/src/android/camera_device.h > >>>> +++ b/src/android/camera_device.h > >>>> @@ -29,16 +29,16 @@ class CameraMetadata; > >>>> > >>>> struct CameraStream { > >>>> public: > >>>> - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i); > >>>> + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i, > >>>> + Encoder *encoder = nullptr); > >>>> ~CameraStream(); > >>>> > >>>> unsigned int index() const { return index_; } > >>>> + Encoder *encoder() const { return encoder_; } > >>>> > >>>> libcamera::PixelFormat format; > >>>> libcamera::Size size; > >>>> > >>>> - Encoder *jpeg; > >>>> - > >>>> private: > >>>> /* > >>>> * The index of the libcamera StreamConfiguration as added during > >>>> @@ -46,6 +46,7 @@ private: > >>>> * one or more streams to the Android framework. > >>>> */ > >>>> unsigned int index_; > >>>> + Encoder *encoder_; > >>>> }; > >>>> > >>>> class CameraDevice : protected libcamera::Loggable > >>> > > > > I would like to manage encoder_ with std::unique_ptr<> in CameraStream.
Hi Hiro, Laurent, On Tue, Sep 08, 2020 at 09:36:12AM +0300, Laurent Pinchart wrote: > Hi Hiro-san, > > On Tue, Sep 08, 2020 at 02:59:06PM +0900, Hirokazu Honda wrote: > > On Tue, Sep 8, 2020 at 2:21 PM Hirokazu Honda <hiroh@chromium.org> wrote: > > > On Mon, Sep 7, 2020 at 5:52 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > >> On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote: > > >>> Hi Jacopo, > > >>> > > >>> Thank you for the patch. > > >>> > > >>> On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote: > > >>>> Make the CameraStream Encoder * a private struct member and require its > > >>>> initialization at construction time. > > >>>> > > >>>> This change dis-allow creating a CameraStream and set the Encoder later, > > >>> > > >>> s/dis-allow/disallows/ > > >>> > > >>>> which shall not happen now that we create CameraStream once we have all > > >>>> the required information in place. > > >>>> > > >>>> No functional changes intended but this change aims to make the code > > >>>> more robust enforcing a stricter CameraStream interface. > > >>> > > >>> Thinking ahead in the future, do you think this will be a good match > > >>> when we'll add other post-processing than JPEG encoding (I'm thinking > > >>> about scaling, rotation and/or format conversion) ? I don't see any > > >>> particular issue, but a second opinion would be useful. > > >> > > >> As discussed with Kieran and briefly with Tomasz, I think we'll have > > >> to define how to establish which Encoder (or whatever name it will > > >> be give) interface derived class to instantiate here. > > > > I am thinking of introducing PostProcessor interface and regard > > JpegEncoder as one of frame conversions. > > I would remove the existing Encoder interface. > > > > PostProcessor > > +-- FrameConverter (for format conversion) > > | +-- JpegEncoder. > > | | > > | +-- JpegDecoder. > > | > > +-- FrameScaler (for frame scaling and cropping) > > +-- YUVScaler > > > > What do you think? > > I agree, I think all post-processing steps should have the same > interface. I'm not sure if the intermediate FrameConverter and > FrameScaler classes are needed though. It may be more efficient in some > cases to perform both scaling and format conversion in one go (for > instance if we use a JPEG hardware encoder that includes a scaler, or if > we need to scale and convert from YUV to RGB). I don't have an idea clear enough to express myself on the layout of the class hierarchy we'll end up implementing at this time. But I agree this is the place where we'll need to instantiate the right Encoder derived class, based on criteria which I like to define soon. Proposals are welcome, also based on the experience on how this is handled in CrOS. Thanks j > > > >> I think this part will need to be greatly expanded, and I'm not yet > > >> sure to which direction (I would likely say a factory of some kind, > > >> but I think establishing the criteria for selecting which derived > > >> class to use is more pressing than the implementation itself). > > >> > > >> Thanks > > >> j > > >> > > >>> > > >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >>> > > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >>> > > >>>> --- > > >>>> src/android/camera_device.cpp | 23 ++++++++++++----------- > > >>>> src/android/camera_device.h | 7 ++++--- > > >>>> 2 files changed, 16 insertions(+), 14 deletions(-) > > >>>> > > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > >>>> index 9bcd1d993c17..28d8e081eab0 100644 > > >>>> --- a/src/android/camera_device.cpp > > >>>> +++ b/src/android/camera_device.cpp > > >>>> @@ -168,14 +168,14 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > > >>>> } > > >>>> } > > >>>> > > >>>> -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i) > > >>>> - : format(f), size(s), jpeg(nullptr), index_(i) > > >>>> +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder) > > >>>> + : format(f), size(s), index_(i), encoder_(encoder) > > >>>> { > > >>>> } > > >>>> > > >>>> CameraStream::~CameraStream() > > >>>> { > > >>>> - delete jpeg; > > >>>> + delete encoder_; > > >>>> }; > > >>>> > > >>>> /* > > >>>> @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > >>>> PixelFormat format = formats::MJPEG; > > >>>> Size size = cfg.size; > > >>>> > > >>>> - CameraStream &cameraStream = streams_.emplace_back(format, size, index); > > >>>> - stream->priv = static_cast<void *>(&cameraStream); > > >>>> - > > >>>> /* > > >>>> * Construct a software encoder for MJPEG streams from the > > >>>> * chosen libcamera source stream. > > >>>> */ > > >>>> - cameraStream.jpeg = new EncoderLibJpeg(); > > >>>> - int ret = cameraStream.jpeg->configure(cfg); > > >>>> + Encoder *encoder = new EncoderLibJpeg(); > > >>>> + int ret = encoder->configure(cfg); > > >>>> if (ret) { > > >>>> - LOG(HAL, Error) > > >>>> - << "Failed to configure encoder"; > > >>>> + LOG(HAL, Error) << "Failed to configure encoder"; > > >>>> + delete encoder; > > >>>> return ret; > > >>>> } > > >>>> + > > >>>> + CameraStream &cameraStream = streams_.emplace_back(format, size, > > >>>> + index, encoder); > > >>>> + stream->priv = static_cast<void *>(&cameraStream); > > >>>> } > > >>>> > > >>>> switch (config_->validate()) { > > >>>> @@ -1481,7 +1482,7 @@ void CameraDevice::requestComplete(Request *request) > > >>>> if (cameraStream->format != formats::MJPEG) > > >>>> continue; > > >>>> > > >>>> - Encoder *encoder = cameraStream->jpeg; > > >>>> + Encoder *encoder = cameraStream->encoder(); > > >>>> if (!encoder) { > > >>>> LOG(HAL, Error) << "Failed to identify encoder"; > > >>>> continue; > > >>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > >>>> index f8f237203ce9..3c57ffec265d 100644 > > >>>> --- a/src/android/camera_device.h > > >>>> +++ b/src/android/camera_device.h > > >>>> @@ -29,16 +29,16 @@ class CameraMetadata; > > >>>> > > >>>> struct CameraStream { > > >>>> public: > > >>>> - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i); > > >>>> + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i, > > >>>> + Encoder *encoder = nullptr); > > >>>> ~CameraStream(); > > >>>> > > >>>> unsigned int index() const { return index_; } > > >>>> + Encoder *encoder() const { return encoder_; } > > >>>> > > >>>> libcamera::PixelFormat format; > > >>>> libcamera::Size size; > > >>>> > > >>>> - Encoder *jpeg; > > >>>> - > > >>>> private: > > >>>> /* > > >>>> * The index of the libcamera StreamConfiguration as added during > > >>>> @@ -46,6 +46,7 @@ private: > > >>>> * one or more streams to the Android framework. > > >>>> */ > > >>>> unsigned int index_; > > >>>> + Encoder *encoder_; > > >>>> }; > > >>>> > > >>>> class CameraDevice : protected libcamera::Loggable > > >>> > > > > > > I would like to manage encoder_ with std::unique_ptr<> in CameraStream. > > -- > Regards, > > Laurent Pinchart
On Tue, Sep 8, 2020 at 8:36 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro-san, > > On Tue, Sep 08, 2020 at 02:59:06PM +0900, Hirokazu Honda wrote: > > On Tue, Sep 8, 2020 at 2:21 PM Hirokazu Honda <hiroh@chromium.org> wrote: > > > On Mon, Sep 7, 2020 at 5:52 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > >> On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote: > > >>> Hi Jacopo, > > >>> > > >>> Thank you for the patch. > > >>> > > >>> On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote: > > >>>> Make the CameraStream Encoder * a private struct member and require its > > >>>> initialization at construction time. > > >>>> > > >>>> This change dis-allow creating a CameraStream and set the Encoder later, > > >>> > > >>> s/dis-allow/disallows/ > > >>> > > >>>> which shall not happen now that we create CameraStream once we have all > > >>>> the required information in place. > > >>>> > > >>>> No functional changes intended but this change aims to make the code > > >>>> more robust enforcing a stricter CameraStream interface. > > >>> > > >>> Thinking ahead in the future, do you think this will be a good match > > >>> when we'll add other post-processing than JPEG encoding (I'm thinking > > >>> about scaling, rotation and/or format conversion) ? I don't see any > > >>> particular issue, but a second opinion would be useful. > > >> > > >> As discussed with Kieran and briefly with Tomasz, I think we'll have > > >> to define how to establish which Encoder (or whatever name it will > > >> be give) interface derived class to instantiate here. > > > > I am thinking of introducing PostProcessor interface and regard > > JpegEncoder as one of frame conversions. > > I would remove the existing Encoder interface. > > > > PostProcessor > > +-- FrameConverter (for format conversion) > > | +-- JpegEncoder. > > | | > > | +-- JpegDecoder. > > | > > +-- FrameScaler (for frame scaling and cropping) > > +-- YUVScaler > > > > What do you think? > > I agree, I think all post-processing steps should have the same > interface. I'm not sure if the intermediate FrameConverter and > FrameScaler classes are needed though. It may be more efficient in some > cases to perform both scaling and format conversion in one go (for > instance if we use a JPEG hardware encoder that includes a scaler, or if > we need to scale and convert from YUV to RGB). FrameConverter and FrameScaler could be interfaces, so both could be implemented at the same time. On the other hand, if we end up with something like V4L2PostProcessor, the same class would likely implement support for post processors which could perform scale+conversion, scale only or conversion only, so maybe such distinction isn't necessary in the end. I guess we may need to dig deeper into the actual implementation idea to see what's the best choice. :) Best regards, Tomasz > > > >> I think this part will need to be greatly expanded, and I'm not yet > > >> sure to which direction (I would likely say a factory of some kind, > > >> but I think establishing the criteria for selecting which derived > > >> class to use is more pressing than the implementation itself). > > >> > > >> Thanks > > >> j > > >> > > >>> > > >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >>> > > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >>> > > >>>> --- > > >>>> src/android/camera_device.cpp | 23 ++++++++++++----------- > > >>>> src/android/camera_device.h | 7 ++++--- > > >>>> 2 files changed, 16 insertions(+), 14 deletions(-) > > >>>> > > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > >>>> index 9bcd1d993c17..28d8e081eab0 100644 > > >>>> --- a/src/android/camera_device.cpp > > >>>> +++ b/src/android/camera_device.cpp > > >>>> @@ -168,14 +168,14 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, > > >>>> } > > >>>> } > > >>>> > > >>>> -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i) > > >>>> - : format(f), size(s), jpeg(nullptr), index_(i) > > >>>> +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder) > > >>>> + : format(f), size(s), index_(i), encoder_(encoder) > > >>>> { > > >>>> } > > >>>> > > >>>> CameraStream::~CameraStream() > > >>>> { > > >>>> - delete jpeg; > > >>>> + delete encoder_; > > >>>> }; > > >>>> > > >>>> /* > > >>>> @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > >>>> PixelFormat format = formats::MJPEG; > > >>>> Size size = cfg.size; > > >>>> > > >>>> - CameraStream &cameraStream = streams_.emplace_back(format, size, index); > > >>>> - stream->priv = static_cast<void *>(&cameraStream); > > >>>> - > > >>>> /* > > >>>> * Construct a software encoder for MJPEG streams from the > > >>>> * chosen libcamera source stream. > > >>>> */ > > >>>> - cameraStream.jpeg = new EncoderLibJpeg(); > > >>>> - int ret = cameraStream.jpeg->configure(cfg); > > >>>> + Encoder *encoder = new EncoderLibJpeg(); > > >>>> + int ret = encoder->configure(cfg); > > >>>> if (ret) { > > >>>> - LOG(HAL, Error) > > >>>> - << "Failed to configure encoder"; > > >>>> + LOG(HAL, Error) << "Failed to configure encoder"; > > >>>> + delete encoder; > > >>>> return ret; > > >>>> } > > >>>> + > > >>>> + CameraStream &cameraStream = streams_.emplace_back(format, size, > > >>>> + index, encoder); > > >>>> + stream->priv = static_cast<void *>(&cameraStream); > > >>>> } > > >>>> > > >>>> switch (config_->validate()) { > > >>>> @@ -1481,7 +1482,7 @@ void CameraDevice::requestComplete(Request *request) > > >>>> if (cameraStream->format != formats::MJPEG) > > >>>> continue; > > >>>> > > >>>> - Encoder *encoder = cameraStream->jpeg; > > >>>> + Encoder *encoder = cameraStream->encoder(); > > >>>> if (!encoder) { > > >>>> LOG(HAL, Error) << "Failed to identify encoder"; > > >>>> continue; > > >>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > >>>> index f8f237203ce9..3c57ffec265d 100644 > > >>>> --- a/src/android/camera_device.h > > >>>> +++ b/src/android/camera_device.h > > >>>> @@ -29,16 +29,16 @@ class CameraMetadata; > > >>>> > > >>>> struct CameraStream { > > >>>> public: > > >>>> - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i); > > >>>> + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i, > > >>>> + Encoder *encoder = nullptr); > > >>>> ~CameraStream(); > > >>>> > > >>>> unsigned int index() const { return index_; } > > >>>> + Encoder *encoder() const { return encoder_; } > > >>>> > > >>>> libcamera::PixelFormat format; > > >>>> libcamera::Size size; > > >>>> > > >>>> - Encoder *jpeg; > > >>>> - > > >>>> private: > > >>>> /* > > >>>> * The index of the libcamera StreamConfiguration as added during > > >>>> @@ -46,6 +46,7 @@ private: > > >>>> * one or more streams to the Android framework. > > >>>> */ > > >>>> unsigned int index_; > > >>>> + Encoder *encoder_; > > >>>> }; > > >>>> > > >>>> class CameraDevice : protected libcamera::Loggable > > >>> > > > > > > I would like to manage encoder_ with std::unique_ptr<> in CameraStream. > > -- > Regards, > > Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 9bcd1d993c17..28d8e081eab0 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -168,14 +168,14 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer, } } -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i) - : format(f), size(s), jpeg(nullptr), index_(i) +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder) + : format(f), size(s), index_(i), encoder_(encoder) { } CameraStream::~CameraStream() { - delete jpeg; + delete encoder_; }; /* @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) PixelFormat format = formats::MJPEG; Size size = cfg.size; - CameraStream &cameraStream = streams_.emplace_back(format, size, index); - stream->priv = static_cast<void *>(&cameraStream); - /* * Construct a software encoder for MJPEG streams from the * chosen libcamera source stream. */ - cameraStream.jpeg = new EncoderLibJpeg(); - int ret = cameraStream.jpeg->configure(cfg); + Encoder *encoder = new EncoderLibJpeg(); + int ret = encoder->configure(cfg); if (ret) { - LOG(HAL, Error) - << "Failed to configure encoder"; + LOG(HAL, Error) << "Failed to configure encoder"; + delete encoder; return ret; } + + CameraStream &cameraStream = streams_.emplace_back(format, size, + index, encoder); + stream->priv = static_cast<void *>(&cameraStream); } switch (config_->validate()) { @@ -1481,7 +1482,7 @@ void CameraDevice::requestComplete(Request *request) if (cameraStream->format != formats::MJPEG) continue; - Encoder *encoder = cameraStream->jpeg; + Encoder *encoder = cameraStream->encoder(); if (!encoder) { LOG(HAL, Error) << "Failed to identify encoder"; continue; diff --git a/src/android/camera_device.h b/src/android/camera_device.h index f8f237203ce9..3c57ffec265d 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -29,16 +29,16 @@ class CameraMetadata; struct CameraStream { public: - CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i); + CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i, + Encoder *encoder = nullptr); ~CameraStream(); unsigned int index() const { return index_; } + Encoder *encoder() const { return encoder_; } libcamera::PixelFormat format; libcamera::Size size; - Encoder *jpeg; - private: /* * The index of the libcamera StreamConfiguration as added during @@ -46,6 +46,7 @@ private: * one or more streams to the Android framework. */ unsigned int index_; + Encoder *encoder_; }; class CameraDevice : protected libcamera::Loggable