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

[Philipp Zabel <p.zabel@pengutronix.de>]

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