| Message ID | 20210913040110.13789-3-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote: > Generation of thumbnail is not occuring currently because > ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed > to PostProcessorJpeg::process(). This is a regression introduced in > 1264628d3c92("android: jpeg: Configure thumbnailer based on request > metadata"). I see the issue, but I'm not sure those commit was wrong. I mean, what that commit does that itroduced a regression is: --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry); + if (ret) { + const int32_t *data = entry.data.i32; + Size thumbnailSize = { static_cast<uint32_t>(data[0]), + static_cast<uint32_t>(data[1]) }; + + if (thumbnailSize != Size(0, 0)) { + std::vector<unsigned char> thumbnail; + generateThumbnail(source, thumbnailSize, &thumbnail); + if (!thumbnail.empty()) + exif.setThumbnail(thumbnail, Exif::Compression::JPEG); + } + + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2); + + /* \todo Use this quality as a parameter to the encoder */ + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry); + if (ret) + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, + entry.data.u8, 1); + } Which I read as: "If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in the request's settings, do not populate it in result metadata". Is this correct in your opinion, or should we populate it regardless of the fact the tag was passed in ? > > The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in > the request metadata template populated by > CameraCapabilities::requestTemplatePreview(). The value for > ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size > reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES. Let's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the capture request template generated for the preview use case. I wonder if the JPEG thunbnail size should be part of the preview template now. > > Fixes: 1264628d3c92("android: jpeg: Configure thumbnailer based on request metadata") > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/camera_capabilities.cpp | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index e92bca42..76dddafd 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -1341,9 +1341,9 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplatePreview() con > { > /* > * \todo Keep this in sync with the actual number of entries. > - * Currently: 20 entries, 35 bytes > + * Currently: 21 entries, 37 bytes > */ > - auto requestTemplate = std::make_unique<CameraMetadata>(21, 36); > + auto requestTemplate = std::make_unique<CameraMetadata>(22, 38); Comment says 21, code says 22. It was there already as the comment was not aligned with the code, but since you're at it you could fix it > if (!requestTemplate->isValid()) { > return nullptr; > } > @@ -1364,6 +1364,16 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplatePreview() con > requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, > entry.data.i32, 2); > > + /* > + * Get thumbnail sizes from static metadata and add the first non-zero > + * size to the template. > + */ > + found = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, > + &entry); > + ASSERT(found && entry.count >= 4); > + requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, > + entry.data.i32 + 2, 2); > + If I were to be extra paranoid I would make sure we actually discard (0, 0). The code assumes the first two entries are (0, 0) which is fine as we populate it after all. If you don't want to unsigned int j = 0; while (j < entry.count / 2) { if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] == 0) continue; requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, entry.data.i32 + j, 2); } Could you at least capture that we assume the first two entries are (0,0) ? Thanks j > uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON; > requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode); > > -- > 2.31.1 >
On Mon, Sep 13, 2021 at 04:08:05PM +0200, Jacopo Mondi wrote: > Hi Umang, > > On Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote: > > Generation of thumbnail is not occuring currently because > > ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed > > to PostProcessorJpeg::process(). This is a regression introduced in > > 1264628d3c92("android: jpeg: Configure thumbnailer based on request > > metadata"). > > I see the issue, but I'm not sure those commit was wrong. I mean, what > that commit does that itroduced a regression is: > > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry); > + if (ret) { > + const int32_t *data = entry.data.i32; > + Size thumbnailSize = { static_cast<uint32_t>(data[0]), > + static_cast<uint32_t>(data[1]) }; > + > + if (thumbnailSize != Size(0, 0)) { > + std::vector<unsigned char> thumbnail; > + generateThumbnail(source, thumbnailSize, &thumbnail); > + if (!thumbnail.empty()) > + exif.setThumbnail(thumbnail, Exif::Compression::JPEG); > + } > + > + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2); > + > + /* \todo Use this quality as a parameter to the encoder */ > + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry); > + if (ret) > + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, > + entry.data.u8, 1); > + } > > > Which I read as: "If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in > the request's settings, do not populate it in result metadata". > > Is this correct in your opinion, or should we populate it regardless > of the fact the tag was passed in ? > > > > > The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in > > the request metadata template populated by > > CameraCapabilities::requestTemplatePreview(). The value for > > ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size > > reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES. > > Let's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the > capture request template generated for the preview use case. I wonder > if the JPEG thunbnail size should be part of the preview template now. > > > > > Fixes: 1264628d3c92("android: jpeg: Configure thumbnailer based on request metadata") > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/android/camera_capabilities.cpp | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > index e92bca42..76dddafd 100644 > > --- a/src/android/camera_capabilities.cpp > > +++ b/src/android/camera_capabilities.cpp > > @@ -1341,9 +1341,9 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplatePreview() con > > { > > /* > > * \todo Keep this in sync with the actual number of entries. > > - * Currently: 20 entries, 35 bytes > > + * Currently: 21 entries, 37 bytes > > */ > > - auto requestTemplate = std::make_unique<CameraMetadata>(21, 36); > > + auto requestTemplate = std::make_unique<CameraMetadata>(22, 38); > > Comment says 21, code says 22. > > It was there already as the comment was not aligned with the code, but > since you're at it you could fix it > > > if (!requestTemplate->isValid()) { > > return nullptr; > > } > > @@ -1364,6 +1364,16 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplatePreview() con > > requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, > > entry.data.i32, 2); > > > > + /* > > + * Get thumbnail sizes from static metadata and add the first non-zero > > + * size to the template. > > + */ > > + found = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, > > + &entry); > > + ASSERT(found && entry.count >= 4); > > + requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, > > + entry.data.i32 + 2, 2); > > + > > If I were to be extra paranoid I would make sure we actually discard > (0, 0). The code assumes the first two entries are (0, 0) which is > fine as we populate it after all. > > If you don't want to > > unsigned int j = 0; > while (j < entry.count / 2) { > if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] == 0) { j += 2; :) > continue; } > > requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, > entry.data.i32 + j, 2); > } > > Could you at least capture that we assume the first two entries are > (0,0) ? > > Thanks > j > > > > uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON; > > requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode); > > > > -- > > 2.31.1 > >
Hi Jacopo On 9/13/21 7:38 PM, Jacopo Mondi wrote: > Hi Umang, > > On Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote: >> Generation of thumbnail is not occuring currently because >> ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed >> to PostProcessorJpeg::process(). This is a regression introduced in >> 1264628d3c92("android: jpeg: Configure thumbnailer based on request >> metadata"). > I see the issue, but I'm not sure those commit was wrong. I mean, what > that commit does that itroduced a regression is: > > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry); > + if (ret) { > + const int32_t *data = entry.data.i32; > + Size thumbnailSize = { static_cast<uint32_t>(data[0]), > + static_cast<uint32_t>(data[1]) }; > + > + if (thumbnailSize != Size(0, 0)) { > + std::vector<unsigned char> thumbnail; > + generateThumbnail(source, thumbnailSize, &thumbnail); > + if (!thumbnail.empty()) > + exif.setThumbnail(thumbnail, Exif::Compression::JPEG); > + } > + > + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2); > + > + /* \todo Use this quality as a parameter to the encoder */ > + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry); > + if (ret) > + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, > + entry.data.u8, 1); > + } > > > Which I read as: "If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in > the request's settings, do not populate it in result metadata". > > Is this correct in your opinion, or should we populate it regardless > of the fact the tag was passed in ? This is correct, but the counterpart of the patch seems missing from that commit, which actually resulted in regression > >> The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in >> the request metadata template populated by >> CameraCapabilities::requestTemplatePreview(). The value for >> ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size >> reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES. > Let's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the > capture request template generated for the preview use case. I wonder > if the JPEG thunbnail size should be part of the preview template now. I wonder that too... > >> Fixes: 1264628d3c92("android: jpeg: Configure thumbnailer based on request metadata") >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> src/android/camera_capabilities.cpp | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp >> index e92bca42..76dddafd 100644 >> --- a/src/android/camera_capabilities.cpp >> +++ b/src/android/camera_capabilities.cpp >> @@ -1341,9 +1341,9 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplatePreview() con >> { >> /* >> * \todo Keep this in sync with the actual number of entries. >> - * Currently: 20 entries, 35 bytes >> + * Currently: 21 entries, 37 bytes >> */ >> - auto requestTemplate = std::make_unique<CameraMetadata>(21, 36); >> + auto requestTemplate = std::make_unique<CameraMetadata>(22, 38); > Comment says 21, code says 22. > > It was there already as the comment was not aligned with the code, but > since you're at it you could fix it Looking at both #entries and #bytes, that gives me an impression that extra entry and data buffer bytes are **intentional**, so I went ahead with it! Are both of them a typo? I'll need some checking tomorrow. > >> if (!requestTemplate->isValid()) { >> return nullptr; >> } >> @@ -1364,6 +1364,16 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplatePreview() con >> requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, >> entry.data.i32, 2); >> >> + /* >> + * Get thumbnail sizes from static metadata and add the first non-zero >> + * size to the template. >> + */ >> + found = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, >> + &entry); >> + ASSERT(found && entry.count >= 4); >> + requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, >> + entry.data.i32 + 2, 2); >> + > If I were to be extra paranoid I would make sure we actually discard > (0, 0). The code assumes the first two entries are (0, 0) which is > fine as we populate it after all. Not only we populate it, it's a requirement that Size (0,0) should be in the vector /and/ the vector needs to be sorted in ascending order [1] if more sizes are provided with. We do the right thing when populating ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES so I won't be extra paranoid here. [1]: https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#JPEG_AVAILABLE_THUMBNAIL_SIZES > > If you don't want to > > unsigned int j = 0; > while (j < entry.count / 2) { > if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] == 0) > continue; > > requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, > entry.data.i32 + j, 2); > } > > Could you at least capture that we assume the first two entries are > (0,0) ? As said above, it's a requirement for ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, so should I make it extra clear again here? > > Thanks > j > > >> uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON; >> requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode); >> >> -- >> 2.31.1 >>
Hi Jacopo On 9/13/21 8:11 PM, Umang Jain wrote: > Hi Jacopo > > On 9/13/21 7:38 PM, Jacopo Mondi wrote: >> Hi Umang, >> >> On Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote: >>> Generation of thumbnail is not occuring currently because >>> ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed >>> to PostProcessorJpeg::process(). This is a regression introduced in >>> 1264628d3c92("android: jpeg: Configure thumbnailer based on request >>> metadata"). >> I see the issue, but I'm not sure those commit was wrong. I mean, what >> that commit does that itroduced a regression is: >> >> --- a/src/android/jpeg/post_processor_jpeg.cpp >> +++ b/src/android/jpeg/post_processor_jpeg.cpp >> + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, >> &entry); >> + if (ret) { >> + const int32_t *data = entry.data.i32; >> + Size thumbnailSize = { static_cast<uint32_t>(data[0]), >> + static_cast<uint32_t>(data[1]) }; >> + >> + if (thumbnailSize != Size(0, 0)) { >> + std::vector<unsigned char> thumbnail; >> + generateThumbnail(source, thumbnailSize, >> &thumbnail); >> + if (!thumbnail.empty()) >> + exif.setThumbnail(thumbnail, >> Exif::Compression::JPEG); >> + } >> + >> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2); >> + >> + /* \todo Use this quality as a parameter to the >> encoder */ >> + ret = >> requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry); >> + if (ret) >> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, >> + entry.data.u8, 1); >> + } >> >> >> Which I read as: "If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in >> the request's settings, do not populate it in result metadata". >> >> Is this correct in your opinion, or should we populate it regardless >> of the fact the tag was passed in ? > > > This is correct, but the counterpart of the patch seems missing from > that commit, which actually resulted in regression > >> >>> The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in >>> the request metadata template populated by >>> CameraCapabilities::requestTemplatePreview(). The value for >>> ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size >>> reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES. >> Let's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the >> capture request template generated for the preview use case. I wonder >> if the JPEG thunbnail size should be part of the preview template now. > > > I wonder that too... > >> >>> Fixes: 1264628d3c92("android: jpeg: Configure thumbnailer based on >>> request metadata") >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> src/android/camera_capabilities.cpp | 14 ++++++++++++-- >>> 1 file changed, 12 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/android/camera_capabilities.cpp >>> b/src/android/camera_capabilities.cpp >>> index e92bca42..76dddafd 100644 >>> --- a/src/android/camera_capabilities.cpp >>> +++ b/src/android/camera_capabilities.cpp >>> @@ -1341,9 +1341,9 @@ std::unique_ptr<CameraMetadata> >>> CameraCapabilities::requestTemplatePreview() con >>> { >>> /* >>> * \todo Keep this in sync with the actual number of entries. >>> - * Currently: 20 entries, 35 bytes >>> + * Currently: 21 entries, 37 bytes >>> */ >>> - auto requestTemplate = std::make_unique<CameraMetadata>(21, 36); >>> + auto requestTemplate = std::make_unique<CameraMetadata>(22, 38); >> Comment says 21, code says 22. >> >> It was there already as the comment was not aligned with the code, but >> since you're at it you could fix it > > > Looking at both #entries and #bytes, that gives me an impression that > extra entry and data buffer bytes are **intentional**, so I went ahead > with it! Are both of them a typo? I'll need some checking tomorrow. Indeed, my assumption was a mistake. I think the mismatch was long sitting under the hood, and never really sync-ed with all the rework around CameraMetadata and request templates however, I found this ancient mistake where it was supposed to match, but unfortunately didn't, and the development/increment (as new entries were added) kept happening on top of it. https://git.linuxtv.org/libcamera.git/commit/?id=637034742f2b0b7524 I'll mention this as a point of divergence in v3 for this particular patch. > >> >>> if (!requestTemplate->isValid()) { >>> return nullptr; >>> } >>> @@ -1364,6 +1364,16 @@ std::unique_ptr<CameraMetadata> >>> CameraCapabilities::requestTemplatePreview() con >>> requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, >>> entry.data.i32, 2); >>> >>> + /* >>> + * Get thumbnail sizes from static metadata and add the first >>> non-zero >>> + * size to the template. >>> + */ >>> + found = >>> staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, >>> + &entry); >>> + ASSERT(found && entry.count >= 4); >>> + requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, >>> + entry.data.i32 + 2, 2); >>> + >> If I were to be extra paranoid I would make sure we actually discard >> (0, 0). The code assumes the first two entries are (0, 0) which is >> fine as we populate it after all. > > > Not only we populate it, it's a requirement that Size (0,0) should be > in the vector /and/ the vector needs to be sorted in ascending order > [1] if more sizes are provided with. We do the right thing when > populating ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES so I won't be extra > paranoid here. > > > [1]: > https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#JPEG_AVAILABLE_THUMBNAIL_SIZES > >> >> If you don't want to >> >> unsigned int j = 0; >> while (j < entry.count / 2) { >> if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] >> == 0) >> continue; >> >> requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, >> entry.data.i32 + j, 2); >> } >> >> Could you at least capture that we assume the first two entries are >> (0,0) ? I am re-thinking this. It makes sense. It's one step more defensive than the ASSERT(found && entry.count >= 4); It's atleast for the better. I'll send a v3 for this patch (the 1/2 has already been merged) and also test out the thumbnail generation with the v3 and we can put an end to this thumbnail saga... Thanks for your thoughts! > > As said above, it's a requirement for > ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, so should I make it extra > clear again here? > >> >> Thanks >> j >> >> >>> uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON; >>> requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode); >>> >>> -- >>> 2.31.1 >>>
On 9/21/21 5:20 PM, Umang Jain wrote: > Hi Jacopo > > On 9/13/21 8:11 PM, Umang Jain wrote: >> Hi Jacopo >> >> On 9/13/21 7:38 PM, Jacopo Mondi wrote: >>> Hi Umang, >>> >>> On Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote: >>>> Generation of thumbnail is not occuring currently because >>>> ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed >>>> to PostProcessorJpeg::process(). This is a regression introduced in >>>> 1264628d3c92("android: jpeg: Configure thumbnailer based on request >>>> metadata"). >>> I see the issue, but I'm not sure those commit was wrong. I mean, what >>> that commit does that itroduced a regression is: >>> >>> --- a/src/android/jpeg/post_processor_jpeg.cpp >>> +++ b/src/android/jpeg/post_processor_jpeg.cpp >>> + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, >>> &entry); >>> + if (ret) { >>> + const int32_t *data = entry.data.i32; >>> + Size thumbnailSize = { static_cast<uint32_t>(data[0]), >>> + static_cast<uint32_t>(data[1]) }; >>> + >>> + if (thumbnailSize != Size(0, 0)) { >>> + std::vector<unsigned char> thumbnail; >>> + generateThumbnail(source, thumbnailSize, >>> &thumbnail); >>> + if (!thumbnail.empty()) >>> + exif.setThumbnail(thumbnail, >>> Exif::Compression::JPEG); >>> + } >>> + >>> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2); >>> + >>> + /* \todo Use this quality as a parameter to the >>> encoder */ >>> + ret = >>> requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry); >>> + if (ret) >>> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, >>> + entry.data.u8, 1); >>> + } >>> >>> >>> Which I read as: "If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in >>> the request's settings, do not populate it in result metadata". >>> >>> Is this correct in your opinion, or should we populate it regardless >>> of the fact the tag was passed in ? >> >> >> This is correct, but the counterpart of the patch seems missing from >> that commit, which actually resulted in regression >> >>> >>>> The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in >>>> the request metadata template populated by >>>> CameraCapabilities::requestTemplatePreview(). The value for >>>> ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size >>>> reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES. >>> Let's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the >>> capture request template generated for the preview use case. I wonder >>> if the JPEG thunbnail size should be part of the preview template now. >> >> >> I wonder that too... >> >>> >>>> Fixes: 1264628d3c92("android: jpeg: Configure thumbnailer based on >>>> request metadata") >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> --- >>>> src/android/camera_capabilities.cpp | 14 ++++++++++++-- >>>> 1 file changed, 12 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/src/android/camera_capabilities.cpp >>>> b/src/android/camera_capabilities.cpp >>>> index e92bca42..76dddafd 100644 >>>> --- a/src/android/camera_capabilities.cpp >>>> +++ b/src/android/camera_capabilities.cpp >>>> @@ -1341,9 +1341,9 @@ std::unique_ptr<CameraMetadata> >>>> CameraCapabilities::requestTemplatePreview() con >>>> { >>>> /* >>>> * \todo Keep this in sync with the actual number of entries. >>>> - * Currently: 20 entries, 35 bytes >>>> + * Currently: 21 entries, 37 bytes >>>> */ >>>> - auto requestTemplate = std::make_unique<CameraMetadata>(21, 36); >>>> + auto requestTemplate = std::make_unique<CameraMetadata>(22, 38); >>> Comment says 21, code says 22. >>> >>> It was there already as the comment was not aligned with the code, but >>> since you're at it you could fix it >> >> >> Looking at both #entries and #bytes, that gives me an impression that >> extra entry and data buffer bytes are **intentional**, so I went >> ahead with it! Are both of them a typo? I'll need some checking >> tomorrow. > > > Indeed, my assumption was a mistake. I think the mismatch was long > sitting under the hood, and never really sync-ed with all the rework > around CameraMetadata and request templates however, > > I found this ancient mistake where it was supposed to match, but > unfortunately didn't, and the development/increment (as new entries > were added) kept happening on top of it. errr, I mean s/ancient mistake/ancient commit/ > > https://git.linuxtv.org/libcamera.git/commit/?id=637034742f2b0b7524 > > I'll mention this as a point of divergence in v3 for this particular > patch. > >> >>> >>>> if (!requestTemplate->isValid()) { >>>> return nullptr; >>>> } >>>> @@ -1364,6 +1364,16 @@ std::unique_ptr<CameraMetadata> >>>> CameraCapabilities::requestTemplatePreview() con >>>> requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, >>>> entry.data.i32, 2); >>>> >>>> + /* >>>> + * Get thumbnail sizes from static metadata and add the first >>>> non-zero >>>> + * size to the template. >>>> + */ >>>> + found = >>>> staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, >>>> + &entry); >>>> + ASSERT(found && entry.count >= 4); >>>> + requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, >>>> + entry.data.i32 + 2, 2); >>>> + >>> If I were to be extra paranoid I would make sure we actually discard >>> (0, 0). The code assumes the first two entries are (0, 0) which is >>> fine as we populate it after all. >> >> >> Not only we populate it, it's a requirement that Size (0,0) should be >> in the vector /and/ the vector needs to be sorted in ascending order >> [1] if more sizes are provided with. We do the right thing when >> populating ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES so I won't be extra >> paranoid here. >> >> >> [1]: >> https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#JPEG_AVAILABLE_THUMBNAIL_SIZES >> >>> >>> If you don't want to >>> >>> unsigned int j = 0; >>> while (j < entry.count / 2) { >>> if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] >>> == 0) >>> continue; >>> >>> requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, >>> entry.data.i32 + j, 2); >>> } >>> >>> Could you at least capture that we assume the first two entries are >>> (0,0) ? > > > I am re-thinking this. It makes sense. It's one step more defensive > than the > > ASSERT(found && entry.count >= 4); > > It's atleast for the better. > > I'll send a v3 for this patch (the 1/2 has already been merged) and > also test out the thumbnail generation with the v3 and we can put an > end to this thumbnail saga... > > Thanks for your thoughts! > > >> >> As said above, it's a requirement for >> ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, so should I make it extra >> clear again here? >> >>> >>> Thanks >>> j >>> >>> >>>> uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON; >>>> requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode); >>>> >>>> -- >>>> 2.31.1 >>>>
Hi Umang, On Tue, Sep 21, 2021 at 8:52 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > > On 9/21/21 5:20 PM, Umang Jain wrote: > > Hi Jacopo > > > > On 9/13/21 8:11 PM, Umang Jain wrote: > >> Hi Jacopo > >> > >> On 9/13/21 7:38 PM, Jacopo Mondi wrote: > >>> Hi Umang, > >>> > >>> On Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote: > >>>> Generation of thumbnail is not occuring currently because > >>>> ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed > >>>> to PostProcessorJpeg::process(). This is a regression introduced in > >>>> 1264628d3c92("android: jpeg: Configure thumbnailer based on request > >>>> metadata"). > >>> I see the issue, but I'm not sure those commit was wrong. I mean, what > >>> that commit does that itroduced a regression is: > >>> > >>> --- a/src/android/jpeg/post_processor_jpeg.cpp > >>> +++ b/src/android/jpeg/post_processor_jpeg.cpp > >>> + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, > >>> &entry); > >>> + if (ret) { > >>> + const int32_t *data = entry.data.i32; > >>> + Size thumbnailSize = { static_cast<uint32_t>(data[0]), > >>> + static_cast<uint32_t>(data[1]) }; > >>> + > >>> + if (thumbnailSize != Size(0, 0)) { > >>> + std::vector<unsigned char> thumbnail; > >>> + generateThumbnail(source, thumbnailSize, > >>> &thumbnail); > >>> + if (!thumbnail.empty()) > >>> + exif.setThumbnail(thumbnail, > >>> Exif::Compression::JPEG); > >>> + } > >>> + > >>> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2); > >>> + > >>> + /* \todo Use this quality as a parameter to the > >>> encoder */ > >>> + ret = > >>> requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry); > >>> + if (ret) > >>> + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, > >>> + entry.data.u8, 1); > >>> + } > >>> > >>> > >>> Which I read as: "If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in > >>> the request's settings, do not populate it in result metadata". > >>> > >>> Is this correct in your opinion, or should we populate it regardless > >>> of the fact the tag was passed in ? > >> > >> > >> This is correct, but the counterpart of the patch seems missing from > >> that commit, which actually resulted in regression > >> > >>> > >>>> The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in > >>>> the request metadata template populated by > >>>> CameraCapabilities::requestTemplatePreview(). The value for > >>>> ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size > >>>> reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES. > >>> Let's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the > >>> capture request template generated for the preview use case. I wonder > >>> if the JPEG thunbnail size should be part of the preview template now. > >> > >> > >> I wonder that too... > >> > >>> > >>>> Fixes: 1264628d3c92("android: jpeg: Configure thumbnailer based on > >>>> request metadata") > >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > >>>> --- > >>>> src/android/camera_capabilities.cpp | 14 ++++++++++++-- > >>>> 1 file changed, 12 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/src/android/camera_capabilities.cpp > >>>> b/src/android/camera_capabilities.cpp > >>>> index e92bca42..76dddafd 100644 > >>>> --- a/src/android/camera_capabilities.cpp > >>>> +++ b/src/android/camera_capabilities.cpp > >>>> @@ -1341,9 +1341,9 @@ std::unique_ptr<CameraMetadata> > >>>> CameraCapabilities::requestTemplatePreview() con > >>>> { > >>>> /* > >>>> * \todo Keep this in sync with the actual number of entries. > >>>> - * Currently: 20 entries, 35 bytes > >>>> + * Currently: 21 entries, 37 bytes > >>>> */ > >>>> - auto requestTemplate = std::make_unique<CameraMetadata>(21, 36); > >>>> + auto requestTemplate = std::make_unique<CameraMetadata>(22, 38); > >>> Comment says 21, code says 22. > >>> > >>> It was there already as the comment was not aligned with the code, but > >>> since you're at it you could fix it > >> > >> > >> Looking at both #entries and #bytes, that gives me an impression that > >> extra entry and data buffer bytes are **intentional**, so I went > >> ahead with it! Are both of them a typo? I'll need some checking > >> tomorrow. > > > > > > Indeed, my assumption was a mistake. I think the mismatch was long > > sitting under the hood, and never really sync-ed with all the rework > > around CameraMetadata and request templates however, > > > > I found this ancient mistake where it was supposed to match, but > > unfortunately didn't, and the development/increment (as new entries > > were added) kept happening on top of it. > > > errr, I mean s/ancient mistake/ancient commit/ > > > > > https://git.linuxtv.org/libcamera.git/commit/?id=637034742f2b0b7524 > > > > I'll mention this as a point of divergence in v3 for this particular > > patch. > > > >> > >>> > >>>> if (!requestTemplate->isValid()) { > >>>> return nullptr; > >>>> } > >>>> @@ -1364,6 +1364,16 @@ std::unique_ptr<CameraMetadata> > >>>> CameraCapabilities::requestTemplatePreview() con > >>>> requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, > >>>> entry.data.i32, 2); > >>>> > >>>> + /* > >>>> + * Get thumbnail sizes from static metadata and add the first > >>>> non-zero > >>>> + * size to the template. > >>>> + */ > >>>> + found = > >>>> staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, > >>>> + &entry); > >>>> + ASSERT(found && entry.count >= 4); > >>>> + requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, > >>>> + entry.data.i32 + 2, 2); > >>>> + > >>> If I were to be extra paranoid I would make sure we actually discard > >>> (0, 0). The code assumes the first two entries are (0, 0) which is > >>> fine as we populate it after all. > >> > >> > >> Not only we populate it, it's a requirement that Size (0,0) should be > >> in the vector /and/ the vector needs to be sorted in ascending order > >> [1] if more sizes are provided with. We do the right thing when > >> populating ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES so I won't be extra > >> paranoid here. > >> > >> > >> [1]: > >> https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#JPEG_AVAILABLE_THUMBNAIL_SIZES > >> > >>> > >>> If you don't want to > >>> > >>> unsigned int j = 0; > >>> while (j < entry.count / 2) { > >>> if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] > >>> == 0) > >>> continue; > >>> > >>> requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, > >>> entry.data.i32 + j, 2); > >>> } > >>> > >>> Could you at least capture that we assume the first two entries are > >>> (0,0) ? > > > > > > I am re-thinking this. It makes sense. It's one step more defensive > > than the > > > > ASSERT(found && entry.count >= 4); > > > > It's atleast for the better. > > > > I'll send a v3 for this patch (the 1/2 has already been merged) and > > also test out the thumbnail generation with the v3 and we can put an > > end to this thumbnail saga... > > > > Thanks for your thoughts! > > > > > >> > >> As said above, it's a requirement for > >> ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, so should I make it extra > >> clear again here? > >> > >>> > >>> Thanks > >>> j > >>> > >>> > >>>> uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON; > >>>> requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode); > >>>> > >>>> -- > >>>> 2.31.1 > >>>>
diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp index e92bca42..76dddafd 100644 --- a/src/android/camera_capabilities.cpp +++ b/src/android/camera_capabilities.cpp @@ -1341,9 +1341,9 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplatePreview() con { /* * \todo Keep this in sync with the actual number of entries. - * Currently: 20 entries, 35 bytes + * Currently: 21 entries, 37 bytes */ - auto requestTemplate = std::make_unique<CameraMetadata>(21, 36); + auto requestTemplate = std::make_unique<CameraMetadata>(22, 38); if (!requestTemplate->isValid()) { return nullptr; } @@ -1364,6 +1364,16 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplatePreview() con requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, entry.data.i32, 2); + /* + * Get thumbnail sizes from static metadata and add the first non-zero + * size to the template. + */ + found = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, + &entry); + ASSERT(found && entry.count >= 4); + requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, + entry.data.i32 + 2, 2); + uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON; requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
Generation of thumbnail is not occuring currently because ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed to PostProcessorJpeg::process(). This is a regression introduced in 1264628d3c92("android: jpeg: Configure thumbnailer based on request metadata"). The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in the request metadata template populated by CameraCapabilities::requestTemplatePreview(). The value for ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES. Fixes: 1264628d3c92("android: jpeg: Configure thumbnailer based on request metadata") Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/android/camera_capabilities.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)