[v1,3/3] libcamera: pipeline: virtual: Set `FrameError` on error
diff mbox series

Message ID 20250203104318.135628-4-pobrn@protonmail.com
State Accepted
Headers show
Series
  • libcamera: pipeline: virtual: Fill buffer's metadata
Related show

Commit Message

Barnabás Pőcze Feb. 3, 2025, 10:43 a.m. UTC
Do not cancel, simply set the buffer's status to `FrameError`
to notify the user about the error condition.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 src/libcamera/pipeline/virtual/virtual.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kieran Bingham Feb. 3, 2025, 11:06 a.m. UTC | #1
Quoting Barnabás Pőcze (2025-02-03 10:43:34)
> Do not cancel, simply set the buffer's status to `FrameError`
> to notify the user about the error condition.
> 
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  src/libcamera/pipeline/virtual/virtual.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> index 1a75f35aa..cbba08c82 100644
> --- a/src/libcamera/pipeline/virtual/virtual.cpp
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -322,7 +322,7 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
>  
>                                 if (streamConfig.frameGenerator->generateFrame(
>                                             stream->configuration().size, buffer))
> -                                       buffer->_d()->cancel();
> +                                       fmd.status = FrameMetadata::Status::FrameError;

I think this sounds right. I always worry we don't handle frame buffers
correctly for errors vs shutdown paths where I think buffers get marked
as error/cancel to let the application know not to re-submit them.

I think that API all feels awkward, but I suspect what you're doing here
is correct as this is just if there is an error to generate the frame.

Can the generateFrame return errors? What might happen?

anyway, it sounds correct already:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  
>                                 completeBuffer(request, buffer);
>                                 break;
> -- 
> 2.48.1
> 
>
Barnabás Pőcze Feb. 3, 2025, 11:17 a.m. UTC | #2
2025. február 3., hétfő 12:06 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> Quoting Barnabás Pőcze (2025-02-03 10:43:34)
> > Do not cancel, simply set the buffer's status to `FrameError`
> > to notify the user about the error condition.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > ---
> >  src/libcamera/pipeline/virtual/virtual.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> > index 1a75f35aa..cbba08c82 100644
> > --- a/src/libcamera/pipeline/virtual/virtual.cpp
> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > @@ -322,7 +322,7 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
> >
> >                                 if (streamConfig.frameGenerator->generateFrame(
> >                                             stream->configuration().size, buffer))
> > -                                       buffer->_d()->cancel();
> > +                                       fmd.status = FrameMetadata::Status::FrameError;
> 
> I think this sounds right. I always worry we don't handle frame buffers
> correctly for errors vs shutdown paths where I think buffers get marked
> as error/cancel to let the application know not to re-submit them.
> 
> I think that API all feels awkward, but I suspect what you're doing here
> is correct as this is just if there is an error to generate the frame.
> 
> Can the generateFrame return errors? What might happen?

I don't think it can, realistically. 


> 
> anyway, it sounds correct already:
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >
> >                                 completeBuffer(request, buffer);
> >                                 break;
> > --
> > 2.48.1
> >
> >
>
Jacopo Mondi Feb. 3, 2025, 5:42 p.m. UTC | #3
Hi Barnabás

On Mon, Feb 03, 2025 at 10:43:34AM +0000, Barnabás Pőcze wrote:
> Do not cancel, simply set the buffer's status to `FrameError`
> to notify the user about the error condition.

In case of errors, Status::FrameError seems more opportune than
Status::FrameCancelled indeed

>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  src/libcamera/pipeline/virtual/virtual.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> index 1a75f35aa..cbba08c82 100644
> --- a/src/libcamera/pipeline/virtual/virtual.cpp
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -322,7 +322,7 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
>
>  				if (streamConfig.frameGenerator->generateFrame(
>  					    stream->configuration().size, buffer))
> -					buffer->_d()->cancel();
> +					fmd.status = FrameMetadata::Status::FrameError;
>
>  				completeBuffer(request, buffer);
>  				break;
> --
> 2.48.1
>
>

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
index 1a75f35aa..cbba08c82 100644
--- a/src/libcamera/pipeline/virtual/virtual.cpp
+++ b/src/libcamera/pipeline/virtual/virtual.cpp
@@ -322,7 +322,7 @@  int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
 
 				if (streamConfig.frameGenerator->generateFrame(
 					    stream->configuration().size, buffer))
-					buffer->_d()->cancel();
+					fmd.status = FrameMetadata::Status::FrameError;
 
 				completeBuffer(request, buffer);
 				break;