[libcamera-devel] [PATCH 5/6] android: camera_device: Create request templates per use-case
Jacopo Mondi
jacopo at jmondi.org
Sat Jul 25 14:34:31 CEST 2020
Hi Kieran, Laurent,
On Fri, Jul 24, 2020 at 07:00:45PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Jul 24, 2020 at 04:21:19PM +0200, Jacopo Mondi wrote:
> > Currently the request template returned from
> > CameraDevice::constructDefaultRequestSettings() is the same for all
> > the supported template types.
> >
> > To prepare to adjust the template depending on the use case, break out
> > the template generation to dedicated function.
>
> s/dedicated/a dedicated/
>
> >
> > As we currently support the PREVIEW use case only, make all the other
> > template types use the captureTemplatePreview() function and just update
> > the capture intent property. This requires a nasty const-cast on the
> > generated template pack which will be removed once the functions will be
> > properly populated.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/android/camera_device.cpp | 146 +++++++++++++++++++++++++---------
> > src/android/camera_device.h | 7 ++
> > 2 files changed, 114 insertions(+), 39 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 48f8090a93db..b8cb118a960e 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -821,51 +821,15 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > return staticMetadata_->get();
> > }
> >
> > -/*
> > - * Produce a metadata pack to be used as template for a capture request.
> > - */
> > -const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> > +const CameraMetadata *CameraDevice::captureTemplatePreview()
> > {
> > - auto it = requestTemplates_.find(type);
> > - if (it != requestTemplates_.end())
> > - return it->second->get();
> > -
> > - /* Use the capture intent matching the requested template type. */
> > - uint8_t captureIntent;
> > - switch (type) {
> > - case CAMERA3_TEMPLATE_PREVIEW:
> > - captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_PREVIEW;
> > - break;
> > - case CAMERA3_TEMPLATE_STILL_CAPTURE:
> > - captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_STILL_CAPTURE;
> > - break;
> > - case CAMERA3_TEMPLATE_VIDEO_RECORD:
> > - captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_RECORD;
> > - break;
> > - case CAMERA3_TEMPLATE_VIDEO_SNAPSHOT:
> > - captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_SNAPSHOT;
> > - break;
> > - case CAMERA3_TEMPLATE_ZERO_SHUTTER_LAG:
> > - captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_ZERO_SHUTTER_LAG;
> > - break;
> > - case CAMERA3_TEMPLATE_MANUAL:
> > - captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_MANUAL;
> > - break;
> > - default:
> > - LOG(HAL, Error) << "Invalid template request type: " << type;
> > - return nullptr;
> > - }
> > -
> > /*
> > * \todo Keep this in sync with the actual number of entries.
> > * Currently: 12 entries, 15 bytes
> > */
> > CameraMetadata *requestTemplate = new CameraMetadata(15, 20);
> > - if (!requestTemplate->isValid()) {
> > - LOG(HAL, Error) << "Failed to allocate template metadata";
> > - delete requestTemplate;
> > + if (!requestTemplate->isValid())
I should delete here, as I wrongly assumed if !isValid() the metadata
allocation failed hence there was no need to delete it. Although, the
CameraMetadata instance should be deleted anyway
> > return nullptr;
> > - }
> >
> > uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
> > requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE,
> > @@ -911,10 +875,114 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> > requestTemplate->addEntry(ANDROID_COLOR_CORRECTION_ABERRATION_MODE,
> > &aberrationMode, 1);
> >
> > + uint8_t captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_PREVIEW;
> > requestTemplate->addEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> > &captureIntent, 1);
> >
> > - if (!requestTemplate->isValid()) {
> > + return requestTemplate;
> > +}
> > +
> > +const CameraMetadata *CameraDevice::captureTemplateStillCapture()
> > +{
> > + const CameraMetadata *requestTemplate = captureTemplatePreview();
> > + if (!requestTemplate || !requestTemplate->isValid())
> > + return nullptr;
>
> You'll leak requestTemplate here if it's not valid. You could just
> return requestTemplate instead.
>
Right, I was betting on the constructDefaultRequestSettings() to do
so, bu then I made all other functions call this one, and failed to
update the clean up paths
> > +
> > + uint8_t captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_STILL_CAPTURE;
> > + const_cast<CameraMetadata *>(requestTemplate)
> > + ->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT, &captureIntent, 1);
>
> You could avoid the const_cast by making these functions return a
> non-const pointer, and setting ANDROID_CONTROL_CAPTURE_INTENT in
> CameraDevice::constructDefaultRequestSettings(). This function would
> then just become
>
> return captureTemplatePreview();
>
> and will be filled later.
Not sure. I started with the intention of filling in all functions,
then I addressed Preview only, and made the others use it. I should
probably just call captureTemplatePreview() from all the switch cases
in constructDefaultRequestSettings() for the time being, update
CAPTURE_INTENT there, and replace each function with a new one as we
support more template types. I think that's what v2 will look like.
>
> > +
> > + return requestTemplate;
> > +}
> > +
> > +const CameraMetadata *CameraDevice::captureTemplateVideoRecord()
> > +{
> > + const CameraMetadata *requestTemplate = captureTemplatePreview();
> > + if (!requestTemplate || !requestTemplate->isValid())
> > + return nullptr;
> > +
> > + uint8_t captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_RECORD;
> > + const_cast<CameraMetadata *>(requestTemplate)
> > + ->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT, &captureIntent, 1);
> > +
> > + return requestTemplate;
> > +}
> > +
> > +const CameraMetadata *CameraDevice::captureTemplateVideoSnapshot()
> > +{
> > + const CameraMetadata *requestTemplate = captureTemplatePreview();
> > + if (!requestTemplate || !requestTemplate->isValid())
> > + return nullptr;
> > +
> > + uint8_t captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_VIDEO_SNAPSHOT;
> > + const_cast<CameraMetadata *>(requestTemplate)
> > + ->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT, &captureIntent, 1);
> > +
> > + return requestTemplate;
> > +}
> > +
> > +const CameraMetadata *CameraDevice::captureTemplateZSL()
> > +{
> > + const CameraMetadata *requestTemplate = captureTemplatePreview();
> > + if (!requestTemplate || !requestTemplate->isValid())
> > + return nullptr;
> > +
> > + uint8_t captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_ZERO_SHUTTER_LAG;
> > + const_cast<CameraMetadata *>(requestTemplate)
> > + ->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT, &captureIntent, 1);
> > +
> > + return requestTemplate;
> > +}
> > +
> > +const CameraMetadata *CameraDevice::captureTemplateManual()
> > +{
> > + const CameraMetadata *requestTemplate = captureTemplatePreview();
> > + if (!requestTemplate || !requestTemplate->isValid())
> > + return nullptr;
> > +
> > + uint8_t captureIntent = ANDROID_CONTROL_CAPTURE_INTENT_MANUAL;
> > + const_cast<CameraMetadata *>(requestTemplate)
> > + ->updateEntry(ANDROID_CONTROL_CAPTURE_INTENT, &captureIntent, 1);
> > +
> > + return requestTemplate;
> > +}
> > +
> > +/*
> > + * Produce a metadata pack to be used as template for a capture request.
> > + */
> > +const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> > +{
> > + auto it = requestTemplates_.find(type);
> > + if (it != requestTemplates_.end())
> > + return it->second->get();
> > +
> > + /* Use the capture intent matching the requested template type. */
> > + const CameraMetadata *requestTemplate;
> > + switch (type) {
> > + case CAMERA3_TEMPLATE_PREVIEW:
> > + requestTemplate = captureTemplatePreview();
> > + break;
> > + case CAMERA3_TEMPLATE_STILL_CAPTURE:
> > + requestTemplate = captureTemplateStillCapture();
> > + break;
> > + case CAMERA3_TEMPLATE_VIDEO_RECORD:
> > + requestTemplate = captureTemplateVideoRecord();
> > + break;
> > + case CAMERA3_TEMPLATE_VIDEO_SNAPSHOT:
> > + requestTemplate = captureTemplateVideoSnapshot();
> > + break;
> > + case CAMERA3_TEMPLATE_ZERO_SHUTTER_LAG:
> > + requestTemplate = captureTemplateZSL();
> > + break;
> > + case CAMERA3_TEMPLATE_MANUAL:
> > + requestTemplate = captureTemplateManual();
> > + break;
> > + default:
> > + LOG(HAL, Error) << "Invalid template request type: " << type;
> > + return nullptr;
> > + }
> > +
> > + if (!requestTemplate || !requestTemplate->isValid()) {
> > LOG(HAL, Error) << "Failed to construct request template";
> > delete requestTemplate;
> > return nullptr;
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index af1b58ab6b4e..7bf68e23f7fe 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -87,6 +87,13 @@ private:
> > std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number,
> > int64_t timestamp);
> >
> > + const CameraMetadata *captureTemplatePreview();
> > + const CameraMetadata *captureTemplateStillCapture();
> > + const CameraMetadata *captureTemplateVideoRecord();
> > + const CameraMetadata *captureTemplateVideoSnapshot();
> > + const CameraMetadata *captureTemplateZSL();
> > + const CameraMetadata *captureTemplateManual();
> > +
> > unsigned int id_;
> > camera3_device_t camera3Device_;
> >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list