| Message ID | 20220812090103.1781246-1-hanlinchen@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Han-Lin On Fri, Aug 12, 2022 at 05:01:03PM +0800, Han-Lin Chen via libcamera-devel wrote: > Using Size::expandTo() to find the max resolution might generate a non-existent > resolution. For example, when application request streams for 1920x1080 and > 1600x1200, the max resolution will be wrongly 1920x1200 and fails the > configuration. > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=139 > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index f65db3c8..47311953 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -247,6 +247,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > */ > unsigned int rawCount = 0; > unsigned int yuvCount = 0; > + Size rawRequirement; > Size maxYuvSize; > Size rawSize; > > @@ -255,10 +256,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { > rawCount++; > - rawSize.expandTo(cfg.size); > + rawSize = std::max(rawSize, cfg.size); > } else { > yuvCount++; > - maxYuvSize.expandTo(cfg.size); > + maxYuvSize = std::max(maxYuvSize, cfg.size); > + rawRequirement.expandTo(cfg.size); > } > } > > @@ -287,17 +289,17 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > * The output YUV streams will be limited in size to the maximum frame > * size requested for the RAW stream, if present. > * > - * If no raw stream is requested generate a size as large as the maximum > - * requested YUV size aligned to the ImgU constraints and bound by the > - * sensor's maximum resolution. See > + * If no raw stream is requested generate a size as large as the span > + * of all requested YUV size aligned to the ImgU constraints and bound > + * by the sensor's maximum resolution. See > * https://bugs.libcamera.org/show_bug.cgi?id=32 > */ > if (rawSize.isNull()) > - rawSize = maxYuvSize.expandedTo({ ImgUDevice::kIFMaxCropWidth, > - ImgUDevice::kIFMaxCropHeight }) > - .grownBy({ ImgUDevice::kOutputMarginWidth, > - ImgUDevice::kOutputMarginHeight }) > - .boundedTo(data_->cio2_.sensor()->resolution()); > + rawSize = rawRequirement.expandedTo({ ImgUDevice::kIFMaxCropWidth, > + ImgUDevice::kIFMaxCropHeight }) > + .grownBy({ ImgUDevice::kOutputMarginWidth, > + ImgUDevice::kOutputMarginHeight }) > + .boundedTo(data_->cio2_.sensor()->resolution()); This seems to fix the issue indeed, and maintain the current behaviour where the RAW stream size limits the YUV output size. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > if (!cio2Configuration_.pixelFormat.isValid()) > -- > 2.37.1.595.g718a3a8f04-goog >
Quoting Jacopo Mondi via libcamera-devel (2022-08-17 09:19:29) > Hi Han-Lin > > On Fri, Aug 12, 2022 at 05:01:03PM +0800, Han-Lin Chen via libcamera-devel wrote: > > Using Size::expandTo() to find the max resolution might generate a non-existent > > resolution. For example, when application request streams for 1920x1080 and > > 1600x1200, the max resolution will be wrongly 1920x1200 and fails the > > configuration. > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=139 > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++---------- > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index f65db3c8..47311953 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -247,6 +247,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > */ > > unsigned int rawCount = 0; > > unsigned int yuvCount = 0; > > + Size rawRequirement; > > Size maxYuvSize; > > Size rawSize; > > > > @@ -255,10 +256,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { > > rawCount++; > > - rawSize.expandTo(cfg.size); > > + rawSize = std::max(rawSize, cfg.size); > > } else { > > yuvCount++; > > - maxYuvSize.expandTo(cfg.size); > > + maxYuvSize = std::max(maxYuvSize, cfg.size); > > + rawRequirement.expandTo(cfg.size); > > } > > } > > > > @@ -287,17 +289,17 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > * The output YUV streams will be limited in size to the maximum frame > > * size requested for the RAW stream, if present. > > * > > - * If no raw stream is requested generate a size as large as the maximum > > - * requested YUV size aligned to the ImgU constraints and bound by the > > - * sensor's maximum resolution. See > > + * If no raw stream is requested generate a size as large as the span I know it's pre-existing, but could you add a comma here? Without it the sentence flows too fast in my head. "If no raw stream is requested, generate a size as ... I think this is called a "First conditional sentence" https://www.grammarly.com/blog/conditional-sentences/ > > + * of all requested YUV size aligned to the ImgU constraints and bound > > + * by the sensor's maximum resolution. See I'm confused here about the use of 'a span of all requested YUV sizes' as that feels like it gets confused with a libcamera::Span concept which I don't think is what it means here. I would say "If no raw stream is requested, generate a size from the largest given YUV stream, aligned to the ImgU constraints and bound by the sensor's maximum resolution. See:" > > * https://bugs.libcamera.org/show_bug.cgi?id=32 > > */ > > if (rawSize.isNull()) > > - rawSize = maxYuvSize.expandedTo({ ImgUDevice::kIFMaxCropWidth, > > - ImgUDevice::kIFMaxCropHeight }) > > - .grownBy({ ImgUDevice::kOutputMarginWidth, > > - ImgUDevice::kOutputMarginHeight }) > > - .boundedTo(data_->cio2_.sensor()->resolution()); > > + rawSize = rawRequirement.expandedTo({ ImgUDevice::kIFMaxCropWidth, > > + ImgUDevice::kIFMaxCropHeight }) > > + .grownBy({ ImgUDevice::kOutputMarginWidth, > > + ImgUDevice::kOutputMarginHeight }) > > + .boundedTo(data_->cio2_.sensor()->resolution()); > > This seems to fix the issue indeed, and maintain the current behaviour > where the RAW stream size limits the YUV output size. > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> With the comment updated in any appropriate way, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Thanks > j > > > > > cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > if (!cio2Configuration_.pixelFormat.isValid()) > > -- > > 2.37.1.595.g718a3a8f04-goog > >
Hi Kieran On Thu, Aug 25, 2022 at 10:55:53AM +0100, Kieran Bingham wrote: > Quoting Jacopo Mondi via libcamera-devel (2022-08-17 09:19:29) > > Hi Han-Lin > > > > On Fri, Aug 12, 2022 at 05:01:03PM +0800, Han-Lin Chen via libcamera-devel wrote: > > > Using Size::expandTo() to find the max resolution might generate a non-existent > > > resolution. For example, when application request streams for 1920x1080 and > > > 1600x1200, the max resolution will be wrongly 1920x1200 and fails the > > > configuration. > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=139 > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++---------- > > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index f65db3c8..47311953 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -247,6 +247,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > */ > > > unsigned int rawCount = 0; > > > unsigned int yuvCount = 0; > > > + Size rawRequirement; > > > Size maxYuvSize; > > > Size rawSize; > > > > > > @@ -255,10 +256,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > > > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { > > > rawCount++; > > > - rawSize.expandTo(cfg.size); > > > + rawSize = std::max(rawSize, cfg.size); > > > } else { > > > yuvCount++; > > > - maxYuvSize.expandTo(cfg.size); > > > + maxYuvSize = std::max(maxYuvSize, cfg.size); > > > + rawRequirement.expandTo(cfg.size); > > > } > > > } > > > > > > @@ -287,17 +289,17 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > * The output YUV streams will be limited in size to the maximum frame > > > * size requested for the RAW stream, if present. > > > * > > > - * If no raw stream is requested generate a size as large as the maximum > > > - * requested YUV size aligned to the ImgU constraints and bound by the > > > - * sensor's maximum resolution. See > > > + * If no raw stream is requested generate a size as large as the span > > I know it's pre-existing, but could you add a comma here? Without it the > sentence flows too fast in my head. > > "If no raw stream is requested, generate a size as ... > > I think this is called a "First conditional sentence" > https://www.grammarly.com/blog/conditional-sentences/ > > > > > > + * of all requested YUV size aligned to the ImgU constraints and bound > > > + * by the sensor's maximum resolution. See > > I'm confused here about the use of 'a span of all requested YUV sizes' > as that feels like it gets confused with a libcamera::Span concept which > I don't think is what it means here. > > I would say > > "If no raw stream is requested, generate a size from the largest given > YUV stream, aligned to the ImgU constraints and bound by the sensor's > maximum resolution. See:" > > > > > * https://bugs.libcamera.org/show_bug.cgi?id=32 > > > */ > > > if (rawSize.isNull()) > > > - rawSize = maxYuvSize.expandedTo({ ImgUDevice::kIFMaxCropWidth, > > > - ImgUDevice::kIFMaxCropHeight }) > > > - .grownBy({ ImgUDevice::kOutputMarginWidth, > > > - ImgUDevice::kOutputMarginHeight }) > > > - .boundedTo(data_->cio2_.sensor()->resolution()); > > > + rawSize = rawRequirement.expandedTo({ ImgUDevice::kIFMaxCropWidth, > > > + ImgUDevice::kIFMaxCropHeight }) > > > + .grownBy({ ImgUDevice::kOutputMarginWidth, > > > + ImgUDevice::kOutputMarginHeight }) > > > + .boundedTo(data_->cio2_.sensor()->resolution()); > > > > This seems to fix the issue indeed, and maintain the current behaviour > > where the RAW stream size limits the YUV output size. > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > With the comment updated in any appropriate way, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> I can update the comment when applying the patch > > > > > Thanks > > j > > > > > > > > cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > > if (!cio2Configuration_.pixelFormat.isValid()) > > > -- > > > 2.37.1.595.g718a3a8f04-goog > > >
On Thu, Aug 25, 2022 at 6:05 PM Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Hi Kieran > > On Thu, Aug 25, 2022 at 10:55:53AM +0100, Kieran Bingham wrote: > > Quoting Jacopo Mondi via libcamera-devel (2022-08-17 09:19:29) > > > Hi Han-Lin > > > > > > On Fri, Aug 12, 2022 at 05:01:03PM +0800, Han-Lin Chen via libcamera-devel wrote: > > > > Using Size::expandTo() to find the max resolution might generate a non-existent > > > > resolution. For example, when application request streams for 1920x1080 and > > > > 1600x1200, the max resolution will be wrongly 1920x1200 and fails the > > > > configuration. > > > > > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=139 > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > --- > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++---------- > > > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > index f65db3c8..47311953 100644 > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > @@ -247,6 +247,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > > */ > > > > unsigned int rawCount = 0; > > > > unsigned int yuvCount = 0; > > > > + Size rawRequirement; > > > > Size maxYuvSize; > > > > Size rawSize; > > > > > > > > @@ -255,10 +256,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > > > > > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { > > > > rawCount++; > > > > - rawSize.expandTo(cfg.size); > > > > + rawSize = std::max(rawSize, cfg.size); > > > > } else { > > > > yuvCount++; > > > > - maxYuvSize.expandTo(cfg.size); > > > > + maxYuvSize = std::max(maxYuvSize, cfg.size); > > > > + rawRequirement.expandTo(cfg.size); > > > > } > > > > } > > > > > > > > @@ -287,17 +289,17 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > > > * The output YUV streams will be limited in size to the maximum frame > > > > * size requested for the RAW stream, if present. > > > > * > > > > - * If no raw stream is requested generate a size as large as the maximum > > > > - * requested YUV size aligned to the ImgU constraints and bound by the > > > > - * sensor's maximum resolution. See > > > > + * If no raw stream is requested generate a size as large as the span > > > > I know it's pre-existing, but could you add a comma here? Without it the > > sentence flows too fast in my head. > > > > "If no raw stream is requested, generate a size as ... > > > > I think this is called a "First conditional sentence" > > https://www.grammarly.com/blog/conditional-sentences/ > > > > > > > > > > + * of all requested YUV size aligned to the ImgU constraints and bound > > > > + * by the sensor's maximum resolution. See > > > > I'm confused here about the use of 'a span of all requested YUV sizes' > > as that feels like it gets confused with a libcamera::Span concept which > > I don't think is what it means here. > > > > I would say > > > > "If no raw stream is requested, generate a size from the largest given > > YUV stream, aligned to the ImgU constraints and bound by the sensor's > > maximum resolution. See:" > > > > > > > > * https://bugs.libcamera.org/show_bug.cgi?id=32 > > > > */ > > > > if (rawSize.isNull()) > > > > - rawSize = maxYuvSize.expandedTo({ ImgUDevice::kIFMaxCropWidth, > > > > - ImgUDevice::kIFMaxCropHeight }) > > > > - .grownBy({ ImgUDevice::kOutputMarginWidth, > > > > - ImgUDevice::kOutputMarginHeight }) > > > > - .boundedTo(data_->cio2_.sensor()->resolution()); > > > > + rawSize = rawRequirement.expandedTo({ ImgUDevice::kIFMaxCropWidth, > > > > + ImgUDevice::kIFMaxCropHeight }) > > > > + .grownBy({ ImgUDevice::kOutputMarginWidth, > > > > + ImgUDevice::kOutputMarginHeight }) > > > > + .boundedTo(data_->cio2_.sensor()->resolution()); > > > > > > This seems to fix the issue indeed, and maintain the current behaviour > > > where the RAW stream size limits the YUV output size. > > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > With the comment updated in any appropriate way, > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > I can update the comment when applying the patch Many thanks Jacopo :) > > > > > > > > > Thanks > > > j > > > > > > > > > > > cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); > > > > if (!cio2Configuration_.pixelFormat.isValid()) > > > > -- > > > > 2.37.1.595.g718a3a8f04-goog > > > >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index f65db3c8..47311953 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -247,6 +247,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() */ unsigned int rawCount = 0; unsigned int yuvCount = 0; + Size rawRequirement; Size maxYuvSize; Size rawSize; @@ -255,10 +256,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) { rawCount++; - rawSize.expandTo(cfg.size); + rawSize = std::max(rawSize, cfg.size); } else { yuvCount++; - maxYuvSize.expandTo(cfg.size); + maxYuvSize = std::max(maxYuvSize, cfg.size); + rawRequirement.expandTo(cfg.size); } } @@ -287,17 +289,17 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() * The output YUV streams will be limited in size to the maximum frame * size requested for the RAW stream, if present. * - * If no raw stream is requested generate a size as large as the maximum - * requested YUV size aligned to the ImgU constraints and bound by the - * sensor's maximum resolution. See + * If no raw stream is requested generate a size as large as the span + * of all requested YUV size aligned to the ImgU constraints and bound + * by the sensor's maximum resolution. See * https://bugs.libcamera.org/show_bug.cgi?id=32 */ if (rawSize.isNull()) - rawSize = maxYuvSize.expandedTo({ ImgUDevice::kIFMaxCropWidth, - ImgUDevice::kIFMaxCropHeight }) - .grownBy({ ImgUDevice::kOutputMarginWidth, - ImgUDevice::kOutputMarginHeight }) - .boundedTo(data_->cio2_.sensor()->resolution()); + rawSize = rawRequirement.expandedTo({ ImgUDevice::kIFMaxCropWidth, + ImgUDevice::kIFMaxCropHeight }) + .grownBy({ ImgUDevice::kOutputMarginWidth, + ImgUDevice::kOutputMarginHeight }) + .boundedTo(data_->cio2_.sensor()->resolution()); cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize); if (!cio2Configuration_.pixelFormat.isValid())
Using Size::expandTo() to find the max resolution might generate a non-existent resolution. For example, when application request streams for 1920x1080 and 1600x1200, the max resolution will be wrongly 1920x1200 and fails the configuration. Bug: https://bugs.libcamera.org/show_bug.cgi?id=139 Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)