| Message ID | 20220623232210.18742-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, thanks for the patch. Le vendredi 24 juin 2022 à 02:21 +0300, Laurent Pinchart a écrit : > The gst_libcamera_buffer_get_frame_buffer() function retrieves a > FrameBuffer corresponding to the GstBuffer. While the GstLibcameraPool > class allocates the GstBuffer instances, associating them to a > FrameBuffer is handled by GstLibcameraAllocator. The > gst_libcamera_buffer_get_frame_buffer() function is ill-placed in > gstlibcamerapool.cpp. Move it to gstlibcameraallocator.cpp, which allows > inlining it in its single caller, simplifying the code flow. Encapsulation wise, I think it was more appropriate before this change. The details that the FrameBuffer is stored in GstMemory as a Quark data, is internal implementation of the allocator class, while the detail of the GstMemory being only memory from our allocator is internal implementation detail of the pool class (since the pool is the buffer factory. As you are seeking for an optimized call stack, you could perhaps must inline these two helpers into their respective headers ? regards, Nicolas > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/gstreamer/gstlibcameraallocator.cpp | 3 ++- > src/gstreamer/gstlibcameraallocator.h | 2 +- > src/gstreamer/gstlibcamerapool.cpp | 7 ------- > src/gstreamer/gstlibcamerapool.h | 2 -- > 4 files changed, 3 insertions(+), 11 deletions(-) > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp > index c740b8fc82a8..53693834eeff 100644 > --- a/src/gstreamer/gstlibcameraallocator.cpp > +++ b/src/gstreamer/gstlibcameraallocator.cpp > @@ -252,8 +252,9 @@ gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self, > } > > FrameBuffer * > -gst_libcamera_memory_get_frame_buffer(GstMemory *mem) > +gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer) > { > + GstMemory *mem = gst_buffer_peek_memory(buffer, 0); > auto *frame = reinterpret_cast<FrameWrap *>(gst_mini_object_get_qdata(GST_MINI_OBJECT_CAST(mem), > FrameWrap::getQuark())); > return frame->buffer_; > diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h > index 0a08c3bb3bbe..b1c038c50257 100644 > --- a/src/gstreamer/gstlibcameraallocator.h > +++ b/src/gstreamer/gstlibcameraallocator.h > @@ -28,4 +28,4 @@ bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self, > gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator, > libcamera::Stream *stream); > > -libcamera::FrameBuffer *gst_libcamera_memory_get_frame_buffer(GstMemory *mem); > +libcamera::FrameBuffer *gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer); > diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp > index 1fde42135119..118bc6db7067 100644 > --- a/src/gstreamer/gstlibcamerapool.cpp > +++ b/src/gstreamer/gstlibcamerapool.cpp > @@ -140,10 +140,3 @@ gst_libcamera_buffer_get_stream(GstBuffer *buffer) > auto *self = (GstLibcameraPool *)buffer->pool; > return self->stream; > } > - > -FrameBuffer * > -gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer) > -{ > - GstMemory *mem = gst_buffer_peek_memory(buffer, 0); > - return gst_libcamera_memory_get_frame_buffer(mem); > -} > diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h > index 05795d21223e..06b38cb296fc 100644 > --- a/src/gstreamer/gstlibcamerapool.h > +++ b/src/gstreamer/gstlibcamerapool.h > @@ -26,5 +26,3 @@ GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator, > libcamera::Stream *gst_libcamera_pool_get_stream(GstLibcameraPool *self); > > libcamera::Stream *gst_libcamera_buffer_get_stream(GstBuffer *buffer); > - > -libcamera::FrameBuffer *gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer);
Hi Nicolas, On Mon, Jun 27, 2022 at 04:44:23PM -0400, Nicolas Dufresne wrote: > Hi Laurent, > > thanks for the patch. > > Le vendredi 24 juin 2022 à 02:21 +0300, Laurent Pinchart a écrit : > > The gst_libcamera_buffer_get_frame_buffer() function retrieves a > > FrameBuffer corresponding to the GstBuffer. While the GstLibcameraPool > > class allocates the GstBuffer instances, associating them to a > > FrameBuffer is handled by GstLibcameraAllocator. The > > gst_libcamera_buffer_get_frame_buffer() function is ill-placed in > > gstlibcamerapool.cpp. Move it to gstlibcameraallocator.cpp, which allows > > inlining it in its single caller, simplifying the code flow. > > Encapsulation wise, I think it was more appropriate before this change. The > details that the FrameBuffer is stored in GstMemory as a Quark data, is internal > implementation of the allocator class, while the detail of the GstMemory being > only memory from our allocator is internal implementation detail of the pool > class (since the pool is the buffer factory. > > As you are seeking for an optimized call stack, you could perhaps must inline > these two helpers into their respective headers ? I wrote this patch when trying to understand the libcamerasrc code, and thought it was first and foremost an improvement in the abstraction, with the nice side effect that the gst_libcamera_buffer_get_frame_buffer() function could then be inlined. If you don't think it makes the abstraction better, then I'll drop it, the rest of the series doesn't depend on it. I'd still like to improve the allocator and pool implementation as I find the way they're intertwined a bit confusing, but that can be done later, maybe when adding a pool of requests. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/gstreamer/gstlibcameraallocator.cpp | 3 ++- > > src/gstreamer/gstlibcameraallocator.h | 2 +- > > src/gstreamer/gstlibcamerapool.cpp | 7 ------- > > src/gstreamer/gstlibcamerapool.h | 2 -- > > 4 files changed, 3 insertions(+), 11 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp > > index c740b8fc82a8..53693834eeff 100644 > > --- a/src/gstreamer/gstlibcameraallocator.cpp > > +++ b/src/gstreamer/gstlibcameraallocator.cpp > > @@ -252,8 +252,9 @@ gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self, > > } > > > > FrameBuffer * > > -gst_libcamera_memory_get_frame_buffer(GstMemory *mem) > > +gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer) > > { > > + GstMemory *mem = gst_buffer_peek_memory(buffer, 0); > > auto *frame = reinterpret_cast<FrameWrap *>(gst_mini_object_get_qdata(GST_MINI_OBJECT_CAST(mem), > > FrameWrap::getQuark())); > > return frame->buffer_; > > diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h > > index 0a08c3bb3bbe..b1c038c50257 100644 > > --- a/src/gstreamer/gstlibcameraallocator.h > > +++ b/src/gstreamer/gstlibcameraallocator.h > > @@ -28,4 +28,4 @@ bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self, > > gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator, > > libcamera::Stream *stream); > > > > -libcamera::FrameBuffer *gst_libcamera_memory_get_frame_buffer(GstMemory *mem); > > +libcamera::FrameBuffer *gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer); > > diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp > > index 1fde42135119..118bc6db7067 100644 > > --- a/src/gstreamer/gstlibcamerapool.cpp > > +++ b/src/gstreamer/gstlibcamerapool.cpp > > @@ -140,10 +140,3 @@ gst_libcamera_buffer_get_stream(GstBuffer *buffer) > > auto *self = (GstLibcameraPool *)buffer->pool; > > return self->stream; > > } > > - > > -FrameBuffer * > > -gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer) > > -{ > > - GstMemory *mem = gst_buffer_peek_memory(buffer, 0); > > - return gst_libcamera_memory_get_frame_buffer(mem); > > -} > > diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h > > index 05795d21223e..06b38cb296fc 100644 > > --- a/src/gstreamer/gstlibcamerapool.h > > +++ b/src/gstreamer/gstlibcamerapool.h > > @@ -26,5 +26,3 @@ GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator, > > libcamera::Stream *gst_libcamera_pool_get_stream(GstLibcameraPool *self); > > > > libcamera::Stream *gst_libcamera_buffer_get_stream(GstBuffer *buffer); > > - > > -libcamera::FrameBuffer *gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer);
diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp index c740b8fc82a8..53693834eeff 100644 --- a/src/gstreamer/gstlibcameraallocator.cpp +++ b/src/gstreamer/gstlibcameraallocator.cpp @@ -252,8 +252,9 @@ gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self, } FrameBuffer * -gst_libcamera_memory_get_frame_buffer(GstMemory *mem) +gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer) { + GstMemory *mem = gst_buffer_peek_memory(buffer, 0); auto *frame = reinterpret_cast<FrameWrap *>(gst_mini_object_get_qdata(GST_MINI_OBJECT_CAST(mem), FrameWrap::getQuark())); return frame->buffer_; diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h index 0a08c3bb3bbe..b1c038c50257 100644 --- a/src/gstreamer/gstlibcameraallocator.h +++ b/src/gstreamer/gstlibcameraallocator.h @@ -28,4 +28,4 @@ bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self, gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator, libcamera::Stream *stream); -libcamera::FrameBuffer *gst_libcamera_memory_get_frame_buffer(GstMemory *mem); +libcamera::FrameBuffer *gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer); diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp index 1fde42135119..118bc6db7067 100644 --- a/src/gstreamer/gstlibcamerapool.cpp +++ b/src/gstreamer/gstlibcamerapool.cpp @@ -140,10 +140,3 @@ gst_libcamera_buffer_get_stream(GstBuffer *buffer) auto *self = (GstLibcameraPool *)buffer->pool; return self->stream; } - -FrameBuffer * -gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer) -{ - GstMemory *mem = gst_buffer_peek_memory(buffer, 0); - return gst_libcamera_memory_get_frame_buffer(mem); -} diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h index 05795d21223e..06b38cb296fc 100644 --- a/src/gstreamer/gstlibcamerapool.h +++ b/src/gstreamer/gstlibcamerapool.h @@ -26,5 +26,3 @@ GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator, libcamera::Stream *gst_libcamera_pool_get_stream(GstLibcameraPool *self); libcamera::Stream *gst_libcamera_buffer_get_stream(GstBuffer *buffer); - -libcamera::FrameBuffer *gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer);
The gst_libcamera_buffer_get_frame_buffer() function retrieves a FrameBuffer corresponding to the GstBuffer. While the GstLibcameraPool class allocates the GstBuffer instances, associating them to a FrameBuffer is handled by GstLibcameraAllocator. The gst_libcamera_buffer_get_frame_buffer() function is ill-placed in gstlibcamerapool.cpp. Move it to gstlibcameraallocator.cpp, which allows inlining it in its single caller, simplifying the code flow. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/gstreamer/gstlibcameraallocator.cpp | 3 ++- src/gstreamer/gstlibcameraallocator.h | 2 +- src/gstreamer/gstlibcamerapool.cpp | 7 ------- src/gstreamer/gstlibcamerapool.h | 2 -- 4 files changed, 3 insertions(+), 11 deletions(-)