[libcamera-devel] android: camera_capabilities: Ensure PixelArrayActiveAreas exists before accessing
diff mbox series

Message ID 20210715140011.122598-1-kieran.bingham@ideasonboard.com
State Rejected
Delegated to: Kieran Bingham
Headers show
Series
  • [libcamera-devel] android: camera_capabilities: Ensure PixelArrayActiveAreas exists before accessing
Related show

Commit Message

Kieran Bingham July 15, 2021, 2 p.m. UTC 
The VIVID pipline handler does not set this. It's likely that the
UVC pipeline will not either.

If not present, we must not access it.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/android/camera_capabilities.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Laurent Pinchart July 15, 2021, 2:06 p.m. UTC | #1 
Hi Kieran,

Thank you for the patch.

On Thu, Jul 15, 2021 at 03:00:11PM +0100, Kieran Bingham wrote:
> The VIVID pipline handler does not set this. It's likely that the
> UVC pipeline will not either.
> 
> If not present, we must not access it.

Wasn't this considered as a mandatory property ?

> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/android/camera_capabilities.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index 6b5edb66fad2..dfc961affe46 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -589,7 +589,7 @@ int CameraCapabilities::initializeStaticMetadata()
>  					  physicalSize);
>  	}
>  
> -	{
> +	if (properties.contains(properties::PixelArrayActiveAreas)) {
>  		const Span<const Rectangle> &rects =
>  			properties.get(properties::PixelArrayActiveAreas);
>  		std::vector<int32_t> data{
Kieran Bingham July 15, 2021, 2:29 p.m. UTC | #2 
Hi Laurent,

On 15/07/2021 15:06, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Jul 15, 2021 at 03:00:11PM +0100, Kieran Bingham wrote:
>> The VIVID pipline handler does not set this. It's likely that the
>> UVC pipeline will not either.
>>
>> If not present, we must not access it.
> 
> Wasn't this considered as a mandatory property ?

By android - or by us?

Can this property be identified on a UVC camera?

Or should we just report the biggest frame size in the pipeline handler
if it's not something we can otherwise determine?

I had thought mandatory properties were going to have a flag in the yaml
- but I don't see it here (or anywhere though).

Do we already define mandatory properties in another location?
--
Kieran


> 
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/android/camera_capabilities.cpp | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
>> index 6b5edb66fad2..dfc961affe46 100644
>> --- a/src/android/camera_capabilities.cpp
>> +++ b/src/android/camera_capabilities.cpp
>> @@ -589,7 +589,7 @@ int CameraCapabilities::initializeStaticMetadata()
>>  					  physicalSize);
>>  	}
>>  
>> -	{
>> +	if (properties.contains(properties::PixelArrayActiveAreas)) {
>>  		const Span<const Rectangle> &rects =
>>  			properties.get(properties::PixelArrayActiveAreas);
>>  		std::vector<int32_t> data{
>
Laurent Pinchart July 15, 2021, 2:35 p.m. UTC | #3 
Hi Kieran,

On Thu, Jul 15, 2021 at 03:29:37PM +0100, Kieran Bingham wrote:
> On 15/07/2021 15:06, Laurent Pinchart wrote:
> > On Thu, Jul 15, 2021 at 03:00:11PM +0100, Kieran Bingham wrote:
> >> The VIVID pipline handler does not set this. It's likely that the
> >> UVC pipeline will not either.
> >>
> >> If not present, we must not access it.
> > 
> > Wasn't this considered as a mandatory property ?
> 
> By android - or by us?

By us.

> Can this property be identified on a UVC camera?

We can set it to the maximum resolution.

> Or should we just report the biggest frame size in the pipeline handler
> if it's not something we can otherwise determine?

There we go :-)

> I had thought mandatory properties were going to have a flag in the yaml
> - but I don't see it here (or anywhere though).

Correct, there should be a flag, but we don't have that yet.

> Do we already define mandatory properties in another location?

No. Should we start adding the flag, even if we don't do anything with
it ?

> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  src/android/camera_capabilities.cpp | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> >> index 6b5edb66fad2..dfc961affe46 100644
> >> --- a/src/android/camera_capabilities.cpp
> >> +++ b/src/android/camera_capabilities.cpp
> >> @@ -589,7 +589,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >>  					  physicalSize);
> >>  	}
> >>  
> >> -	{
> >> +	if (properties.contains(properties::PixelArrayActiveAreas)) {
> >>  		const Span<const Rectangle> &rects =
> >>  			properties.get(properties::PixelArrayActiveAreas);
> >>  		std::vector<int32_t> data{
Kieran Bingham July 15, 2021, 2:37 p.m. UTC | #4 
On 15/07/2021 15:35, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Thu, Jul 15, 2021 at 03:29:37PM +0100, Kieran Bingham wrote:
>> On 15/07/2021 15:06, Laurent Pinchart wrote:
>>> On Thu, Jul 15, 2021 at 03:00:11PM +0100, Kieran Bingham wrote:
>>>> The VIVID pipline handler does not set this. It's likely that the
>>>> UVC pipeline will not either.
>>>>
>>>> If not present, we must not access it.
>>>
>>> Wasn't this considered as a mandatory property ?
>>
>> By android - or by us?
> 
> By us.
> 
>> Can this property be identified on a UVC camera?
> 
> We can set it to the maximum resolution.
> 
>> Or should we just report the biggest frame size in the pipeline handler
>> if it's not something we can otherwise determine?
> 
> There we go :-)

Ok - so we can do that then. If it's mandatory - then the Vivid pipeline
should be updated and document what a reader should implement (while the
Vivid simply sets the biggest supported size)


>> I had thought mandatory properties were going to have a flag in the yaml
>> - but I don't see it here (or anywhere though).
> 
> Correct, there should be a flag, but we don't have that yet.
> 
>> Do we already define mandatory properties in another location?
> 
> No. Should we start adding the flag, even if we don't do anything with
> it ?

I would have thought that's at least better to act as documentation of
the intentions?



>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>> ---
>>>>  src/android/camera_capabilities.cpp | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
>>>> index 6b5edb66fad2..dfc961affe46 100644
>>>> --- a/src/android/camera_capabilities.cpp
>>>> +++ b/src/android/camera_capabilities.cpp
>>>> @@ -589,7 +589,7 @@ int CameraCapabilities::initializeStaticMetadata()
>>>>  					  physicalSize);
>>>>  	}
>>>>  
>>>> -	{
>>>> +	if (properties.contains(properties::PixelArrayActiveAreas)) {
>>>>  		const Span<const Rectangle> &rects =
>>>>  			properties.get(properties::PixelArrayActiveAreas);
>>>>  		std::vector<int32_t> data{
>
Laurent Pinchart July 15, 2021, 2:46 p.m. UTC | #5 
On Thu, Jul 15, 2021 at 03:37:21PM +0100, Kieran Bingham wrote:
> On 15/07/2021 15:35, Laurent Pinchart wrote:
> > Hi Kieran,
> > 
> > On Thu, Jul 15, 2021 at 03:29:37PM +0100, Kieran Bingham wrote:
> >> On 15/07/2021 15:06, Laurent Pinchart wrote:
> >>> On Thu, Jul 15, 2021 at 03:00:11PM +0100, Kieran Bingham wrote:
> >>>> The VIVID pipline handler does not set this. It's likely that the
> >>>> UVC pipeline will not either.
> >>>>
> >>>> If not present, we must not access it.
> >>>
> >>> Wasn't this considered as a mandatory property ?
> >>
> >> By android - or by us?
> > 
> > By us.
> > 
> >> Can this property be identified on a UVC camera?
> > 
> > We can set it to the maximum resolution.
> > 
> >> Or should we just report the biggest frame size in the pipeline handler
> >> if it's not something we can otherwise determine?
> > 
> > There we go :-)
> 
> Ok - so we can do that then. If it's mandatory - then the Vivid pipeline
> should be updated and document what a reader should implement (while the
> Vivid simply sets the biggest supported size)
> 
> >> I had thought mandatory properties were going to have a flag in the yaml
> >> - but I don't see it here (or anywhere though).
> > 
> > Correct, there should be a flag, but we don't have that yet.
> > 
> >> Do we already define mandatory properties in another location?
> > 
> > No. Should we start adding the flag, even if we don't do anything with
> > it ?
> 
> I would have thought that's at least better to act as documentation of
> the intentions?

Agreed.

> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>>> ---
> >>>>  src/android/camera_capabilities.cpp | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> >>>> index 6b5edb66fad2..dfc961affe46 100644
> >>>> --- a/src/android/camera_capabilities.cpp
> >>>> +++ b/src/android/camera_capabilities.cpp
> >>>> @@ -589,7 +589,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >>>>  					  physicalSize);
> >>>>  	}
> >>>>  
> >>>> -	{
> >>>> +	if (properties.contains(properties::PixelArrayActiveAreas)) {
> >>>>  		const Span<const Rectangle> &rects =
> >>>>  			properties.get(properties::PixelArrayActiveAreas);
> >>>>  		std::vector<int32_t> data{

Patch
diff mbox series 

diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index 6b5edb66fad2..dfc961affe46 100644
--- a/src/android/camera_capabilities.cpp
+++ b/src/android/camera_capabilities.cpp
@@ -589,7 +589,7 @@  int CameraCapabilities::initializeStaticMetadata()
 					  physicalSize);
 	}
 
-	{
+	if (properties.contains(properties::PixelArrayActiveAreas)) {
 		const Span<const Rectangle> &rects =
 			properties.get(properties::PixelArrayActiveAreas);
 		std::vector<int32_t> data{