| Message ID | 20230103113313.5423-2-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David On Tue, Jan 03, 2023 at 11:33:12AM +0000, David Plowman via libcamera-devel wrote: > The intention is that the "main" colour space is the colour space of > the largest non-raw stream. Unfortunately the use of "config_[i].size" > is clearly incorrect, and has been copied from prior versions of the > code. This small change merely corrects the error. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/libcamera/camera.cpp | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 2d947a44..0da167a7 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -361,6 +361,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > * largest non-raw stream with a defined color space (if there is one). > */ > std::optional<ColorSpace> colorSpace; > + Size size; > > for (auto [i, cfg] : utils::enumerate(config_)) { > if (!cfg.colorSpace) > @@ -369,9 +370,10 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > if (cfg.colorSpace->adjust(cfg.pixelFormat)) > status = Adjusted; > > - if (cfg.colorSpace != ColorSpace::Raw && > - (!colorSpace || cfg.size > config_[i].size)) > + if (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) { I'm not too familiar with this part of the code, but seeing for (auto [i, cfg] : utils::enumerate(config_)) { ... if (cfg.size > config_[i].size)) makes me indeed think that cfg.size == config_[i].size However I wonder why the condition was preceded by !colorSpace.. The same condition was expressed in 5e5eadabd812 ("libcamera: camera: Add validateColorSpaces to CameraConfiguration class") as } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) { index = i; And I guess it was because the second part of the condition was always false :) Now that I've tracked down the full history of the bug Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > colorSpace = cfg.colorSpace; > + size = cfg.size; > + } > } > > if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace)) > -- > 2.30.2 >
Hi Jacopo Thanks for the review. I was wondering if you knew anything about how rkisp1 uses this function (I think it's the only other caller apart from Raspberry Pi, at least at the moment). The reason I ask is because I think the whole "streams sharing a colour space" thing isn't terribly useful any more (YUV and RGB streams are no longer allowed to share a ColorSpace), so I'm wondering about removing it, but one would have to understand the rkisp1 requirements first! Would you know anything about that? Thanks! David On Tue, 3 Jan 2023 at 17:37, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi David > > On Tue, Jan 03, 2023 at 11:33:12AM +0000, David Plowman via libcamera-devel wrote: > > The intention is that the "main" colour space is the colour space of > > the largest non-raw stream. Unfortunately the use of "config_[i].size" > > is clearly incorrect, and has been copied from prior versions of the > > code. This small change merely corrects the error. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/libcamera/camera.cpp | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 2d947a44..0da167a7 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -361,6 +361,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > * largest non-raw stream with a defined color space (if there is one). > > */ > > std::optional<ColorSpace> colorSpace; > > + Size size; > > > > for (auto [i, cfg] : utils::enumerate(config_)) { > > if (!cfg.colorSpace) > > @@ -369,9 +370,10 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > if (cfg.colorSpace->adjust(cfg.pixelFormat)) > > status = Adjusted; > > > > - if (cfg.colorSpace != ColorSpace::Raw && > > - (!colorSpace || cfg.size > config_[i].size)) > > + if (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) { > > I'm not too familiar with this part of the code, but seeing > > for (auto [i, cfg] : utils::enumerate(config_)) { > ... > if (cfg.size > config_[i].size)) > > makes me indeed think that cfg.size == config_[i].size > > However I wonder why the condition was preceded by !colorSpace.. > The same condition was expressed in > 5e5eadabd812 ("libcamera: camera: Add validateColorSpaces to CameraConfiguration class") > > as > > } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) { > index = i; > > And I guess it was because the second part of the condition was always > false :) > > Now that I've tracked down the full history of the bug > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > > > colorSpace = cfg.colorSpace; > > + size = cfg.size; > > + } > > } > > > > if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace)) > > -- > > 2.30.2 > >
Hi David, On Wed, Jan 04, 2023 at 09:41:54AM +0000, David Plowman wrote: > Hi Jacopo > > Thanks for the review. > > I was wondering if you knew anything about how rkisp1 uses this > function (I think it's the only other caller apart from Raspberry Pi, > at least at the moment). It seems to me as well it's the only other user apart RPi > > The reason I ask is because I think the whole "streams sharing a > colour space" thing isn't terribly useful any more (YUV and RGB > streams are no longer allowed to share a ColorSpace), so I'm wondering I've missed good parts of the discussion, but why "YUV and RGB streams are -no longer- allowed to share a ColorSpace" ? > about removing it, but one would have to understand the rkisp1 > requirements first! Would you know anything about that? I don't know the details, but looking at the commit message of 4267e0bab86d ("libcamera: pipeline: rkisp1: Implement color space support") 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. and the comment in the code + /* + * 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; It seems clear to me that the RkISP1 requires a single colorspace for all outputs. That's why it's interesting to know why "YUV and RGB are no longer allowed to share a ColorSpace". Ofc if you move away from CameraConfiguration::validateColorSpaces() and RkISP1 it's the only remaining user we can consider moving the function back to the pipeline handler implementation. Thanks j > > Thanks! > David > > On Tue, 3 Jan 2023 at 17:37, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi David > > > > On Tue, Jan 03, 2023 at 11:33:12AM +0000, David Plowman via libcamera-devel wrote: > > > The intention is that the "main" colour space is the colour space of > > > the largest non-raw stream. Unfortunately the use of "config_[i].size" > > > is clearly incorrect, and has been copied from prior versions of the > > > code. This small change merely corrects the error. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/libcamera/camera.cpp | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index 2d947a44..0da167a7 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -361,6 +361,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > > * largest non-raw stream with a defined color space (if there is one). > > > */ > > > std::optional<ColorSpace> colorSpace; > > > + Size size; > > > > > > for (auto [i, cfg] : utils::enumerate(config_)) { > > > if (!cfg.colorSpace) > > > @@ -369,9 +370,10 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > > if (cfg.colorSpace->adjust(cfg.pixelFormat)) > > > status = Adjusted; > > > > > > - if (cfg.colorSpace != ColorSpace::Raw && > > > - (!colorSpace || cfg.size > config_[i].size)) > > > + if (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) { > > > > I'm not too familiar with this part of the code, but seeing > > > > for (auto [i, cfg] : utils::enumerate(config_)) { > > ... > > if (cfg.size > config_[i].size)) > > > > makes me indeed think that cfg.size == config_[i].size > > > > However I wonder why the condition was preceded by !colorSpace.. > > The same condition was expressed in > > 5e5eadabd812 ("libcamera: camera: Add validateColorSpaces to CameraConfiguration class") > > > > as > > > > } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) { > > index = i; > > > > And I guess it was because the second part of the condition was always > > false :) > > > > Now that I've tracked down the full history of the bug > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Thanks > > j > > > > > colorSpace = cfg.colorSpace; > > > + size = cfg.size; > > > + } > > > } > > > > > > if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace)) > > > -- > > > 2.30.2 > > >
Hi David, Thank you for this fix! On Tue, 3 Jan 2023 at 11:33, David Plowman via libcamera-devel < libcamera-devel@lists.libcamera.org> wrote: > The intention is that the "main" colour space is the colour space of > the largest non-raw stream. Unfortunately the use of "config_[i].size" > is clearly incorrect, and has been copied from prior versions of the > code. This small change merely corrects the error. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/camera.cpp | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 2d947a44..0da167a7 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -361,6 +361,7 @@ CameraConfiguration::Status > CameraConfiguration::validateColorSpaces(ColorSpaceF > * largest non-raw stream with a defined color space (if there is > one). > */ > std::optional<ColorSpace> colorSpace; > + Size size; > > for (auto [i, cfg] : utils::enumerate(config_)) { > if (!cfg.colorSpace) > @@ -369,9 +370,10 @@ CameraConfiguration::Status > CameraConfiguration::validateColorSpaces(ColorSpaceF > if (cfg.colorSpace->adjust(cfg.pixelFormat)) > status = Adjusted; > > - if (cfg.colorSpace != ColorSpace::Raw && > - (!colorSpace || cfg.size > config_[i].size)) > + if (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) { > colorSpace = cfg.colorSpace; > + size = cfg.size; > + } > } > > if (!colorSpace || !(flags & > ColorSpaceFlag::StreamsShareColorSpace)) > -- > 2.30.2 > >
Hi Jacopo On Thu, 5 Jan 2023 at 09:24, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi David, > > On Wed, Jan 04, 2023 at 09:41:54AM +0000, David Plowman wrote: > > Hi Jacopo > > > > Thanks for the review. > > > > I was wondering if you knew anything about how rkisp1 uses this > > function (I think it's the only other caller apart from Raspberry Pi, > > at least at the moment). > > It seems to me as well it's the only other user apart RPi > > > > > The reason I ask is because I think the whole "streams sharing a > > colour space" thing isn't terribly useful any more (YUV and RGB > > streams are no longer allowed to share a ColorSpace), so I'm wondering > > I've missed good parts of the discussion, but why "YUV and RGB streams > are -no longer- allowed to share a ColorSpace" ? A change was made whereby streams that are producing an RGB output are forced to have "None" for the YCbCr encoding, and "Full" for the range. This means if you have a YUV and an RGB stream, they can't share the same ColorSpace. Previously those fields were simply ignored for output formats that didn't need them. > > > about removing it, but one would have to understand the rkisp1 > > requirements first! Would you know anything about that? > > I don't know the details, but looking at the commit message of > > 4267e0bab86d ("libcamera: pipeline: rkisp1: Implement color space support") > > 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. > > and the comment in the code > > + /* > + * 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; > > It seems clear to me that the RkISP1 requires a single colorspace for > all outputs. That's why it's interesting to know why "YUV and RGB are > no longer allowed to share a ColorSpace". It depends if RkISP1 can create a YUV and an RGB stream simultaneously. If so, someone who understands it needs to look into the ColorSpace question - it may actually be broken at the moment, I can't tell. David > > Ofc if you move away from CameraConfiguration::validateColorSpaces() > and RkISP1 it's the only remaining user we can consider moving the > function back to the pipeline handler implementation. > > Thanks > j > > > > > Thanks! > > David > > > > On Tue, 3 Jan 2023 at 17:37, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi David > > > > > > On Tue, Jan 03, 2023 at 11:33:12AM +0000, David Plowman via libcamera-devel wrote: > > > > The intention is that the "main" colour space is the colour space of > > > > the largest non-raw stream. Unfortunately the use of "config_[i].size" > > > > is clearly incorrect, and has been copied from prior versions of the > > > > code. This small change merely corrects the error. > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > src/libcamera/camera.cpp | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > > index 2d947a44..0da167a7 100644 > > > > --- a/src/libcamera/camera.cpp > > > > +++ b/src/libcamera/camera.cpp > > > > @@ -361,6 +361,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > > > * largest non-raw stream with a defined color space (if there is one). > > > > */ > > > > std::optional<ColorSpace> colorSpace; > > > > + Size size; > > > > > > > > for (auto [i, cfg] : utils::enumerate(config_)) { > > > > if (!cfg.colorSpace) > > > > @@ -369,9 +370,10 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > > > if (cfg.colorSpace->adjust(cfg.pixelFormat)) > > > > status = Adjusted; > > > > > > > > - if (cfg.colorSpace != ColorSpace::Raw && > > > > - (!colorSpace || cfg.size > config_[i].size)) > > > > + if (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) { > > > > > > I'm not too familiar with this part of the code, but seeing > > > > > > for (auto [i, cfg] : utils::enumerate(config_)) { > > > ... > > > if (cfg.size > config_[i].size)) > > > > > > makes me indeed think that cfg.size == config_[i].size > > > > > > However I wonder why the condition was preceded by !colorSpace.. > > > The same condition was expressed in > > > 5e5eadabd812 ("libcamera: camera: Add validateColorSpaces to CameraConfiguration class") > > > > > > as > > > > > > } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) { > > > index = i; > > > > > > And I guess it was because the second part of the condition was always > > > false :) > > > > > > Now that I've tracked down the full history of the bug > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > Thanks > > > j > > > > > > > colorSpace = cfg.colorSpace; > > > > + size = cfg.size; > > > > + } > > > > } > > > > > > > > if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace)) > > > > -- > > > > 2.30.2 > > > >
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 2d947a44..0da167a7 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -361,6 +361,7 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF * largest non-raw stream with a defined color space (if there is one). */ std::optional<ColorSpace> colorSpace; + Size size; for (auto [i, cfg] : utils::enumerate(config_)) { if (!cfg.colorSpace) @@ -369,9 +370,10 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF if (cfg.colorSpace->adjust(cfg.pixelFormat)) status = Adjusted; - if (cfg.colorSpace != ColorSpace::Raw && - (!colorSpace || cfg.size > config_[i].size)) + if (cfg.colorSpace != ColorSpace::Raw && cfg.size > size) { colorSpace = cfg.colorSpace; + size = cfg.size; + } } if (!colorSpace || !(flags & ColorSpaceFlag::StreamsShareColorSpace))
The intention is that the "main" colour space is the colour space of the largest non-raw stream. Unfortunately the use of "config_[i].size" is clearly incorrect, and has been copied from prior versions of the code. This small change merely corrects the error. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/libcamera/camera.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)