| Message ID | 20240826134639.1398830-1-chenghaoyang@google.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Han-Lin On Mon, Aug 26, 2024 at 01:46:30PM GMT, Harvey Yang wrote: > From: Han-Lin Chen <hanlinchen@chromium.org> > > There seems to be an assumption that a FrameBuffer is success unless > the pipeline handler canceled the frame, or there is a failure > processing the FrameBuffer. I'm not sure this is actually assumed anywhere. The FrameMetadata status is set by the V4L2VideoDevice class at dequeue buffer time src/libcamera/v4l2_videodevice.cpp- metadata.status = buf.flags & V4L2_BUF_FLAG_ERROR src/libcamera/v4l2_videodevice.cpp- ? FrameMetadata::FrameError src/libcamera/v4l2_videodevice.cpp: : FrameMetadata::FrameSuccess; > > Make the assumption specific. > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > --- > include/libcamera/framebuffer.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h > index 5ae2270b3..71202a1d2 100644 > --- a/include/libcamera/framebuffer.h > +++ b/include/libcamera/framebuffer.h > @@ -33,7 +33,7 @@ struct FrameMetadata { > unsigned int bytesused; > }; > > - Status status; > + Status status = FrameSuccess; > unsigned int sequence; > uint64_t timestamp; However, I don't think this hurts but I wonder why sequence or timestamp are any different. > > -- > 2.46.0.295.g3b9ea8a38a-goog >
Hi Jacopo, On Tue, Aug 27, 2024 at 6:06 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi Han-Lin > > On Mon, Aug 26, 2024 at 01:46:30PM GMT, Harvey Yang wrote: > > From: Han-Lin Chen <hanlinchen@chromium.org> > > > > There seems to be an assumption that a FrameBuffer is success unless > > the pipeline handler canceled the frame, or there is a failure > > processing the FrameBuffer. > > I'm not sure this is actually assumed anywhere. > > The FrameMetadata status is set by the V4L2VideoDevice class at > dequeue buffer time > > src/libcamera/v4l2_videodevice.cpp- metadata.status = buf.flags & > V4L2_BUF_FLAG_ERROR > src/libcamera/v4l2_videodevice.cpp- ? > FrameMetadata::FrameError > src/libcamera/v4l2_videodevice.cpp: : > FrameMetadata::FrameSuccess; > Thanks! The patch was a temporary workaround in our branch, and accidentally copied here. Sorry for the confusion. There was a time when the platform was not following the V4L2 spec and some buffers may not go through the V4L2VideoDevice class, and the status remains undefined, causing some problems during development. > > > Make the assumption specific. > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > --- > > include/libcamera/framebuffer.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/libcamera/framebuffer.h > b/include/libcamera/framebuffer.h > > index 5ae2270b3..71202a1d2 100644 > > --- a/include/libcamera/framebuffer.h > > +++ b/include/libcamera/framebuffer.h > > @@ -33,7 +33,7 @@ struct FrameMetadata { > > unsigned int bytesused; > > }; > > > > - Status status; > > + Status status = FrameSuccess; > > unsigned int sequence; > > uint64_t timestamp; > > However, I don't think this hurts but I wonder why sequence or > timestamp are any different. > We should re-format the patch to simply add default values to all fields, with a clearer description. > > > > > -- > > 2.46.0.295.g3b9ea8a38a-goog > > >
diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h index 5ae2270b3..71202a1d2 100644 --- a/include/libcamera/framebuffer.h +++ b/include/libcamera/framebuffer.h @@ -33,7 +33,7 @@ struct FrameMetadata { unsigned int bytesused; }; - Status status; + Status status = FrameSuccess; unsigned int sequence; uint64_t timestamp;