[libcamera-devel] Debayering

[Laurent Pinchart]

On Tue, Nov 16, 2021 at 04:00:11PM +0100, Jacopo Mondi wrote:
> On Tue, Nov 16, 2021 at 03:15:21PM +0100, Dorota Czaplejewicz wrote:
> > On Tue, 16 Nov 2021 13:48:59 +0100 Jacopo Mondi wrote:
> > > On Tue, Nov 16, 2021 at 12:41:50PM +0100, Dorota Czaplejewicz wrote:
> > > > On Mon, 15 Nov 2021 12:16:39 +0200 Laurent Pinchart wrote:
> > > > > 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.

Ouch. If documentation appeared to not exist, no wonder you had trouble.

> > > 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.
> 
> What I meant is that camera.cpp is pretty well documented, and the
> documentation is visible. There's not only the documentation about the
> file, all the classes and each of their public/protected members
> are documented too!
> 
> I can understand a complaint about the documentation quality, but when
> it comes to quantity, hey, there are more documentation lines than
> code lines in that file. It's rather hard to miss whatever editor one
> uses :)
> 
> search for the \class, \fn and \var tags in the code.
> 
> > > 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.
> 
> Documentation is very different from comments in the code in my
> opinion. What you're looking at is a possibly not enough
> commented implementation, not missing documentation of the core
> infrastructure.
> 
> Single pipeline handler implementations are not doxygen-documented, so
> it might be very well possible that some parts are missing. As usual,
> how much to comment on an implementation is subject to discussions.
> Sometimes less is more, sometimes too much is just bad, sometimes as
> in this case less it might be just... not enough :)
> 
> I do however stand by the choice about not enforcing documentation in
> the single pipeline handler implementations, it would be like
> requiring to kernel-doc each single driver, while the only thing that
> matter are the framework components.

We should however add more comments to pipeline handlers, which could
include documenting data structures (staying with the same analogy,
kernel driver often documents their custom structures). I agree that
documentation for individual pipeline handlers should not be compiled.

> > > > 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),
> 
> That's very much true, private members are under-documented. Doxygen
> skips them (I think it's configurable) so they went ignored also
> because we decided to document in the .cpp file and placing comments
> in the .h felt like out of place. But yes, some fields might have been
> worth a comment line. I would be against documenting -all- of them just
> as we do for public/protected members, as private fields rarely have
> value outside of the single class implementations and additional
> comments would be just noise.
> 
> > 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 said we decided at the very beginning to have documentation in the
> .cpp file. I still think it doesn't really make a huge difference as
> long as the documentation is there but I'm sorry if it's causing you
> troubles.
> 
> On this very specific case, for SimpleCameraData::setupFormats() you
> won't find nothing special. It's just a function inside an
> implementation of a pipeline handler, so all the comments relative to
> that function, if any, are in the function definition itself.
> 
> > 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.
> 
> Mmm, as I've said looking back we could have gone for documenting in
> the headers. I'm still not convinced it's 100% better, but yes, sometimes
> we had to do weird things like adding a .cpp file just to contain the
> documentation, so it might have made sense going for the headers.

There are pros and cons, if there were a perfect solution, everybody
would be using it already. Keeping documentation out of header files
means that you can in most cases have an overview of a class in a single
page of header code. As Jacopo mentioned, we've also considered that
storing function documentation in .cpp files would keep it closer to the
implementation, which could change more often than the function
prototype. I'm sure I've also been influenced by Qt when making the
decision. There are of course drawbacks, as mentioned above.

> Doing so now on a codebase of this size seems like an herculean effort
> which I bet nobody is willing to undergo, also considering you can get
> what you want with a little grep-fu. Am I just lazy :) ?

-- 
Regards,

Laurent Pinchart