| Message ID | 20210226132932.165484-8-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote: > In order to prepare to support more memory backends, make the > CameraBuffer class implement the PIMPL (pointer-to-implementation) > pattern. Is this the right design pattern for the job ? The pimpl/d-pointer pattern is used to hide the implementation from the interface of the class, mostly to help maintaining ABI compatibility. In this specific case, what you need is likely just an overload, with a base class defining virtual methods. Another option, if you want to simplify the implementation (I'm actually not entirely sure it would be simpler) is to define the interface in camera_buffer.h, and multiple implementations in different .cpp files, all named CameraBuffer. The .cpp file corresponding to the platform would then be selected at compilation time. The drawback is that you then would need to duplicate all functions in all implementations, even the ones that could be shared. > Define the CameraBuffer class interface whose actual implementation is > delegated to an inner CameraBufferImpl class. > > Temporary maintain libcamera::MappedBuffer as the CameraBuffer base > class to maintain compatibility of the CameraStream::process() interface > that requires a MappedBuffer * as second argument and will be converted > to use a CameraBuffer in the next patch. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_buffer.h | 12 ++++ > src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++- > 2 files changed, 91 insertions(+), 1 deletion(-) > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h > index 0590cd84652b..2a91e6a3c9c1 100644 > --- a/src/android/camera_buffer.h > +++ b/src/android/camera_buffer.h > @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer > public: > CameraBuffer(buffer_handle_t camera3Buffer, int flags); > ~CameraBuffer(); > + > + bool isValid() const; > + > + unsigned int numPlanes() const; > + ssize_t planeSize(unsigned int plane) const; > + > + const uint8_t *plane(unsigned int plane) const; > + uint8_t *plane(unsigned int plane); > + > +private: > + class CameraBufferImpl; > + CameraBufferImpl *impl_; > }; > > #endif /* __ANDROID_CAMERA_BUFFER_H__ */ > diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp > index 807304a9e42d..10a43a61bd4d 100644 > --- a/src/android/mm/android_generic_buffer.cpp > +++ b/src/android/mm/android_generic_buffer.cpp > @@ -13,7 +13,21 @@ using namespace libcamera; > > LOG_DECLARE_CATEGORY(HAL) > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer > +{ > +public: > + CameraBufferImpl(buffer_handle_t camera3Buffer, int flags); > + ~CameraBufferImpl(); > + > + unsigned int numPlanes() const; > + ssize_t planeSize(unsigned int plane) const; > + > + const uint8_t *plane(unsigned int plane) const; > + uint8_t *plane(unsigned int plane); > +}; > + > +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer, > + int flags) > { > maps_.reserve(camera3Buffer->numFds); > error_ = 0; > @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > } > } > > +CameraBuffer::CameraBufferImpl::~CameraBufferImpl() > +{ > +} > + > +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const > +{ > + return maps_.size(); > +} > + > +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const > +{ > + if (plane >= maps_.size()) > + return -EINVAL; > + > + return maps_[plane].size(); > +} > + > +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const > +{ > + if (plane >= maps_.size()) > + return nullptr; > + > + return maps_[plane].data(); > +} > + > +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) > +{ > + if (plane >= maps_.size()) > + return nullptr; > + > + return maps_[plane].data(); > +} > + > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > + : impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags)) > +{ > +} > + > CameraBuffer::~CameraBuffer() > { > + delete impl_; > +} > + > +bool CameraBuffer::isValid() const > +{ > + return impl_->isValid(); > +} > + > +unsigned int CameraBuffer::numPlanes() const > +{ > + return impl_->numPlanes(); > +} > + > +ssize_t CameraBuffer::planeSize(unsigned int plane) const > +{ > + return impl_->planeSize(plane); > +} > + > +const uint8_t *CameraBuffer::plane(unsigned int plane) const > +{ > + return impl_->plane(plane); > +} > + > +uint8_t *CameraBuffer::plane(unsigned int plane) > +{ > + return impl_->plane(plane); > }
Hi Jacopo, On Sun, Feb 28, 2021 at 08:36:23PM +0200, Laurent Pinchart wrote: > On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote: > > In order to prepare to support more memory backends, make the > > CameraBuffer class implement the PIMPL (pointer-to-implementation) > > pattern. > > Is this the right design pattern for the job ? The pimpl/d-pointer > pattern is used to hide the implementation from the interface of the > class, mostly to help maintaining ABI compatibility. In this specific > case, what you need is likely just an overload, with a base class > defining virtual methods. > > Another option, if you want to simplify the implementation (I'm actually > not entirely sure it would be simpler) is to define the interface in > camera_buffer.h, and multiple implementations in different .cpp files, > all named CameraBuffer. The .cpp file corresponding to the platform > would then be selected at compilation time. The drawback is that you > then would need to duplicate all functions in all implementations, even > the ones that could be shared. After reviewing the rest of the series I see that you can't expose the backend-specific private members in CameraBuffer, which explains why you're using this design pattern. You could keep using this pattern, with the CameraBufferImpl only storing the data, not duplicating every function. In that case I'd inherit from libcamera::Extensible instead of creating a manual implementation. The other option is to use a base class and multiple derived classes as mentioned above. We've discussed this previously and I agreed that we could just select which file to compile as a simple implementation, but I didn't realize then that we would need private data. I'm wondering whether the implementation wouldn't be simpler with inheritance than with private data. Sorry for not thinking about it in the beginning. > > Define the CameraBuffer class interface whose actual implementation is > > delegated to an inner CameraBufferImpl class. > > > > Temporary maintain libcamera::MappedBuffer as the CameraBuffer base > > class to maintain compatibility of the CameraStream::process() interface > > that requires a MappedBuffer * as second argument and will be converted > > to use a CameraBuffer in the next patch. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_buffer.h | 12 ++++ > > src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++- > > 2 files changed, 91 insertions(+), 1 deletion(-) > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h > > index 0590cd84652b..2a91e6a3c9c1 100644 > > --- a/src/android/camera_buffer.h > > +++ b/src/android/camera_buffer.h > > @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer > > public: > > CameraBuffer(buffer_handle_t camera3Buffer, int flags); > > ~CameraBuffer(); > > + > > + bool isValid() const; > > + > > + unsigned int numPlanes() const; > > + ssize_t planeSize(unsigned int plane) const; > > + > > + const uint8_t *plane(unsigned int plane) const; > > + uint8_t *plane(unsigned int plane); > > + > > +private: > > + class CameraBufferImpl; > > + CameraBufferImpl *impl_; > > }; > > > > #endif /* __ANDROID_CAMERA_BUFFER_H__ */ > > diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp > > index 807304a9e42d..10a43a61bd4d 100644 > > --- a/src/android/mm/android_generic_buffer.cpp > > +++ b/src/android/mm/android_generic_buffer.cpp > > @@ -13,7 +13,21 @@ using namespace libcamera; > > > > LOG_DECLARE_CATEGORY(HAL) > > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer > > +{ > > +public: > > + CameraBufferImpl(buffer_handle_t camera3Buffer, int flags); > > + ~CameraBufferImpl(); > > + > > + unsigned int numPlanes() const; > > + ssize_t planeSize(unsigned int plane) const; > > + > > + const uint8_t *plane(unsigned int plane) const; > > + uint8_t *plane(unsigned int plane); > > +}; > > + > > +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer, > > + int flags) > > { > > maps_.reserve(camera3Buffer->numFds); > > error_ = 0; > > @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > } > > } > > > > +CameraBuffer::CameraBufferImpl::~CameraBufferImpl() > > +{ > > +} > > + > > +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const > > +{ > > + return maps_.size(); > > +} > > + > > +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const > > +{ > > + if (plane >= maps_.size()) > > + return -EINVAL; > > + > > + return maps_[plane].size(); > > +} > > + > > +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const > > +{ > > + if (plane >= maps_.size()) > > + return nullptr; > > + > > + return maps_[plane].data(); > > +} > > + > > +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) > > +{ > > + if (plane >= maps_.size()) > > + return nullptr; > > + > > + return maps_[plane].data(); > > +} > > + > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > + : impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags)) > > +{ > > +} > > + > > CameraBuffer::~CameraBuffer() > > { > > + delete impl_; > > +} > > + > > +bool CameraBuffer::isValid() const > > +{ > > + return impl_->isValid(); > > +} > > + > > +unsigned int CameraBuffer::numPlanes() const > > +{ > > + return impl_->numPlanes(); > > +} > > + > > +ssize_t CameraBuffer::planeSize(unsigned int plane) const > > +{ > > + return impl_->planeSize(plane); > > +} > > + > > +const uint8_t *CameraBuffer::plane(unsigned int plane) const > > +{ > > + return impl_->plane(plane); > > +} > > + > > +uint8_t *CameraBuffer::plane(unsigned int plane) > > +{ > > + return impl_->plane(plane); > > }
Hi Laurent, On Sun, Feb 28, 2021 at 09:10:21PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Sun, Feb 28, 2021 at 08:36:23PM +0200, Laurent Pinchart wrote: > > On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote: > > > In order to prepare to support more memory backends, make the > > > CameraBuffer class implement the PIMPL (pointer-to-implementation) > > > pattern. > > > > Is this the right design pattern for the job ? The pimpl/d-pointer > > pattern is used to hide the implementation from the interface of the > > class, mostly to help maintaining ABI compatibility. In this specific > > case, what you need is likely just an overload, with a base class > > defining virtual methods. > > > > Another option, if you want to simplify the implementation (I'm actually > > not entirely sure it would be simpler) is to define the interface in > > camera_buffer.h, and multiple implementations in different .cpp files, > > all named CameraBuffer. The .cpp file corresponding to the platform > > would then be selected at compilation time. The drawback is that you > > then would need to duplicate all functions in all implementations, even > > the ones that could be shared. > > After reviewing the rest of the series I see that you can't expose the > backend-specific private members in CameraBuffer, which explains why > you're using this design pattern. That's exactly the reason why I went in that directin > > You could keep using this pattern, with the CameraBufferImpl only > storing the data, not duplicating every function. In that case I'd > inherit from libcamera::Extensible instead of creating a manual > implementation. > > The other option is to use a base class and multiple derived classes as > mentioned above. We've discussed this previously and I agreed that we > could just select which file to compile as a simple implementation, but > I didn't realize then that we would need private data. I'm wondering > whether the implementation wouldn't be simpler with inheritance than > with private data. Sorry for not thinking about it in the beginning. I didn't realize that in the beginning as well. For the cros backend we could get away by getting an instance of the cros::CameraBufferManager through the static singleton constructor in every function, by changing the interface and adding the current buffer as parameter, but that would make the whole hierachy stateless, something I am not sure we can guarantee for all the backends we'll have to implement. I would so refrain from tying our hands, so using a pointer to the actual implementation seems the less risky way. I'll see how Extensible looks like and send a v2. Thanks for review j > > > > Define the CameraBuffer class interface whose actual implementation is > > > delegated to an inner CameraBufferImpl class. > > > > > > Temporary maintain libcamera::MappedBuffer as the CameraBuffer base > > > class to maintain compatibility of the CameraStream::process() interface > > > that requires a MappedBuffer * as second argument and will be converted > > > to use a CameraBuffer in the next patch. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/android/camera_buffer.h | 12 ++++ > > > src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++- > > > 2 files changed, 91 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h > > > index 0590cd84652b..2a91e6a3c9c1 100644 > > > --- a/src/android/camera_buffer.h > > > +++ b/src/android/camera_buffer.h > > > @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer > > > public: > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags); > > > ~CameraBuffer(); > > > + > > > + bool isValid() const; > > > + > > > + unsigned int numPlanes() const; > > > + ssize_t planeSize(unsigned int plane) const; > > > + > > > + const uint8_t *plane(unsigned int plane) const; > > > + uint8_t *plane(unsigned int plane); > > > + > > > +private: > > > + class CameraBufferImpl; > > > + CameraBufferImpl *impl_; > > > }; > > > > > > #endif /* __ANDROID_CAMERA_BUFFER_H__ */ > > > diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp > > > index 807304a9e42d..10a43a61bd4d 100644 > > > --- a/src/android/mm/android_generic_buffer.cpp > > > +++ b/src/android/mm/android_generic_buffer.cpp > > > @@ -13,7 +13,21 @@ using namespace libcamera; > > > > > > LOG_DECLARE_CATEGORY(HAL) > > > > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > > +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer > > > +{ > > > +public: > > > + CameraBufferImpl(buffer_handle_t camera3Buffer, int flags); > > > + ~CameraBufferImpl(); > > > + > > > + unsigned int numPlanes() const; > > > + ssize_t planeSize(unsigned int plane) const; > > > + > > > + const uint8_t *plane(unsigned int plane) const; > > > + uint8_t *plane(unsigned int plane); > > > +}; > > > + > > > +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer, > > > + int flags) > > > { > > > maps_.reserve(camera3Buffer->numFds); > > > error_ = 0; > > > @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > > } > > > } > > > > > > +CameraBuffer::CameraBufferImpl::~CameraBufferImpl() > > > +{ > > > +} > > > + > > > +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const > > > +{ > > > + return maps_.size(); > > > +} > > > + > > > +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const > > > +{ > > > + if (plane >= maps_.size()) > > > + return -EINVAL; > > > + > > > + return maps_[plane].size(); > > > +} > > > + > > > +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const > > > +{ > > > + if (plane >= maps_.size()) > > > + return nullptr; > > > + > > > + return maps_[plane].data(); > > > +} > > > + > > > +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) > > > +{ > > > + if (plane >= maps_.size()) > > > + return nullptr; > > > + > > > + return maps_[plane].data(); > > > +} > > > + > > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > > + : impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags)) > > > +{ > > > +} > > > + > > > CameraBuffer::~CameraBuffer() > > > { > > > + delete impl_; > > > +} > > > + > > > +bool CameraBuffer::isValid() const > > > +{ > > > + return impl_->isValid(); > > > +} > > > + > > > +unsigned int CameraBuffer::numPlanes() const > > > +{ > > > + return impl_->numPlanes(); > > > +} > > > + > > > +ssize_t CameraBuffer::planeSize(unsigned int plane) const > > > +{ > > > + return impl_->planeSize(plane); > > > +} > > > + > > > +const uint8_t *CameraBuffer::plane(unsigned int plane) const > > > +{ > > > + return impl_->plane(plane); > > > +} > > > + > > > +uint8_t *CameraBuffer::plane(unsigned int plane) > > > +{ > > > + return impl_->plane(plane); > > > } > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Mar 01, 2021 at 10:02:08AM +0100, Jacopo Mondi wrote: > On Sun, Feb 28, 2021 at 09:10:21PM +0200, Laurent Pinchart wrote: > > On Sun, Feb 28, 2021 at 08:36:23PM +0200, Laurent Pinchart wrote: > > > On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote: > > > > In order to prepare to support more memory backends, make the > > > > CameraBuffer class implement the PIMPL (pointer-to-implementation) > > > > pattern. > > > > > > Is this the right design pattern for the job ? The pimpl/d-pointer > > > pattern is used to hide the implementation from the interface of the > > > class, mostly to help maintaining ABI compatibility. In this specific > > > case, what you need is likely just an overload, with a base class > > > defining virtual methods. > > > > > > Another option, if you want to simplify the implementation (I'm actually > > > not entirely sure it would be simpler) is to define the interface in > > > camera_buffer.h, and multiple implementations in different .cpp files, > > > all named CameraBuffer. The .cpp file corresponding to the platform > > > would then be selected at compilation time. The drawback is that you > > > then would need to duplicate all functions in all implementations, even > > > the ones that could be shared. > > > > After reviewing the rest of the series I see that you can't expose the > > backend-specific private members in CameraBuffer, which explains why > > you're using this design pattern. > > That's exactly the reason why I went in that directin > > > You could keep using this pattern, with the CameraBufferImpl only > > storing the data, not duplicating every function. In that case I'd > > inherit from libcamera::Extensible instead of creating a manual > > implementation. > > > > The other option is to use a base class and multiple derived classes as > > mentioned above. We've discussed this previously and I agreed that we > > could just select which file to compile as a simple implementation, but > > I didn't realize then that we would need private data. I'm wondering > > whether the implementation wouldn't be simpler with inheritance than > > with private data. Sorry for not thinking about it in the beginning. > > I didn't realize that in the beginning as well. > > For the cros backend we could get away by getting an instance of the > cros::CameraBufferManager through the static singleton constructor in > every function, by changing the interface and adding the current > buffer as parameter, but that would make the whole hierachy stateless, > something I am not sure we can guarantee for all the backends we'll > have to implement. I would so refrain from tying our hands, Agreed. > so using a pointer to the actual implementation seems the less risky > way. That I'm not sure about :-) You can store any amount of custom data in a derived class, so a base abstract class for the interface plus derived classes that implement that interface could work fine too. An implementation based on Extensible could be fine too, please just don't rule the base + derived option out yet in your investigations. > I'll see how Extensible looks like and send a v2. > > > > > Define the CameraBuffer class interface whose actual implementation is > > > > delegated to an inner CameraBufferImpl class. > > > > > > > > Temporary maintain libcamera::MappedBuffer as the CameraBuffer base > > > > class to maintain compatibility of the CameraStream::process() interface > > > > that requires a MappedBuffer * as second argument and will be converted > > > > to use a CameraBuffer in the next patch. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/android/camera_buffer.h | 12 ++++ > > > > src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++- > > > > 2 files changed, 91 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h > > > > index 0590cd84652b..2a91e6a3c9c1 100644 > > > > --- a/src/android/camera_buffer.h > > > > +++ b/src/android/camera_buffer.h > > > > @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer > > > > public: > > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags); > > > > ~CameraBuffer(); > > > > + > > > > + bool isValid() const; > > > > + > > > > + unsigned int numPlanes() const; > > > > + ssize_t planeSize(unsigned int plane) const; > > > > + > > > > + const uint8_t *plane(unsigned int plane) const; > > > > + uint8_t *plane(unsigned int plane); > > > > + > > > > +private: > > > > + class CameraBufferImpl; > > > > + CameraBufferImpl *impl_; > > > > }; > > > > > > > > #endif /* __ANDROID_CAMERA_BUFFER_H__ */ > > > > diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp > > > > index 807304a9e42d..10a43a61bd4d 100644 > > > > --- a/src/android/mm/android_generic_buffer.cpp > > > > +++ b/src/android/mm/android_generic_buffer.cpp > > > > @@ -13,7 +13,21 @@ using namespace libcamera; > > > > > > > > LOG_DECLARE_CATEGORY(HAL) > > > > > > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > > > +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer > > > > +{ > > > > +public: > > > > + CameraBufferImpl(buffer_handle_t camera3Buffer, int flags); > > > > + ~CameraBufferImpl(); > > > > + > > > > + unsigned int numPlanes() const; > > > > + ssize_t planeSize(unsigned int plane) const; > > > > + > > > > + const uint8_t *plane(unsigned int plane) const; > > > > + uint8_t *plane(unsigned int plane); > > > > +}; > > > > + > > > > +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer, > > > > + int flags) > > > > { > > > > maps_.reserve(camera3Buffer->numFds); > > > > error_ = 0; > > > > @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > > > } > > > > } > > > > > > > > +CameraBuffer::CameraBufferImpl::~CameraBufferImpl() > > > > +{ > > > > +} > > > > + > > > > +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const > > > > +{ > > > > + return maps_.size(); > > > > +} > > > > + > > > > +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const > > > > +{ > > > > + if (plane >= maps_.size()) > > > > + return -EINVAL; > > > > + > > > > + return maps_[plane].size(); > > > > +} > > > > + > > > > +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const > > > > +{ > > > > + if (plane >= maps_.size()) > > > > + return nullptr; > > > > + > > > > + return maps_[plane].data(); > > > > +} > > > > + > > > > +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) > > > > +{ > > > > + if (plane >= maps_.size()) > > > > + return nullptr; > > > > + > > > > + return maps_[plane].data(); > > > > +} > > > > + > > > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > > > + : impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags)) > > > > +{ > > > > +} > > > > + > > > > CameraBuffer::~CameraBuffer() > > > > { > > > > + delete impl_; > > > > +} > > > > + > > > > +bool CameraBuffer::isValid() const > > > > +{ > > > > + return impl_->isValid(); > > > > +} > > > > + > > > > +unsigned int CameraBuffer::numPlanes() const > > > > +{ > > > > + return impl_->numPlanes(); > > > > +} > > > > + > > > > +ssize_t CameraBuffer::planeSize(unsigned int plane) const > > > > +{ > > > > + return impl_->planeSize(plane); > > > > +} > > > > + > > > > +const uint8_t *CameraBuffer::plane(unsigned int plane) const > > > > +{ > > > > + return impl_->plane(plane); > > > > +} > > > > + > > > > +uint8_t *CameraBuffer::plane(unsigned int plane) > > > > +{ > > > > + return impl_->plane(plane); > > > > }
Hi Laurent, On Mon, Mar 01, 2021 at 11:24:22AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Mon, Mar 01, 2021 at 10:02:08AM +0100, Jacopo Mondi wrote: > > On Sun, Feb 28, 2021 at 09:10:21PM +0200, Laurent Pinchart wrote: > > > On Sun, Feb 28, 2021 at 08:36:23PM +0200, Laurent Pinchart wrote: > > > > On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote: > > > > > In order to prepare to support more memory backends, make the > > > > > CameraBuffer class implement the PIMPL (pointer-to-implementation) > > > > > pattern. > > > > > > > > Is this the right design pattern for the job ? The pimpl/d-pointer > > > > pattern is used to hide the implementation from the interface of the > > > > class, mostly to help maintaining ABI compatibility. In this specific > > > > case, what you need is likely just an overload, with a base class > > > > defining virtual methods. > > > > > > > > Another option, if you want to simplify the implementation (I'm actually > > > > not entirely sure it would be simpler) is to define the interface in > > > > camera_buffer.h, and multiple implementations in different .cpp files, > > > > all named CameraBuffer. The .cpp file corresponding to the platform > > > > would then be selected at compilation time. The drawback is that you > > > > then would need to duplicate all functions in all implementations, even > > > > the ones that could be shared. > > > > > > After reviewing the rest of the series I see that you can't expose the > > > backend-specific private members in CameraBuffer, which explains why > > > you're using this design pattern. > > > > That's exactly the reason why I went in that directin > > > > > You could keep using this pattern, with the CameraBufferImpl only > > > storing the data, not duplicating every function. In that case I'd > > > inherit from libcamera::Extensible instead of creating a manual > > > implementation. > > > > > > The other option is to use a base class and multiple derived classes as > > > mentioned above. We've discussed this previously and I agreed that we > > > could just select which file to compile as a simple implementation, but > > > I didn't realize then that we would need private data. I'm wondering > > > whether the implementation wouldn't be simpler with inheritance than > > > with private data. Sorry for not thinking about it in the beginning. > > > > I didn't realize that in the beginning as well. > > > > For the cros backend we could get away by getting an instance of the > > cros::CameraBufferManager through the static singleton constructor in > > every function, by changing the interface and adding the current > > buffer as parameter, but that would make the whole hierachy stateless, > > something I am not sure we can guarantee for all the backends we'll > > have to implement. I would so refrain from tying our hands, > > Agreed. > > > so using a pointer to the actual implementation seems the less risky > > way. > > That I'm not sure about :-) You can store any amount of custom data in a > derived class, so a base abstract class for the interface plus derived > classes that implement that interface could work fine too. Yes, but then what instantiating the right derived class using a compile-time option would require a factory or a chain of #ifdef However, if we compile in a single dervide class at the time, we can instantiate a CameraBuffer unconditionally ? I'll try this one as well, it might work and be simpler than using Extensible. > > An implementation based on Extensible could be fine too, please just > don't rule the base + derived option out yet in your investigations. > > > I'll see how Extensible looks like and send a v2. > > > > > > > Define the CameraBuffer class interface whose actual implementation is > > > > > delegated to an inner CameraBufferImpl class. > > > > > > > > > > Temporary maintain libcamera::MappedBuffer as the CameraBuffer base > > > > > class to maintain compatibility of the CameraStream::process() interface > > > > > that requires a MappedBuffer * as second argument and will be converted > > > > > to use a CameraBuffer in the next patch. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > --- > > > > > src/android/camera_buffer.h | 12 ++++ > > > > > src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++- > > > > > 2 files changed, 91 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h > > > > > index 0590cd84652b..2a91e6a3c9c1 100644 > > > > > --- a/src/android/camera_buffer.h > > > > > +++ b/src/android/camera_buffer.h > > > > > @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer > > > > > public: > > > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags); > > > > > ~CameraBuffer(); > > > > > + > > > > > + bool isValid() const; > > > > > + > > > > > + unsigned int numPlanes() const; > > > > > + ssize_t planeSize(unsigned int plane) const; > > > > > + > > > > > + const uint8_t *plane(unsigned int plane) const; > > > > > + uint8_t *plane(unsigned int plane); > > > > > + > > > > > +private: > > > > > + class CameraBufferImpl; > > > > > + CameraBufferImpl *impl_; > > > > > }; > > > > > > > > > > #endif /* __ANDROID_CAMERA_BUFFER_H__ */ > > > > > diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp > > > > > index 807304a9e42d..10a43a61bd4d 100644 > > > > > --- a/src/android/mm/android_generic_buffer.cpp > > > > > +++ b/src/android/mm/android_generic_buffer.cpp > > > > > @@ -13,7 +13,21 @@ using namespace libcamera; > > > > > > > > > > LOG_DECLARE_CATEGORY(HAL) > > > > > > > > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > > > > +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer > > > > > +{ > > > > > +public: > > > > > + CameraBufferImpl(buffer_handle_t camera3Buffer, int flags); > > > > > + ~CameraBufferImpl(); > > > > > + > > > > > + unsigned int numPlanes() const; > > > > > + ssize_t planeSize(unsigned int plane) const; > > > > > + > > > > > + const uint8_t *plane(unsigned int plane) const; > > > > > + uint8_t *plane(unsigned int plane); > > > > > +}; > > > > > + > > > > > +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer, > > > > > + int flags) > > > > > { > > > > > maps_.reserve(camera3Buffer->numFds); > > > > > error_ = 0; > > > > > @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > > > > } > > > > > } > > > > > > > > > > +CameraBuffer::CameraBufferImpl::~CameraBufferImpl() > > > > > +{ > > > > > +} > > > > > + > > > > > +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const > > > > > +{ > > > > > + return maps_.size(); > > > > > +} > > > > > + > > > > > +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const > > > > > +{ > > > > > + if (plane >= maps_.size()) > > > > > + return -EINVAL; > > > > > + > > > > > + return maps_[plane].size(); > > > > > +} > > > > > + > > > > > +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const > > > > > +{ > > > > > + if (plane >= maps_.size()) > > > > > + return nullptr; > > > > > + > > > > > + return maps_[plane].data(); > > > > > +} > > > > > + > > > > > +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) > > > > > +{ > > > > > + if (plane >= maps_.size()) > > > > > + return nullptr; > > > > > + > > > > > + return maps_[plane].data(); > > > > > +} > > > > > + > > > > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > > > > + : impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags)) > > > > > +{ > > > > > +} > > > > > + > > > > > CameraBuffer::~CameraBuffer() > > > > > { > > > > > + delete impl_; > > > > > +} > > > > > + > > > > > +bool CameraBuffer::isValid() const > > > > > +{ > > > > > + return impl_->isValid(); > > > > > +} > > > > > + > > > > > +unsigned int CameraBuffer::numPlanes() const > > > > > +{ > > > > > + return impl_->numPlanes(); > > > > > +} > > > > > + > > > > > +ssize_t CameraBuffer::planeSize(unsigned int plane) const > > > > > +{ > > > > > + return impl_->planeSize(plane); > > > > > +} > > > > > + > > > > > +const uint8_t *CameraBuffer::plane(unsigned int plane) const > > > > > +{ > > > > > + return impl_->plane(plane); > > > > > +} > > > > > + > > > > > +uint8_t *CameraBuffer::plane(unsigned int plane) > > > > > +{ > > > > > + return impl_->plane(plane); > > > > > } > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Mar 01, 2021 at 10:40:13AM +0100, Jacopo Mondi wrote: > On Mon, Mar 01, 2021 at 11:24:22AM +0200, Laurent Pinchart wrote: > > On Mon, Mar 01, 2021 at 10:02:08AM +0100, Jacopo Mondi wrote: > > > On Sun, Feb 28, 2021 at 09:10:21PM +0200, Laurent Pinchart wrote: > > > > On Sun, Feb 28, 2021 at 08:36:23PM +0200, Laurent Pinchart wrote: > > > > > On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote: > > > > > > In order to prepare to support more memory backends, make the > > > > > > CameraBuffer class implement the PIMPL (pointer-to-implementation) > > > > > > pattern. > > > > > > > > > > Is this the right design pattern for the job ? The pimpl/d-pointer > > > > > pattern is used to hide the implementation from the interface of the > > > > > class, mostly to help maintaining ABI compatibility. In this specific > > > > > case, what you need is likely just an overload, with a base class > > > > > defining virtual methods. > > > > > > > > > > Another option, if you want to simplify the implementation (I'm actually > > > > > not entirely sure it would be simpler) is to define the interface in > > > > > camera_buffer.h, and multiple implementations in different .cpp files, > > > > > all named CameraBuffer. The .cpp file corresponding to the platform > > > > > would then be selected at compilation time. The drawback is that you > > > > > then would need to duplicate all functions in all implementations, even > > > > > the ones that could be shared. > > > > > > > > After reviewing the rest of the series I see that you can't expose the > > > > backend-specific private members in CameraBuffer, which explains why > > > > you're using this design pattern. > > > > > > That's exactly the reason why I went in that directin > > > > > > > You could keep using this pattern, with the CameraBufferImpl only > > > > storing the data, not duplicating every function. In that case I'd > > > > inherit from libcamera::Extensible instead of creating a manual > > > > implementation. > > > > > > > > The other option is to use a base class and multiple derived classes as > > > > mentioned above. We've discussed this previously and I agreed that we > > > > could just select which file to compile as a simple implementation, but > > > > I didn't realize then that we would need private data. I'm wondering > > > > whether the implementation wouldn't be simpler with inheritance than > > > > with private data. Sorry for not thinking about it in the beginning. > > > > > > I didn't realize that in the beginning as well. > > > > > > For the cros backend we could get away by getting an instance of the > > > cros::CameraBufferManager through the static singleton constructor in > > > every function, by changing the interface and adding the current > > > buffer as parameter, but that would make the whole hierachy stateless, > > > something I am not sure we can guarantee for all the backends we'll > > > have to implement. I would so refrain from tying our hands, > > > > Agreed. > > > > > so using a pointer to the actual implementation seems the less risky > > > way. > > > > That I'm not sure about :-) You can store any amount of custom data in a > > derived class, so a base abstract class for the interface plus derived > > classes that implement that interface could work fine too. > > Yes, but then what instantiating the right derived class using a > compile-time option would require a factory or a chain of #ifdef Yes. I'd go for #ifdef (at least for now) for simplicity I think. > However, if we compile in a single dervide class at the time, we can > instantiate a CameraBuffer unconditionally ? I'll try this one as > well, it might work and be simpler than using Extensible. If we have multiple implementations of CameraBuffer, without a base class, then we can instantiate CameraBuffer unconditionally, but we need private data that would be different depending on the implementation, and so we'll need Extensible. Otherwise, with a base class, we need to instantiate the right derived class (using #ifdef), but we won't need Extensible. Up to you :-) > > An implementation based on Extensible could be fine too, please just > > don't rule the base + derived option out yet in your investigations. > > > > > I'll see how Extensible looks like and send a v2. > > > > > > > > > Define the CameraBuffer class interface whose actual implementation is > > > > > > delegated to an inner CameraBufferImpl class. > > > > > > > > > > > > Temporary maintain libcamera::MappedBuffer as the CameraBuffer base > > > > > > class to maintain compatibility of the CameraStream::process() interface > > > > > > that requires a MappedBuffer * as second argument and will be converted > > > > > > to use a CameraBuffer in the next patch. > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > --- > > > > > > src/android/camera_buffer.h | 12 ++++ > > > > > > src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++- > > > > > > 2 files changed, 91 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h > > > > > > index 0590cd84652b..2a91e6a3c9c1 100644 > > > > > > --- a/src/android/camera_buffer.h > > > > > > +++ b/src/android/camera_buffer.h > > > > > > @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer > > > > > > public: > > > > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags); > > > > > > ~CameraBuffer(); > > > > > > + > > > > > > + bool isValid() const; > > > > > > + > > > > > > + unsigned int numPlanes() const; > > > > > > + ssize_t planeSize(unsigned int plane) const; > > > > > > + > > > > > > + const uint8_t *plane(unsigned int plane) const; > > > > > > + uint8_t *plane(unsigned int plane); > > > > > > + > > > > > > +private: > > > > > > + class CameraBufferImpl; > > > > > > + CameraBufferImpl *impl_; > > > > > > }; > > > > > > > > > > > > #endif /* __ANDROID_CAMERA_BUFFER_H__ */ > > > > > > diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp > > > > > > index 807304a9e42d..10a43a61bd4d 100644 > > > > > > --- a/src/android/mm/android_generic_buffer.cpp > > > > > > +++ b/src/android/mm/android_generic_buffer.cpp > > > > > > @@ -13,7 +13,21 @@ using namespace libcamera; > > > > > > > > > > > > LOG_DECLARE_CATEGORY(HAL) > > > > > > > > > > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > > > > > +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer > > > > > > +{ > > > > > > +public: > > > > > > + CameraBufferImpl(buffer_handle_t camera3Buffer, int flags); > > > > > > + ~CameraBufferImpl(); > > > > > > + > > > > > > + unsigned int numPlanes() const; > > > > > > + ssize_t planeSize(unsigned int plane) const; > > > > > > + > > > > > > + const uint8_t *plane(unsigned int plane) const; > > > > > > + uint8_t *plane(unsigned int plane); > > > > > > +}; > > > > > > + > > > > > > +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer, > > > > > > + int flags) > > > > > > { > > > > > > maps_.reserve(camera3Buffer->numFds); > > > > > > error_ = 0; > > > > > > @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > > > > > } > > > > > > } > > > > > > > > > > > > +CameraBuffer::CameraBufferImpl::~CameraBufferImpl() > > > > > > +{ > > > > > > +} > > > > > > + > > > > > > +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const > > > > > > +{ > > > > > > + return maps_.size(); > > > > > > +} > > > > > > + > > > > > > +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const > > > > > > +{ > > > > > > + if (plane >= maps_.size()) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + return maps_[plane].size(); > > > > > > +} > > > > > > + > > > > > > +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const > > > > > > +{ > > > > > > + if (plane >= maps_.size()) > > > > > > + return nullptr; > > > > > > + > > > > > > + return maps_[plane].data(); > > > > > > +} > > > > > > + > > > > > > +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) > > > > > > +{ > > > > > > + if (plane >= maps_.size()) > > > > > > + return nullptr; > > > > > > + > > > > > > + return maps_[plane].data(); > > > > > > +} > > > > > > + > > > > > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) > > > > > > + : impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags)) > > > > > > +{ > > > > > > +} > > > > > > + > > > > > > CameraBuffer::~CameraBuffer() > > > > > > { > > > > > > + delete impl_; > > > > > > +} > > > > > > + > > > > > > +bool CameraBuffer::isValid() const > > > > > > +{ > > > > > > + return impl_->isValid(); > > > > > > +} > > > > > > + > > > > > > +unsigned int CameraBuffer::numPlanes() const > > > > > > +{ > > > > > > + return impl_->numPlanes(); > > > > > > +} > > > > > > + > > > > > > +ssize_t CameraBuffer::planeSize(unsigned int plane) const > > > > > > +{ > > > > > > + return impl_->planeSize(plane); > > > > > > +} > > > > > > + > > > > > > +const uint8_t *CameraBuffer::plane(unsigned int plane) const > > > > > > +{ > > > > > > + return impl_->plane(plane); > > > > > > +} > > > > > > + > > > > > > +uint8_t *CameraBuffer::plane(unsigned int plane) > > > > > > +{ > > > > > > + return impl_->plane(plane); > > > > > > }
diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h index 0590cd84652b..2a91e6a3c9c1 100644 --- a/src/android/camera_buffer.h +++ b/src/android/camera_buffer.h @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer public: CameraBuffer(buffer_handle_t camera3Buffer, int flags); ~CameraBuffer(); + + bool isValid() const; + + unsigned int numPlanes() const; + ssize_t planeSize(unsigned int plane) const; + + const uint8_t *plane(unsigned int plane) const; + uint8_t *plane(unsigned int plane); + +private: + class CameraBufferImpl; + CameraBufferImpl *impl_; }; #endif /* __ANDROID_CAMERA_BUFFER_H__ */ diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp index 807304a9e42d..10a43a61bd4d 100644 --- a/src/android/mm/android_generic_buffer.cpp +++ b/src/android/mm/android_generic_buffer.cpp @@ -13,7 +13,21 @@ using namespace libcamera; LOG_DECLARE_CATEGORY(HAL) -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer +{ +public: + CameraBufferImpl(buffer_handle_t camera3Buffer, int flags); + ~CameraBufferImpl(); + + unsigned int numPlanes() const; + ssize_t planeSize(unsigned int plane) const; + + const uint8_t *plane(unsigned int plane) const; + uint8_t *plane(unsigned int plane); +}; + +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer, + int flags) { maps_.reserve(camera3Buffer->numFds); error_ = 0; @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) } } +CameraBuffer::CameraBufferImpl::~CameraBufferImpl() +{ +} + +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const +{ + return maps_.size(); +} + +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const +{ + if (plane >= maps_.size()) + return -EINVAL; + + return maps_[plane].size(); +} + +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const +{ + if (plane >= maps_.size()) + return nullptr; + + return maps_[plane].data(); +} + +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) +{ + if (plane >= maps_.size()) + return nullptr; + + return maps_[plane].data(); +} + +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) + : impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags)) +{ +} + CameraBuffer::~CameraBuffer() { + delete impl_; +} + +bool CameraBuffer::isValid() const +{ + return impl_->isValid(); +} + +unsigned int CameraBuffer::numPlanes() const +{ + return impl_->numPlanes(); +} + +ssize_t CameraBuffer::planeSize(unsigned int plane) const +{ + return impl_->planeSize(plane); +} + +const uint8_t *CameraBuffer::plane(unsigned int plane) const +{ + return impl_->plane(plane); +} + +uint8_t *CameraBuffer::plane(unsigned int plane) +{ + return impl_->plane(plane); }
In order to prepare to support more memory backends, make the CameraBuffer class implement the PIMPL (pointer-to-implementation) pattern. Define the CameraBuffer class interface whose actual implementation is delegated to an inner CameraBufferImpl class. Temporary maintain libcamera::MappedBuffer as the CameraBuffer base class to maintain compatibility of the CameraStream::process() interface that requires a MappedBuffer * as second argument and will be converted to use a CameraBuffer in the next patch. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_buffer.h | 12 ++++ src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++- 2 files changed, 91 insertions(+), 1 deletion(-)