| Message ID | 20220823174314.14881-6-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Tue, Aug 23, 2022 at 08:43:13PM +0300, Laurent Pinchart via libcamera-devel wrote: I think some changelog would be nice to have :) Paul > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 38 ++++++++++++++++++++---- > 1 file changed, 33 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 03bbe6b467ae..25fbf9f1a0a9 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -18,6 +18,7 @@ > #include <libcamera/base/utils.h> > > #include <libcamera/camera.h> > +#include <libcamera/color_space.h> > #include <libcamera/control_ids.h> > #include <libcamera/formats.h> > #include <libcamera/framebuffer.h> > @@ -416,11 +417,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > { > const CameraSensor *sensor = data_->sensor_.get(); > unsigned int pathCount = data_->selfPath_ ? 2 : 1; > - Status status = Valid; > + Status status; > > if (config_.empty()) > return Invalid; > > + status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace); > + > if (transform != Transform::Identity) { > transform = Transform::Identity; > status = Adjusted; > @@ -547,21 +550,44 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > if (roles.empty()) > return config; > > + /* > + * As the ISP can't output different color spaces for the main and self > + * path, pick a sensible default color space based on the role of the > + * first stream and use it for all streams. > + */ > + std::optional<ColorSpace> colorSpace; > + > bool mainPathAvailable = true; > bool selfPathAvailable = data->selfPath_; > + > for (const StreamRole role : roles) { > bool useMainPath; > > switch (role) { > - case StreamRole::StillCapture: { > + case StreamRole::StillCapture: > useMainPath = mainPathAvailable; > + /* JPEG encoders typically expect sYCC. */ > + if (!colorSpace) > + colorSpace = ColorSpace::Sycc; > break; > - } > + > case StreamRole::Viewfinder: > - case StreamRole::VideoRecording: { > useMainPath = !selfPathAvailable; > + /* > + * sYCC is the YCbCr encoding of sRGB, which is commonly > + * used by displays. > + */ > + if (!colorSpace) > + colorSpace = ColorSpace::Sycc; > break; > - } > + > + case StreamRole::VideoRecording: > + useMainPath = !selfPathAvailable; > + /* Rec. 709 is a good default for HD video recording. */ > + if (!colorSpace) > + colorSpace = ColorSpace::Rec709; > + break; > + > default: > LOG(RkISP1, Warning) > << "Requested stream role not supported: " << role; > @@ -580,6 +606,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > selfPathAvailable = false; > } > > + cfg.colorSpace = colorSpace; > config->addConfiguration(cfg); > } > > @@ -642,6 +669,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > if (ret < 0) > return ret; > > + format.colorSpace = config->at(0).colorSpace; > ret = isp_->setFormat(2, &format); > if (ret < 0) > return ret; > -- > Regards, > > Laurent Pinchart >
Hi Paul, On Thu, Aug 25, 2022 at 03:08:13PM -0500, paul.elder@ideasonboard.com wrote: > On Tue, Aug 23, 2022 at 08:43:13PM +0300, Laurent Pinchart via libcamera-devel wrote: > > I think some changelog would be nice to have :) Oops :-) I'll add Implement color space support in the rkisp1 pipeline handler, in the configuration generation, configuration validation and camera configuration. As all the processing related to the color space is performed in the part of the pipeline shared by all streams, a single color space must cover all stream configurations. This is enforced manually when generating the configuration, and with the validateColorSpaces() helper when validating it. Only the Y'CbCr encoding and quantization range are currently taken into account, and they are programmed through the V4L2 color space API. The primary colors chromaticities and the transfer function need to be configured in the ISP parameters buffer, and thus conveyed to the IPA, but the rkisp1 driver does not currently support tone mapping, so this will be done later. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 38 ++++++++++++++++++++---- > > 1 file changed, 33 insertions(+), 5 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 03bbe6b467ae..25fbf9f1a0a9 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -18,6 +18,7 @@ > > #include <libcamera/base/utils.h> > > > > #include <libcamera/camera.h> > > +#include <libcamera/color_space.h> > > #include <libcamera/control_ids.h> > > #include <libcamera/formats.h> > > #include <libcamera/framebuffer.h> > > @@ -416,11 +417,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > { > > const CameraSensor *sensor = data_->sensor_.get(); > > unsigned int pathCount = data_->selfPath_ ? 2 : 1; > > - Status status = Valid; > > + Status status; > > > > if (config_.empty()) > > return Invalid; > > > > + status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace); > > + > > if (transform != Transform::Identity) { > > transform = Transform::Identity; > > status = Adjusted; > > @@ -547,21 +550,44 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > if (roles.empty()) > > return config; > > > > + /* > > + * As the ISP can't output different color spaces for the main and self > > + * path, pick a sensible default color space based on the role of the > > + * first stream and use it for all streams. > > + */ > > + std::optional<ColorSpace> colorSpace; > > + > > bool mainPathAvailable = true; > > bool selfPathAvailable = data->selfPath_; > > + > > for (const StreamRole role : roles) { > > bool useMainPath; > > > > switch (role) { > > - case StreamRole::StillCapture: { > > + case StreamRole::StillCapture: > > useMainPath = mainPathAvailable; > > + /* JPEG encoders typically expect sYCC. */ > > + if (!colorSpace) > > + colorSpace = ColorSpace::Sycc; > > break; > > - } > > + > > case StreamRole::Viewfinder: > > - case StreamRole::VideoRecording: { > > useMainPath = !selfPathAvailable; > > + /* > > + * sYCC is the YCbCr encoding of sRGB, which is commonly > > + * used by displays. > > + */ > > + if (!colorSpace) > > + colorSpace = ColorSpace::Sycc; > > break; > > - } > > + > > + case StreamRole::VideoRecording: > > + useMainPath = !selfPathAvailable; > > + /* Rec. 709 is a good default for HD video recording. */ > > + if (!colorSpace) > > + colorSpace = ColorSpace::Rec709; > > + break; > > + > > default: > > LOG(RkISP1, Warning) > > << "Requested stream role not supported: " << role; > > @@ -580,6 +606,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > selfPathAvailable = false; > > } > > > > + cfg.colorSpace = colorSpace; > > config->addConfiguration(cfg); > > } > > > > @@ -642,6 +669,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > if (ret < 0) > > return ret; > > > > + format.colorSpace = config->at(0).colorSpace; > > ret = isp_->setFormat(2, &format); > > if (ret < 0) > > return ret;
Hi Laurent, On Thu, Aug 25, 2022 at 11:17:11PM +0300, Laurent Pinchart wrote: > Hi Paul, > > On Thu, Aug 25, 2022 at 03:08:13PM -0500, paul.elder@ideasonboard.com wrote: > > On Tue, Aug 23, 2022 at 08:43:13PM +0300, Laurent Pinchart via libcamera-devel wrote: > > > > I think some changelog would be nice to have :) > > Oops :-) I'll add > > Implement color space support in the rkisp1 pipeline handler, in the > configuration generation, configuration validation and camera > configuration. As all the processing related to the color space is > performed in the part of the pipeline shared by all streams, a single > color space must cover all stream configurations. This is enforced > manually when generating the configuration, and with the > validateColorSpaces() helper when validating it. > > Only the Y'CbCr encoding and quantization range are currently taken into > account, and they are programmed through the V4L2 color space API. The > primary colors chromaticities and the transfer function need to be > configured in the ISP parameters buffer, and thus conveyed to the IPA, > but the rkisp1 driver does not currently support tone mapping, so this > will be done later. Looks good. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 38 ++++++++++++++++++++---- > > > 1 file changed, 33 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 03bbe6b467ae..25fbf9f1a0a9 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -18,6 +18,7 @@ > > > #include <libcamera/base/utils.h> > > > > > > #include <libcamera/camera.h> > > > +#include <libcamera/color_space.h> > > > #include <libcamera/control_ids.h> > > > #include <libcamera/formats.h> > > > #include <libcamera/framebuffer.h> > > > @@ -416,11 +417,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > { > > > const CameraSensor *sensor = data_->sensor_.get(); > > > unsigned int pathCount = data_->selfPath_ ? 2 : 1; > > > - Status status = Valid; > > > + Status status; > > > > > > if (config_.empty()) > > > return Invalid; > > > > > > + status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace); > > > + > > > if (transform != Transform::Identity) { > > > transform = Transform::Identity; > > > status = Adjusted; > > > @@ -547,21 +550,44 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > if (roles.empty()) > > > return config; > > > > > > + /* > > > + * As the ISP can't output different color spaces for the main and self > > > + * path, pick a sensible default color space based on the role of the > > > + * first stream and use it for all streams. > > > + */ > > > + std::optional<ColorSpace> colorSpace; > > > + > > > bool mainPathAvailable = true; > > > bool selfPathAvailable = data->selfPath_; > > > + > > > for (const StreamRole role : roles) { > > > bool useMainPath; > > > > > > switch (role) { > > > - case StreamRole::StillCapture: { > > > + case StreamRole::StillCapture: > > > useMainPath = mainPathAvailable; > > > + /* JPEG encoders typically expect sYCC. */ > > > + if (!colorSpace) > > > + colorSpace = ColorSpace::Sycc; > > > break; > > > - } > > > + > > > case StreamRole::Viewfinder: > > > - case StreamRole::VideoRecording: { > > > useMainPath = !selfPathAvailable; > > > + /* > > > + * sYCC is the YCbCr encoding of sRGB, which is commonly > > > + * used by displays. > > > + */ > > > + if (!colorSpace) > > > + colorSpace = ColorSpace::Sycc; > > > break; > > > - } > > > + > > > + case StreamRole::VideoRecording: > > > + useMainPath = !selfPathAvailable; > > > + /* Rec. 709 is a good default for HD video recording. */ > > > + if (!colorSpace) > > > + colorSpace = ColorSpace::Rec709; > > > + break; > > > + > > > default: > > > LOG(RkISP1, Warning) > > > << "Requested stream role not supported: " << role; > > > @@ -580,6 +606,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > selfPathAvailable = false; > > > } > > > > > > + cfg.colorSpace = colorSpace; > > > config->addConfiguration(cfg); > > > } > > > > > > @@ -642,6 +669,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > > if (ret < 0) > > > return ret; > > > > > > + format.colorSpace = config->at(0).colorSpace; > > > ret = isp_->setFormat(2, &format); > > > if (ret < 0) > > > return ret; > > -- > Regards, > > Laurent Pinchart
Hi Laurent, Reviewed-by: Florian Sylvestre <fsylvestre@baylibre.com> On Thu, 25 Aug 2022 at 23:17, Paul Elder via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Hi Laurent, > > On Thu, Aug 25, 2022 at 11:17:11PM +0300, Laurent Pinchart wrote: > > Hi Paul, > > > > On Thu, Aug 25, 2022 at 03:08:13PM -0500, paul.elder@ideasonboard.com wrote: > > > On Tue, Aug 23, 2022 at 08:43:13PM +0300, Laurent Pinchart via libcamera-devel wrote: > > > > > > I think some changelog would be nice to have :) > > > > Oops :-) I'll add > > > > Implement color space support in the rkisp1 pipeline handler, in the > > configuration generation, configuration validation and camera > > configuration. As all the processing related to the color space is > > performed in the part of the pipeline shared by all streams, a single > > color space must cover all stream configurations. This is enforced > > manually when generating the configuration, and with the > > validateColorSpaces() helper when validating it. > > > > Only the Y'CbCr encoding and quantization range are currently taken into > > account, and they are programmed through the V4L2 color space API. The > > primary colors chromaticities and the transfer function need to be > > configured in the ISP parameters buffer, and thus conveyed to the IPA, > > but the rkisp1 driver does not currently support tone mapping, so this > > will be done later. > > Looks good. > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 38 ++++++++++++++++++++---- > > > > 1 file changed, 33 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > index 03bbe6b467ae..25fbf9f1a0a9 100644 > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > @@ -18,6 +18,7 @@ > > > > #include <libcamera/base/utils.h> > > > > > > > > #include <libcamera/camera.h> > > > > +#include <libcamera/color_space.h> > > > > #include <libcamera/control_ids.h> > > > > #include <libcamera/formats.h> > > > > #include <libcamera/framebuffer.h> > > > > @@ -416,11 +417,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > > { > > > > const CameraSensor *sensor = data_->sensor_.get(); > > > > unsigned int pathCount = data_->selfPath_ ? 2 : 1; > > > > - Status status = Valid; > > > > + Status status; > > > > > > > > if (config_.empty()) > > > > return Invalid; > > > > > > > > + status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace); > > > > + > > > > if (transform != Transform::Identity) { > > > > transform = Transform::Identity; > > > > status = Adjusted; > > > > @@ -547,21 +550,44 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > > if (roles.empty()) > > > > return config; > > > > > > > > + /* > > > > + * As the ISP can't output different color spaces for the main and self > > > > + * path, pick a sensible default color space based on the role of the > > > > + * first stream and use it for all streams. > > > > + */ > > > > + std::optional<ColorSpace> colorSpace; > > > > + > > > > bool mainPathAvailable = true; > > > > bool selfPathAvailable = data->selfPath_; > > > > + > > > > for (const StreamRole role : roles) { > > > > bool useMainPath; > > > > > > > > switch (role) { > > > > - case StreamRole::StillCapture: { > > > > + case StreamRole::StillCapture: > > > > useMainPath = mainPathAvailable; > > > > + /* JPEG encoders typically expect sYCC. */ > > > > + if (!colorSpace) > > > > + colorSpace = ColorSpace::Sycc; > > > > break; > > > > - } > > > > + > > > > case StreamRole::Viewfinder: > > > > - case StreamRole::VideoRecording: { > > > > useMainPath = !selfPathAvailable; > > > > + /* > > > > + * sYCC is the YCbCr encoding of sRGB, which is commonly > > > > + * used by displays. > > > > + */ > > > > + if (!colorSpace) > > > > + colorSpace = ColorSpace::Sycc; > > > > break; > > > > - } > > > > + > > > > + case StreamRole::VideoRecording: > > > > + useMainPath = !selfPathAvailable; > > > > + /* Rec. 709 is a good default for HD video recording. */ > > > > + if (!colorSpace) > > > > + colorSpace = ColorSpace::Rec709; > > > > + break; > > > > + > > > > default: > > > > LOG(RkISP1, Warning) > > > > << "Requested stream role not supported: " << role; > > > > @@ -580,6 +606,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera > > > > selfPathAvailable = false; > > > > } > > > > > > > > + cfg.colorSpace = colorSpace; > > > > config->addConfiguration(cfg); > > > > } > > > > > > > > @@ -642,6 +669,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > > > if (ret < 0) > > > > return ret; > > > > > > > > + format.colorSpace = config->at(0).colorSpace; > > > > ret = isp_->setFormat(2, &format); > > > > if (ret < 0) > > > > return ret; > > > > -- > > Regards, > > > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 03bbe6b467ae..25fbf9f1a0a9 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -18,6 +18,7 @@ #include <libcamera/base/utils.h> #include <libcamera/camera.h> +#include <libcamera/color_space.h> #include <libcamera/control_ids.h> #include <libcamera/formats.h> #include <libcamera/framebuffer.h> @@ -416,11 +417,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() { const CameraSensor *sensor = data_->sensor_.get(); unsigned int pathCount = data_->selfPath_ ? 2 : 1; - Status status = Valid; + Status status; if (config_.empty()) return Invalid; + status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace); + if (transform != Transform::Identity) { transform = Transform::Identity; status = Adjusted; @@ -547,21 +550,44 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera if (roles.empty()) return config; + /* + * As the ISP can't output different color spaces for the main and self + * path, pick a sensible default color space based on the role of the + * first stream and use it for all streams. + */ + std::optional<ColorSpace> colorSpace; + bool mainPathAvailable = true; bool selfPathAvailable = data->selfPath_; + for (const StreamRole role : roles) { bool useMainPath; switch (role) { - case StreamRole::StillCapture: { + case StreamRole::StillCapture: useMainPath = mainPathAvailable; + /* JPEG encoders typically expect sYCC. */ + if (!colorSpace) + colorSpace = ColorSpace::Sycc; break; - } + case StreamRole::Viewfinder: - case StreamRole::VideoRecording: { useMainPath = !selfPathAvailable; + /* + * sYCC is the YCbCr encoding of sRGB, which is commonly + * used by displays. + */ + if (!colorSpace) + colorSpace = ColorSpace::Sycc; break; - } + + case StreamRole::VideoRecording: + useMainPath = !selfPathAvailable; + /* Rec. 709 is a good default for HD video recording. */ + if (!colorSpace) + colorSpace = ColorSpace::Rec709; + break; + default: LOG(RkISP1, Warning) << "Requested stream role not supported: " << role; @@ -580,6 +606,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera selfPathAvailable = false; } + cfg.colorSpace = colorSpace; config->addConfiguration(cfg); } @@ -642,6 +669,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) if (ret < 0) return ret; + format.colorSpace = config->at(0).colorSpace; ret = isp_->setFormat(2, &format); if (ret < 0) return ret;
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 38 ++++++++++++++++++++---- 1 file changed, 33 insertions(+), 5 deletions(-)