| Message ID | 20210910070638.467294-6-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Fri, Sep 10, 2021 at 12:36:34PM +0530, Umang Jain wrote: > CameraStream takes care of any post-processing required via the > CameraStream::process(). Since the post-processor will be moved > to a separate thread (in subsequent commits), the caller of > PostProcessor::process() will notify the associated CameraStream > instance about the post-processing completion via processComplete > signal. The CameraStream class should handle this event to query > the status and take next steps of reporting it back to upper layers > (i.e. CameraDevice). > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/camera_stream.cpp | 13 +++++++++++++ > src/android/camera_stream.h | 13 ++++++++++++- > 2 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index d1c54797..996779c4 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -81,6 +81,9 @@ int CameraStream::configure() > int ret = postProcessor_->configure(configuration(), output); > if (ret) > return ret; > + > + postProcessor_->processComplete.connect( > + this, &CameraStream::postProcessingComplete); > } > > if (allocator_) { > @@ -123,6 +126,16 @@ int CameraStream::process(const FrameBuffer *source, > resultMetadata, context); > } > > +void CameraStream::postProcessingComplete(PostProcessor::Status status, > + const Camera3RequestDescriptor *context) > +{ > + ProcessStatus processStatus; > + if (status == PostProcessor::Status::Succeeded) > + processStatus = ProcessStatus::Succeeded; > + else > + processStatus = ProcessStatus::Failed; > +} I'd squash this patch with 6/9, it doesn't do much on its own, and fails to compile due to the unused processStatus variable. > + > FrameBuffer *CameraStream::getBuffer() > { > if (!allocator_) > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > index d589f699..be076995 100644 > --- a/src/android/camera_stream.h > +++ b/src/android/camera_stream.h > @@ -13,15 +13,18 @@ > > #include <hardware/camera3.h> > > +#include <libcamera/base/signal.h> I don't think you need the signal header here. > + > #include <libcamera/camera.h> > #include <libcamera/framebuffer.h> > #include <libcamera/framebuffer_allocator.h> > #include <libcamera/geometry.h> > #include <libcamera/pixel_format.h> > > +#include "post_processor.h" > + > class CameraDevice; > class CameraMetadata; > -class PostProcessor; > > struct Camera3RequestDescriptor; > > @@ -128,7 +131,15 @@ public: > libcamera::FrameBuffer *getBuffer(); > void putBuffer(libcamera::FrameBuffer *buffer); > > + enum ProcessStatus { > + Succeeded, > + Failed, > + }; Do we need this enum, could we use PostProcessor::Status everywhere ? > + > private: > + void postProcessingComplete(PostProcessor::Status status, > + const Camera3RequestDescriptor *context); > + > CameraDevice *const cameraDevice_; > const libcamera::CameraConfiguration *config_; > const Type type_;
Hi Laurent, On 9/13/21 5:49 AM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Fri, Sep 10, 2021 at 12:36:34PM +0530, Umang Jain wrote: >> CameraStream takes care of any post-processing required via the >> CameraStream::process(). Since the post-processor will be moved >> to a separate thread (in subsequent commits), the caller of >> PostProcessor::process() will notify the associated CameraStream >> instance about the post-processing completion via processComplete >> signal. The CameraStream class should handle this event to query >> the status and take next steps of reporting it back to upper layers >> (i.e. CameraDevice). >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/android/camera_stream.cpp | 13 +++++++++++++ >> src/android/camera_stream.h | 13 ++++++++++++- >> 2 files changed, 25 insertions(+), 1 deletion(-) >> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp >> index d1c54797..996779c4 100644 >> --- a/src/android/camera_stream.cpp >> +++ b/src/android/camera_stream.cpp >> @@ -81,6 +81,9 @@ int CameraStream::configure() >> int ret = postProcessor_->configure(configuration(), output); >> if (ret) >> return ret; >> + >> + postProcessor_->processComplete.connect( >> + this, &CameraStream::postProcessingComplete); >> } >> >> if (allocator_) { >> @@ -123,6 +126,16 @@ int CameraStream::process(const FrameBuffer *source, >> resultMetadata, context); >> } >> >> +void CameraStream::postProcessingComplete(PostProcessor::Status status, >> + const Camera3RequestDescriptor *context) >> +{ >> + ProcessStatus processStatus; >> + if (status == PostProcessor::Status::Succeeded) >> + processStatus = ProcessStatus::Succeeded; >> + else >> + processStatus = ProcessStatus::Failed; >> +} > I'd squash this patch with 6/9, it doesn't do much on its own, and fails > to compile due to the unused processStatus variable. > >> + >> FrameBuffer *CameraStream::getBuffer() >> { >> if (!allocator_) >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h >> index d589f699..be076995 100644 >> --- a/src/android/camera_stream.h >> +++ b/src/android/camera_stream.h >> @@ -13,15 +13,18 @@ >> >> #include <hardware/camera3.h> >> >> +#include <libcamera/base/signal.h> > I don't think you need the signal header here. > >> + >> #include <libcamera/camera.h> >> #include <libcamera/framebuffer.h> >> #include <libcamera/framebuffer_allocator.h> >> #include <libcamera/geometry.h> >> #include <libcamera/pixel_format.h> >> >> +#include "post_processor.h" >> + >> class CameraDevice; >> class CameraMetadata; >> -class PostProcessor; >> >> struct Camera3RequestDescriptor; >> >> @@ -128,7 +131,15 @@ public: >> libcamera::FrameBuffer *getBuffer(); >> void putBuffer(libcamera::FrameBuffer *buffer); >> >> + enum ProcessStatus { >> + Succeeded, >> + Failed, >> + }; > Do we need this enum, could we use PostProcessor::Status everywhere ? Yes, that would be my preference as well. But I think is if we do, we will have PostProcessor:: in CameraDevice, which currently doesn't interact with PostProcessor layer atleast directly. And I would like to keep it that way. Maybe keep the status enum in CameraStream class and use it both in PostProcessor and CameraDevice. That way we achieve Status:: stuff everywhere probably. > >> + >> private: >> + void postProcessingComplete(PostProcessor::Status status, >> + const Camera3RequestDescriptor *context); >> + >> CameraDevice *const cameraDevice_; >> const libcamera::CameraConfiguration *config_; >> const Type type_;
Hi Umang, On Mon, Sep 13, 2021 at 11:55:35AM +0530, Umang Jain wrote: > On 9/13/21 5:49 AM, Laurent Pinchart wrote: > > On Fri, Sep 10, 2021 at 12:36:34PM +0530, Umang Jain wrote: > >> CameraStream takes care of any post-processing required via the > >> CameraStream::process(). Since the post-processor will be moved > >> to a separate thread (in subsequent commits), the caller of > >> PostProcessor::process() will notify the associated CameraStream > >> instance about the post-processing completion via processComplete > >> signal. The CameraStream class should handle this event to query > >> the status and take next steps of reporting it back to upper layers > >> (i.e. CameraDevice). > >> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> src/android/camera_stream.cpp | 13 +++++++++++++ > >> src/android/camera_stream.h | 13 ++++++++++++- > >> 2 files changed, 25 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > >> index d1c54797..996779c4 100644 > >> --- a/src/android/camera_stream.cpp > >> +++ b/src/android/camera_stream.cpp > >> @@ -81,6 +81,9 @@ int CameraStream::configure() > >> int ret = postProcessor_->configure(configuration(), output); > >> if (ret) > >> return ret; > >> + > >> + postProcessor_->processComplete.connect( > >> + this, &CameraStream::postProcessingComplete); > >> } > >> > >> if (allocator_) { > >> @@ -123,6 +126,16 @@ int CameraStream::process(const FrameBuffer *source, > >> resultMetadata, context); > >> } > >> > >> +void CameraStream::postProcessingComplete(PostProcessor::Status status, > >> + const Camera3RequestDescriptor *context) > >> +{ > >> + ProcessStatus processStatus; > >> + if (status == PostProcessor::Status::Succeeded) > >> + processStatus = ProcessStatus::Succeeded; > >> + else > >> + processStatus = ProcessStatus::Failed; > >> +} > > > > I'd squash this patch with 6/9, it doesn't do much on its own, and fails > > to compile due to the unused processStatus variable. > > > >> + > >> FrameBuffer *CameraStream::getBuffer() > >> { > >> if (!allocator_) > >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h > >> index d589f699..be076995 100644 > >> --- a/src/android/camera_stream.h > >> +++ b/src/android/camera_stream.h > >> @@ -13,15 +13,18 @@ > >> > >> #include <hardware/camera3.h> > >> > >> +#include <libcamera/base/signal.h> > > > > I don't think you need the signal header here. > > > >> + > >> #include <libcamera/camera.h> > >> #include <libcamera/framebuffer.h> > >> #include <libcamera/framebuffer_allocator.h> > >> #include <libcamera/geometry.h> > >> #include <libcamera/pixel_format.h> > >> > >> +#include "post_processor.h" > >> + > >> class CameraDevice; > >> class CameraMetadata; > >> -class PostProcessor; > >> > >> struct Camera3RequestDescriptor; > >> > >> @@ -128,7 +131,15 @@ public: > >> libcamera::FrameBuffer *getBuffer(); > >> void putBuffer(libcamera::FrameBuffer *buffer); > >> > >> + enum ProcessStatus { > >> + Succeeded, > >> + Failed, > >> + }; > > > > Do we need this enum, could we use PostProcessor::Status everywhere ? > > Yes, that would be my preference as well. But I think is if we do, we > will have PostProcessor:: in CameraDevice, which currently doesn't > interact with PostProcessor layer atleast directly. And I would like to > keep it that way. I was expecting that objection :-) > Maybe keep the status enum in CameraStream class and use it both in > PostProcessor and CameraDevice. That way we achieve Status:: stuff > everywhere probably. How about adding the enum to Camera3RequestDescriptor ? It's a request completion status after all, so it would make sense to centralize it there. > >> + > >> private: > >> + void postProcessingComplete(PostProcessor::Status status, > >> + const Camera3RequestDescriptor *context); > >> + > >> CameraDevice *const cameraDevice_; > >> const libcamera::CameraConfiguration *config_; > >> const Type type_;
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index d1c54797..996779c4 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -81,6 +81,9 @@ int CameraStream::configure() int ret = postProcessor_->configure(configuration(), output); if (ret) return ret; + + postProcessor_->processComplete.connect( + this, &CameraStream::postProcessingComplete); } if (allocator_) { @@ -123,6 +126,16 @@ int CameraStream::process(const FrameBuffer *source, resultMetadata, context); } +void CameraStream::postProcessingComplete(PostProcessor::Status status, + const Camera3RequestDescriptor *context) +{ + ProcessStatus processStatus; + if (status == PostProcessor::Status::Succeeded) + processStatus = ProcessStatus::Succeeded; + else + processStatus = ProcessStatus::Failed; +} + FrameBuffer *CameraStream::getBuffer() { if (!allocator_) diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h index d589f699..be076995 100644 --- a/src/android/camera_stream.h +++ b/src/android/camera_stream.h @@ -13,15 +13,18 @@ #include <hardware/camera3.h> +#include <libcamera/base/signal.h> + #include <libcamera/camera.h> #include <libcamera/framebuffer.h> #include <libcamera/framebuffer_allocator.h> #include <libcamera/geometry.h> #include <libcamera/pixel_format.h> +#include "post_processor.h" + class CameraDevice; class CameraMetadata; -class PostProcessor; struct Camera3RequestDescriptor; @@ -128,7 +131,15 @@ public: libcamera::FrameBuffer *getBuffer(); void putBuffer(libcamera::FrameBuffer *buffer); + enum ProcessStatus { + Succeeded, + Failed, + }; + private: + void postProcessingComplete(PostProcessor::Status status, + const Camera3RequestDescriptor *context); + CameraDevice *const cameraDevice_; const libcamera::CameraConfiguration *config_; const Type type_;
CameraStream takes care of any post-processing required via the CameraStream::process(). Since the post-processor will be moved to a separate thread (in subsequent commits), the caller of PostProcessor::process() will notify the associated CameraStream instance about the post-processing completion via processComplete signal. The CameraStream class should handle this event to query the status and take next steps of reporting it back to upper layers (i.e. CameraDevice). Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/android/camera_stream.cpp | 13 +++++++++++++ src/android/camera_stream.h | 13 ++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-)