[libcamera-devel] [PATCH] src: gstreamer: use the streams of CameraConfiguration when allocating buffers

[Laurent Pinchart]

Hi Dafna,

On Mon, Mar 15, 2021 at 12:35:15AM +0200, Laurent Pinchart wrote:
> On Thu, Feb 25, 2021 at 04:41:39PM +0100, Dafna Hirschfeld wrote:
> > On 24.02.21 19:33, Nicolas Dufresne wrote:
> > > Le mercredi 24 février 2021 à 18:19 +0100, Dafna Hirschfeld a écrit :
> > >> Currently, when allocating buffers, the streams of
> > >> the Camera object are used. Instead the streams of
> > >> the CameraConfiguration object should be used.
> > > 
> > > The changes looks fine, but perhaps you could extend, I wrote this, still I
> > > don't understand why enumerating the streams in the configuration would make any
> > > difference after the camera has been configured.
> > 
> > The Camera holds a list of all available streams.
> > For rkisp1 those are mainpath and selfpath.
> > Then only subset of those streams are chosen which are
> > the stream kept in the CameraConfiguration object.
> > Specifically using gstreamer with rkisp1 I got:
> > 
> > [3:59:32.003159780] [2499] ERROR Allocator framebuffer_allocator.cpp:97 Stream is not part of /base/i2c at ff3d0000/sensor at 10 active configuration
> > 
> > > Minor comment, other GStreamer patches commit message was simply "gst:
> > > Description", just mentioning for consistency, but perhaps there is new rules.
> > 
> > Ill send v2,
> 
> Do you still plan to send a v2 ?

I see you've sent it already, sorry about that.

> > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > >>
> > >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld at collabora.com>
> > >> ---
> > >>   src/gstreamer/gstlibcameraallocator.cpp | 6 ++++--
> > >>   src/gstreamer/gstlibcameraallocator.h   | 4 +++-
> > >>   src/gstreamer/gstlibcamerasrc.cpp       | 2 +-
> > >>   3 files changed, 8 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/src/gstreamer/gstlibcameraallocator.cpp
> > >> b/src/gstreamer/gstlibcameraallocator.cpp
> > >> index 13c6b493..7bd8ba2d 100644
> > >> --- a/src/gstreamer/gstlibcameraallocator.cpp
> > >> +++ b/src/gstreamer/gstlibcameraallocator.cpp
> > >> @@ -183,13 +183,15 @@ gst_libcamera_allocator_class_init(GstLibcameraAllocatorClass *klass)
> > >>   }
> > >>   
> > >>   GstLibcameraAllocator *
> > >> -gst_libcamera_allocator_new(std::shared_ptr<Camera> camera)
> > >> +gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
> > >> +                           CameraConfiguration *config_)
> > >>   {
> > >>          auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> > >>                                                            nullptr));
> > >>   
> > >>          self->fb_allocator = new FrameBufferAllocator(camera);
> > >> -       for (Stream *stream : camera->streams()) {
> > >> +       for (StreamConfiguration &streamCfg : *config_) {
> > >> +               Stream *stream = streamCfg.stream();
> > >>                  gint ret;
> > >>   
> > >>                  ret = self->fb_allocator->allocate(stream);
> > >> diff --git a/src/gstreamer/gstlibcameraallocator.h
> > >> b/src/gstreamer/gstlibcameraallocator.h
> > >> index befdcad6..f7fa6acd 100644
> > >> --- a/src/gstreamer/gstlibcameraallocator.h
> > >> +++ b/src/gstreamer/gstlibcameraallocator.h
> > >> @@ -13,12 +13,14 @@
> > >>   #include <gst/allocators/allocators.h>
> > >>   
> > >>   #include <libcamera/stream.h>
> > >> +#include <libcamera/camera.h>
> 
> Alphabetical order please :-)
> 
> With this addressed, and an updated commit message,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > >>   
> > >>   #define GST_TYPE_LIBCAMERA_ALLOCATOR gst_libcamera_allocator_get_type()
> > >>   G_DECLARE_FINAL_TYPE(GstLibcameraAllocator, gst_libcamera_allocator,
> > >>                       GST_LIBCAMERA, ALLOCATOR, GstDmaBufAllocator)
> > >>   
> > >> -GstLibcameraAllocator *gst_libcamera_allocator_new(std::shared_ptr<libcamera::Camera> camera);
> > >> + GstLibcameraAllocator *gst_libcamera_allocator_new(std::shared_ptr<libcamera::Camera> camera,
> > >> +						   libcamera::CameraConfiguration *config_);
> > >>   
> > >>   bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,
> > >>                                              libcamera::Stream *stream,
> > >> diff --git a/src/gstreamer/gstlibcamerasrc.cpp
> > >> b/src/gstreamer/gstlibcamerasrc.cpp
> > >> index 636c14df..7b13667b 100644
> > >> --- a/src/gstreamer/gstlibcamerasrc.cpp
> > >> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > >> @@ -429,7 +429,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
> > >>                  return;
> > >>          }
> > >>   
> > >> -       self->allocator = gst_libcamera_allocator_new(state->cam_);
> > >> +       self->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get());
> > >>          if (!self->allocator) {
> > >>                  GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,
> > >>                                    ("Failed to allocate memory"),

-- 
Regards,

Laurent Pinchart