[libcamera-devel] [PATCH] android: Rework request completion notification

[Jacopo Mondi]

Hi Niklas,

On Thu, May 06, 2021 at 11:03:09AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2021-05-06 10:53:02 +0200, Jacopo Mondi wrote:
>
> <snip>
>
> > > >
> > > > -	resultMetadata = getResultMetadata(descriptor);
> > > > +	/*
> > > > +	 * Generate the metadata associated with the captured buffers.
> > > > +	 *
> > > > +	 * Notify if the metadata generation has failed, but continue processing
> > > > +	 * buffers and return an empty metadata pack.
> > > > +	 */
> > > > +	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(descriptor);
> > > > +	if (!resultMetadata)
> > > > +		notifyError(descriptor.frameNumber_, descriptor.buffers_[0].stream,
> > > > +			    CAMERA3_MSG_ERROR_RESULT);
> > > > +	captureResult.result = resultMetadata->get();
> > >
> > > Hum if !resultMetadata will not resultMetadata->get() give us a nullptr
> > > exception here?
> > >
> >
> > According to the std::unique_ptr<>::get() documentation:
> >
> > "Returns a pointer to the managed object or nullptr if no object is owned."
> >
> > as getResultMetadata() returns nullptr on error, and the function
> > signature is
> >         std::unique_ptr<CameraMetadata> CameraDevice::getResultMetadata()
> >
> > my understanding is that the returned unique_ptr<> will own no object,
> > hence get() should return nullptr. Or have I missed something about
> > the "no object is owned" vs "a unique_ptr<> that owns nullptr" ?
>
> I share your understanding of std::unique_ptr<>::get() but that would be
>
>     resultMetadata.get()
>
> and not
>
>     resultMetadata->get()
>
> The later calls CameraMetadata::get() which returns camera_metadata_t *
> which is the correct datatype for the captureResult.result right? So I
> think the code calls the correct get(), only the manual protection for
> the nullptr is missing.

AAAA! very good catch! thanks, I'm a bit slow

Yes, unique_ptr<>::operator-> of course says
        The behavior is undefined if get() == nullptr.

so yes, I should put a check there. Sorry, I overlooked this! thanks
for spotting, I'll fix

Thanks
   j

>
> --
> Regards,
> Niklas Söderlund