Message ID | 20200628161723.30625-2-jacopo@jmondi.org |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Jacopo, Thanks for your work. On 2020-06-28 18:17:21 +0200, Jacopo Mondi wrote: > The generateConfiguration() implementation does not check if the > requested list of roles can actually be satisfied. The camera API > documentation prescribes the function shall fail in that case, instead > of silently adjust the returned confiuguration. > > Fix this by implementing the same logic as the validate() function > implements, as the pipeline handler supports one raw stream and up to > two output streams. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index dcd737a1d1a0..d1338b640e3c 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -526,6 +526,8 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > if (roles.empty()) > return config; > > + unsigned int rawCount = 0; > + unsigned int outCount = 0; > for (const StreamRole role : roles) { > switch (role) { > case StreamRole::StillCaptureRaw: > @@ -535,6 +537,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > pixelFormat = sensorFormat.fourcc.toPixelFormat(); > ASSERT(pixelFormat.isValid()); > bufferCount = 1; > + rawCount++; > break; > > case StreamRole::StillCapture: > @@ -543,6 +546,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > /* Return the largest sensor resolution. */ > size = data->sensor_->resolution(); > bufferCount = 1; > + outCount++; > break; > > case StreamRole::VideoRecording: > @@ -550,6 +554,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > pixelFormat = formats::NV12; > size = { 1920, 1080 }; > bufferCount = 4; > + outCount++; > break; > > case StreamRole::Viewfinder: > @@ -557,6 +562,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > pixelFormat = formats::ARGB8888; > size = { 800, 600 }; > bufferCount = 4; > + outCount++; > break; > > default: > @@ -565,6 +571,11 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > break; > } > > + if (rawCount > 1 || outCount > 2) { > + delete config; > + return nullptr; > + } > + > /* Translate the V4L2PixelFormat to PixelFormat. */ > std::map<PixelFormat, std::vector<SizeRange>> deviceFormats; > std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.end()), > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. On Sun, Jun 28, 2020 at 06:17:21PM +0200, Jacopo Mondi wrote: > The generateConfiguration() implementation does not check if the > requested list of roles can actually be satisfied. The camera API > documentation prescribes the function shall fail in that case, instead > of silently adjust the returned confiuguration. s/confiuguration/configuration/ > Fix this by implementing the same logic as the validate() function > implements, as the pipeline handler supports one raw stream and up to > two output streams. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index dcd737a1d1a0..d1338b640e3c 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -526,6 +526,8 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > if (roles.empty()) > return config; > > + unsigned int rawCount = 0; > + unsigned int outCount = 0; > for (const StreamRole role : roles) { > switch (role) { > case StreamRole::StillCaptureRaw: > @@ -535,6 +537,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > pixelFormat = sensorFormat.fourcc.toPixelFormat(); > ASSERT(pixelFormat.isValid()); > bufferCount = 1; > + rawCount++; > break; > > case StreamRole::StillCapture: > @@ -543,6 +546,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > /* Return the largest sensor resolution. */ > size = data->sensor_->resolution(); > bufferCount = 1; > + outCount++; > break; > > case StreamRole::VideoRecording: > @@ -550,6 +554,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > pixelFormat = formats::NV12; > size = { 1920, 1080 }; > bufferCount = 4; > + outCount++; > break; > > case StreamRole::Viewfinder: > @@ -557,6 +562,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > pixelFormat = formats::ARGB8888; > size = { 800, 600 }; > bufferCount = 4; > + outCount++; > break; > > default: > @@ -565,6 +571,11 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > break; > } > > + if (rawCount > 1 || outCount > 2) { Would an error message be a good idea ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + delete config; > + return nullptr; > + } > + > /* Translate the V4L2PixelFormat to PixelFormat. */ > std::map<PixelFormat, std::vector<SizeRange>> deviceFormats; > std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.end()),
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index dcd737a1d1a0..d1338b640e3c 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -526,6 +526,8 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, if (roles.empty()) return config; + unsigned int rawCount = 0; + unsigned int outCount = 0; for (const StreamRole role : roles) { switch (role) { case StreamRole::StillCaptureRaw: @@ -535,6 +537,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, pixelFormat = sensorFormat.fourcc.toPixelFormat(); ASSERT(pixelFormat.isValid()); bufferCount = 1; + rawCount++; break; case StreamRole::StillCapture: @@ -543,6 +546,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, /* Return the largest sensor resolution. */ size = data->sensor_->resolution(); bufferCount = 1; + outCount++; break; case StreamRole::VideoRecording: @@ -550,6 +554,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, pixelFormat = formats::NV12; size = { 1920, 1080 }; bufferCount = 4; + outCount++; break; case StreamRole::Viewfinder: @@ -557,6 +562,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, pixelFormat = formats::ARGB8888; size = { 800, 600 }; bufferCount = 4; + outCount++; break; default: @@ -565,6 +571,11 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, break; } + if (rawCount > 1 || outCount > 2) { + delete config; + return nullptr; + } + /* Translate the V4L2PixelFormat to PixelFormat. */ std::map<PixelFormat, std::vector<SizeRange>> deviceFormats; std::transform(fmts.begin(), fmts.end(), std::inserter(deviceFormats, deviceFormats.end()),
The generateConfiguration() implementation does not check if the requested list of roles can actually be satisfied. The camera API documentation prescribes the function shall fail in that case, instead of silently adjust the returned confiuguration. Fix this by implementing the same logic as the validate() function implements, as the pipeline handler supports one raw stream and up to two output streams. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+)