| Message ID | 20241215181835.802589-1-pobrn@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Another thing: FrameBufferAllocator does not consider it an error if the underlying Camera's `exportFrameBuffers()` returns. This should be clarified. 2024. december 15., vasárnap 19:18 keltezéssel, Barnabás Pőcze <pobrn@protonmail.com> írta: > `FrameBufferAllocator::allocate()` might return a negative error code, > but currently this is handled the same way as success. So instead > of continuing, abort the construction. > > Signed-off-by: Barnabás Pőcze pobrn@protonmail.com > > --- > src/gstreamer/gstlibcameraallocator.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp > index 7e4c904da..b0c84893a 100644 > --- a/src/gstreamer/gstlibcameraallocator.cpp > +++ b/src/gstreamer/gstlibcameraallocator.cpp > @@ -214,7 +214,7 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera, > > Stream *stream = streamCfg.stream(); > > ret = self->fb_allocator->allocate(stream); > > - if (ret == 0) > + if (ret <= 0) > return nullptr; > > GQueue *pool = g_queue_new(); > -- > 2.47.1
Hi Barnabás, Thank you for the patch. On Sun, Dec 15, 2024 at 06:18:38PM +0000, Barnabás Pőcze wrote: > `FrameBufferAllocator::allocate()` might return a negative error code, > but currently this is handled the same way as success. So instead > of continuing, abort the construction. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/gstreamer/gstlibcameraallocator.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp > index 7e4c904da..b0c84893a 100644 > --- a/src/gstreamer/gstlibcameraallocator.cpp > +++ b/src/gstreamer/gstlibcameraallocator.cpp > @@ -214,7 +214,7 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera, > Stream *stream = streamCfg.stream(); > > ret = self->fb_allocator->allocate(stream); > - if (ret == 0) > + if (ret <= 0) > return nullptr; > > GQueue *pool = g_queue_new();
On Sun, Dec 15, 2024 at 06:20:57PM +0000, Barnabás Pőcze wrote: > Another thing: FrameBufferAllocator does not consider it an error > if the underlying Camera's `exportFrameBuffers()` returns. I'd be worried if the function never returned :-) Did you mean if it returns 0 ? Maybe we could return -ENOMEM in that case, and document that 0 is not a valid return value ? > This should be clarified. > > 2024. december 15., vasárnap 19:18 keltezéssel, Barnabás Pőcze írta: > > > `FrameBufferAllocator::allocate()` might return a negative error code, > > but currently this is handled the same way as success. So instead > > of continuing, abort the construction. > > > > Signed-off-by: Barnabás Pőcze pobrn@protonmail.com > > > > --- > > src/gstreamer/gstlibcameraallocator.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp > > index 7e4c904da..b0c84893a 100644 > > --- a/src/gstreamer/gstlibcameraallocator.cpp > > +++ b/src/gstreamer/gstlibcameraallocator.cpp > > @@ -214,7 +214,7 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera, > > > > Stream *stream = streamCfg.stream(); > > > > ret = self->fb_allocator->allocate(stream); > > > > - if (ret == 0) > > + if (ret <= 0) > > return nullptr; > > > > GQueue *pool = g_queue_new();
2024. december 15., vasárnap 22:38 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > On Sun, Dec 15, 2024 at 06:20:57PM +0000, Barnabás Pőcze wrote: > > > Another thing: FrameBufferAllocator does not consider it an error > > if the underlying Camera's `exportFrameBuffers()` returns. > > > I'd be worried if the function never returned :-) Did you mean if it > returns 0 ? Maybe we could return -ENOMEM in that case, and document > that 0 is not a valid return value ? Yes, "returns 0" is what I wanted to write. Regards, Barnabás Pőcze > > > This should be clarified. > > > > 2024. december 15., vasárnap 19:18 keltezéssel, Barnabás Pőcze írta: > > > > > `FrameBufferAllocator::allocate()` might return a negative error code, > > > but currently this is handled the same way as success. So instead > > > of continuing, abort the construction. > > > > > > Signed-off-by: Barnabás Pőcze pobrn@protonmail.com > > > > > > --- > > > src/gstreamer/gstlibcameraallocator.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp > > > index 7e4c904da..b0c84893a 100644 > > > --- a/src/gstreamer/gstlibcameraallocator.cpp > > > +++ b/src/gstreamer/gstlibcameraallocator.cpp > > > @@ -214,7 +214,7 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera, > > > > > > Stream *stream = streamCfg.stream(); > > > > > > ret = self->fb_allocator->allocate(stream); > > > > > > - if (ret == 0) > > > + if (ret <= 0) > > > return nullptr; > > > > > > GQueue *pool = g_queue_new(); > > > -- > Regards, > > Laurent Pinchart
diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp index 7e4c904da..b0c84893a 100644 --- a/src/gstreamer/gstlibcameraallocator.cpp +++ b/src/gstreamer/gstlibcameraallocator.cpp @@ -214,7 +214,7 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera, Stream *stream = streamCfg.stream(); ret = self->fb_allocator->allocate(stream); - if (ret == 0) + if (ret <= 0) return nullptr; GQueue *pool = g_queue_new();
`FrameBufferAllocator::allocate()` might return a negative error code, but currently this is handled the same way as success. So instead of continuing, abort the construction. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/gstreamer/gstlibcameraallocator.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)