[01/27] libcamera: MappedFrameBuffer: Add tracking of mmap file descriptors to MappedFrameBuffer

23214 diff mbox series

In order to import via eglCreateImageKHR via an file descriptor we need to
have access to the file descriptor associated with the plane.

Extend MappedFrameBuffer to track the file descriptor giving an accessor
method in-line with the existing planes_ accessor.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 include/libcamera/internal/mapped_framebuffer.h | 2 ++
 src/libcamera/mapped_framebuffer.cpp            | 1 +
 2 files changed, 3 insertions(+)

Quoting Bryan O'Donoghue (2025-04-22 22:58:54)
> In order to import via eglCreateImageKHR via an file descriptor we need to
> have access to the file descriptor associated with the plane.
> 
> Extend MappedFrameBuffer to track the file descriptor giving an accessor
> method in-line with the existing planes_ accessor.
> 

I expect the answers are later in the series, but I would expect if you
need the file descriptors - you should get them from the FrameBuffer
directly, not the 'MappedFrameBuffer' otherwise you're just making a
copy of something you already have to be able to access it ? (It could
be fine - but depends on what the use case is)



> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  include/libcamera/internal/mapped_framebuffer.h | 2 ++
>  src/libcamera/mapped_framebuffer.cpp            | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
> index 6aaabf50..cb81243e 100644
> --- a/include/libcamera/internal/mapped_framebuffer.h
> +++ b/include/libcamera/internal/mapped_framebuffer.h
> @@ -31,6 +31,7 @@ public:
>         bool isValid() const { return error_ == 0; }
>         int error() const { return error_; }
>         const std::vector<Plane> &planes() const { return planes_; }
> +       const std::vector<int> &fds() const { return fds_; }
>  
>  protected:
>         MappedBuffer();
> @@ -38,6 +39,7 @@ protected:
>         int error_;
>         std::vector<Plane> planes_;
>         std::vector<Plane> maps_;
> +       std::vector<int> fds_;
>  
>  private:
>         LIBCAMERA_DISABLE_COPY(MappedBuffer)
> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> index f54bbf21..82ab132c 100644
> --- a/src/libcamera/mapped_framebuffer.cpp
> +++ b/src/libcamera/mapped_framebuffer.cpp
> @@ -237,6 +237,7 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
>                 }
>  
>                 planes_.emplace_back(info.address + plane.offset, plane.length);
> +               fds_.emplace_back(fd);
>         }
>  }
>  
> -- 
> 2.49.0
>

On Wed, Apr 23, 2025 at 10:35:12AM +0100, Kieran Bingham wrote:
> Quoting Bryan O'Donoghue (2025-04-22 22:58:54)
> > In order to import via eglCreateImageKHR via an file descriptor we need to
> > have access to the file descriptor associated with the plane.
> > 
> > Extend MappedFrameBuffer to track the file descriptor giving an accessor
> > method in-line with the existing planes_ accessor.
> 
> I expect the answers are later in the series, but I would expect if you
> need the file descriptors - you should get them from the FrameBuffer
> directly, not the 'MappedFrameBuffer' otherwise you're just making a
> copy of something you already have to be able to access it ? (It could
> be fine - but depends on what the use case is)

Storing them here as plain integers opens the door to use-after-free. I
also expect answers later in the series, let's see if we can make the
code harder to use incorrectly.

> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> > ---
> >  include/libcamera/internal/mapped_framebuffer.h | 2 ++
> >  src/libcamera/mapped_framebuffer.cpp            | 1 +
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h
> > index 6aaabf50..cb81243e 100644
> > --- a/include/libcamera/internal/mapped_framebuffer.h
> > +++ b/include/libcamera/internal/mapped_framebuffer.h
> > @@ -31,6 +31,7 @@ public:
> >         bool isValid() const { return error_ == 0; }
> >         int error() const { return error_; }
> >         const std::vector<Plane> &planes() const { return planes_; }
> > +       const std::vector<int> &fds() const { return fds_; }
> >  
> >  protected:
> >         MappedBuffer();
> > @@ -38,6 +39,7 @@ protected:
> >         int error_;
> >         std::vector<Plane> planes_;
> >         std::vector<Plane> maps_;
> > +       std::vector<int> fds_;
> >  
> >  private:
> >         LIBCAMERA_DISABLE_COPY(MappedBuffer)
> > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp
> > index f54bbf21..82ab132c 100644
> > --- a/src/libcamera/mapped_framebuffer.cpp
> > +++ b/src/libcamera/mapped_framebuffer.cpp
> > @@ -237,6 +237,7 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)
> >                 }
> >  
> >                 planes_.emplace_back(info.address + plane.offset, plane.length);
> > +               fds_.emplace_back(fd);
> >         }
> >  }
> >