| Message ID | 20241211152542.1095857-1-pobrn@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Wed, Dec 11, 2024 at 03:25:45PM +0000, Barnabás Pőcze wrote: > There is no reason make copies, these functions return > const lvalue references, access the data through those. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> with one comment below. > --- > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +- > src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 2 +- > src/libcamera/pipeline/virtual/virtual.cpp | 3 +-- > 3 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > index 277efbb09..d1545b5d9 100644 > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > @@ -129,7 +129,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > > MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > > - auto planes = mappedFrameBuffer.planes(); > + const auto &planes = mappedFrameBuffer.planes(); > > /* Loop only around the number of images available */ > frameIndex_ %= imageFrameDatas_.size(); > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > index 7bc2b338c..47d341919 100644 > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > @@ -25,7 +25,7 @@ int TestPatternGenerator::generateFrame(const Size &size, > MappedFrameBuffer mappedFrameBuffer(buffer, > MappedFrameBuffer::MapFlag::Write); > > - auto planes = mappedFrameBuffer.planes(); > + const auto &planes = mappedFrameBuffer.planes(); > > shiftLeft(size); > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > index 1b7cd5cb3..3126bdd7d 100644 > --- a/src/libcamera/pipeline/virtual/virtual.cpp > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > @@ -275,8 +275,7 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera, > return -ENOBUFS; > > const StreamConfiguration &config = stream->configuration(); > - > - auto info = PixelFormatInfo::info(config.pixelFormat); > + const PixelFormatInfo &info = PixelFormatInfo::info(config.pixelFormat); There's also a similar issue in src/libcamera/pipeline/imx8-isi/imx8-isi.cpp: const PixelFormatInfo info = PixelFormatInfo::info(config_[0].pixelFormat); It would be nice to disable the PixelFormatInfo copy constructor (using LIBCAMERA_DISABLE_COPY() or even LIBCAMERA_DISABLE_COPY_AND_MOVE()), but that interferes with the designated initializer syntax we use in formats.cpp to populate the pixelFormatInfo array as aggregate initialization is disabled for types that have user-declared constructors (even if deleted). One option would be to define a constructor for the class, but we then lose the ability to use designated initializers. Maybe that's not a big issue ? Could you have a look at this ? > > std::vector<unsigned int> planeSizes; > for (size_t i = 0; i < info.planes.size(); ++i)
Hi 2024. december 11., szerda 17:40 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > Hi Barnabás, > > Thank you for the patch. > > On Wed, Dec 11, 2024 at 03:25:45PM +0000, Barnabás Pőcze wrote: > > There is no reason make copies, these functions return > > const lvalue references, access the data through those. > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > with one comment below. > > > --- > > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +- > > src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 2 +- > > src/libcamera/pipeline/virtual/virtual.cpp | 3 +-- > > 3 files changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > index 277efbb09..d1545b5d9 100644 > > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > @@ -129,7 +129,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > > > > MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > > > > - auto planes = mappedFrameBuffer.planes(); > > + const auto &planes = mappedFrameBuffer.planes(); > > > > /* Loop only around the number of images available */ > > frameIndex_ %= imageFrameDatas_.size(); > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > index 7bc2b338c..47d341919 100644 > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > @@ -25,7 +25,7 @@ int TestPatternGenerator::generateFrame(const Size &size, > > MappedFrameBuffer mappedFrameBuffer(buffer, > > MappedFrameBuffer::MapFlag::Write); > > > > - auto planes = mappedFrameBuffer.planes(); > > + const auto &planes = mappedFrameBuffer.planes(); > > > > shiftLeft(size); > > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > > index 1b7cd5cb3..3126bdd7d 100644 > > --- a/src/libcamera/pipeline/virtual/virtual.cpp > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > > @@ -275,8 +275,7 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera, > > return -ENOBUFS; > > > > const StreamConfiguration &config = stream->configuration(); > > - > > - auto info = PixelFormatInfo::info(config.pixelFormat); > > + const PixelFormatInfo &info = PixelFormatInfo::info(config.pixelFormat); > > There's also a similar issue in src/libcamera/pipeline/imx8-isi/imx8-isi.cpp: > > const PixelFormatInfo info = PixelFormatInfo::info(config_[0].pixelFormat); > > It would be nice to disable the PixelFormatInfo copy constructor (using > LIBCAMERA_DISABLE_COPY() or even LIBCAMERA_DISABLE_COPY_AND_MOVE()), but > that interferes with the designated initializer syntax we use in > formats.cpp to populate the pixelFormatInfo array as aggregate > initialization is disabled for types that have user-declared > constructors (even if deleted). One option would be to define a > constructor for the class, but we then lose the ability to use > designated initializers. Maybe that's not a big issue ? > > Could you have a look at this ? > [...] That map is initialized from an `std::initializer_list` and that provides a `const` view of the data, so the map copy constructs the elements from the list. Deleting the copy constructor would make that not work. I think using an array and `std::move_iterator` could maybe work around that. Returning a pointer from `PixelFormatInfo::info()` would also eliminate this kind of issue. Also, there is the "performance-unnecessary-copy-initialization" clang-tidy check, but it seems it does not want to diagnose this situation. (Seemingly because the reference is returned by a static member function.) Regards, Barnabás Pőcze
Quoting Barnabás Pőcze (2024-12-11 15:25:45) > There is no reason make copies, these functions return > const lvalue references, access the data through those. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +- > src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 2 +- > src/libcamera/pipeline/virtual/virtual.cpp | 3 +-- > 3 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > index 277efbb09..d1545b5d9 100644 > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > @@ -129,7 +129,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > > MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > > - auto planes = mappedFrameBuffer.planes(); > + const auto &planes = mappedFrameBuffer.planes(); > > /* Loop only around the number of images available */ > frameIndex_ %= imageFrameDatas_.size(); > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > index 7bc2b338c..47d341919 100644 > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > @@ -25,7 +25,7 @@ int TestPatternGenerator::generateFrame(const Size &size, > MappedFrameBuffer mappedFrameBuffer(buffer, > MappedFrameBuffer::MapFlag::Write); > > - auto planes = mappedFrameBuffer.planes(); > + const auto &planes = mappedFrameBuffer.planes(); > > shiftLeft(size); > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > index 1b7cd5cb3..3126bdd7d 100644 > --- a/src/libcamera/pipeline/virtual/virtual.cpp > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > @@ -275,8 +275,7 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera, > return -ENOBUFS; > > const StreamConfiguration &config = stream->configuration(); > - > - auto info = PixelFormatInfo::info(config.pixelFormat); > + const PixelFormatInfo &info = PixelFormatInfo::info(config.pixelFormat); > > std::vector<unsigned int> planeSizes; > for (size_t i = 0; i < info.planes.size(); ++i) > -- > 2.47.1 > >
Hi Barnabás, On Wed, Dec 11, 2024 at 05:10:23PM +0000, Barnabás Pőcze wrote: > 2024. december 11., szerda 17:40 keltezéssel, Laurent Pinchart írta: > > On Wed, Dec 11, 2024 at 03:25:45PM +0000, Barnabás Pőcze wrote: > > > There is no reason make copies, these functions return > > > const lvalue references, access the data through those. > > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > with one comment below. > > > > > --- > > > src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +- > > > src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 2 +- > > > src/libcamera/pipeline/virtual/virtual.cpp | 3 +-- > > > 3 files changed, 3 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > index 277efbb09..d1545b5d9 100644 > > > --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp > > > @@ -129,7 +129,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff > > > > > > MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); > > > > > > - auto planes = mappedFrameBuffer.planes(); > > > + const auto &planes = mappedFrameBuffer.planes(); > > > > > > /* Loop only around the number of images available */ > > > frameIndex_ %= imageFrameDatas_.size(); > > > diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > index 7bc2b338c..47d341919 100644 > > > --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp > > > @@ -25,7 +25,7 @@ int TestPatternGenerator::generateFrame(const Size &size, > > > MappedFrameBuffer mappedFrameBuffer(buffer, > > > MappedFrameBuffer::MapFlag::Write); > > > > > > - auto planes = mappedFrameBuffer.planes(); > > > + const auto &planes = mappedFrameBuffer.planes(); > > > > > > shiftLeft(size); > > > > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > > > index 1b7cd5cb3..3126bdd7d 100644 > > > --- a/src/libcamera/pipeline/virtual/virtual.cpp > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > > > @@ -275,8 +275,7 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera, > > > return -ENOBUFS; > > > > > > const StreamConfiguration &config = stream->configuration(); > > > - > > > - auto info = PixelFormatInfo::info(config.pixelFormat); > > > + const PixelFormatInfo &info = PixelFormatInfo::info(config.pixelFormat); > > > > There's also a similar issue in src/libcamera/pipeline/imx8-isi/imx8-isi.cpp: > > > > const PixelFormatInfo info = PixelFormatInfo::info(config_[0].pixelFormat); > > > > It would be nice to disable the PixelFormatInfo copy constructor (using > > LIBCAMERA_DISABLE_COPY() or even LIBCAMERA_DISABLE_COPY_AND_MOVE()), but > > that interferes with the designated initializer syntax we use in > > formats.cpp to populate the pixelFormatInfo array as aggregate > > initialization is disabled for types that have user-declared > > constructors (even if deleted). One option would be to define a > > constructor for the class, but we then lose the ability to use > > designated initializers. Maybe that's not a big issue ? > > > > Could you have a look at this ? > > [...] > > That map is initialized from an `std::initializer_list` and that provides a `const` > view of the data, so the map copy constructs the elements from the list. Deleting > the copy constructor would make that not work. I think using an array and `std::move_iterator` > could maybe work around that. > > Returning a pointer from `PixelFormatInfo::info()` would also eliminate this kind of issue. > > Also, there is the "performance-unnecessary-copy-initialization" clang-tidy check, > but it seems it does not want to diagnose this situation. (Seemingly because > the reference is returned by a static member function.) OK, let's ignore this for the time being then.
diff --git a/src/libcamera/pipeline/virtual/image_frame_generator.cpp b/src/libcamera/pipeline/virtual/image_frame_generator.cpp index 277efbb09..d1545b5d9 100644 --- a/src/libcamera/pipeline/virtual/image_frame_generator.cpp +++ b/src/libcamera/pipeline/virtual/image_frame_generator.cpp @@ -129,7 +129,7 @@ int ImageFrameGenerator::generateFrame(const Size &size, const FrameBuffer *buff MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); - auto planes = mappedFrameBuffer.planes(); + const auto &planes = mappedFrameBuffer.planes(); /* Loop only around the number of images available */ frameIndex_ %= imageFrameDatas_.size(); diff --git a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp index 7bc2b338c..47d341919 100644 --- a/src/libcamera/pipeline/virtual/test_pattern_generator.cpp +++ b/src/libcamera/pipeline/virtual/test_pattern_generator.cpp @@ -25,7 +25,7 @@ int TestPatternGenerator::generateFrame(const Size &size, MappedFrameBuffer mappedFrameBuffer(buffer, MappedFrameBuffer::MapFlag::Write); - auto planes = mappedFrameBuffer.planes(); + const auto &planes = mappedFrameBuffer.planes(); shiftLeft(size); diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp index 1b7cd5cb3..3126bdd7d 100644 --- a/src/libcamera/pipeline/virtual/virtual.cpp +++ b/src/libcamera/pipeline/virtual/virtual.cpp @@ -275,8 +275,7 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera, return -ENOBUFS; const StreamConfiguration &config = stream->configuration(); - - auto info = PixelFormatInfo::info(config.pixelFormat); + const PixelFormatInfo &info = PixelFormatInfo::info(config.pixelFormat); std::vector<unsigned int> planeSizes; for (size_t i = 0; i < info.planes.size(); ++i)
There is no reason make copies, these functions return const lvalue references, access the data through those. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/libcamera/pipeline/virtual/image_frame_generator.cpp | 2 +- src/libcamera/pipeline/virtual/test_pattern_generator.cpp | 2 +- src/libcamera/pipeline/virtual/virtual.cpp | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-)