From patchwork Fri Mar 31 12:24:00 2017
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
X-Patchwork-Submitter: Philipp Zabel
X-Patchwork-Id: 9656209
Return-Path:
Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org
[172.30.200.125])
by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id
5B09D60351 for ;
Fri, 31 Mar 2017 12:24:07 +0000 (UTC)
Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1])
by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4B27E28630
for ;
Fri, 31 Mar 2017 12:24:07 +0000 (UTC)
Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486)
id 4018D2867F; Fri, 31 Mar 2017 12:24:07 +0000 (UTC)
X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on
pdx-wl-mail.web.codeaurora.org
X-Spam-Level:
X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI
autolearn=ham version=3.3.1
Received: from vger.kernel.org (vger.kernel.org [209.132.180.67])
by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 81B6E28630
for ;
Fri, 31 Mar 2017 12:24:06 +0000 (UTC)
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S932927AbdCaMYG (ORCPT
);
Fri, 31 Mar 2017 08:24:06 -0400
Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:43149 "EHLO
metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK)
by vger.kernel.org with ESMTP id S1751297AbdCaMYF (ORCPT
);
Fri, 31 Mar 2017 08:24:05 -0400
Received: from lupine.hi.pengutronix.de
([2001:67c:670:100:3ad5:47ff:feaf:1a17] helo=lupine)
by metis.ext.pengutronix.de with esmtp (Exim 4.84_2)
(envelope-from )
id 1ctvb0-0004ZP-Jo; Fri, 31 Mar 2017 14:24:02 +0200
Message-ID: <1490963040.2371.55.camel@pengutronix.de>
Subject: Re: [PATCH] [media] docs-rst: clarify field vs frame height in the
subdev API
From: Philipp Zabel
To: Hans Verkuil
Cc: Laurent Pinchart ,
linux-media@vger.kernel.org, Hans Verkuil ,
Sakari Ailus
Date: Fri, 31 Mar 2017 14:24:00 +0200
In-Reply-To: <9fe503ab-fcf1-c56a-5acd-b1350a317d6f@xs4all.nl>
References: <20170330153820.14853-1-p.zabel@pengutronix.de>
<1790355.cli1gBmIc5@avalon> <1490950514.2371.21.camel@pengutronix.de>
<9fe503ab-fcf1-c56a-5acd-b1350a317d6f@xs4all.nl>
X-Mailer: Evolution 3.12.9-1+b1
Mime-Version: 1.0
X-SA-Exim-Connect-IP: 2001:67c:670:100:3ad5:47ff:feaf:1a17
X-SA-Exim-Mail-From: p.zabel@pengutronix.de
X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de);
SAEximRunCond expanded to false
X-PTX-Original-Recipient: linux-media@vger.kernel.org
Sender: linux-media-owner@vger.kernel.org
Precedence: bulk
List-ID:
X-Mailing-List: linux-media@vger.kernel.org
X-Virus-Scanned: ClamAV using ClamSMTP
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<----------
---------->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
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,