| Message ID | 20250124215806.158024-1-mzamazal@redhat.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, On Fri, Jan 24, 2025 at 10:57:51PM +0100, Milan Zamazal wrote: > This makes raw streams working again in ‘simple’ pipeline when software > ISP is enabled for the given device. At most one raw stream and one > processed stream (possibly both at once) are supported. > > An example ‘cam’ invocation requesting a raw stream rather than a debayered stream: > > cam -c1 -C8 -s role=raw,width=1920,height=1080 -Ffile#.raw > > Or for both raw and processed streams: > > cam -c1 -C8 -s role=raw,width=1920,height=1080,pixelformat=SRGGB8 -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 -Ffile# > > When only a raw stream is requested, there are no exposure/gain > adjustments applied. This could be improved in future, once software > ISP gets a mechanism to gather image statistics without processing and > using them to make the adjustments, or once manual exposure controls are > added to software ISP. In the meantime, exposure must be changed > externally. This is fine. > A part of the patches is a small change to ‘cam’ PPM file handling, to > be able to produce PPM files together with raw files when both processed > and raw streams are requested. > > The patches are RFC because I’m not sure whether they are implemented > properly. It’s also necessary to determine the right balance between > the requested functionality (single raw + processed streams should be > enough for current needs), cleanness of the implementation and the > complexity of the patches. The famous yak shaving balance. As you'll see in the review of individual patches, I think there's some cleanup to be done. The good news is that we don't need a full yak shaving, just trimming some edges :-) > Changes in v2: > - Completely reworked. > - Extended to be able to produce a raw stream together with a processed > stream. > > Milan Zamazal (13): > libcamera: software_isp: Move a non-loop condition out of the loop > libcamera: simple: Increase the default number of streams to 2 > libcamera: simple: Don't use raw output formats with conversions > libcamera: simple: Add plain output configurations to software ISP > libcamera: simple: Identify requested stream roles > libcamera: simple: Protect against null maxPipeConfig > libcamera: simple: Consider raw output configurations > libcamera: simple: Handle adjusted and raw configurations separately > libcamera: simple: Don't use conversion with an added raw stream > libcamera: simple: Make raw streams working > apps: ppm_writer: Add a missing include > apps: ppm_writer: Return EIO on I/O errors > apps: cam: Write raw file if PPM cannot be written > > src/apps/cam/file_sink.cpp | 16 +- > src/apps/common/ppm_writer.cpp | 7 +- > src/libcamera/pipeline/simple/simple.cpp | 188 ++++++++++++++++---- > src/libcamera/software_isp/software_isp.cpp | 7 +- > 4 files changed, 168 insertions(+), 50 deletions(-)
I've pushed patches 01/13, 11/13 and 12/13 already. On Sun, Jan 26, 2025 at 11:58:17PM +0200, Laurent Pinchart wrote: > On Fri, Jan 24, 2025 at 10:57:51PM +0100, Milan Zamazal wrote: > > This makes raw streams working again in ‘simple’ pipeline when software > > ISP is enabled for the given device. At most one raw stream and one > > processed stream (possibly both at once) are supported. > > > > An example ‘cam’ invocation requesting a raw stream rather than a debayered stream: > > > > cam -c1 -C8 -s role=raw,width=1920,height=1080 -Ffile#.raw > > > > Or for both raw and processed streams: > > > > cam -c1 -C8 -s role=raw,width=1920,height=1080,pixelformat=SRGGB8 -s role=viewfinder,width=1920,height=1080,pixelformat=RGB888 -Ffile# > > > > When only a raw stream is requested, there are no exposure/gain > > adjustments applied. This could be improved in future, once software > > ISP gets a mechanism to gather image statistics without processing and > > using them to make the adjustments, or once manual exposure controls are > > added to software ISP. In the meantime, exposure must be changed > > externally. > > This is fine. > > > A part of the patches is a small change to ‘cam’ PPM file handling, to > > be able to produce PPM files together with raw files when both processed > > and raw streams are requested. > > > > The patches are RFC because I’m not sure whether they are implemented > > properly. It’s also necessary to determine the right balance between > > the requested functionality (single raw + processed streams should be > > enough for current needs), cleanness of the implementation and the > > complexity of the patches. > > The famous yak shaving balance. > > As you'll see in the review of individual patches, I think there's some > cleanup to be done. The good news is that we don't need a full yak > shaving, just trimming some edges :-) > > > Changes in v2: > > - Completely reworked. > > - Extended to be able to produce a raw stream together with a processed > > stream. > > > > Milan Zamazal (13): > > libcamera: software_isp: Move a non-loop condition out of the loop > > libcamera: simple: Increase the default number of streams to 2 > > libcamera: simple: Don't use raw output formats with conversions > > libcamera: simple: Add plain output configurations to software ISP > > libcamera: simple: Identify requested stream roles > > libcamera: simple: Protect against null maxPipeConfig > > libcamera: simple: Consider raw output configurations > > libcamera: simple: Handle adjusted and raw configurations separately > > libcamera: simple: Don't use conversion with an added raw stream > > libcamera: simple: Make raw streams working > > apps: ppm_writer: Add a missing include > > apps: ppm_writer: Return EIO on I/O errors > > apps: cam: Write raw file if PPM cannot be written > > > > src/apps/cam/file_sink.cpp | 16 +- > > src/apps/common/ppm_writer.cpp | 7 +- > > src/libcamera/pipeline/simple/simple.cpp | 188 ++++++++++++++++---- > > src/libcamera/software_isp/software_isp.cpp | 7 +- > > 4 files changed, 168 insertions(+), 50 deletions(-)
Hi Laurent, thank you for the reviews. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > On Fri, Jan 24, 2025 at 10:57:51PM +0100, Milan Zamazal wrote: >> This makes raw streams working again in ‘simple’ pipeline when software >> ISP is enabled for the given device. At most one raw stream and one >> processed stream (possibly both at once) are supported. >> >> An example ‘cam’ invocation requesting a raw stream rather than a debayered stream: >> >> cam -c1 -C8 -s role=raw,width=1920,height=1080 -Ffile#.raw >> >> Or for both raw and processed streams: >> >> cam -c1 -C8 -s role=raw,width=1920,height=1080,pixelformat=SRGGB8 -s >> role=viewfinder,width=1920,height=1080,pixelformat=RGB888 -Ffile# >> >> When only a raw stream is requested, there are no exposure/gain >> adjustments applied. This could be improved in future, once software >> ISP gets a mechanism to gather image statistics without processing and >> using them to make the adjustments, or once manual exposure controls are >> added to software ISP. In the meantime, exposure must be changed >> externally. > > This is fine. > >> A part of the patches is a small change to ‘cam’ PPM file handling, to >> be able to produce PPM files together with raw files when both processed >> and raw streams are requested. >> >> The patches are RFC because I’m not sure whether they are implemented >> properly. It’s also necessary to determine the right balance between >> the requested functionality (single raw + processed streams should be >> enough for current needs), cleanness of the implementation and the >> complexity of the patches. > > The famous yak shaving balance. > > As you'll see in the review of individual patches, I think there's some > cleanup to be done. I agree completely. I think v3 is a much better version and a good step in the iterative process of progressing from a proof of concept and guesswork to a better understanding and making a usable code. > The good news is that we don't need a full yak shaving, just trimming > some edges :-) I tried to save the poor yak and although the v3 patches are significantly reworked, I tried to make them relatively modest. Let's see how much it works for you; I admit I have been struggling with all the details when working on v3 and may have missed the bigger picture. >> Changes in v2: >> - Completely reworked. >> - Extended to be able to produce a raw stream together with a processed >> stream. >> >> Milan Zamazal (13): >> libcamera: software_isp: Move a non-loop condition out of the loop >> libcamera: simple: Increase the default number of streams to 2 >> libcamera: simple: Don't use raw output formats with conversions >> libcamera: simple: Add plain output configurations to software ISP >> libcamera: simple: Identify requested stream roles >> libcamera: simple: Protect against null maxPipeConfig >> libcamera: simple: Consider raw output configurations >> libcamera: simple: Handle adjusted and raw configurations separately >> libcamera: simple: Don't use conversion with an added raw stream >> libcamera: simple: Make raw streams working >> apps: ppm_writer: Add a missing include >> apps: ppm_writer: Return EIO on I/O errors >> apps: cam: Write raw file if PPM cannot be written >> >> src/apps/cam/file_sink.cpp | 16 +- >> src/apps/common/ppm_writer.cpp | 7 +- >> src/libcamera/pipeline/simple/simple.cpp | 188 ++++++++++++++++---- >> src/libcamera/software_isp/software_isp.cpp | 7 +- >> 4 files changed, 168 insertions(+), 50 deletions(-)