| Message ID | 20210202123620.45200-1-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 02/02/2021 12:36, Jacopo Mondi wrote: > Calculate the JPEG maximum size using the maximum preview format size > multiplied by a 1.5 factor. > > The same multiplication factor is used in the existing HAL > implementation in ChromeOS. This is definitely better than the somewhat arbitrary fixed value I picked from another HAL. > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index a50b0ebfe60e..cb87d97888ed 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -347,12 +347,6 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer > { > camera_->requestCompleted.connect(this, &CameraDevice::requestComplete); > > - /* > - * \todo Determine a more accurate value for this during > - * streamConfiguration. > - */ > - maxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */ > - > maker_ = "libcamera"; > model_ = "cameraModel"; > > @@ -629,6 +623,7 @@ int CameraDevice::initializeStreamConfigurations() > mappedFormat, > cameraResolutions); > > + Size maxJpegSize; > for (const Size &res : resolutions) { > streamConfigurations_.push_back({ res, androidFormat }); > > @@ -643,9 +638,17 @@ int CameraDevice::initializeStreamConfigurations() > * \todo Support JPEG streams produced by the Camera > * natively. > */ > - if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) > + if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) { > streamConfigurations_.push_back( > { res, HAL_PIXEL_FORMAT_BLOB }); > + > + if (res > maxJpegSize) { Aha, so this runs over each of the supported stream configurations and thus only the max gets through ? is that right? (I think it is, so there's no issue) > + maxJpegSize = res; > + maxJpegBufferSize_ = maxJpegSize.width > + * maxJpegSize.height > + * 1.5; > + } > + } > } > } > > @@ -878,10 +881,6 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > availableThumbnailSizes.data(), > availableThumbnailSizes.size()); > > - /* > - * \todo Calculate the maximum JPEG buffer size by asking the encoder > - * giving the maximum frame size required. > - */ Is this no longer worth considering? I sort of agree at the moment. And if we decide we want to ask the encoders we can handle that later. I don't think we need to worry about the todo. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1); > > /* Sensor static metadata. */ >
Hi Kieran, On Tue, Feb 02, 2021 at 03:23:22PM +0000, Kieran Bingham wrote: > Hi Jacopo, > > On 02/02/2021 12:36, Jacopo Mondi wrote: > > Calculate the JPEG maximum size using the maximum preview format size > > multiplied by a 1.5 factor. > > > > The same multiplication factor is used in the existing HAL > > implementation in ChromeOS. > > This is definitely better than the somewhat arbitrary fixed value I > picked from another HAL. > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 21 ++++++++++----------- > > 1 file changed, 10 insertions(+), 11 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index a50b0ebfe60e..cb87d97888ed 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -347,12 +347,6 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer > > { > > camera_->requestCompleted.connect(this, &CameraDevice::requestComplete); > > > > - /* > > - * \todo Determine a more accurate value for this during > > - * streamConfiguration. > > - */ > > - maxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */ > > - > > maker_ = "libcamera"; > > model_ = "cameraModel"; > > > > @@ -629,6 +623,7 @@ int CameraDevice::initializeStreamConfigurations() > > mappedFormat, > > cameraResolutions); > > > > + Size maxJpegSize; > > for (const Size &res : resolutions) { > > streamConfigurations_.push_back({ res, androidFormat }); > > > > @@ -643,9 +638,17 @@ int CameraDevice::initializeStreamConfigurations() > > * \todo Support JPEG streams produced by the Camera > > * natively. > > */ > > - if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) > > + if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) { > > streamConfigurations_.push_back( > > { res, HAL_PIXEL_FORMAT_BLOB }); > > + > > + if (res > maxJpegSize) { > > Aha, so this runs over each of the supported stream configurations and > thus only the max gets through ? is that right? yes, this collects the maximum YUV size from which we encode JPEG > > (I think it is, so there's no issue) > > > + maxJpegSize = res; > > + maxJpegBufferSize_ = maxJpegSize.width > > + * maxJpegSize.height > > + * 1.5; > > + } > > + } > > } > > } > > > > @@ -878,10 +881,6 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > > availableThumbnailSizes.data(), > > availableThumbnailSizes.size()); > > > > - /* > > - * \todo Calculate the maximum JPEG buffer size by asking the encoder > > - * giving the maximum frame size required. > > - */ > > Is this no longer worth considering? > I sort of agree at the moment. And if we decide we want to ask the > encoders we can handle that later. I don't think we need to worry about > the todo. I don't know for sure :/ Also the 1.5 ratio is a copy&paste from the existing HALs > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1); > > > > /* Sensor static metadata. */ > > > > -- > Regards > -- > Kieran
Hi Jacopo On 2/2/21 6:06 PM, Jacopo Mondi wrote: > Calculate the JPEG maximum size using the maximum preview format size > multiplied by a 1.5 factor. > > The same multiplication factor is used in the existing HAL > implementation in ChromeOS. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index a50b0ebfe60e..cb87d97888ed 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -347,12 +347,6 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer > { > camera_->requestCompleted.connect(this, &CameraDevice::requestComplete); > > - /* > - * \todo Determine a more accurate value for this during > - * streamConfiguration. > - */ > - maxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */ > - Since we are removing from constructor, I would add a initialize value to 0, in constructor initializer list above, since this is (now) used inside a conditional block in initializeStreamConfigurations() Other than that; Reviewed-by: Umang Jain <email@uajain.com> > maker_ = "libcamera"; > model_ = "cameraModel"; > > @@ -629,6 +623,7 @@ int CameraDevice::initializeStreamConfigurations() > mappedFormat, > cameraResolutions); > > + Size maxJpegSize; > for (const Size &res : resolutions) { > streamConfigurations_.push_back({ res, androidFormat }); > > @@ -643,9 +638,17 @@ int CameraDevice::initializeStreamConfigurations() > * \todo Support JPEG streams produced by the Camera > * natively. > */ > - if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) > + if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) { > streamConfigurations_.push_back( > { res, HAL_PIXEL_FORMAT_BLOB }); > + > + if (res > maxJpegSize) { > + maxJpegSize = res; > + maxJpegBufferSize_ = maxJpegSize.width > + * maxJpegSize.height > + * 1.5; > + } > + } > } > } > > @@ -878,10 +881,6 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > availableThumbnailSizes.data(), > availableThumbnailSizes.size()); > > - /* > - * \todo Calculate the maximum JPEG buffer size by asking the encoder > - * giving the maximum frame size required. > - */ > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1); > > /* Sensor static metadata. */
Hi Jacopo, Thank you for the patch. On Tue, Feb 02, 2021 at 04:50:46PM +0100, Jacopo Mondi wrote: > On Tue, Feb 02, 2021 at 03:23:22PM +0000, Kieran Bingham wrote: > > On 02/02/2021 12:36, Jacopo Mondi wrote: > > > Calculate the JPEG maximum size using the maximum preview format size > > > multiplied by a 1.5 factor. > > > > > > The same multiplication factor is used in the existing HAL > > > implementation in ChromeOS. > > > > This is definitely better than the somewhat arbitrary fixed value I > > picked from another HAL. > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/android/camera_device.cpp | 21 ++++++++++----------- > > > 1 file changed, 10 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index a50b0ebfe60e..cb87d97888ed 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -347,12 +347,6 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer > > > { > > > camera_->requestCompleted.connect(this, &CameraDevice::requestComplete); > > > > > > - /* > > > - * \todo Determine a more accurate value for this during > > > - * streamConfiguration. > > > - */ > > > - maxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */ > > > - > > > maker_ = "libcamera"; > > > model_ = "cameraModel"; > > > > > > @@ -629,6 +623,7 @@ int CameraDevice::initializeStreamConfigurations() > > > mappedFormat, > > > cameraResolutions); > > > > > > + Size maxJpegSize; > > > for (const Size &res : resolutions) { > > > streamConfigurations_.push_back({ res, androidFormat }); > > > > > > @@ -643,9 +638,17 @@ int CameraDevice::initializeStreamConfigurations() > > > * \todo Support JPEG streams produced by the Camera > > > * natively. > > > */ > > > - if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) > > > + if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) { > > > streamConfigurations_.push_back( > > > { res, HAL_PIXEL_FORMAT_BLOB }); > > > + > > > + if (res > maxJpegSize) { > > > > Aha, so this runs over each of the supported stream configurations and > > thus only the max gets through ? is that right? > > yes, this collects the maximum YUV size from which we encode JPEG > > > (I think it is, so there's no issue) > > > > > + maxJpegSize = res; > > > + maxJpegBufferSize_ = maxJpegSize.width > > > + * maxJpegSize.height > > > + * 1.5; You could move the calculation of maxJpegBufferSize_ after the loop. Youl could also write the maxJpegSize calculation as maxJpegSize = std::max(maxJpegSize, res); I'd do at least the former to make sure maxJpegBufferSize_ gets initialized in all code paths. Up to you for the latter. > > > + } > > > + } > > > } > > > } > > > > > > @@ -878,10 +881,6 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > > > availableThumbnailSizes.data(), > > > availableThumbnailSizes.size()); > > > > > > - /* > > > - * \todo Calculate the maximum JPEG buffer size by asking the encoder > > > - * giving the maximum frame size required. > > > - */ > > > > Is this no longer worth considering? > > I sort of agree at the moment. And if we decide we want to ask the > > encoders we can handle that later. I don't think we need to worry about > > the todo. > > I don't know for sure :/ > Also the 1.5 ratio is a copy&paste from the existing HALs I think it's worth keeping the todo. You could move it to CameraDevice::initializeStreamConfigurations() right before setting maxJpegBufferSize_. We're one step closer to addressing this as we now calculate the maximum frame size, the last remaining matter will be to move the maxJpegBufferSize calculation to the encoder class. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1); > > > > > > /* Sensor static metadata. */
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index a50b0ebfe60e..cb87d97888ed 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -347,12 +347,6 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer { camera_->requestCompleted.connect(this, &CameraDevice::requestComplete); - /* - * \todo Determine a more accurate value for this during - * streamConfiguration. - */ - maxJpegBufferSize_ = 13 << 20; /* 13631488 from USB HAL */ - maker_ = "libcamera"; model_ = "cameraModel"; @@ -629,6 +623,7 @@ int CameraDevice::initializeStreamConfigurations() mappedFormat, cameraResolutions); + Size maxJpegSize; for (const Size &res : resolutions) { streamConfigurations_.push_back({ res, androidFormat }); @@ -643,9 +638,17 @@ int CameraDevice::initializeStreamConfigurations() * \todo Support JPEG streams produced by the Camera * natively. */ - if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) + if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) { streamConfigurations_.push_back( { res, HAL_PIXEL_FORMAT_BLOB }); + + if (res > maxJpegSize) { + maxJpegSize = res; + maxJpegBufferSize_ = maxJpegSize.width + * maxJpegSize.height + * 1.5; + } + } } } @@ -878,10 +881,6 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() availableThumbnailSizes.data(), availableThumbnailSizes.size()); - /* - * \todo Calculate the maximum JPEG buffer size by asking the encoder - * giving the maximum frame size required. - */ staticMetadata_->addEntry(ANDROID_JPEG_MAX_SIZE, &maxJpegBufferSize_, 1); /* Sensor static metadata. */
Calculate the JPEG maximum size using the maximum preview format size multiplied by a 1.5 factor. The same multiplication factor is used in the existing HAL implementation in ChromeOS. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)