| Message ID | 20210204143503.33498-1-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 04/02/2021 14:35, Jacopo Mondi wrote: > The list of the available thumbnail sizes is generated from the > list of available JPEG resolution, one for each aspect ratio. > > This change fixes the CTS test > android.hardware.cts.CameraTest#testJpegThumbnailSize > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 39 ++++++++++++++++++++++++++++++----- > 1 file changed, 34 insertions(+), 5 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index df5e295656d7..0b699c957990 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -871,12 +871,41 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > &availableControlModes, 1); > > /* JPEG static metadata. */ > - std::vector<int32_t> availableThumbnailSizes = { > - 0, 0, > - }; > + > + /* > + * Create the list of supported tumbnail sizes by inspecting the s/tumbnail/thumbnail/ > + * available JPEG resolutions collected in streamConfigurations_ and > + * generate one entry for each aspect ratio. > + * > + * The JPEG thumbnailer can freely scale, so pick an arbitrary > + * (160, 120) size as designated thumbnail size. Shouldn't we give a square size here? 160,160? That way a 9:16 ratio would be the same 'size' as a 16:9 for example. (Thinking of phones/devices taking portrait rather than landscape pictures). > + */ > + constexpr Size maxJpegThumbnail(160, 120); > + std::vector<Size> thumbnailSizes; > + thumbnailSizes.push_back({ 0, 0 }); > + for (const auto &entry : streamConfigurations_) { > + if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB) > + continue; > + > + Size thumbnailSize = maxJpegThumbnail > + .boundedToAspectRatio({ entry.resolution.width, > + entry.resolution.height }); I love how easy those helpers make maintaining aspect ratios. > + thumbnailSizes.push_back(thumbnailSize); > + } > + > + std::sort(thumbnailSizes.begin(), thumbnailSizes.end()); > + auto last = std::unique(thumbnailSizes.begin(), thumbnailSizes.end()); > + thumbnailSizes.erase(last, thumbnailSizes.end()); > + > + /* Transform it in a list of integers that can be consumed. */ 'in to a list' ? > + std::vector<int32_t> thumbnailEntries; > + thumbnailEntries.reserve(thumbnailSizes.size() * 2); > + for (const auto &size : thumbnailSizes) { > + thumbnailEntries.push_back(size.width); > + thumbnailEntries.push_back(size.height); > + } > staticMetadata_->addEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, > - availableThumbnailSizes.data(), > - availableThumbnailSizes.size()); > + thumbnailEntries.data(), thumbnailEntries.size()); The number of times we add a vector to addEntry() shouldn't we have an addEntry(id, vector) so we don't duplicate .data(), .size() each time? But that's a digression - not for this patch. Otherwise sounds good to me. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > /* > * \todo Calculate the maximum JPEG buffer size by asking the encoder >
Hi Kieran, On Thu, Feb 04, 2021 at 02:47:13PM +0000, Kieran Bingham wrote: > Hi Jacopo, > > On 04/02/2021 14:35, Jacopo Mondi wrote: > > The list of the available thumbnail sizes is generated from the > > list of available JPEG resolution, one for each aspect ratio. > > > > This change fixes the CTS test > > android.hardware.cts.CameraTest#testJpegThumbnailSize > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 39 ++++++++++++++++++++++++++++++----- > > 1 file changed, 34 insertions(+), 5 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index df5e295656d7..0b699c957990 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -871,12 +871,41 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() > > &availableControlModes, 1); > > > > /* JPEG static metadata. */ > > - std::vector<int32_t> availableThumbnailSizes = { > > - 0, 0, > > - }; > > + > > + /* > > + * Create the list of supported tumbnail sizes by inspecting the > > s/tumbnail/thumbnail/ > > > + * available JPEG resolutions collected in streamConfigurations_ and > > + * generate one entry for each aspect ratio. > > + * > > + * The JPEG thumbnailer can freely scale, so pick an arbitrary > > + * (160, 120) size as designated thumbnail size. > > Shouldn't we give a square size here? 160,160? > > That way a 9:16 ratio would be the same 'size' as a 16:9 for example. > (Thinking of phones/devices taking portrait rather than landscape pictures). (160, 120) is arbitrary, and you're probably right here. I'll test with 160, 160 and resend with your comments taken in. Thanks j > > > + */ > > + constexpr Size maxJpegThumbnail(160, 120); > > + std::vector<Size> thumbnailSizes; > > + thumbnailSizes.push_back({ 0, 0 }); > > + for (const auto &entry : streamConfigurations_) { > > + if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB) > > + continue; > > + > > + Size thumbnailSize = maxJpegThumbnail > > + .boundedToAspectRatio({ entry.resolution.width, > > + entry.resolution.height }); > > I love how easy those helpers make maintaining aspect ratios. > > > + thumbnailSizes.push_back(thumbnailSize); > > + } > > + > > + std::sort(thumbnailSizes.begin(), thumbnailSizes.end()); > > + auto last = std::unique(thumbnailSizes.begin(), thumbnailSizes.end()); > > + thumbnailSizes.erase(last, thumbnailSizes.end()); > > + > > + /* Transform it in a list of integers that can be consumed. */ > > 'in to a list' ? > > > + std::vector<int32_t> thumbnailEntries; > > + thumbnailEntries.reserve(thumbnailSizes.size() * 2); > > + for (const auto &size : thumbnailSizes) { > > + thumbnailEntries.push_back(size.width); > > + thumbnailEntries.push_back(size.height); > > + } > > staticMetadata_->addEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, > > - availableThumbnailSizes.data(), > > - availableThumbnailSizes.size()); > > + thumbnailEntries.data(), thumbnailEntries.size()); > > The number of times we add a vector to addEntry() shouldn't we have an > addEntry(id, vector) so we don't duplicate .data(), .size() each time? > > But that's a digression - not for this patch. > > Otherwise sounds good to me. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > /* > > * \todo Calculate the maximum JPEG buffer size by asking the encoder > > > > -- > Regards > -- > Kieran
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index df5e295656d7..0b699c957990 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -871,12 +871,41 @@ const camera_metadata_t *CameraDevice::getStaticMetadata() &availableControlModes, 1); /* JPEG static metadata. */ - std::vector<int32_t> availableThumbnailSizes = { - 0, 0, - }; + + /* + * Create the list of supported tumbnail sizes by inspecting the + * available JPEG resolutions collected in streamConfigurations_ and + * generate one entry for each aspect ratio. + * + * The JPEG thumbnailer can freely scale, so pick an arbitrary + * (160, 120) size as designated thumbnail size. + */ + constexpr Size maxJpegThumbnail(160, 120); + std::vector<Size> thumbnailSizes; + thumbnailSizes.push_back({ 0, 0 }); + for (const auto &entry : streamConfigurations_) { + if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB) + continue; + + Size thumbnailSize = maxJpegThumbnail + .boundedToAspectRatio({ entry.resolution.width, + entry.resolution.height }); + thumbnailSizes.push_back(thumbnailSize); + } + + std::sort(thumbnailSizes.begin(), thumbnailSizes.end()); + auto last = std::unique(thumbnailSizes.begin(), thumbnailSizes.end()); + thumbnailSizes.erase(last, thumbnailSizes.end()); + + /* Transform it in a list of integers that can be consumed. */ + std::vector<int32_t> thumbnailEntries; + thumbnailEntries.reserve(thumbnailSizes.size() * 2); + for (const auto &size : thumbnailSizes) { + thumbnailEntries.push_back(size.width); + thumbnailEntries.push_back(size.height); + } staticMetadata_->addEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, - availableThumbnailSizes.data(), - availableThumbnailSizes.size()); + thumbnailEntries.data(), thumbnailEntries.size()); /* * \todo Calculate the maximum JPEG buffer size by asking the encoder
The list of the available thumbnail sizes is generated from the list of available JPEG resolution, one for each aspect ratio. This change fixes the CTS test android.hardware.cts.CameraTest#testJpegThumbnailSize Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 39 ++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)