| Message ID | 20210312054727.852622-4-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On 2021-03-12 05:47:22 +0000, Kieran Bingham wrote: > Buffers queued to a pipeline handler may not be yet queued to a device > when the request is cancelled. > > This can lead to the FrameMetadata having never been explicitly set by > an underlying V4L2 device. > > The status field on this is used to check the state of the buffer to > determine if it was correctly filled, or if it was cancelled. > > In the event that the buffer is not used, it must be marked as Error as > the metadata associated with that frame will not be valid. > > Initialise the FrameMetadata to FrameError to prevent uninitialised access. > Furthermore, swap the Enum values of the Status such that the first > state represents the initial Error state. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I like this patch and I fear it could expose issues, have it been tested with all supported pipelines? In either case I think it's the right thing. Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/buffer.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index 302fe3d3e86b..3f5d0f1b6363 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -19,8 +19,8 @@ class Request; > > struct FrameMetadata { > enum Status { > - FrameSuccess, > FrameError, > + FrameSuccess, > FrameCancelled, > }; > > @@ -28,7 +28,7 @@ struct FrameMetadata { > unsigned int bytesused; > }; > > - Status status; > + Status status = FrameError; > unsigned int sequence; > uint64_t timestamp; > std::vector<Plane> planes; > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On Fri, Mar 12, 2021 at 05:47:22AM +0000, Kieran Bingham wrote: > Buffers queued to a pipeline handler may not be yet queued to a device > when the request is cancelled. > > This can lead to the FrameMetadata having never been explicitly set by > an underlying V4L2 device. > > The status field on this is used to check the state of the buffer to > determine if it was correctly filled, or if it was cancelled. > > In the event that the buffer is not used, it must be marked as Error as > the metadata associated with that frame will not be valid. > > Initialise the FrameMetadata to FrameError to prevent uninitialised access. > Furthermore, swap the Enum values of the Status such that the first > state represents the initial Error state. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Probably a test from users of RkISP1 and simple pipeline would solve Niklas' concerns which I do share as some code paths might bet of the fact that the status is set to FrameSuccess on construction. > --- > include/libcamera/buffer.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index 302fe3d3e86b..3f5d0f1b6363 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -19,8 +19,8 @@ class Request; > > struct FrameMetadata { > enum Status { > - FrameSuccess, > FrameError, > + FrameSuccess, > FrameCancelled, > }; > > @@ -28,7 +28,7 @@ struct FrameMetadata { > unsigned int bytesused; > }; > > - Status status; > + Status status = FrameError; > unsigned int sequence; > uint64_t timestamp; > std::vector<Plane> planes; > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, Thank you for the patch. On Fri, Mar 12, 2021 at 05:47:22AM +0000, Kieran Bingham wrote: > Buffers queued to a pipeline handler may not be yet queued to a device > when the request is cancelled. > > This can lead to the FrameMetadata having never been explicitly set by > an underlying V4L2 device. > > The status field on this is used to check the state of the buffer to > determine if it was correctly filled, or if it was cancelled. > > In the event that the buffer is not used, it must be marked as Error as > the metadata associated with that frame will not be valid. Shouldn't it be Cancelled then ? * \var FrameMetadata::FrameError * An error occurred during capture of the frame. The frame data may be partly * or fully invalid. The sequence and timestamp fields of the FrameMetadata * structure is valid, the other fields may be invalid. * \var FrameMetadata::FrameCancelled * Capture stopped before the frame completed. The frame data is not valid. All * fields of the FrameMetadata structure but the status field are invalid. > Initialise the FrameMetadata to FrameError to prevent uninitialised access. > Furthermore, swap the Enum values of the Status such that the first > state represents the initial Error state. Now that the status is initialized by default, is this needed ? I'm not opposed to the change, just trying to see if it has any functional impact. > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/buffer.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > index 302fe3d3e86b..3f5d0f1b6363 100644 > --- a/include/libcamera/buffer.h > +++ b/include/libcamera/buffer.h > @@ -19,8 +19,8 @@ class Request; > > struct FrameMetadata { > enum Status { > - FrameSuccess, > FrameError, > + FrameSuccess, > FrameCancelled, > }; > > @@ -28,7 +28,7 @@ struct FrameMetadata { > unsigned int bytesused; > }; > > - Status status; > + Status status = FrameError; This will achieve the stated goal when a buffer is first allocated and queued. After the buffer completes, its status will be FrameSuccess (assuming capture completed successfully). When the buffer is then requeued as part of a new request, it will reach the pipeline handler with state set to FrameSuccess. Wouldn't it be better to set the state explicitly in Camera::queueRequest() instead ? Given that the pipeline handlers are supposed to set the buffer state before completing it, I wonder if this change could hide other pipeline handler issues. Another option would be to add a fourth state (FramePending for instance, to mimic RequestPending), set the buffer state to FramePending in Camera::queueRequest(), and add an ASSERT(status != FramePending) in PipelineHandler::completeBuffer(). That way we would catch incorrect pipeline handler behaviour right away. > unsigned int sequence; > uint64_t timestamp; > std::vector<Plane> planes;
Hi Niklas, On 12/03/2021 23:23, Niklas Söderlund wrote: > Hi Kieran, > > On 2021-03-12 05:47:22 +0000, Kieran Bingham wrote: >> Buffers queued to a pipeline handler may not be yet queued to a device >> when the request is cancelled. >> >> This can lead to the FrameMetadata having never been explicitly set by >> an underlying V4L2 device. >> >> The status field on this is used to check the state of the buffer to >> determine if it was correctly filled, or if it was cancelled. >> >> In the event that the buffer is not used, it must be marked as Error as >> the metadata associated with that frame will not be valid. >> >> Initialise the FrameMetadata to FrameError to prevent uninitialised access. >> Furthermore, swap the Enum values of the Status such that the first >> state represents the initial Error state. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > I like this patch and I fear it could expose issues, have it been tested > with all supported pipelines? In either case I think it's the right > thing. > This series has been tested on Raspberry Pi - but not Rockchip. I don't have an easy way to test that currently. Would you be able to run this series on your test platform please? > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> --- >> include/libcamera/buffer.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h >> index 302fe3d3e86b..3f5d0f1b6363 100644 >> --- a/include/libcamera/buffer.h >> +++ b/include/libcamera/buffer.h >> @@ -19,8 +19,8 @@ class Request; >> >> struct FrameMetadata { >> enum Status { >> - FrameSuccess, >> FrameError, >> + FrameSuccess, >> FrameCancelled, >> }; >> >> @@ -28,7 +28,7 @@ struct FrameMetadata { >> unsigned int bytesused; >> }; >> >> - Status status; >> + Status status = FrameError; >> unsigned int sequence; >> uint64_t timestamp; >> std::vector<Plane> planes; >> -- >> 2.25.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Laurent, On 13/03/2021 22:47, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Mar 12, 2021 at 05:47:22AM +0000, Kieran Bingham wrote: >> Buffers queued to a pipeline handler may not be yet queued to a device >> when the request is cancelled. >> >> This can lead to the FrameMetadata having never been explicitly set by >> an underlying V4L2 device. >> >> The status field on this is used to check the state of the buffer to >> determine if it was correctly filled, or if it was cancelled. >> >> In the event that the buffer is not used, it must be marked as Error as >> the metadata associated with that frame will not be valid. > > Shouldn't it be Cancelled then ? > > * \var FrameMetadata::FrameError > * An error occurred during capture of the frame. The frame data may be partly > * or fully invalid. The sequence and timestamp fields of the FrameMetadata > * structure is valid, the other fields may be invalid. > * \var FrameMetadata::FrameCancelled > * Capture stopped before the frame completed. The frame data is not valid. All > * fields of the FrameMetadata structure but the status field are invalid. > >> Initialise the FrameMetadata to FrameError to prevent uninitialised access. >> Furthermore, swap the Enum values of the Status such that the first >> state represents the initial Error state. > > Now that the status is initialized by default, is this needed ? I'm not > opposed to the change, just trying to see if it has any functional > impact. Maybe not, but my aim was to ensure/convey that buffers which are freshly allocated are not considered as Valid. >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> include/libcamera/buffer.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h >> index 302fe3d3e86b..3f5d0f1b6363 100644 >> --- a/include/libcamera/buffer.h >> +++ b/include/libcamera/buffer.h >> @@ -19,8 +19,8 @@ class Request; >> >> struct FrameMetadata { >> enum Status { >> - FrameSuccess, >> FrameError, >> + FrameSuccess, >> FrameCancelled, >> }; >> >> @@ -28,7 +28,7 @@ struct FrameMetadata { >> unsigned int bytesused; >> }; >> >> - Status status; >> + Status status = FrameError; > > This will achieve the stated goal when a buffer is first allocated and > queued. After the buffer completes, its status will be FrameSuccess > (assuming capture completed successfully). When the buffer is then > requeued as part of a new request, it will reach the pipeline handler > with state set to FrameSuccess. Wouldn't it be better to set the state > explicitly in Camera::queueRequest() instead ? Yes, I like the sound of that better. It ensures they always get set. (If it's never queued, well then we don't care about it). > Given that the pipeline handlers are supposed to set the buffer state > before completing it, I wonder if this change could hide other pipeline > handler issues. Another option would be to add a fourth state > (FramePending for instance, to mimic RequestPending), set the buffer > state to FramePending in Camera::queueRequest(), and add an > ASSERT(status != FramePending) in PipelineHandler::completeBuffer(). > That way we would catch incorrect pipeline handler behaviour right away. Interesting, that might indeed be worthwhile. I was also contemplating extending the Request states too. We use Pending from 'new' and 'queued into the pipeline handler'... I wonder if that should be: RequestNew (freshly allocated, owned by the application) RequestPending (queued into the PH) RequestComplete RequestCancelled So there might be the same to apply to buffer states too. > >> unsigned int sequence; >> uint64_t timestamp; >> std::vector<Plane> planes; >
Hi Kieran, On Mon, Mar 15, 2021 at 12:47:26PM +0000, Kieran Bingham wrote: > On 13/03/2021 22:47, Laurent Pinchart wrote: > > On Fri, Mar 12, 2021 at 05:47:22AM +0000, Kieran Bingham wrote: > >> Buffers queued to a pipeline handler may not be yet queued to a device > >> when the request is cancelled. > >> > >> This can lead to the FrameMetadata having never been explicitly set by > >> an underlying V4L2 device. > >> > >> The status field on this is used to check the state of the buffer to > >> determine if it was correctly filled, or if it was cancelled. > >> > >> In the event that the buffer is not used, it must be marked as Error as > >> the metadata associated with that frame will not be valid. > > > > Shouldn't it be Cancelled then ? > > > > * \var FrameMetadata::FrameError > > * An error occurred during capture of the frame. The frame data may be partly > > * or fully invalid. The sequence and timestamp fields of the FrameMetadata > > * structure is valid, the other fields may be invalid. > > * \var FrameMetadata::FrameCancelled > > * Capture stopped before the frame completed. The frame data is not valid. All > > * fields of the FrameMetadata structure but the status field are invalid. > > > >> Initialise the FrameMetadata to FrameError to prevent uninitialised access. > >> Furthermore, swap the Enum values of the Status such that the first > >> state represents the initial Error state. > > > > Now that the status is initialized by default, is this needed ? I'm not > > opposed to the change, just trying to see if it has any functional > > impact. > > Maybe not, but my aim was to ensure/convey that buffers which are > freshly allocated are not considered as Valid. Sorry, I wasn't very precise, my question was related to swapping the enum values only. > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> include/libcamera/buffer.h | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h > >> index 302fe3d3e86b..3f5d0f1b6363 100644 > >> --- a/include/libcamera/buffer.h > >> +++ b/include/libcamera/buffer.h > >> @@ -19,8 +19,8 @@ class Request; > >> > >> struct FrameMetadata { > >> enum Status { > >> - FrameSuccess, > >> FrameError, > >> + FrameSuccess, > >> FrameCancelled, > >> }; > >> > >> @@ -28,7 +28,7 @@ struct FrameMetadata { > >> unsigned int bytesused; > >> }; > >> > >> - Status status; > >> + Status status = FrameError; > > > > This will achieve the stated goal when a buffer is first allocated and > > queued. After the buffer completes, its status will be FrameSuccess > > (assuming capture completed successfully). When the buffer is then > > requeued as part of a new request, it will reach the pipeline handler > > with state set to FrameSuccess. Wouldn't it be better to set the state > > explicitly in Camera::queueRequest() instead ? > > Yes, I like the sound of that better. It ensures they always get set. > (If it's never queued, well then we don't care about it). > > > Given that the pipeline handlers are supposed to set the buffer state > > before completing it, I wonder if this change could hide other pipeline > > handler issues. Another option would be to add a fourth state > > (FramePending for instance, to mimic RequestPending), set the buffer > > state to FramePending in Camera::queueRequest(), and add an > > ASSERT(status != FramePending) in PipelineHandler::completeBuffer(). > > That way we would catch incorrect pipeline handler behaviour right away. > > Interesting, that might indeed be worthwhile. > > I was also contemplating extending the Request states too. > > We use Pending from 'new' and 'queued into the pipeline handler'... I > wonder if that should be: > > RequestNew (freshly allocated, owned by the application) > RequestPending (queued into the PH) > RequestComplete > RequestCancelled > > So there might be the same to apply to buffer states too. We could do that. I'm not sure if telling "new" and "pending" apart would really be useful though, as the new state will never be seen by libcamera (the request state will be set to pending by Camera::queueRequest()). It would thus be mostly be a debugging aid for applications. That doesn't make it worthless, but I haven't thought enough about common bug patterns on the application side to tell if this is what applications will need. > >> unsigned int sequence; > >> uint64_t timestamp; > >> std::vector<Plane> planes;
Hi Laurent, On 15/03/2021 23:24, Laurent Pinchart wrote: > Hi Kieran, > > On Mon, Mar 15, 2021 at 12:47:26PM +0000, Kieran Bingham wrote: >> On 13/03/2021 22:47, Laurent Pinchart wrote: >>> On Fri, Mar 12, 2021 at 05:47:22AM +0000, Kieran Bingham wrote: >>>> Buffers queued to a pipeline handler may not be yet queued to a device >>>> when the request is cancelled. >>>> >>>> This can lead to the FrameMetadata having never been explicitly set by >>>> an underlying V4L2 device. >>>> >>>> The status field on this is used to check the state of the buffer to >>>> determine if it was correctly filled, or if it was cancelled. >>>> >>>> In the event that the buffer is not used, it must be marked as Error as >>>> the metadata associated with that frame will not be valid. >>> >>> Shouldn't it be Cancelled then ? >>> >>> * \var FrameMetadata::FrameError >>> * An error occurred during capture of the frame. The frame data may be partly >>> * or fully invalid. The sequence and timestamp fields of the FrameMetadata >>> * structure is valid, the other fields may be invalid. >>> * \var FrameMetadata::FrameCancelled >>> * Capture stopped before the frame completed. The frame data is not valid. All >>> * fields of the FrameMetadata structure but the status field are invalid. >>> >>>> Initialise the FrameMetadata to FrameError to prevent uninitialised access. >>>> Furthermore, swap the Enum values of the Status such that the first >>>> state represents the initial Error state. >>> >>> Now that the status is initialized by default, is this needed ? I'm not >>> opposed to the change, just trying to see if it has any functional >>> impact. >> >> Maybe not, but my aim was to ensure/convey that buffers which are >> freshly allocated are not considered as Valid. > > Sorry, I wasn't very precise, my question was related to swapping the > enum values only. Yes, I realised that. There is no expected functional change in swapping the order *as long as* the FrameMetadata is always correctly initialised. My statement above was referring to the fact that by ordering the states as FrameError first, it leads towards mirroring the expected states. I.e. Frames start in FrameError, and move to FrameSuccess or FrameCancelled. If FrameSuccess is the first state, it implies that frames start successfully, and move to either error or cancelled... That's the reason for reordering the states. Maybe that is different if they should start in FrameCancelled. I'll think about that more when I get back to reworking for v2. -- Kieran > >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> --- >>>> include/libcamera/buffer.h | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h >>>> index 302fe3d3e86b..3f5d0f1b6363 100644 >>>> --- a/include/libcamera/buffer.h >>>> +++ b/include/libcamera/buffer.h >>>> @@ -19,8 +19,8 @@ class Request; >>>> >>>> struct FrameMetadata { >>>> enum Status { >>>> - FrameSuccess, >>>> FrameError, >>>> + FrameSuccess, >>>> FrameCancelled, >>>> }; >>>> >>>> @@ -28,7 +28,7 @@ struct FrameMetadata { >>>> unsigned int bytesused; >>>> }; >>>> >>>> - Status status; >>>> + Status status = FrameError; >>> >>> This will achieve the stated goal when a buffer is first allocated and >>> queued. After the buffer completes, its status will be FrameSuccess >>> (assuming capture completed successfully). When the buffer is then >>> requeued as part of a new request, it will reach the pipeline handler >>> with state set to FrameSuccess. Wouldn't it be better to set the state >>> explicitly in Camera::queueRequest() instead ? >> >> Yes, I like the sound of that better. It ensures they always get set. >> (If it's never queued, well then we don't care about it). >> >>> Given that the pipeline handlers are supposed to set the buffer state >>> before completing it, I wonder if this change could hide other pipeline >>> handler issues. Another option would be to add a fourth state >>> (FramePending for instance, to mimic RequestPending), set the buffer >>> state to FramePending in Camera::queueRequest(), and add an >>> ASSERT(status != FramePending) in PipelineHandler::completeBuffer(). >>> That way we would catch incorrect pipeline handler behaviour right away. >> >> Interesting, that might indeed be worthwhile. >> >> I was also contemplating extending the Request states too. >> >> We use Pending from 'new' and 'queued into the pipeline handler'... I >> wonder if that should be: >> >> RequestNew (freshly allocated, owned by the application) >> RequestPending (queued into the PH) >> RequestComplete >> RequestCancelled >> >> So there might be the same to apply to buffer states too. > > We could do that. I'm not sure if telling "new" and "pending" apart > would really be useful though, as the new state will never be seen by > libcamera (the request state will be set to pending by > Camera::queueRequest()). It would thus be mostly be a debugging aid for > applications. That doesn't make it worthless, but I haven't thought > enough about common bug patterns on the application side to tell if this > is what applications will need. >>>> unsigned int sequence; >>>> uint64_t timestamp; >>>> std::vector<Plane> planes; >
diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h index 302fe3d3e86b..3f5d0f1b6363 100644 --- a/include/libcamera/buffer.h +++ b/include/libcamera/buffer.h @@ -19,8 +19,8 @@ class Request; struct FrameMetadata { enum Status { - FrameSuccess, FrameError, + FrameSuccess, FrameCancelled, }; @@ -28,7 +28,7 @@ struct FrameMetadata { unsigned int bytesused; }; - Status status; + Status status = FrameError; unsigned int sequence; uint64_t timestamp; std::vector<Plane> planes;
Buffers queued to a pipeline handler may not be yet queued to a device when the request is cancelled. This can lead to the FrameMetadata having never been explicitly set by an underlying V4L2 device. The status field on this is used to check the state of the buffer to determine if it was correctly filled, or if it was cancelled. In the event that the buffer is not used, it must be marked as Error as the metadata associated with that frame will not be valid. Initialise the FrameMetadata to FrameError to prevent uninitialised access. Furthermore, swap the Enum values of the Status such that the first state represents the initial Error state. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- include/libcamera/buffer.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)