| Message ID | c4b4be65-e615-3077-fee0-bfcdbb3f9f39@ideasonboard.com |
|---|---|
| State | Rejected |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 07/05/2021 06:24, Umang Jain wrote: > Hi all, > > If I compile the master branch with --prefix=/usr > /meson --prefix=/usr -Dpipelines=uvcvideo,ipu3 build && sudo ninja -C > build install/ > > > The headers turns out to have a directory structure as below: > > > [uajain@localhost libcamera]$ tree /usr/include/libcamera/ > /usr/include/libcamera/ > ├── ipa > │ ├── ipa_controls.h > │ ├── ipa_interface.h > │ └── ipa_module_info.h > └── libcamera > ├── bound_method.h > ├── buffer.h > ├── camera.h > ├── camera_manager.h > ├── class.h > ├── compiler.h > ├── control_ids.h > ├── controls.h > ├── file_descriptor.h > ├── formats.h > ├── framebuffer_allocator.h > ├── geometry.h > ├── libcamera.h > ├── logging.h > ├── object.h > ├── pixel_format.h > ├── property_ids.h > ├── request.h > ├── signal.h > ├── span.h > ├── stream.h > ├── transform.h > └── version.h > > 2 directories, 26 files > > > My question is, is this how we want public headers to be placed ? For > e.g. the absolute path of signal.h > is/usr/include/libcamera/libcamera/signal.h, hence, even the below diff > will compile successfully (due to linker find path magic) > > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp > index cd83c684..5f005d1b 100644 > --- a/src/libcamera/object.cpp > +++ b/src/libcamera/object.cpp > @@ -9,7 +9,7 @@ > > #include <algorithm> > > -#include <libcamera/signal.h> > +#include <libcamera/libcamera/signal.h> This gets 'found' because the include path is including "/usr/include/" and your include statement specifies the double libcamera, which then maps to the full /usr/include/libcamera/libcamera/signal.h path. However, it's not correct to do so. The path there is: /usr/include/<project>/<component>/<header.h> In this instance, both the project and the component have the same name. > Considering the case where we also need to expose some of the internal > libcamera headers on system locations (for IPAs to use) - It's better to > kick off some dicussions around how we would structure the header > locations. This will also clear some of my ongoing meson cleanup work > (not all patches, but only a couple). We already have /usr/include/libcamera/ipa/ for IPA interfaces to be installed to. The issue you are facing is that the IPA interfaces are referencing 'internal' and not installed or public headers from <sourcetree>/include/libcamera/internal/<private files> We need to identify which files are required by the IPA interfaces, and find a way (or location) to install them, which makes them available to the IPA interface, but makes it clear that they are /not/ a part of the public API interfaces, and should not be accessible by applications. Given that they are only to be used by the IPA, we could perhaps install a subset of them into libcamera/ipa/internal/ or something like that. I would like to be able to see a list of what files are required. That would help in how we decide what files need abstracting or installing, and where they should go. -- Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Kieran On 5/7/21 2:30 PM, Kieran Bingham wrote: > Hi Umang, > > On 07/05/2021 06:24, Umang Jain wrote: >> Hi all, >> >> If I compile the master branch with --prefix=/usr >> /meson --prefix=/usr -Dpipelines=uvcvideo,ipu3 build && sudo ninja -C >> build install/ >> >> >> The headers turns out to have a directory structure as below: >> >> >> [uajain@localhost libcamera]$ tree /usr/include/libcamera/ >> /usr/include/libcamera/ >> ├── ipa >> │ ├── ipa_controls.h >> │ ├── ipa_interface.h >> │ └── ipa_module_info.h >> └── libcamera >> ├── bound_method.h >> ├── buffer.h >> ├── camera.h >> ├── camera_manager.h >> ├── class.h >> ├── compiler.h >> ├── control_ids.h >> ├── controls.h >> ├── file_descriptor.h >> ├── formats.h >> ├── framebuffer_allocator.h >> ├── geometry.h >> ├── libcamera.h >> ├── logging.h >> ├── object.h >> ├── pixel_format.h >> ├── property_ids.h >> ├── request.h >> ├── signal.h >> ├── span.h >> ├── stream.h >> ├── transform.h >> └── version.h >> >> 2 directories, 26 files >> >> >> My question is, is this how we want public headers to be placed ? For >> e.g. the absolute path of signal.h >> is/usr/include/libcamera/libcamera/signal.h, hence, even the below diff >> will compile successfully (due to linker find path magic) >> >> diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp >> index cd83c684..5f005d1b 100644 >> --- a/src/libcamera/object.cpp >> +++ b/src/libcamera/object.cpp >> @@ -9,7 +9,7 @@ >> >> #include <algorithm> >> >> -#include <libcamera/signal.h> >> +#include <libcamera/libcamera/signal.h> > > This gets 'found' because the include path is including "/usr/include/" > and your include statement specifies the double libcamera, which then > maps to the full /usr/include/libcamera/libcamera/signal.h path. > > However, it's not correct to do so. > > The path there is: > > /usr/include/<project>/<component>/<header.h> Ok, this makes it clear - probably we should document <project> vs <component> split somewhere? > > In this instance, both the project and the component have the same name. > >> Considering the case where we also need to expose some of the internal >> libcamera headers on system locations (for IPAs to use) - It's better to >> kick off some dicussions around how we would structure the header >> locations. This will also clear some of my ongoing meson cleanup work >> (not all patches, but only a couple). > We already have /usr/include/libcamera/ipa/ for IPA interfaces to be > installed to. > > The issue you are facing is that the IPA interfaces are referencing > 'internal' and not installed or public headers from > <sourcetree>/include/libcamera/internal/<private files> This + internal headers required for the actual IPA If I take current status quo: The IPA interface/usr/include/libcamera/ipa/ipa_interface.h needs internal header libcamera/internal/camera_sensor.h And now I were to talk about the actual IPA (out-of-tree one), it would require #include "libcamera/internal/log.h" #include "libcamera/internal/file.h" #include "libcamera/internal/buffer.h" *1 > We need to identify which files are required by the IPA interfaces, and > find a way (or location) to install them, which makes them available to > the IPA interface, but makes it clear that they are /not/ a part of the > public API interfaces, and should not be accessible by applications. Do you mean the headers(*1) for actual IPA should be provided via the IPA interface, i.e. the IPA interface shall wrap any internal headers the /actual/ IPA might require? > > Given that they are only to be used by the IPA, we could perhaps install > a subset of them into libcamera/ipa/internal/ or something like that. > > I would like to be able to see a list of what files are required. That > would help in how we decide what files need abstracting or installing, > and where they should go. > > -- > Kieran > > >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >>
On Fri, May 07, 2021 at 04:12:46PM +0530, Umang Jain wrote: > On 5/7/21 2:30 PM, Kieran Bingham wrote: > > On 07/05/2021 06:24, Umang Jain wrote: > >> Hi all, > >> > >> If I compile the master branch with --prefix=/usr > >> /meson --prefix=/usr -Dpipelines=uvcvideo,ipu3 build && sudo ninja -C > >> build install/ > >> > >> > >> The headers turns out to have a directory structure as below: > >> > >> > >> [uajain@localhost libcamera]$ tree /usr/include/libcamera/ > >> /usr/include/libcamera/ > >> ├── ipa > >> │ ├── ipa_controls.h > >> │ ├── ipa_interface.h > >> │ └── ipa_module_info.h > >> └── libcamera > >> ├── bound_method.h > >> ├── buffer.h > >> ├── camera.h > >> ├── camera_manager.h > >> ├── class.h > >> ├── compiler.h > >> ├── control_ids.h > >> ├── controls.h > >> ├── file_descriptor.h > >> ├── formats.h > >> ├── framebuffer_allocator.h > >> ├── geometry.h > >> ├── libcamera.h > >> ├── logging.h > >> ├── object.h > >> ├── pixel_format.h > >> ├── property_ids.h > >> ├── request.h > >> ├── signal.h > >> ├── span.h > >> ├── stream.h > >> ├── transform.h > >> └── version.h > >> > >> 2 directories, 26 files > >> > >> > >> My question is, is this how we want public headers to be placed ? For > >> e.g. the absolute path of signal.h > >> is/usr/include/libcamera/libcamera/signal.h, hence, even the below diff > >> will compile successfully (due to linker find path magic) > >> > >> diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp > >> index cd83c684..5f005d1b 100644 > >> --- a/src/libcamera/object.cpp > >> +++ b/src/libcamera/object.cpp > >> @@ -9,7 +9,7 @@ > >> > >> #include <algorithm> > >> > >> -#include <libcamera/signal.h> > >> +#include <libcamera/libcamera/signal.h> > > > > This gets 'found' because the include path is including "/usr/include/" > > and your include statement specifies the double libcamera, which then > > maps to the full /usr/include/libcamera/libcamera/signal.h path. > > > > However, it's not correct to do so. > > > > The path there is: > > > > /usr/include/<project>/<component>/<header.h> > > Ok, this makes it clear - probably we should document <project> vs > <component> split somewhere? If you look at the pkgconfig file that meson generates, we have prefix=/usr/local libdir=${prefix}/lib64 includedir=${prefix}/include Name: libcamera Description: Complex Camera Support Library Version: 1.0 Libs: -L${libdir} -lcamera Cflags: -I${includedir}/libcamera The <project> part of the include path will likely become 'libcamera-1.0' once we release a first stable version. It will be handled by pkgconfig. The <component> part of the path will always be 'libcamera', and that's the part that has to be specified in the #include directive. > > In this instance, both the project and the component have the same name. > > > >> Considering the case where we also need to expose some of the internal > >> libcamera headers on system locations (for IPAs to use) - It's better to > >> kick off some dicussions around how we would structure the header > >> locations. This will also clear some of my ongoing meson cleanup work > >> (not all patches, but only a couple). > > > > We already have /usr/include/libcamera/ipa/ for IPA interfaces to be > > installed to. > > > > The issue you are facing is that the IPA interfaces are referencing > > 'internal' and not installed or public headers from > > <sourcetree>/include/libcamera/internal/<private files> > > This + internal headers required for the actual IPA > > If I take current status quo: > The IPA interface/usr/include/libcamera/ipa/ipa_interface.h needs > internal header libcamera/internal/camera_sensor.h We need to either move CameraSensorInfo to IPA headers (just the class, not the whole camera_sensor.h), or create an IPACameraSensorInfo that can be constructed from a CameraSensorInfo. > And now I were to talk about the actual IPA (out-of-tree one), it would > require > > #include "libcamera/internal/log.h" > #include "libcamera/internal/file.h" > #include "libcamera/internal/buffer.h" > *1 > > > We need to identify which files are required by the IPA interfaces, and > > find a way (or location) to install them, which makes them available to > > the IPA interface, but makes it clear that they are /not/ a part of the > > public API interfaces, and should not be accessible by applications. > > Do you mean the headers(*1) for actual IPA should be provided via the > IPA interface, i.e. the IPA interface shall wrap any internal headers > the /actual/ IPA might require? > > > Given that they are only to be used by the IPA, we could perhaps install > > a subset of them into libcamera/ipa/internal/ or something like that. They're not part of libipa, I'd rather split them to a utils library (whose ABI will not be guaranteed as stable). > > I would like to be able to see a list of what files are required. That > > would help in how we decide what files need abstracting or installing, > > and where they should go.
diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp index cd83c684..5f005d1b 100644 --- a/src/libcamera/object.cpp +++ b/src/libcamera/object.cpp @@ -9,7 +9,7 @@ #include <algorithm> -#include <libcamera/signal.h> +#include <libcamera/libcamera/signal.h> Considering the case where we also need to expose some of the internal