| Message ID | 20260121090705.274081-2-uajain@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
2026. 01. 21. 10:07 keltezéssel, Umang Jain írta: > Directly include libcamera headers instead of including them indirectly > via frame_generator.h. > > Signed-off-by: Umang Jain <uajain@igalia.com> > --- Looks ok. Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > src/libcamera/pipeline/virtual/image_frame_generator.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h > index 42a077ba..851ddbc0 100644 > --- a/src/libcamera/pipeline/virtual/image_frame_generator.h > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > @@ -13,6 +13,9 @@ > #include <sys/types.h> > #include <vector> > > +#include <libcamera/framebuffer.h> > +#include <libcamera/geometry.h> > + > #include "frame_generator.h" > > namespace libcamera {
Hi Umang, Thank you for the patch. On Wed, Jan 21, 2026 at 02:37:03PM +0530, Umang Jain wrote: > Directly include libcamera headers instead of including them indirectly > via frame_generator.h. > > Signed-off-by: Umang Jain <uajain@igalia.com> > --- > src/libcamera/pipeline/virtual/image_frame_generator.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h > index 42a077ba..851ddbc0 100644 > --- a/src/libcamera/pipeline/virtual/image_frame_generator.h > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > @@ -13,6 +13,9 @@ > #include <sys/types.h> > #include <vector> > > +#include <libcamera/framebuffer.h> > +#include <libcamera/geometry.h> > + The types from those headers used in the ImageFrameGenerator class (FrameBuffer and Size) are used as arguments of virtual functions defined in the base FrameGenerator class. They are therefore guaranteed to be provided by the base class header (either by including framebuffer.h and geometry.h, or by using forward declarations). There's no need to include the headers explicitly. > #include "frame_generator.h" > > namespace libcamera {
2026. 01. 21. 22:35 keltezéssel, Laurent Pinchart írta: > Hi Umang, > > Thank you for the patch. > > On Wed, Jan 21, 2026 at 02:37:03PM +0530, Umang Jain wrote: >> Directly include libcamera headers instead of including them indirectly >> via frame_generator.h. >> >> Signed-off-by: Umang Jain <uajain@igalia.com> >> --- >> src/libcamera/pipeline/virtual/image_frame_generator.h | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h >> index 42a077ba..851ddbc0 100644 >> --- a/src/libcamera/pipeline/virtual/image_frame_generator.h >> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h >> @@ -13,6 +13,9 @@ >> #include <sys/types.h> >> #include <vector> >> >> +#include <libcamera/framebuffer.h> >> +#include <libcamera/geometry.h> >> + > > The types from those headers used in the ImageFrameGenerator class > (FrameBuffer and Size) are used as arguments of virtual functions > defined in the base FrameGenerator class. They are therefore guaranteed > to be provided by the base class header (either by including > framebuffer.h and geometry.h, or by using forward declarations). There's > no need to include the headers explicitly. These are also included in `test_pattern_generator.cpp`, so I think one could argue that this makes things more consistent. > >> #include "frame_generator.h" >> >> namespace libcamera { >
On Thu, Jan 22, 2026 at 02:36:33PM +0100, Barnabás Pőcze wrote: > 2026. 01. 21. 22:35 keltezéssel, Laurent Pinchart írta: > > On Wed, Jan 21, 2026 at 02:37:03PM +0530, Umang Jain wrote: > >> Directly include libcamera headers instead of including them indirectly > >> via frame_generator.h. > >> > >> Signed-off-by: Umang Jain <uajain@igalia.com> > >> --- > >> src/libcamera/pipeline/virtual/image_frame_generator.h | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h > >> index 42a077ba..851ddbc0 100644 > >> --- a/src/libcamera/pipeline/virtual/image_frame_generator.h > >> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h > >> @@ -13,6 +13,9 @@ > >> #include <sys/types.h> > >> #include <vector> > >> > >> +#include <libcamera/framebuffer.h> > >> +#include <libcamera/geometry.h> > >> + > > > > The types from those headers used in the ImageFrameGenerator class > > (FrameBuffer and Size) are used as arguments of virtual functions > > defined in the base FrameGenerator class. They are therefore guaranteed > > to be provided by the base class header (either by including > > framebuffer.h and geometry.h, or by using forward declarations). There's > > no need to include the headers explicitly. > > These are also included in `test_pattern_generator.cpp`, so I think one could > argue that this makes things more consistent. Do you mean this patch makes things more consistent, or my proposal makes things more consistent ? The current rules (which we never documented) are - don't rely on indirect includes - use forward declarations as much as possible in .h files, and include headers in .cpp files - when a type is guaranteed to be provided indirectly because we inherit from a base class that provides it, the .h file can skip the forward declaration > >> #include "frame_generator.h" > >> > >> namespace libcamera {
2026. 01. 23. 0:43 keltezéssel, Laurent Pinchart írta: > On Thu, Jan 22, 2026 at 02:36:33PM +0100, Barnabás Pőcze wrote: >> 2026. 01. 21. 22:35 keltezéssel, Laurent Pinchart írta: >>> On Wed, Jan 21, 2026 at 02:37:03PM +0530, Umang Jain wrote: >>>> Directly include libcamera headers instead of including them indirectly >>>> via frame_generator.h. >>>> >>>> Signed-off-by: Umang Jain <uajain@igalia.com> >>>> --- >>>> src/libcamera/pipeline/virtual/image_frame_generator.h | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h >>>> index 42a077ba..851ddbc0 100644 >>>> --- a/src/libcamera/pipeline/virtual/image_frame_generator.h >>>> +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h >>>> @@ -13,6 +13,9 @@ >>>> #include <sys/types.h> >>>> #include <vector> >>>> >>>> +#include <libcamera/framebuffer.h> >>>> +#include <libcamera/geometry.h> >>>> + >>> >>> The types from those headers used in the ImageFrameGenerator class >>> (FrameBuffer and Size) are used as arguments of virtual functions >>> defined in the base FrameGenerator class. They are therefore guaranteed >>> to be provided by the base class header (either by including >>> framebuffer.h and geometry.h, or by using forward declarations). There's >>> no need to include the headers explicitly. >> >> These are also included in `test_pattern_generator.cpp`, so I think one could >> argue that this makes things more consistent. > > Do you mean this patch makes things more consistent, or my proposal > makes things more consistent ? > > The current rules (which we never documented) are > > - don't rely on indirect includes > - use forward declarations as much as possible in .h files, and include > headers in .cpp files > - when a type is guaranteed to be provided indirectly because we inherit > from a base class that provides it, the .h file can skip the forward > declaration > I believe what I wanted to say is that either adding the headers (as done in this change), or removing them from `test_pattern_generator.cpp` makes things more consistent. >>>> #include "frame_generator.h" >>>> >>>> namespace libcamera { >
diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.h b/src/libcamera/pipeline/virtual/image_frame_generator.h index 42a077ba..851ddbc0 100644 --- a/src/libcamera/pipeline/virtual/image_frame_generator.h +++ b/src/libcamera/pipeline/virtual/image_frame_generator.h @@ -13,6 +13,9 @@ #include <sys/types.h> #include <vector> +#include <libcamera/framebuffer.h> +#include <libcamera/geometry.h> + #include "frame_generator.h" namespace libcamera {
Directly include libcamera headers instead of including them indirectly via frame_generator.h. Signed-off-by: Umang Jain <uajain@igalia.com> --- src/libcamera/pipeline/virtual/image_frame_generator.h | 3 +++ 1 file changed, 3 insertions(+)