| Message ID | 20260624082705.272795-2-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this! On Wed, 24 Jun 2026 at 09:35, Naushir Patuck <naush@raspberrypi.com> wrote: > > In generateConfiguration(), set the default packing on RAW streams to > BayerFormat::Packing::None. This default is easier to handle over > CSI2 packed or PiSP Bayer compressed packaing options. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index 5a5acf6a164b..e0f364272746 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -410,7 +410,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole > size = sensorSize; > sensorFormat = data->findBestFormat(size, defaultRawBitDepth); > pixelFormat = mbusCodeToPixelFormat(sensorFormat.code, > - BayerFormat::Packing::CSI2); > + BayerFormat::Packing::None); > ASSERT(pixelFormat.isValid()); > colorSpace = ColorSpace::Raw; > bufferCount = 2; > -- > 2.53.0 > I think we probably just want to check first whether all our application-level code explicitly sets the packing after a generateConfiguration() or not, as otherwise it would be a bit of a behaviour change. Will get back to you on this...! Thanks! David
On Wed, Jun 24, 2026 at 09:41:52AM +0100, David Plowman wrote: > On Wed, 24 Jun 2026 at 09:35, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > In generateConfiguration(), set the default packing on RAW streams to > > BayerFormat::Packing::None. This default is easier to handle over > > CSI2 packed or PiSP Bayer compressed packaing options. s/packaing/packing/ > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > index 5a5acf6a164b..e0f364272746 100644 > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > @@ -410,7 +410,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole > > size = sensorSize; > > sensorFormat = data->findBestFormat(size, defaultRawBitDepth); > > pixelFormat = mbusCodeToPixelFormat(sensorFormat.code, > > - BayerFormat::Packing::CSI2); > > + BayerFormat::Packing::None); > > ASSERT(pixelFormat.isValid()); > > colorSpace = ColorSpace::Raw; > > bufferCount = 2; > > I think we probably just want to check first whether all our > application-level code explicitly sets the packing after a > generateConfiguration() or not, as otherwise it would be a bit of a > behaviour change. Will get back to you on this...! Good idea. I'd be fine defaulting to CSI-2 packing as that's standard, but on Pi 5 that's not a valid option. We could make this conditional and default to CSI-2 for Pi 4 and None for Pi 5 if desired.
Hi Laurent On Wed, 24 Jun 2026 at 21:14, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Wed, Jun 24, 2026 at 09:41:52AM +0100, David Plowman wrote: > > On Wed, 24 Jun 2026 at 09:35, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > In generateConfiguration(), set the default packing on RAW streams to > > > BayerFormat::Packing::None. This default is easier to handle over > > > CSI2 packed or PiSP Bayer compressed packaing options. > > s/packaing/packing/ > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > index 5a5acf6a164b..e0f364272746 100644 > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > @@ -410,7 +410,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole > > > size = sensorSize; > > > sensorFormat = data->findBestFormat(size, defaultRawBitDepth); > > > pixelFormat = mbusCodeToPixelFormat(sensorFormat.code, > > > - BayerFormat::Packing::CSI2); > > > + BayerFormat::Packing::None); > > > ASSERT(pixelFormat.isValid()); > > > colorSpace = ColorSpace::Raw; > > > bufferCount = 2; > > > > I think we probably just want to check first whether all our > > application-level code explicitly sets the packing after a > > generateConfiguration() or not, as otherwise it would be a bit of a > > behaviour change. Will get back to you on this...! > > Good idea. > > I'd be fine defaulting to CSI-2 packing as that's standard, but on Pi 5 > that's not a valid option. We could make this conditional and default to > CSI-2 for Pi 4 and None for Pi 5 if desired. It looks to me like all our application code does normally specify what it wants, which would mean it's pretty safe to change the behaviour. There might be a couple of corner cases, for example when you explicitly ask *not* to have the raw stream, but this is not the norm. There will also be a very small number of people writing applications from the ground up who might be affected. I'm also a little unsure whether the default behaviour should give you the "easiest to use" format (and if you haven't defined the format explicitly then presumably you have no intention of using it?) or the most efficient (= better performance for probably everyone in this situation), but mostly I can't escape the feeling that the whole thing doesn't matter all that much. Though I wouldn't want purposefully to introduce (further!) differences between Pi 4s and 5s if I didn't have to. We do also publish decompression code, in both C/C++ and Python/numpy. After all that, I think this is probably OK so: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> though I've probably spent too much time thinking about this already!! David > > -- > Regards, > > Laurent Pinchart
On Thu, Jun 25, 2026 at 10:01:45AM +0100, David Plowman wrote: > On Wed, 24 Jun 2026 at 21:14, Laurent Pinchart wrote: > > On Wed, Jun 24, 2026 at 09:41:52AM +0100, David Plowman wrote: > > > On Wed, 24 Jun 2026 at 09:35, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > > > In generateConfiguration(), set the default packing on RAW streams to > > > > BayerFormat::Packing::None. This default is easier to handle over > > > > CSI2 packed or PiSP Bayer compressed packaing options. > > > > s/packaing/packing/ > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > index 5a5acf6a164b..e0f364272746 100644 > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > @@ -410,7 +410,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole > > > > size = sensorSize; > > > > sensorFormat = data->findBestFormat(size, defaultRawBitDepth); > > > > pixelFormat = mbusCodeToPixelFormat(sensorFormat.code, > > > > - BayerFormat::Packing::CSI2); > > > > + BayerFormat::Packing::None); > > > > ASSERT(pixelFormat.isValid()); > > > > colorSpace = ColorSpace::Raw; > > > > bufferCount = 2; > > > > > > I think we probably just want to check first whether all our > > > application-level code explicitly sets the packing after a > > > generateConfiguration() or not, as otherwise it would be a bit of a > > > behaviour change. Will get back to you on this...! > > > > Good idea. > > > > I'd be fine defaulting to CSI-2 packing as that's standard, but on Pi 5 > > that's not a valid option. We could make this conditional and default to > > CSI-2 for Pi 4 and None for Pi 5 if desired. > > It looks to me like all our application code does normally specify > what it wants, which would mean it's pretty safe to change the > behaviour. There might be a couple of corner cases, for example when > you explicitly ask *not* to have the raw stream, but this is not the > norm. There will also be a very small number of people writing > applications from the ground up who might be affected. > > I'm also a little unsure whether the default behaviour should give you > the "easiest to use" format (and if you haven't defined the format > explicitly then presumably you have no intention of using it?) or the > most efficient (= better performance for probably everyone in this > situation), but mostly I can't escape the feeling that the whole thing > doesn't matter all that much. Though I wouldn't want purposefully to > introduce (further!) differences between Pi 4s and 5s if I didn't have > to. It's a good question. The Camera class has a generateConfiguration() function that takes a list of stream roles and is documented as returning a configuration "containing suitable streams and their suggested default configurations". It doesn't tell what a suggest default should be, and I see pros and cons in providing a standard format or an optimized format. What is however pretty clear to me is that the default should be a valid configuration, and at the moment, that's not the case on Pi 5 as CSI2 packing is not supported. > We do also publish decompression code, in both C/C++ and Python/numpy. That's a good point. Maybe I'm overly concerned, and selecting Packing::PISP on Pi 5 would be fine too. I'll leave it up to Naush and you. Please let me know if you want to continue with this patch, or use a different approach. > After all that, I think this is probably OK so: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > though I've probably spent too much time thinking about this already!! > > David
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index 5a5acf6a164b..e0f364272746 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp @@ -410,7 +410,7 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole size = sensorSize; sensorFormat = data->findBestFormat(size, defaultRawBitDepth); pixelFormat = mbusCodeToPixelFormat(sensorFormat.code, - BayerFormat::Packing::CSI2); + BayerFormat::Packing::None); ASSERT(pixelFormat.isValid()); colorSpace = ColorSpace::Raw; bufferCount = 2;
In generateConfiguration(), set the default packing on RAW streams to BayerFormat::Packing::None. This default is easier to handle over CSI2 packed or PiSP Bayer compressed packaing options. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)