[libcamera-devel] Debayering

Tue Nov 16 15:15:21 CET 2021

Hi Jacopo,

On Tue, 16 Nov 2021 13:48:59 +0100
Jacopo Mondi <jacopo at jmondi.org> wrote:

> Hi Dorota,
> 
> On Tue, Nov 16, 2021 at 12:41:50PM +0100, Dorota Czaplejewicz wrote:
> > On Mon, 15 Nov 2021 12:16:39 +0200
> > Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:
> >  
> > > Hi Dorota,
> > >
> > > On Sat, Nov 13, 2021 at 11:56:04AM +0100, Dorota Czaplejewicz wrote:  
> > > >
> > > > "Quick hacks" is not what I intend to merge. Rather, I found the
> > > > libcamera codebase difficult to learn, and I don't see the point in  
> > >
> > > Is there anything in particular that would have made it easier ?  
> >
> > After digging in some more, I can point out some things already.
> >
> > Initially I thought there was nearly zero documentation on the internals. I used three ways to access the code: stepping through executing code, reading header files, and looking through the documentation web site.  
> 
> even if we strive for 'zero-Doxygen warnings' (aka all the public
> fields in the external and internal API should have a documentation
> block associated) I understand putting all the pieces together might
> not be easy. Can I suggest you to have a look at the pipeline handler
> development guide in $builddir/Documentation/html/guides/pipeline-handler.html
> which might be a bit stale in some parts but gives an overview about
> the internal interaction between the Camera and the pipeline
> handlers. We do also have an IPA implementer guide in
> $builddir//Documentation/html/guides/ipa.html which should instead
> provide an overview of the pipeline handler - IPA interaction model.

Thanks, I'll check them out.
> 
> When it comes to the single classes, as said, we aim to zero doxygen
> warnings and all publicly visible fields in the code base are
> documented. For sure the documentation has to be expanded and
> enriched but if there's one thing we tried to be inflexible about was
> having a piece of documentation associated with each (internally and
> externally) visible component of the library.
> 
> >
> > Then I discovered that there's documentation in the beginning of the .cpp file, as well as some strings interspersed through some files (notably, simple.cpp, and camera.cpp). But it took me a long time to notice them, and I only found out where the documentation for base classes is hours after I started looking at the code.  
> 
> Well, each core library .cpp has Doxygen documentation for the file,
> the classes it contains and all public visible fields in the class. The
> /** block are pretty visible. As you mention camera.cpp, it starts
> with a 80 lines documentation block, it's hard to miss it :)
> 
It's actually trivial to miss – I don't look at the beginning of a file when single-stepping, I don't look at it when I switch from the declaration to the definition (my IDE takes me to the relevant line), and HTML docs seem to skip it. I think it's still useful to document a file, but I also think that documenting a file is not more important than documenting classes and methods – files are mostly orthogonal to the code structure they contain.

> The single pipeline handler implementations are not Doxygen documented
> as they all implement the same interface as defined by the
> PipelineHandler base class. We could reconsider that if we have a way
> to formally document each implementation without demanding all public
> fields to have a (redundant) documentation block associated.
> 
If we consider SimpleCameraData as part of the handler implementation, then I would consider that underdocumented. All fields containing "converter" are left without description, leaving me to guess their purpose. Missing extra information about how frame buffers work, I had to spend some time trying to figure out what kind of buffers are stored in "converterQueue_", and why. My current hypothesis: allocated but invalid buffers, where output frames will be placed. Presumably to avoid allocating them anew at each frame. I don't really know why each item is a map though.

> >
> > The main problem with learning libcamera is that it's complex. I don't yet know if it's incidental or not, having discovered the docs literally 5 minutes ago.
> >
> > The other problem is that documentation is far separated from the things it documents. Part of that is caused by the choice of C++, which splits headers and source files, which necessarily creates a choice between:
> > 1. prototypes being separate from documentation,
> > 2. bodies being separate from documentation,
> > 3. documentation being duplicated, or
> > 4. documentation being entirely separate from code, like needing a separate browser.
> >
> > In the case of class fields and abstract methods, there's only one place where they are mentioned, which is the declaration. In libcamera, they are not documented there, but instead the documentation is inserted in the corresponding cpp file, which… doesn't have either the declaration of the body of the documented item, making it honestly confusing. I think moving those to header files would have saved me a lot of wandering about the sources.
> >  
> We initially discusses if it was more appropriate to document in the
> header files or in the .cpp files, and as reported by the codying
> style document we decided to go for the .cpp to keep documentation
> closer to implementation. Looking back, the documentation is mostly
> about the API, so we could have gone with documentation in the header
> as well, but honestly once the documentation is compiled it doesn't
> make much difference.
> 
> Do you find the Doxygen documentation unhelpful because not enough
> complete ? Or what is missing is a general overview of the internal
> architecture ?
> 
I find HTML docs unhelpful for two reasons:

1. the docs skip private members, so they don't give a complete picture (for me, seeing the shape of the data is most important for learning the architecture),
2. when I'm in my editor at, say, SimpleCameraData::setupFormats(), and I want to know what it is, I need to switch to the browser, go to the class list, find it, click, and then find the member which interests me, before coming back to the editor. That is frustrating compared to having the description right next to the definition.

As for comments being separated from the declarations, there's an additional downside: I will normally never think about looking for abstract items among the definitions. Abstract means that there is no definition, so there's no reason to switch to the .cpp file. My IDE includes a C++ code model, and it agrees with me: searching for PipelineHandler::generateConfiguration doesn't yield any results in pipeline_handler.cpp, so it will never lead me to the documentation.

Cheers,
Dorota
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211116/a0d7b94d/attachment-0001.sig>