[libcamera-devel] [PATCH 02/13] gstreamer: Inline gst_libcamera_buffer_get_frame_buffer()

[Laurent Pinchart]

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 at 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);

-- 
Regards,

Laurent Pinchart