[libcamera-devel] Docs about buffers

[Kieran Bingham]

Quoting Kieran Bingham (2021-11-18 17:31:24)
> Quoting Dorota Czaplejewicz (2021-11-18 17:06:23)
> > On Thu, 18 Nov 2021 15:32:23 +0000
> > Kieran Bingham <kieran.bingham at ideasonboard.com> wrote:
> > 
> > > I would suspect that the move to make FrameBuffer 'extensible' hasn't
> > > moved all possible things to the private implementation yet. Cancelling
> > > a buffer shouldn't be exposed from the public API.
> > > 
> > > If you'd like to fix this as an exercise to help you learn the code
> > > base, I'll let you fix this. If you don't want to - I'll fix it and
> > > submit a patch. But this might be an 'easy' task that might help you dig
> > > deeper around the codebase, so I don't want to take the fix away from
> > > you if you want it.
> > > 
> > > FrameBuffer::cancel() should likely be moved to
> > > FrameBuffer::Private::cancel().
> > > 
> > > You'll see that there is an 'include/libcamera/internal/framebuffer.h'
> > > with the 'private' implementation, while
> > > 'include/libcamera/framebuffer.h' has the public API version.
> > 
> > I'll take it for the learning experience.
> > 
> > From what I'm seeing, the intention is to have the public class final, and the private class extensible, regardless of member visibility.
> 
> Yes, the objects in the Public API should more or less all become
> 'Extensible' so that we can change the internal implementation without
> breaking the public ABI/interface.
> 
> This is sometimes known as the Pimpl ... or 'private implementation'
> 
>   https://en.cppreference.com/w/cpp/language/pimpl
> 
>  
> > I'm not familiar with this pattern, so is the plan to move all public API members of private visibility to the private class? I don't see a need for them after all, as they will never be accessible from interfacing code.
> 
> Not all public API members no. Only 'private' ones.
> You've found one that is in public but as you've noted, shouldn't be
> exposed to applications as a public API - it's an internal detail.
> 
> Pipeline handlers will 'know' when to cancel a buffer, because they are
> responsible for handling the flow of buffers along the pipeline. So you
> will see that if a pipeline handler detects it is shutting down, or
> streamOff or such - it's likely got to handle 'cancelling' any currently
> queued buffers.
> 
> (The same will happen with Requests, which can only be cancelled by
> Pipeline handlers, but that is not yet Extensible... it's coming very
> soon though).
> 
> 
> 
> > 
> > I'm asking because ::cancel references MetaData, which is currently part of the public API too, and the ::Private class holds no reference to it.
> 
> The private class can obtain the public version through the O_PTR
> 
> The only code example using this so far is the CameraManager...
> 
> src/libcamera/camera_manager.cpp:
> void CameraManager::Private::createPipelineHandlers()
> {
>         CameraManager *const o = LIBCAMERA_O_PTR();
>         ...
> }
> 
> 
> If needed, the metadata could also get moved into the private
> allocation, and the 
>   const FrameMetadata &metadata() const;
> 
> method would then have to be implemented in the public class as an
> internal call to retrieve it from the private class.

Note that public and private instances of a class are 'friends' so their
internal implementations can access each others members.

So the FrameBuffer::Private::cancel() can access FrameBuffer::metadata_
directly (through the O_PTR).

> And now I'm fearing if this becomes more of a rabbit hole, than an easy
> quick 'get started' task. Feel free to ping me on any questions on
> IRC/Matrix if you need anything.
> 
> --
> Kieran