[v1,2/3] libcamera: pipeline: virtual: Fill buffer's metadata
diff mbox series

Message ID 20250203104318.135628-3-pobrn@protonmail.com
State Accepted
Headers show
Series
  • libcamera: pipeline: virtual: Fill buffer's metadata
Related show

Commit Message

Barnabás Pőcze Feb. 3, 2025, 10:43 a.m. UTC
Fill the `FrameMetadata` object of the `FrameBuffer`s because it
should not be left uninitialized as users expect to be able to
access it and find reasonable data there.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=245
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 src/libcamera/pipeline/virtual/virtual.cpp | 18 +++++++++++++++++-
 src/libcamera/pipeline/virtual/virtual.h   |  1 +
 2 files changed, 18 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Feb. 3, 2025, 11:04 a.m. UTC | #1
Quoting Barnabás Pőcze (2025-02-03 10:43:31)
> Fill the `FrameMetadata` object of the `FrameBuffer`s because it
> should not be left uninitialized as users expect to be able to
> access it and find reasonable data there.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=245
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>

Aha, also looks good. Does this get caught by lc-compliance now ?


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/libcamera/pipeline/virtual/virtual.cpp | 18 +++++++++++++++++-
>  src/libcamera/pipeline/virtual/virtual.h   |  1 +
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> index 94c2bd91e..1a75f35aa 100644
> --- a/src/libcamera/pipeline/virtual/virtual.cpp
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -286,6 +286,11 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,
>  int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,
>                                   [[maybe_unused]] const ControlList *controls)
>  {
> +       VirtualCameraData *data = cameraData(camera);
> +
> +       for (auto &s : data->streamConfigs_)
> +               s.seq = 0;
> +
>         return 0;
>  }
>  
> @@ -297,13 +302,24 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
>                                                Request *request)
>  {
>         VirtualCameraData *data = cameraData(camera);
> +       const auto timestamp = currentTimestamp();
>  
>         for (auto const &[stream, buffer] : request->buffers()) {
>                 bool found = false;
>                 /* map buffer and fill test patterns */
>                 for (auto &streamConfig : data->streamConfigs_) {
>                         if (stream == &streamConfig.stream) {
> +                               FrameMetadata &fmd = buffer->_d()->metadata();
> +
> +                               fmd.status = FrameMetadata::Status::FrameSuccess;
> +                               fmd.sequence = streamConfig.seq++;
> +                               fmd.timestamp = timestamp;
> +
> +                               for (const auto [i, p] : utils::enumerate(buffer->planes()))
> +                                       fmd.planes()[i].bytesused = p.length;
> +
>                                 found = true;
> +
>                                 if (streamConfig.frameGenerator->generateFrame(
>                                             stream->configuration().size, buffer))
>                                         buffer->_d()->cancel();
> @@ -315,7 +331,7 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
>                 ASSERT(found);
>         }
>  
> -       request->metadata().set(controls::SensorTimestamp, currentTimestamp());
> +       request->metadata().set(controls::SensorTimestamp, timestamp);
>         completeRequest(request);
>  
>         return 0;
> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h
> index 92ad7d4a9..683cb82b4 100644
> --- a/src/libcamera/pipeline/virtual/virtual.h
> +++ b/src/libcamera/pipeline/virtual/virtual.h
> @@ -37,6 +37,7 @@ public:
>         struct StreamConfig {
>                 Stream stream;
>                 std::unique_ptr<FrameGenerator> frameGenerator;
> +               unsigned int seq = 0;
>         };
>         /* The config file is parsed to the Configuration struct */
>         struct Configuration {
> -- 
> 2.48.1
> 
>
Barnabás Pőcze Feb. 3, 2025, 11:14 a.m. UTC | #2
2025. február 3., hétfő 12:04 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> Quoting Barnabás Pőcze (2025-02-03 10:43:31)
> > Fill the `FrameMetadata` object of the `FrameBuffer`s because it
> > should not be left uninitialized as users expect to be able to
> > access it and find reasonable data there.
> >
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=245
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> 
> Aha, also looks good. Does this get caught by lc-compliance now ?

Not yet.


> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > ---
> >  src/libcamera/pipeline/virtual/virtual.cpp | 18 +++++++++++++++++-
> >  src/libcamera/pipeline/virtual/virtual.h   |  1 +
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> > index 94c2bd91e..1a75f35aa 100644
> > --- a/src/libcamera/pipeline/virtual/virtual.cpp
> > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > @@ -286,6 +286,11 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,
> >  int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,
> >                                   [[maybe_unused]] const ControlList *controls)
> >  {
> > +       VirtualCameraData *data = cameraData(camera);
> > +
> > +       for (auto &s : data->streamConfigs_)
> > +               s.seq = 0;
> > +
> >         return 0;
> >  }
> >
> > @@ -297,13 +302,24 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
> >                                                Request *request)
> >  {
> >         VirtualCameraData *data = cameraData(camera);
> > +       const auto timestamp = currentTimestamp();
> >
> >         for (auto const &[stream, buffer] : request->buffers()) {
> >                 bool found = false;
> >                 /* map buffer and fill test patterns */
> >                 for (auto &streamConfig : data->streamConfigs_) {
> >                         if (stream == &streamConfig.stream) {
> > +                               FrameMetadata &fmd = buffer->_d()->metadata();
> > +
> > +                               fmd.status = FrameMetadata::Status::FrameSuccess;
> > +                               fmd.sequence = streamConfig.seq++;
> > +                               fmd.timestamp = timestamp;
> > +
> > +                               for (const auto [i, p] : utils::enumerate(buffer->planes()))
> > +                                       fmd.planes()[i].bytesused = p.length;
> > +
> >                                 found = true;
> > +
> >                                 if (streamConfig.frameGenerator->generateFrame(
> >                                             stream->configuration().size, buffer))
> >                                         buffer->_d()->cancel();
> > @@ -315,7 +331,7 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
> >                 ASSERT(found);
> >         }
> >
> > -       request->metadata().set(controls::SensorTimestamp, currentTimestamp());
> > +       request->metadata().set(controls::SensorTimestamp, timestamp);
> >         completeRequest(request);
> >
> >         return 0;
> > diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h
> > index 92ad7d4a9..683cb82b4 100644
> > --- a/src/libcamera/pipeline/virtual/virtual.h
> > +++ b/src/libcamera/pipeline/virtual/virtual.h
> > @@ -37,6 +37,7 @@ public:
> >         struct StreamConfig {
> >                 Stream stream;
> >                 std::unique_ptr<FrameGenerator> frameGenerator;
> > +               unsigned int seq = 0;
> >         };
> >         /* The config file is parsed to the Configuration struct */
> >         struct Configuration {
> > --
> > 2.48.1
> >
> >
>
Jacopo Mondi Feb. 3, 2025, 5:33 p.m. UTC | #3
Hi Barnabás

On Mon, Feb 03, 2025 at 10:43:31AM +0000, Barnabás Pőcze wrote:
> Fill the `FrameMetadata` object of the `FrameBuffer`s because it
> should not be left uninitialized as users expect to be able to
> access it and find reasonable data there.
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=245
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  src/libcamera/pipeline/virtual/virtual.cpp | 18 +++++++++++++++++-
>  src/libcamera/pipeline/virtual/virtual.h   |  1 +
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
> index 94c2bd91e..1a75f35aa 100644
> --- a/src/libcamera/pipeline/virtual/virtual.cpp
> +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> @@ -286,6 +286,11 @@ int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,
>  int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,
>  				  [[maybe_unused]] const ControlList *controls)
>  {
> +	VirtualCameraData *data = cameraData(camera);
> +
> +	for (auto &s : data->streamConfigs_)
> +		s.seq = 0;
> +
>  	return 0;
>  }
>
> @@ -297,13 +302,24 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
>  					       Request *request)
>  {
>  	VirtualCameraData *data = cameraData(camera);
> +	const auto timestamp = currentTimestamp();
>
>  	for (auto const &[stream, buffer] : request->buffers()) {
>  		bool found = false;
>  		/* map buffer and fill test patterns */
>  		for (auto &streamConfig : data->streamConfigs_) {
>  			if (stream == &streamConfig.stream) {
> +				FrameMetadata &fmd = buffer->_d()->metadata();
> +
> +				fmd.status = FrameMetadata::Status::FrameSuccess;
> +				fmd.sequence = streamConfig.seq++;
> +				fmd.timestamp = timestamp;
> +
> +				for (const auto [i, p] : utils::enumerate(buffer->planes()))
> +					fmd.planes()[i].bytesused = p.length;
> +
>  				found = true;
> +
>  				if (streamConfig.frameGenerator->generateFrame(
>  					    stream->configuration().size, buffer))
>  					buffer->_d()->cancel();
> @@ -315,7 +331,7 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
>  		ASSERT(found);
>  	}
>
> -	request->metadata().set(controls::SensorTimestamp, currentTimestamp());
> +	request->metadata().set(controls::SensorTimestamp, timestamp);
>  	completeRequest(request);
>
>  	return 0;
> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h
> index 92ad7d4a9..683cb82b4 100644
> --- a/src/libcamera/pipeline/virtual/virtual.h
> +++ b/src/libcamera/pipeline/virtual/virtual.h
> @@ -37,6 +37,7 @@ public:
>  	struct StreamConfig {
>  		Stream stream;
>  		std::unique_ptr<FrameGenerator> frameGenerator;
> +		unsigned int seq = 0;
>  	};
>  	/* The config file is parsed to the Configuration struct */
>  	struct Configuration {
> --
> 2.48.1
>
>

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp
index 94c2bd91e..1a75f35aa 100644
--- a/src/libcamera/pipeline/virtual/virtual.cpp
+++ b/src/libcamera/pipeline/virtual/virtual.cpp
@@ -286,6 +286,11 @@  int PipelineHandlerVirtual::exportFrameBuffers([[maybe_unused]] Camera *camera,
 int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,
 				  [[maybe_unused]] const ControlList *controls)
 {
+	VirtualCameraData *data = cameraData(camera);
+
+	for (auto &s : data->streamConfigs_)
+		s.seq = 0;
+
 	return 0;
 }
 
@@ -297,13 +302,24 @@  int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
 					       Request *request)
 {
 	VirtualCameraData *data = cameraData(camera);
+	const auto timestamp = currentTimestamp();
 
 	for (auto const &[stream, buffer] : request->buffers()) {
 		bool found = false;
 		/* map buffer and fill test patterns */
 		for (auto &streamConfig : data->streamConfigs_) {
 			if (stream == &streamConfig.stream) {
+				FrameMetadata &fmd = buffer->_d()->metadata();
+
+				fmd.status = FrameMetadata::Status::FrameSuccess;
+				fmd.sequence = streamConfig.seq++;
+				fmd.timestamp = timestamp;
+
+				for (const auto [i, p] : utils::enumerate(buffer->planes()))
+					fmd.planes()[i].bytesused = p.length;
+
 				found = true;
+
 				if (streamConfig.frameGenerator->generateFrame(
 					    stream->configuration().size, buffer))
 					buffer->_d()->cancel();
@@ -315,7 +331,7 @@  int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,
 		ASSERT(found);
 	}
 
-	request->metadata().set(controls::SensorTimestamp, currentTimestamp());
+	request->metadata().set(controls::SensorTimestamp, timestamp);
 	completeRequest(request);
 
 	return 0;
diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h
index 92ad7d4a9..683cb82b4 100644
--- a/src/libcamera/pipeline/virtual/virtual.h
+++ b/src/libcamera/pipeline/virtual/virtual.h
@@ -37,6 +37,7 @@  public:
 	struct StreamConfig {
 		Stream stream;
 		std::unique_ptr<FrameGenerator> frameGenerator;
+		unsigned int seq = 0;
 	};
 	/* The config file is parsed to the Configuration struct */
 	struct Configuration {