[PATCH] [media] docs-rst: clarify field vs frame height in the subdev API

[PATCH] [media] docs-rst: clarify field vs frame height in the subdev API

[Philipp Zabel]

VIDIOC_SUBDEV_G/S_FMT take the field size if V4L2_FIELD_ALTERNATE field
order is set, but the VIDIOC_SUBDEV_G/S_SELECTION rectangles still refer
to frame size, regardless of the field order setting.
VIDIOC_SUBDEV_ENUM_FRAME_SIZES always returns frame sizes as opposed to
field sizes.

This was not immediately clear to me when reading the documentation, so
this patch adds some clarifications in the relevant places.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 Documentation/media/uapi/v4l/dev-subdev.rst              | 16 ++++++++++++----
 Documentation/media/uapi/v4l/subdev-formats.rst          |  3 ++-
 .../media/uapi/v4l/vidioc-subdev-enum-frame-size.rst     |  4 ++++
 .../media/uapi/v4l/vidioc-subdev-g-selection.rst         |  2 ++
 4 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst
index cd28701802086..2f0a41f3796f0 100644
--- a/Documentation/media/uapi/v4l/dev-subdev.rst
+++ b/Documentation/media/uapi/v4l/dev-subdev.rst
@@ -82,7 +82,8 @@ Pad-level Formats
 .. note::
 
     For the purpose of this section, the term *format* means the
-    combination of media bus data format, frame width and frame height.
+    combination of media bus data format, frame width and frame height,
+    unless otherwise noted.
 
 Image formats are typically negotiated on video capture and output
 devices using the format and
@@ -120,7 +121,9 @@ can expose pad-level image format configuration to applications. When
 they do, applications can use the
 :ref:`VIDIOC_SUBDEV_G_FMT <VIDIOC_SUBDEV_G_FMT>` and
 :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctls. to
-negotiate formats on a per-pad basis.
+negotiate formats on a per-pad basis. Note that when those ioctls are
+called with or return the field order set to ``V4L2_FIELD_ALTERNATE``,
+the format contains the field height, which is half the frame height.
 
 Applications are responsible for configuring coherent parameters on the
 whole pipeline and making sure that connected pads have compatible
@@ -379,7 +382,10 @@ is supported by the hardware.
    pad for further processing.
 
 2. Sink pad actual crop selection. The sink pad crop defines the crop
-   performed to the sink pad format.
+   performed to the sink pad format. The crop rectangle always refers to
+   the frame size, even if the sink pad format has field order set to
+   ``V4L2_FIELD_ALTERNATE`` and the actual processed images are only
+   field sized.
 
 3. Sink pad actual compose selection. The size of the sink pad compose
    rectangle defines the scaling ratio compared to the size of the sink
@@ -393,7 +399,9 @@ is supported by the hardware.
 5. Source pad format. The source pad format defines the output pixel
    format of the subdev, as well as the other parameters with the
    exception of the image width and height. Width and height are defined
-   by the size of the source pad actual crop selection.
+   by the size of the source pad actual crop selection. If the source pad
+   format has field order set to ``V4L2_FIELD_ALTERNATE``, the source pad
+   field height is half the source pad crop selection height.
 
 Accessing any of the above rectangles not supported by the subdev will
 return ``EINVAL``. Any rectangle referring to a previous unsupported
diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst b/Documentation/media/uapi/v4l/subdev-formats.rst
index d6152c907b8ba..f7195e5ee6e78 100644
--- a/Documentation/media/uapi/v4l/subdev-formats.rst
+++ b/Documentation/media/uapi/v4l/subdev-formats.rst
@@ -19,7 +19,8 @@ Media Bus Formats
       - Image width, in pixels.
     * - __u32
       - ``height``
-      - Image height, in pixels.
+      - Image height, in pixels. This is the field height for
+        ``V4L2_FIELD_ALTERNATE`` field order, or the frame height otherwise.
     * - __u32
       - ``code``
       - Format code, from enum
diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-size.rst b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-size.rst
index 746c24ed97a05..a78ae138f8a87 100644
--- a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-size.rst
+++ b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-size.rst
@@ -55,6 +55,10 @@ maximum values. Applications must use the
 :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctl to try the
 sub-device for an exact supported frame size.
 
+Note that if ``V4L2_FIELD_ALTERNATE`` field order is chosen in the
+:ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctls, those take
+the field size, which is only half the height of the frame size.
+
 Available frame sizes may depend on the current 'try' formats at other
 pads of the sub-device, as well as on the current active links and the
 current values of V4L2 controls. See
diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst
index 071d9c033db6b..253e0ccb78224 100644
--- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst
+++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst
@@ -45,6 +45,8 @@ function of the crop API, and more, are supported by the selections API.
 See :ref:`subdev` for more information on how each selection target
 affects the image processing pipeline inside the subdevice.
 
+Note that selection rectangles always refer to frame sizes, not field sizes.
+
 
 Types of selection targets
 --------------------------
-- 
2.11.0

Re: [PATCH] [media] docs-rst: clarify field vs frame height in the subdev API

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

On Thursday 30 Mar 2017 17:38:20 Philipp Zabel wrote:
> VIDIOC_SUBDEV_G/S_FMT take the field size if V4L2_FIELD_ALTERNATE field
> order is set, but the VIDIOC_SUBDEV_G/S_SELECTION rectangles still refer
> to frame size, regardless of the field order setting.
> VIDIOC_SUBDEV_ENUM_FRAME_SIZES always returns frame sizes as opposed to
> field sizes.
> 
> This was not immediately clear to me when reading the documentation, so
> this patch adds some clarifications in the relevant places.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  Documentation/media/uapi/v4l/dev-subdev.rst              | 16 +++++++++----
>  Documentation/media/uapi/v4l/subdev-formats.rst          |  3 ++-
>  .../media/uapi/v4l/vidioc-subdev-enum-frame-size.rst     |  4 ++++
>  .../media/uapi/v4l/vidioc-subdev-g-selection.rst         |  2 ++
>  4 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst
> b/Documentation/media/uapi/v4l/dev-subdev.rst index
> cd28701802086..2f0a41f3796f0 100644
> --- a/Documentation/media/uapi/v4l/dev-subdev.rst
> +++ b/Documentation/media/uapi/v4l/dev-subdev.rst
> @@ -82,7 +82,8 @@ Pad-level Formats
>  .. note::
> 
>      For the purpose of this section, the term *format* means the
> -    combination of media bus data format, frame width and frame height.
> +    combination of media bus data format, frame width and frame height,
> +    unless otherwise noted.
> 
>  Image formats are typically negotiated on video capture and output
>  devices using the format and
> @@ -120,7 +121,9 @@ can expose pad-level image format configuration to
> applications. When they do, applications can use the
> 
>  :ref:`VIDIOC_SUBDEV_G_FMT <VIDIOC_SUBDEV_G_FMT>` and
>  :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctls. to
> 
> -negotiate formats on a per-pad basis.
> +negotiate formats on a per-pad basis. Note that when those ioctls are
> +called with or return the field order set to ``V4L2_FIELD_ALTERNATE``,
> +the format contains the field height, which is half the frame height.

Isn't that also the case for the TOP and BOTTOM field orders ?

>  Applications are responsible for configuring coherent parameters on the
>  whole pipeline and making sure that connected pads have compatible
> @@ -379,7 +382,10 @@ is supported by the hardware.
>     pad for further processing.
> 
>  2. Sink pad actual crop selection. The sink pad crop defines the crop
> -   performed to the sink pad format.
> +   performed to the sink pad format. The crop rectangle always refers to
> +   the frame size, even if the sink pad format has field order set to
> +   ``V4L2_FIELD_ALTERNATE`` and the actual processed images are only
> +   field sized.

I'm not sure to agree with this. I think all selection rectangle coordinates 
should be expressed relative to the format of the pad they refer to. For sink 
pad crop rectangles, if the sink pad receives alternate (or top or bottom 
only) fields, the rectangle coordinates should be relative to the field size. 
Similarly, if the source pad produces alternate/top/bottom fields, the 
rectangle coordinates should also be relative to the field size. If the subdev 
transforms alternate fields to progressive or interlaced frames, then the sink 
crop rectangle should be relative to the frame size.

The rationale behind this is that a subdev that receives and outputs alternate 
fields should only care about fields and shouldn't be aware about the full 
frame size.

>  3. Sink pad actual compose selection. The size of the sink pad compose
>     rectangle defines the scaling ratio compared to the size of the sink
> @@ -393,7 +399,9 @@ is supported by the hardware.
>  5. Source pad format. The source pad format defines the output pixel
>     format of the subdev, as well as the other parameters with the
>     exception of the image width and height. Width and height are defined
> -   by the size of the source pad actual crop selection.
> +   by the size of the source pad actual crop selection. If the source pad
> +   format has field order set to ``V4L2_FIELD_ALTERNATE``, the source pad
> +   field height is half the source pad crop selection height.
> 
>  Accessing any of the above rectangles not supported by the subdev will
>  return ``EINVAL``. Any rectangle referring to a previous unsupported
> diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst
> b/Documentation/media/uapi/v4l/subdev-formats.rst index
> d6152c907b8ba..f7195e5ee6e78 100644
> --- a/Documentation/media/uapi/v4l/subdev-formats.rst
> +++ b/Documentation/media/uapi/v4l/subdev-formats.rst
> @@ -19,7 +19,8 @@ Media Bus Formats
>        - Image width, in pixels.
>      * - __u32
>        - ``height``
> -      - Image height, in pixels.
> +      - Image height, in pixels. This is the field height for
> +        ``V4L2_FIELD_ALTERNATE`` field order, or the frame height
> otherwise.
>      * - __u32
>        - ``code``
>        - Format code, from enum
> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-size.rst
> b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-size.rst index
> 746c24ed97a05..a78ae138f8a87 100644
> --- a/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-size.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-enum-frame-size.rst
> @@ -55,6 +55,10 @@ maximum values. Applications must use the
> 
>  :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctl to try the
> 
>  sub-device for an exact supported frame size.
> 
> +Note that if ``V4L2_FIELD_ALTERNATE`` field order is chosen in the
> +:ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctls, those take
> +the field size, which is only half the height of the frame size.
> +
>  Available frame sizes may depend on the current 'try' formats at other
>  pads of the sub-device, as well as on the current active links and the
>  current values of V4L2 controls. See
> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst
> b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst index
> 071d9c033db6b..253e0ccb78224 100644
> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst
> @@ -45,6 +45,8 @@ function of the crop API, and more, are supported by the
> selections API. See :ref:`subdev` for more information on how each
> selection target affects the image processing pipeline inside the
> subdevice.
> 
> +Note that selection rectangles always refer to frame sizes, not field
> sizes. +
> 
>  Types of selection targets
>  --------------------------

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] docs-rst: clarify field vs frame height in the subdev API

[Philipp Zabel]

Hi Laurent,

On Fri, 2017-03-31 at 11:09 +0300, Laurent Pinchart wrote:
> Hi Philipp,
> 
> Thank you for the patch.
> 
> On Thursday 30 Mar 2017 17:38:20 Philipp Zabel wrote:
> > VIDIOC_SUBDEV_G/S_FMT take the field size if V4L2_FIELD_ALTERNATE field
> > order is set, but the VIDIOC_SUBDEV_G/S_SELECTION rectangles still refer
> > to frame size, regardless of the field order setting.
> > VIDIOC_SUBDEV_ENUM_FRAME_SIZES always returns frame sizes as opposed to
> > field sizes.
> > 
> > This was not immediately clear to me when reading the documentation, so
> > this patch adds some clarifications in the relevant places.
> > 
> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  Documentation/media/uapi/v4l/dev-subdev.rst              | 16 +++++++++----
> >  Documentation/media/uapi/v4l/subdev-formats.rst          |  3 ++-
> >  .../media/uapi/v4l/vidioc-subdev-enum-frame-size.rst     |  4 ++++
> >  .../media/uapi/v4l/vidioc-subdev-g-selection.rst         |  2 ++
> >  4 files changed, 20 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst
> > b/Documentation/media/uapi/v4l/dev-subdev.rst index
> > cd28701802086..2f0a41f3796f0 100644
> > --- a/Documentation/media/uapi/v4l/dev-subdev.rst
> > +++ b/Documentation/media/uapi/v4l/dev-subdev.rst
> > @@ -82,7 +82,8 @@ Pad-level Formats
> >  .. note::
> > 
> >      For the purpose of this section, the term *format* means the
> > -    combination of media bus data format, frame width and frame height.
> > +    combination of media bus data format, frame width and frame height,
> > +    unless otherwise noted.
> > 
> >  Image formats are typically negotiated on video capture and output
> >  devices using the format and
> > @@ -120,7 +121,9 @@ can expose pad-level image format configuration to
> > applications. When they do, applications can use the
> > 
> >  :ref:`VIDIOC_SUBDEV_G_FMT <VIDIOC_SUBDEV_G_FMT>` and
> >  :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctls. to
> > 
> > -negotiate formats on a per-pad basis.
> > +negotiate formats on a per-pad basis. Note that when those ioctls are
> > +called with or return the field order set to ``V4L2_FIELD_ALTERNATE``,
> > +the format contains the field height, which is half the frame height.
> 
> Isn't that also the case for the TOP and BOTTOM field orders ?

Oh, those exist, too. I'll change all occurences to list ALTERNATE, TOP,
and BOTTOM. I hope this is not going to be too verbose for people that
don't have to care about interlacing.

> >  Applications are responsible for configuring coherent parameters on the
> >  whole pipeline and making sure that connected pads have compatible
> > @@ -379,7 +382,10 @@ is supported by the hardware.
> >     pad for further processing.
> > 
> >  2. Sink pad actual crop selection. The sink pad crop defines the crop
> > -   performed to the sink pad format.
> > +   performed to the sink pad format. The crop rectangle always refers to
> > +   the frame size, even if the sink pad format has field order set to
> > +   ``V4L2_FIELD_ALTERNATE`` and the actual processed images are only
> > +   field sized.
> 
> I'm not sure to agree with this. I think all selection rectangle coordinates 
> should be expressed relative to the format of the pad they refer to.

But that's not how I understood Hans yesterday, and it shows that you
were quite on point with your suggestion to extend the docs.

> For sink pad crop rectangles, if the sink pad receives alternate (or
> top or bottom only) fields, the rectangle coordinates should be
> relative to the field size. Similarly, if the source pad produces
> alternate/top/bottom fields, the rectangle coordinates should also be
> relative to the field size.

That's also not how TVP5150 currently implements it. The crop rectangle
is frame sized even though the pad format reports alternating fields,
the same is true for vivid capture, even though that is not using the
subdev selection API.

Personally, I don't care whether the selection rectangles refer to frame
size or to the field size depending on the respective pad's field order
setting, but I'd really like to have it clearly spelled out in the
places this patch modifies.

> If the subdev transforms alternate fields to progressive or interlaced
> frames, then the sink crop rectangle should be relative to the frame
> size.

I'm confused. The sink pad is set to alternate fields in this case,
didn't you just argue that the sink crop/compose rectangles should refer
to field size?

Actually, this is exactly the case I want to handle. The CSI receives
FIELD_ALTERNATE frames from the TVP5150 with BT.656 synchronisation, but
it produces SEQ_TB or SEQ_BT (depending on standard) at its output pad.
If the input pad height is 288 lines for example, the output pad height
is 576 lines (in case of no cropping or scaling), and there's a sink
crop and a sink compose rectangle. Should those refer to the 288 lines
per field, or to the 576 lines per frame?

> The rationale behind this is that a subdev that receives and outputs alternate 
> fields should only care about fields and shouldn't be aware about the full 
> frame size.

regards
Philipp

Re: [PATCH] [media] docs-rst: clarify field vs frame height in the subdev API

[Hans Verkuil]

On 31/03/17 10:55, Philipp Zabel wrote:
> Hi Laurent,
> 
> On Fri, 2017-03-31 at 11:09 +0300, Laurent Pinchart wrote:
>> Hi Philipp,
>>
>> Thank you for the patch.
>>
>> On Thursday 30 Mar 2017 17:38:20 Philipp Zabel wrote:
>>> VIDIOC_SUBDEV_G/S_FMT take the field size if V4L2_FIELD_ALTERNATE field
>>> order is set, but the VIDIOC_SUBDEV_G/S_SELECTION rectangles still refer
>>> to frame size, regardless of the field order setting.
>>> VIDIOC_SUBDEV_ENUM_FRAME_SIZES always returns frame sizes as opposed to
>>> field sizes.
>>>
>>> This was not immediately clear to me when reading the documentation, so
>>> this patch adds some clarifications in the relevant places.
>>>
>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>  Documentation/media/uapi/v4l/dev-subdev.rst              | 16 +++++++++----
>>>  Documentation/media/uapi/v4l/subdev-formats.rst          |  3 ++-
>>>  .../media/uapi/v4l/vidioc-subdev-enum-frame-size.rst     |  4 ++++
>>>  .../media/uapi/v4l/vidioc-subdev-g-selection.rst         |  2 ++
>>>  4 files changed, 20 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst
>>> b/Documentation/media/uapi/v4l/dev-subdev.rst index
>>> cd28701802086..2f0a41f3796f0 100644
>>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst
>>> +++ b/Documentation/media/uapi/v4l/dev-subdev.rst
>>> @@ -82,7 +82,8 @@ Pad-level Formats
>>>  .. note::
>>>
>>>      For the purpose of this section, the term *format* means the
>>> -    combination of media bus data format, frame width and frame height.
>>> +    combination of media bus data format, frame width and frame height,
>>> +    unless otherwise noted.
>>>
>>>  Image formats are typically negotiated on video capture and output
>>>  devices using the format and
>>> @@ -120,7 +121,9 @@ can expose pad-level image format configuration to
>>> applications. When they do, applications can use the
>>>
>>>  :ref:`VIDIOC_SUBDEV_G_FMT <VIDIOC_SUBDEV_G_FMT>` and
>>>  :ref:`VIDIOC_SUBDEV_S_FMT <VIDIOC_SUBDEV_G_FMT>` ioctls. to
>>>
>>> -negotiate formats on a per-pad basis.
>>> +negotiate formats on a per-pad basis. Note that when those ioctls are
>>> +called with or return the field order set to ``V4L2_FIELD_ALTERNATE``,
>>> +the format contains the field height, which is half the frame height.
>>
>> Isn't that also the case for the TOP and BOTTOM field orders ?
> 
> Oh, those exist, too. I'll change all occurences to list ALTERNATE, TOP,
> and BOTTOM. I hope this is not going to be too verbose for people that
> don't have to care about interlacing.
> 
>>>  Applications are responsible for configuring coherent parameters on the
>>>  whole pipeline and making sure that connected pads have compatible
>>> @@ -379,7 +382,10 @@ is supported by the hardware.
>>>     pad for further processing.
>>>
>>>  2. Sink pad actual crop selection. The sink pad crop defines the crop
>>> -   performed to the sink pad format.
>>> +   performed to the sink pad format. The crop rectangle always refers to
>>> +   the frame size, even if the sink pad format has field order set to
>>> +   ``V4L2_FIELD_ALTERNATE`` and the actual processed images are only
>>> +   field sized.
>>
>> I'm not sure to agree with this. I think all selection rectangle coordinates 
>> should be expressed relative to the format of the pad they refer to.
> 
> But that's not how I understood Hans yesterday, and it shows that you
> were quite on point with your suggestion to extend the docs.

Actually, it is a bit different from what I said yesterday. Sorry about that.

Whether the top and height fields in struct v4l2_rect are for fields or
frames depends on whether it describes memory or video. Historically
VIDIOC_CROPCAP and VIDIOC_G/S_CROP used frame coordinates for video
capture (crop rectangle) and video output (compose rectangle, i.e. what is
composed into the video transmitter).

When the selection API was added we could also describe how video is
composed into a memory buffer (for capture) or cropped from a memory buffer
(for output). Since this deals with memory the v4l2_rect struct contains
field coordinates, for the same reason that G/S/TRY_FMT does.

The vivid driver *should* do all of this correctly. Since this driver
supports any combination of cropping/composing/scaler features it gets
quite complicated, so it is always possible that there are bugs, but I
did a lot of testing at the time.

>> For sink pad crop rectangles, if the sink pad receives alternate (or
>> top or bottom only) fields, the rectangle coordinates should be
>> relative to the field size. Similarly, if the source pad produces
>> alternate/top/bottom fields, the rectangle coordinates should also be
>> relative to the field size.
> 
> That's also not how TVP5150 currently implements it. The crop rectangle
> is frame sized even though the pad format reports alternating fields,

It is undefined today what the subdev selection rectangles should use.
I am inclined to *always* use frame coordinates while dealing with hardware
(receivers, transmitters, busses) and only use field coordinates when dealing
with actual memory buffers.

This will avoid having to change any subdev drivers as well, which is a nice
bonus. It also is consistent with the way the original API was designed:
frame coordinates everywhere, except when dealing with buffers in memory.

For the record: the DV_TIMINGS ioctls also define the height as frame height,
not field height. And the height in struct v4l2_mbus_framefmt is also defined
as a frame height.

> the same is true for vivid capture, even though that is not using the
> subdev selection API.

??? vivid uses frame height for crop coordinates when FIELD_ALTERNATE is
selected. Where did you see a field height when using vivid?

Note: by default vivid implements a scaler and composer. So switching to
field_alternate would still show a height of 576.

After disabling the scaler and composer:

v4l2-ctl -c enable_capture_scaler=0
v4l2-ctl -c enable_capture_composing=0

it will now be 288.

> 
> Personally, I don't care whether the selection rectangles refer to frame
> size or to the field size depending on the respective pad's field order
> setting, but I'd really like to have it clearly spelled out in the
> places this patch modifies.

Right.

>> If the subdev transforms alternate fields to progressive or interlaced
>> frames, then the sink crop rectangle should be relative to the frame
>> size.
> 
> I'm confused. The sink pad is set to alternate fields in this case,
> didn't you just argue that the sink crop/compose rectangles should refer
> to field size?
> 
> Actually, this is exactly the case I want to handle. The CSI receives
> FIELD_ALTERNATE frames from the TVP5150 with BT.656 synchronisation, but
> it produces SEQ_TB or SEQ_BT (depending on standard) at its output pad.
> If the input pad height is 288 lines for example, the output pad height
> is 576 lines (in case of no cropping or scaling), and there's a sink
> crop and a sink compose rectangle. Should those refer to the 288 lines
> per field, or to the 576 lines per frame?

The output pad of the tvp5150 would say FIELD_ALTERNATE and height 576.

The CSI output pad would be FIELD_SEQ_BT/TB and height 576.

The sink crop and sink compose rectangles should all use frame heights.
Of course, at the low level the driver will have to check the 'field'
value and program the hardware accordingly by dividing the top/height by
two when dealing with top/bottom/alternate formats.

Regards,

	Hans

>> The rationale behind this is that a subdev that receives and outputs alternate 
>> fields should only care about fields and shouldn't be aware about the full 
>> frame size.
> 
> regards
> Philipp
> 

Re: [PATCH] [media] docs-rst: clarify field vs frame height in the subdev API

[Philipp Zabel]

On Fri, 2017-03-31 at 13:08 +0200, Hans Verkuil wrote:
[...]
> >>>  Applications are responsible for configuring coherent parameters on the
> >>>  whole pipeline and making sure that connected pads have compatible
> >>> @@ -379,7 +382,10 @@ is supported by the hardware.
> >>>     pad for further processing.
> >>>
> >>>  2. Sink pad actual crop selection. The sink pad crop defines the crop
> >>> -   performed to the sink pad format.
> >>> +   performed to the sink pad format. The crop rectangle always refers to
> >>> +   the frame size, even if the sink pad format has field order set to
> >>> +   ``V4L2_FIELD_ALTERNATE`` and the actual processed images are only
> >>> +   field sized.
> >>
> >> I'm not sure to agree with this. I think all selection rectangle coordinates 
> >> should be expressed relative to the format of the pad they refer to.
> > 
> > But that's not how I understood Hans yesterday, and it shows that you
> > were quite on point with your suggestion to extend the docs.
> 
> Actually, it is a bit different from what I said yesterday. Sorry about that.
> 
> Whether the top and height fields in struct v4l2_rect are for fields or
> frames depends on whether it describes memory or video. Historically
> VIDIOC_CROPCAP and VIDIOC_G/S_CROP used frame coordinates for video
> capture (crop rectangle) and video output (compose rectangle, i.e. what is
> composed into the video transmitter).

Ok.

> When the selection API was added we could also describe how video is
> composed into a memory buffer (for capture) or cropped from a memory buffer
> (for output). Since this deals with memory the v4l2_rect struct contains
> field coordinates, for the same reason that G/S/TRY_FMT does.

Ok. This would not apply to subdevices that only handle video streams,
so that would mean that the v4l2_mbus_framefmt passed to
VIDIOC_SUBDEV_G/S_FMT also should always contain the frame size, never
the field size.

> The vivid driver *should* do all of this correctly. Since this driver
> supports any combination of cropping/composing/scaler features it gets
> quite complicated, so it is always possible that there are bugs, but I
> did a lot of testing at the time.

I haven't played much with vivid in this regard yet, I've only looked at
the capture device, and that behaved as I expected after your
explanation.

> >> For sink pad crop rectangles, if the sink pad receives alternate (or
> >> top or bottom only) fields, the rectangle coordinates should be
> >> relative to the field size. Similarly, if the source pad produces
> >> alternate/top/bottom fields, the rectangle coordinates should also be
> >> relative to the field size.
> > 
> > That's also not how TVP5150 currently implements it. The crop rectangle
> > is frame sized even though the pad format reports alternating fields,
> 
> It is undefined today what the subdev selection rectangles should use.
> I am inclined to *always* use frame coordinates while dealing with hardware
> (receivers, transmitters, busses) and only use field coordinates when dealing
> with actual memory buffers.
>
> This will avoid having to change any subdev drivers as well, which is a nice
> bonus. It also is consistent with the way the original API was designed:
> frame coordinates everywhere, except when dealing with buffers in memory.

Ok, I'll revise this patch accordingly.

> For the record: the DV_TIMINGS ioctls also define the height as frame height,
> not field height. And the height in struct v4l2_mbus_framefmt is also defined
> as a frame height.

The v4l2_mbus_framefmt height is defined as "image height", and it
wasn't clear to me what image meant in this context.

> > the same is true for vivid capture, even though that is not using the
> > subdev selection API.
> 
> ??? vivid uses frame height for crop coordinates when FIELD_ALTERNATE is
> selected. Where did you see a field height when using vivid?

That's what I meant with "the same": "The crop rectangle is frame sized
even though the [pad^W] format reports alternating fields".

> Note: by default vivid implements a scaler and composer. So switching to
> field_alternate would still show a height of 576.
>
> After disabling the scaler and composer:
> 
> v4l2-ctl -c enable_capture_scaler=0
> v4l2-ctl -c enable_capture_composing=0
> 
> it will now be 288.

So in this case the field size is used because S/G_FMT refer to memory.

[...]
> > Actually, this is exactly the case I want to handle. The CSI receives
> > FIELD_ALTERNATE frames from the TVP5150 with BT.656 synchronisation, but
> > it produces SEQ_TB or SEQ_BT (depending on standard) at its output pad.
> > If the input pad height is 288 lines for example, the output pad height
> > is 576 lines (in case of no cropping or scaling), and there's a sink
> > crop and a sink compose rectangle. Should those refer to the 288 lines
> > per field, or to the 576 lines per frame?
> 
> The output pad of the tvp5150 would say FIELD_ALTERNATE and height 576.

Aha, this didn't occur to me at all. This is not what happens currently.
Commit 4f57d27be2a5 ("[media] tvp5150: fix tvp5150_fill_fmt()") reduced
the height to half when switching to FIELD_ALTERNATE:

----------8<----------
diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index 437f1a7ecb96e..c277caaad8be8 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -852,10 +852,10 @@ static int tvp5150_fill_fmt(struct v4l2_subdev *sd,
        tvp5150_reset(sd, 0);
 
        f->width = decoder->rect.width;
-       f->height = decoder->rect.height;
+       f->height = decoder->rect.height / 2;
 
        f->code = MEDIA_BUS_FMT_UYVY8_2X8;
-       f->field = V4L2_FIELD_SEQ_TB;
+       f->field = V4L2_FIELD_ALTERNATE;
        f->colorspace = V4L2_COLORSPACE_SMPTE170M;
 
        v4l2_dbg(1, debug, sd, "width = %d, height = %d\n", f->width,
---------->8----------

So this should be partially reverted to say:
        f->height = decoder->rect.height;
again.

> The CSI output pad would be FIELD_SEQ_BT/TB and height 576.

That I understood.

> The sink crop and sink compose rectangles should all use frame heights.

But I wasn't clear about what height those and the CSI sink pad should
have. I now understand it should be all frame heights, both pads formats
and selection rectangles, regardless of the field setting, as none of
those refer to memory.

> Of course, at the low level the driver will have to check the 'field'
> value and program the hardware accordingly by dividing the top/height by
> two when dealing with top/bottom/alternate formats.

thanks
Philipp

Re: [PATCH] [media] docs-rst: clarify field vs frame height in the subdev API

[Hans Verkuil]

On 31/03/17 14:24, Philipp Zabel wrote:
> On Fri, 2017-03-31 at 13:08 +0200, Hans Verkuil wrote:
> [...]
>>>>>  Applications are responsible for configuring coherent parameters on the
>>>>>  whole pipeline and making sure that connected pads have compatible
>>>>> @@ -379,7 +382,10 @@ is supported by the hardware.
>>>>>     pad for further processing.
>>>>>
>>>>>  2. Sink pad actual crop selection. The sink pad crop defines the crop
>>>>> -   performed to the sink pad format.
>>>>> +   performed to the sink pad format. The crop rectangle always refers to
>>>>> +   the frame size, even if the sink pad format has field order set to
>>>>> +   ``V4L2_FIELD_ALTERNATE`` and the actual processed images are only
>>>>> +   field sized.
>>>>
>>>> I'm not sure to agree with this. I think all selection rectangle coordinates 
>>>> should be expressed relative to the format of the pad they refer to.
>>>
>>> But that's not how I understood Hans yesterday, and it shows that you
>>> were quite on point with your suggestion to extend the docs.
>>
>> Actually, it is a bit different from what I said yesterday. Sorry about that.
>>
>> Whether the top and height fields in struct v4l2_rect are for fields or
>> frames depends on whether it describes memory or video. Historically
>> VIDIOC_CROPCAP and VIDIOC_G/S_CROP used frame coordinates for video
>> capture (crop rectangle) and video output (compose rectangle, i.e. what is
>> composed into the video transmitter).
> 
> Ok.
> 
>> When the selection API was added we could also describe how video is
>> composed into a memory buffer (for capture) or cropped from a memory buffer
>> (for output). Since this deals with memory the v4l2_rect struct contains
>> field coordinates, for the same reason that G/S/TRY_FMT does.
> 
> Ok. This would not apply to subdevices that only handle video streams,
> so that would mean that the v4l2_mbus_framefmt passed to
> VIDIOC_SUBDEV_G/S_FMT also should always contain the frame size, never
> the field size.
> 
>> The vivid driver *should* do all of this correctly. Since this driver
>> supports any combination of cropping/composing/scaler features it gets
>> quite complicated, so it is always possible that there are bugs, but I
>> did a lot of testing at the time.
> 
> I haven't played much with vivid in this regard yet, I've only looked at
> the capture device, and that behaved as I expected after your
> explanation.
> 
>>>> For sink pad crop rectangles, if the sink pad receives alternate (or
>>>> top or bottom only) fields, the rectangle coordinates should be
>>>> relative to the field size. Similarly, if the source pad produces
>>>> alternate/top/bottom fields, the rectangle coordinates should also be
>>>> relative to the field size.
>>>
>>> That's also not how TVP5150 currently implements it. The crop rectangle
>>> is frame sized even though the pad format reports alternating fields,
>>
>> It is undefined today what the subdev selection rectangles should use.
>> I am inclined to *always* use frame coordinates while dealing with hardware
>> (receivers, transmitters, busses) and only use field coordinates when dealing
>> with actual memory buffers.
>>
>> This will avoid having to change any subdev drivers as well, which is a nice
>> bonus. It also is consistent with the way the original API was designed:
>> frame coordinates everywhere, except when dealing with buffers in memory.
> 
> Ok, I'll revise this patch accordingly.
> 
>> For the record: the DV_TIMINGS ioctls also define the height as frame height,
>> not field height. And the height in struct v4l2_mbus_framefmt is also defined
>> as a frame height.
> 
> The v4l2_mbus_framefmt height is defined as "image height", and it
> wasn't clear to me what image meant in this context.

I looked at the header: include/uapi/linux/v4l2-mediabus.h.

There it says "frame height".

> 
>>> the same is true for vivid capture, even though that is not using the
>>> subdev selection API.
>>
>> ??? vivid uses frame height for crop coordinates when FIELD_ALTERNATE is
>> selected. Where did you see a field height when using vivid?
> 
> That's what I meant with "the same": "The crop rectangle is frame sized
> even though the [pad^W] format reports alternating fields".
> 
>> Note: by default vivid implements a scaler and composer. So switching to
>> field_alternate would still show a height of 576.
>>
>> After disabling the scaler and composer:
>>
>> v4l2-ctl -c enable_capture_scaler=0
>> v4l2-ctl -c enable_capture_composing=0
>>
>> it will now be 288.
> 
> So in this case the field size is used because S/G_FMT refer to memory.

Right.

> 
> [...]
>>> Actually, this is exactly the case I want to handle. The CSI receives
>>> FIELD_ALTERNATE frames from the TVP5150 with BT.656 synchronisation, but
>>> it produces SEQ_TB or SEQ_BT (depending on standard) at its output pad.
>>> If the input pad height is 288 lines for example, the output pad height
>>> is 576 lines (in case of no cropping or scaling), and there's a sink
>>> crop and a sink compose rectangle. Should those refer to the 288 lines
>>> per field, or to the 576 lines per frame?
>>
>> The output pad of the tvp5150 would say FIELD_ALTERNATE and height 576.
> 
> Aha, this didn't occur to me at all. This is not what happens currently.
> Commit 4f57d27be2a5 ("[media] tvp5150: fix tvp5150_fill_fmt()") reduced
> the height to half when switching to FIELD_ALTERNATE:
> 
> ----------8<----------
> diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> index 437f1a7ecb96e..c277caaad8be8 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -852,10 +852,10 @@ static int tvp5150_fill_fmt(struct v4l2_subdev *sd,
>         tvp5150_reset(sd, 0);
>  
>         f->width = decoder->rect.width;
> -       f->height = decoder->rect.height;
> +       f->height = decoder->rect.height / 2;
>  
>         f->code = MEDIA_BUS_FMT_UYVY8_2X8;
> -       f->field = V4L2_FIELD_SEQ_TB;
> +       f->field = V4L2_FIELD_ALTERNATE;
>         f->colorspace = V4L2_COLORSPACE_SMPTE170M;
>  
>         v4l2_dbg(1, debug, sd, "width = %d, height = %d\n", f->width,
> ---------->8----------
> 
> So this should be partially reverted to say:
>         f->height = decoder->rect.height;
> again.
> 
>> The CSI output pad would be FIELD_SEQ_BT/TB and height 576.
> 
> That I understood.
> 
>> The sink crop and sink compose rectangles should all use frame heights.
> 
> But I wasn't clear about what height those and the CSI sink pad should
> have. I now understand it should be all frame heights, both pads formats
> and selection rectangles, regardless of the field setting, as none of
> those refer to memory.

That's why I think should happen, yes.

Note: interlaced formats over subdevs are rare, the use of TOP, BOTTOM
and ALTERNATE is even rarer. Seeing inconsistent behavior in drivers is
to be expected given how seldom it is used. And everyone I know of tries
to avoid interlaced formats like the plague :-)

Regards,

	Hans

> 
>> Of course, at the low level the driver will have to check the 'field'
>> value and program the hardware accordingly by dividing the top/height by
>> two when dealing with top/bottom/alternate formats.
> 
> thanks
> Philipp
>