| Message ID | 20200924145143.117733-2-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Thu, Sep 24, 2020 at 04:51:41PM +0200, Jacopo Mondi wrote: > When requesting one RAW stream and one YUV stream the > StreamConfiguration assigned to the RAW stream is the first one > added to the CameraConfiguration, while the YUV stream gets assigned to > the main output. > > At configure() time the viewfinder output needs to be configured with > the same format as the main output, but since the introduction of RAW > capture support, the pipeline has not been updated and still assumes > the main output configuration is the first one in the > CameraConfiguration. This causes the viewfinder to be configured > with the same format as the raw stream, breaking capture operations. Oops... You mentioned no CTS regression, is there also no CTS tests fixed by this ? > Before this commit the following command fails and the ImgU does not > produce frames: > cam -srole=stillraw -srole=viewfinder -c2 -C > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 221259c7fe61..9cea6c7e9611 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -476,25 +476,23 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > return ret; > > /* Apply the format to the configured streams output devices. */ > - bool outActive = false; > - bool vfActive = false; > + StreamConfiguration *mainCfg = nullptr; > + StreamConfiguration *vfCfg = nullptr; > > for (unsigned int i = 0; i < config->size(); ++i) { > StreamConfiguration &cfg = (*config)[i]; > Stream *stream = cfg.stream(); > > if (stream == outStream) { > + mainCfg = &cfg; > ret = imgu->configureOutput(cfg, &outputFormat); > if (ret) > return ret; > - > - outActive = true; > } else if (stream == vfStream) { > + vfCfg = &cfg; > ret = imgu->configureViewfinder(cfg, &outputFormat); > if (ret) > return ret; > - > - vfActive = true; > } > } > > @@ -503,14 +501,14 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > * the configuration of the active one for that purpose (there should > * be at least one active stream in the configuration request). > */ > - if (!outActive) { > - ret = imgu->configureOutput(config->at(0), &outputFormat); > + if (!mainCfg) { > + ret = imgu->configureOutput(*vfCfg, &outputFormat); What if the only stream requested is the raw stream ? Won't both mainCfg and vfCfg be null ? > if (ret) > return ret; > } > > - if (!vfActive) { > - ret = imgu->configureViewfinder(config->at(0), &outputFormat); > + if (!vfCfg) { > + ret = imgu->configureViewfinder(*mainCfg, &outputFormat); > if (ret) > return ret; > } > @@ -529,7 +527,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > /* Apply the "pipe_mode" control to the ImgU subdevice. */ > ControlList ctrls(imgu->imgu_->controls()); > ctrls.set(V4L2_CID_IPU3_PIPE_MODE, > - static_cast<int32_t>(vfActive ? IPU3PipeModeVideo : > + static_cast<int32_t>(vfCfg ? IPU3PipeModeVideo : > IPU3PipeModeStillCapture)); > ret = imgu->imgu_->setControls(&ctrls); > if (ret) {
On Tue, Sep 29, 2020 at 02:20:27AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Sep 24, 2020 at 04:51:41PM +0200, Jacopo Mondi wrote: > > When requesting one RAW stream and one YUV stream the > > StreamConfiguration assigned to the RAW stream is the first one > > added to the CameraConfiguration, while the YUV stream gets assigned to > > the main output. > > > > At configure() time the viewfinder output needs to be configured with > > the same format as the main output, but since the introduction of RAW > > capture support, the pipeline has not been updated and still assumes > > the main output configuration is the first one in the > > CameraConfiguration. This causes the viewfinder to be configured > > with the same format as the raw stream, breaking capture operations. > > Oops... You mentioned no CTS regression, is there also no CTS tests > fixed by this ? > > > Before this commit the following command fails and the ImgU does not > > produce frames: > > cam -srole=stillraw -srole=viewfinder -c2 -C > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 20 +++++++++----------- > > 1 file changed, 9 insertions(+), 11 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 221259c7fe61..9cea6c7e9611 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -476,25 +476,23 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > return ret; > > > > /* Apply the format to the configured streams output devices. */ > > - bool outActive = false; > > - bool vfActive = false; > > + StreamConfiguration *mainCfg = nullptr; > > + StreamConfiguration *vfCfg = nullptr; > > > > for (unsigned int i = 0; i < config->size(); ++i) { > > StreamConfiguration &cfg = (*config)[i]; > > Stream *stream = cfg.stream(); > > > > if (stream == outStream) { > > + mainCfg = &cfg; > > ret = imgu->configureOutput(cfg, &outputFormat); > > if (ret) > > return ret; > > - > > - outActive = true; > > } else if (stream == vfStream) { > > + vfCfg = &cfg; > > ret = imgu->configureViewfinder(cfg, &outputFormat); > > if (ret) > > return ret; > > - > > - vfActive = true; > > } > > } > > > > @@ -503,14 +501,14 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > * the configuration of the active one for that purpose (there should > > * be at least one active stream in the configuration request). > > */ > > - if (!outActive) { > > - ret = imgu->configureOutput(config->at(0), &outputFormat); > > + if (!mainCfg) { > > + ret = imgu->configureOutput(*vfCfg, &outputFormat); > > What if the only stream requested is the raw stream ? Won't both mainCfg > and vfCfg be null ? Scratch this, there's a check above the returns early if only a raw stream is requested. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > if (ret) > > return ret; > > } > > > > - if (!vfActive) { > > - ret = imgu->configureViewfinder(config->at(0), &outputFormat); > > + if (!vfCfg) { > > + ret = imgu->configureViewfinder(*mainCfg, &outputFormat); > > if (ret) > > return ret; > > } > > @@ -529,7 +527,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > /* Apply the "pipe_mode" control to the ImgU subdevice. */ > > ControlList ctrls(imgu->imgu_->controls()); > > ctrls.set(V4L2_CID_IPU3_PIPE_MODE, > > - static_cast<int32_t>(vfActive ? IPU3PipeModeVideo : > > + static_cast<int32_t>(vfCfg ? IPU3PipeModeVideo : > > IPU3PipeModeStillCapture)); > > ret = imgu->imgu_->setControls(&ctrls); > > if (ret) {
Hi Jacopo, Thanks for your work. On 2020-09-24 16:51:41 +0200, Jacopo Mondi wrote: > When requesting one RAW stream and one YUV stream the > StreamConfiguration assigned to the RAW stream is the first one > added to the CameraConfiguration, while the YUV stream gets assigned to > the main output. > > At configure() time the viewfinder output needs to be configured with > the same format as the main output, but since the introduction of RAW > capture support, the pipeline has not been updated and still assumes > the main output configuration is the first one in the > CameraConfiguration. This causes the viewfinder to be configured > with the same format as the raw stream, breaking capture operations. > > Before this commit the following command fails and the ImgU does not > produce frames: > cam -srole=stillraw -srole=viewfinder -c2 -C > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 221259c7fe61..9cea6c7e9611 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -476,25 +476,23 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > return ret; > > /* Apply the format to the configured streams output devices. */ > - bool outActive = false; > - bool vfActive = false; > + StreamConfiguration *mainCfg = nullptr; > + StreamConfiguration *vfCfg = nullptr; > > for (unsigned int i = 0; i < config->size(); ++i) { > StreamConfiguration &cfg = (*config)[i]; > Stream *stream = cfg.stream(); > > if (stream == outStream) { > + mainCfg = &cfg; > ret = imgu->configureOutput(cfg, &outputFormat); > if (ret) > return ret; > - > - outActive = true; > } else if (stream == vfStream) { > + vfCfg = &cfg; > ret = imgu->configureViewfinder(cfg, &outputFormat); > if (ret) > return ret; > - > - vfActive = true; > } > } > > @@ -503,14 +501,14 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > * the configuration of the active one for that purpose (there should > * be at least one active stream in the configuration request). > */ > - if (!outActive) { > - ret = imgu->configureOutput(config->at(0), &outputFormat); > + if (!mainCfg) { > + ret = imgu->configureOutput(*vfCfg, &outputFormat); > if (ret) > return ret; > } > > - if (!vfActive) { > - ret = imgu->configureViewfinder(config->at(0), &outputFormat); > + if (!vfCfg) { > + ret = imgu->configureViewfinder(*mainCfg, &outputFormat); > if (ret) > return ret; > } > @@ -529,7 +527,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > /* Apply the "pipe_mode" control to the ImgU subdevice. */ > ControlList ctrls(imgu->imgu_->controls()); > ctrls.set(V4L2_CID_IPU3_PIPE_MODE, > - static_cast<int32_t>(vfActive ? IPU3PipeModeVideo : > + static_cast<int32_t>(vfCfg ? IPU3PipeModeVideo : > IPU3PipeModeStillCapture)); > ret = imgu->imgu_->setControls(&ctrls); > if (ret) { > -- > 2.28.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 221259c7fe61..9cea6c7e9611 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -476,25 +476,23 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) return ret; /* Apply the format to the configured streams output devices. */ - bool outActive = false; - bool vfActive = false; + StreamConfiguration *mainCfg = nullptr; + StreamConfiguration *vfCfg = nullptr; for (unsigned int i = 0; i < config->size(); ++i) { StreamConfiguration &cfg = (*config)[i]; Stream *stream = cfg.stream(); if (stream == outStream) { + mainCfg = &cfg; ret = imgu->configureOutput(cfg, &outputFormat); if (ret) return ret; - - outActive = true; } else if (stream == vfStream) { + vfCfg = &cfg; ret = imgu->configureViewfinder(cfg, &outputFormat); if (ret) return ret; - - vfActive = true; } } @@ -503,14 +501,14 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) * the configuration of the active one for that purpose (there should * be at least one active stream in the configuration request). */ - if (!outActive) { - ret = imgu->configureOutput(config->at(0), &outputFormat); + if (!mainCfg) { + ret = imgu->configureOutput(*vfCfg, &outputFormat); if (ret) return ret; } - if (!vfActive) { - ret = imgu->configureViewfinder(config->at(0), &outputFormat); + if (!vfCfg) { + ret = imgu->configureViewfinder(*mainCfg, &outputFormat); if (ret) return ret; } @@ -529,7 +527,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) /* Apply the "pipe_mode" control to the ImgU subdevice. */ ControlList ctrls(imgu->imgu_->controls()); ctrls.set(V4L2_CID_IPU3_PIPE_MODE, - static_cast<int32_t>(vfActive ? IPU3PipeModeVideo : + static_cast<int32_t>(vfCfg ? IPU3PipeModeVideo : IPU3PipeModeStillCapture)); ret = imgu->imgu_->setControls(&ctrls); if (ret) {
When requesting one RAW stream and one YUV stream the StreamConfiguration assigned to the RAW stream is the first one added to the CameraConfiguration, while the YUV stream gets assigned to the main output. At configure() time the viewfinder output needs to be configured with the same format as the main output, but since the introduction of RAW capture support, the pipeline has not been updated and still assumes the main output configuration is the first one in the CameraConfiguration. This causes the viewfinder to be configured with the same format as the raw stream, breaking capture operations. Before this commit the following command fails and the ImgU does not produce frames: cam -srole=stillraw -srole=viewfinder -c2 -C Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)