[libcamera-devel] libcamera: control_ids: Keep draft controls last

10470 diff mbox series

Let's try not to mix draft controls and regular controls.

Keep draft controls at the end of the control_ids.yaml file and
add a comment to make clear where the draft controls section begins.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/control_ids.yaml | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

Hi Jacopo,

On 23/11/2020 09:03, Jacopo Mondi wrote:
> Let's try not to mix draft controls and regular controls.
> 
> Keep draft controls at the end of the control_ids.yaml file and
> add a comment to make clear where the draft controls section begins.
> 

I won't directly object to this if you are concerned about the existing
ordering - but it is not required to keep draft controls together.

Changing a control from Draft to non-draft will not change it's
underlying control id value/enum value - but moving them around the file
will.


Of course, we're likely to hit re-ordering anyway, as we find ourselves
refactoring draft controls ...


Perhaps that's a good reason to keep draft controls at the end anyway.
That way we won't affect the ID of any non-draft controls...

Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/control_ids.yaml | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index c8874fa91965..a883e27e22e9 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -273,6 +273,22 @@ controls:
>  
>        size: [3x3]
>  
> +  - ScalerCrop:
> +      type: Rectangle
> +      description: |
> +        Sets the image portion that will be scaled to form the whole of
> +        the final output image. The (x,y) location of this rectangle is
> +        relative to the PixelArrayActiveAreas that is being used. The units
> +        remain native sensor pixels, even if the sensor is being used in
> +        a binning or skipping mode.
> +
> +        This control is only present when the pipeline supports scaling. Its
> +        maximum valid value is given by the properties::ScalerCropMaximum
> +        property, and the two can be used to implement digital zoom.
> +
> +  # ----------------------------------------------------------------------------
> +  # Draft controls section
> +
>    - AePrecaptureTrigger:
>        type: int32_t
>        draft: true
> @@ -518,16 +534,4 @@ controls:
>          detection, additional format conversions etc) count as an additional
>          pipeline stage.
>  
> -  - ScalerCrop:
> -      type: Rectangle
> -      description: |
> -        Sets the image portion that will be scaled to form the whole of
> -        the final output image. The (x,y) location of this rectangle is
> -        relative to the PixelArrayActiveAreas that is being used. The units
> -        remain native sensor pixels, even if the sensor is being used in
> -        a binning or skipping mode.
> -
> -        This control is only present when the pipeline supports scaling. Its
> -        maximum valid value is given by the properties::ScalerCropMaximum
> -        property, and the two can be used to implement digital zoom.
>  ...
>

Hi Kieran,

On Mon, Nov 23, 2020 at 09:21:23AM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 23/11/2020 09:03, Jacopo Mondi wrote:
> > Let's try not to mix draft controls and regular controls.
> >
> > Keep draft controls at the end of the control_ids.yaml file and
> > add a comment to make clear where the draft controls section begins.
> >
>
> I won't directly object to this if you are concerned about the existing
> ordering - but it is not required to keep draft controls together.
>
> Changing a control from Draft to non-draft will not change it's
> underlying control id value/enum value - but moving them around the file
> will.

Well, that's not entirely true. Changing a control from draft to
non-draft might imply changing its values, we might even decide we
want to express with two or more controls what is now expressed with a
single one, or maybe merge two draft controls in a single one. Who
knows :)

>
>
> Of course, we're likely to hit re-ordering anyway, as we find ourselves
> refactoring draft controls ...
>
>
> Perhaps that's a good reason to keep draft controls at the end anyway.
> That way we won't affect the ID of any non-draft controls...

You know, I sent this out mostly for frivolous reasons ("all draft
controls shall go at the end of the file!"), but your point of having
'stable' controls first, with a non-changing ID has much more value,
even if we're not looking for ABI stability at this point.

>
> Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>

Thanks
  j

>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/control_ids.yaml | 28 ++++++++++++++++------------
> >  1 file changed, 16 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index c8874fa91965..a883e27e22e9 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -273,6 +273,22 @@ controls:
> >
> >        size: [3x3]
> >
> > +  - ScalerCrop:
> > +      type: Rectangle
> > +      description: |
> > +        Sets the image portion that will be scaled to form the whole of
> > +        the final output image. The (x,y) location of this rectangle is
> > +        relative to the PixelArrayActiveAreas that is being used. The units
> > +        remain native sensor pixels, even if the sensor is being used in
> > +        a binning or skipping mode.
> > +
> > +        This control is only present when the pipeline supports scaling. Its
> > +        maximum valid value is given by the properties::ScalerCropMaximum
> > +        property, and the two can be used to implement digital zoom.
> > +
> > +  # ----------------------------------------------------------------------------
> > +  # Draft controls section
> > +
> >    - AePrecaptureTrigger:
> >        type: int32_t
> >        draft: true
> > @@ -518,16 +534,4 @@ controls:
> >          detection, additional format conversions etc) count as an additional
> >          pipeline stage.
> >
> > -  - ScalerCrop:
> > -      type: Rectangle
> > -      description: |
> > -        Sets the image portion that will be scaled to form the whole of
> > -        the final output image. The (x,y) location of this rectangle is
> > -        relative to the PixelArrayActiveAreas that is being used. The units
> > -        remain native sensor pixels, even if the sensor is being used in
> > -        a binning or skipping mode.
> > -
> > -        This control is only present when the pipeline supports scaling. Its
> > -        maximum valid value is given by the properties::ScalerCropMaximum
> > -        property, and the two can be used to implement digital zoom.
> >  ...
> >
>
> --
> Regards
> --
> Kieran

On 23/11/2020 09:29, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Mon, Nov 23, 2020 at 09:21:23AM +0000, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 23/11/2020 09:03, Jacopo Mondi wrote:
>>> Let's try not to mix draft controls and regular controls.
>>>
>>> Keep draft controls at the end of the control_ids.yaml file and
>>> add a comment to make clear where the draft controls section begins.
>>>
>>
>> I won't directly object to this if you are concerned about the existing
>> ordering - but it is not required to keep draft controls together.
>>
>> Changing a control from Draft to non-draft will not change it's
>> underlying control id value/enum value - but moving them around the file
>> will.
> 
> Well, that's not entirely true. Changing a control from draft to
> non-draft might imply changing its values, we might even decide we
> want to express with two or more controls what is now expressed with a
> single one, or maybe merge two draft controls in a single one. Who
> knows :)


I was referring only to the underlying ID values. ;-)



>> Of course, we're likely to hit re-ordering anyway, as we find ourselves
>> refactoring draft controls ...
>>
>>
>> Perhaps that's a good reason to keep draft controls at the end anyway.
>> That way we won't affect the ID of any non-draft controls...
> 
> You know, I sent this out mostly for frivolous reasons ("all draft
> controls shall go at the end of the file!"), but your point of having
> 'stable' controls first, with a non-changing ID has much more value,
> even if we're not looking for ABI stability at this point.

I agree here.

Mixing unstable controls with stable ones, will cause instability to
'stable' ids.

No need for that ;-) And also - no need for any actual sort order in
this file.

We can sort the documentation as a post-processing step in gen-controls
if we want the doxygen to be alphabetical, and keep the ID's as a
continuous - consecutive enum.


So yes, lets keep drafts at the bottom.

--
Kieran


>> Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
> 
> Thanks
>   j
> 
>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  src/libcamera/control_ids.yaml | 28 ++++++++++++++++------------
>>>  1 file changed, 16 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
>>> index c8874fa91965..a883e27e22e9 100644
>>> --- a/src/libcamera/control_ids.yaml
>>> +++ b/src/libcamera/control_ids.yaml
>>> @@ -273,6 +273,22 @@ controls:
>>>
>>>        size: [3x3]
>>>
>>> +  - ScalerCrop:
>>> +      type: Rectangle
>>> +      description: |
>>> +        Sets the image portion that will be scaled to form the whole of
>>> +        the final output image. The (x,y) location of this rectangle is
>>> +        relative to the PixelArrayActiveAreas that is being used. The units
>>> +        remain native sensor pixels, even if the sensor is being used in
>>> +        a binning or skipping mode.
>>> +
>>> +        This control is only present when the pipeline supports scaling. Its
>>> +        maximum valid value is given by the properties::ScalerCropMaximum
>>> +        property, and the two can be used to implement digital zoom.
>>> +
>>> +  # ----------------------------------------------------------------------------
>>> +  # Draft controls section
>>> +
>>>    - AePrecaptureTrigger:
>>>        type: int32_t
>>>        draft: true
>>> @@ -518,16 +534,4 @@ controls:
>>>          detection, additional format conversions etc) count as an additional
>>>          pipeline stage.
>>>
>>> -  - ScalerCrop:
>>> -      type: Rectangle
>>> -      description: |
>>> -        Sets the image portion that will be scaled to form the whole of
>>> -        the final output image. The (x,y) location of this rectangle is
>>> -        relative to the PixelArrayActiveAreas that is being used. The units
>>> -        remain native sensor pixels, even if the sensor is being used in
>>> -        a binning or skipping mode.
>>> -
>>> -        This control is only present when the pipeline supports scaling. Its
>>> -        maximum valid value is given by the properties::ScalerCropMaximum
>>> -        property, and the two can be used to implement digital zoom.
>>>  ...
>>>
>>
>> --
>> Regards
>> --
>> Kieran

Hi Jacopo,

Thanks for your work.

On 2020-11-23 10:03:29 +0100, Jacopo Mondi wrote:
> Let's try not to mix draft controls and regular controls.
> 
> Keep draft controls at the end of the control_ids.yaml file and
> add a comment to make clear where the draft controls section begins.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/libcamera/control_ids.yaml | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index c8874fa91965..a883e27e22e9 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -273,6 +273,22 @@ controls:
>  
>        size: [3x3]
>  
> +  - ScalerCrop:
> +      type: Rectangle
> +      description: |
> +        Sets the image portion that will be scaled to form the whole of
> +        the final output image. The (x,y) location of this rectangle is
> +        relative to the PixelArrayActiveAreas that is being used. The units
> +        remain native sensor pixels, even if the sensor is being used in
> +        a binning or skipping mode.
> +
> +        This control is only present when the pipeline supports scaling. Its
> +        maximum valid value is given by the properties::ScalerCropMaximum
> +        property, and the two can be used to implement digital zoom.
> +
> +  # ----------------------------------------------------------------------------
> +  # Draft controls section
> +
>    - AePrecaptureTrigger:
>        type: int32_t
>        draft: true
> @@ -518,16 +534,4 @@ controls:
>          detection, additional format conversions etc) count as an additional
>          pipeline stage.
>  
> -  - ScalerCrop:
> -      type: Rectangle
> -      description: |
> -        Sets the image portion that will be scaled to form the whole of
> -        the final output image. The (x,y) location of this rectangle is
> -        relative to the PixelArrayActiveAreas that is being used. The units
> -        remain native sensor pixels, even if the sensor is being used in
> -        a binning or skipping mode.
> -
> -        This control is only present when the pipeline supports scaling. Its
> -        maximum valid value is given by the properties::ScalerCropMaximum
> -        property, and the two can be used to implement digital zoom.
>  ...
> -- 
> 2.29.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thank you for the patch.

On Mon, Nov 23, 2020 at 10:03:29AM +0100, Jacopo Mondi wrote:
> Let's try not to mix draft controls and regular controls.
> 
> Keep draft controls at the end of the control_ids.yaml file and
> add a comment to make clear where the draft controls section begins.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/libcamera/control_ids.yaml | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index c8874fa91965..a883e27e22e9 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -273,6 +273,22 @@ controls:
>  
>        size: [3x3]
>  
> +  - ScalerCrop:
> +      type: Rectangle
> +      description: |
> +        Sets the image portion that will be scaled to form the whole of
> +        the final output image. The (x,y) location of this rectangle is
> +        relative to the PixelArrayActiveAreas that is being used. The units
> +        remain native sensor pixels, even if the sensor is being used in
> +        a binning or skipping mode.
> +
> +        This control is only present when the pipeline supports scaling. Its
> +        maximum valid value is given by the properties::ScalerCropMaximum
> +        property, and the two can be used to implement digital zoom.
> +
> +  # ----------------------------------------------------------------------------
> +  # Draft controls section
> +
>    - AePrecaptureTrigger:
>        type: int32_t
>        draft: true
> @@ -518,16 +534,4 @@ controls:
>          detection, additional format conversions etc) count as an additional
>          pipeline stage.
>  
> -  - ScalerCrop:
> -      type: Rectangle
> -      description: |
> -        Sets the image portion that will be scaled to form the whole of
> -        the final output image. The (x,y) location of this rectangle is
> -        relative to the PixelArrayActiveAreas that is being used. The units
> -        remain native sensor pixels, even if the sensor is being used in
> -        a binning or skipping mode.
> -
> -        This control is only present when the pipeline supports scaling. Its
> -        maximum valid value is given by the properties::ScalerCropMaximum
> -        property, and the two can be used to implement digital zoom.
>  ...