On Fri, 2016-07-15 at 19:50 +0200, Hans Verkuil wrote:
> On 07/14/2016 08:27 AM, tiffany lin wrote:
> > Hi Hans,
> >
> >
> > On Mon, 2016-07-11 at 06:32 +0200, Hans Verkuil wrote:
> >> Hi Tiffany,
> >>
> >> My apologies for the delay, but here is my review at last:
> >>
> >> On 05/30/2016 09:52 AM, Tiffany Lin wrote:
> >>> This patch add g/s_selection support for MT8173
> >>>
> >>> Signed-off-by: Tiffany Lin <tiffany.lin@xxxxxxxxxxxx>
> >>> ---
> >>> drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 74 ++++++++++++++++++++
> >>> 1 file changed, 74 insertions(+)
> >>>
> >>> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> >>> index 6e72d73..23ef9a1 100644
> >>> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> >>> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> >>> @@ -630,6 +630,77 @@ static int vidioc_try_fmt_vid_out_mplane(struct file *file, void *priv,
> >>> return vidioc_try_fmt(f, fmt);
> >>> }
> >>>
> >>> +static int vidioc_venc_g_selection(struct file *file, void *priv,
> >>> + struct v4l2_selection *s)
> >>> +{
> >>> + struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
> >>> + struct mtk_q_data *q_data;
> >>> +
> >>> + /* crop means compose for output devices */
> >>> + switch (s->target) {
> >>> + case V4L2_SEL_TGT_CROP_DEFAULT:
> >>> + case V4L2_SEL_TGT_CROP_BOUNDS:
> >>> + case V4L2_SEL_TGT_CROP:
> >>> + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> >>> + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> >>> + case V4L2_SEL_TGT_COMPOSE:
> >>> + if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> >>> + mtk_v4l2_err("Invalid s->type = %d", s->type);
> >>> + return -EINVAL;
> >>> + }
> >>> + break;
> >>> + default:
> >>> + mtk_v4l2_err("Invalid s->target = %d", s->target);
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + q_data = mtk_venc_get_q_data(ctx, s->type);
> >>> + if (!q_data)
> >>> + return -EINVAL;
> >>> +
> >>> + s->r.top = 0;
> >>> + s->r.left = 0;
> >>> + s->r.width = q_data->visible_width;
> >>> + s->r.height = q_data->visible_height;
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int vidioc_venc_s_selection(struct file *file, void *priv,
> >>> + struct v4l2_selection *s)
> >>> +{
> >>> + struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
> >>> + struct mtk_q_data *q_data;
> >>> +
> >>> + switch (s->target) {
> >>> + case V4L2_SEL_TGT_CROP_DEFAULT:
> >>> + case V4L2_SEL_TGT_CROP_BOUNDS:
> >>> + case V4L2_SEL_TGT_CROP:
> >>> + case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> >>> + case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> >>> + case V4L2_SEL_TGT_COMPOSE:
> >>> + if (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> >>> + mtk_v4l2_err("Invalid s->type = %d", s->type);
> >>> + return -EINVAL;
> >>> + }
> >>> + break;
> >>> + default:
> >>> + mtk_v4l2_err("Invalid s->target = %d", s->target);
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + q_data = mtk_venc_get_q_data(ctx, s->type);
> >>> + if (!q_data)
> >>> + return -EINVAL;
> >>> +
> >>> + s->r.top = 0;
> >>> + s->r.left = 0;
> >>> + q_data->visible_width = s->r.width;
> >>> + q_data->visible_height = s->r.height;
> >>
> >> This makes no sense.
> >>
> >> See this page:
> >>
> >> https://hverkuil.home.xs4all.nl/spec/media.html#selection-api
> >>
> >> For the video output direction (memory -> HW encoder) the data source is
> >> the memory, the data sink is the HW encoder. For the video capture direction
> >> (HW encoder -> memory) the data source is the HW encoder and the data sink
> >> is the memory.
> >>
> >> Usually for m2m devices the video output direction may support cropping and
> >> the video capture direction may support composing.
> >>
> >> It's not clear what you intend here, especially since you set left and right
> >> to 0. That's not what the selection operation is supposed to do.
> >>
> > I am confused about about g/s_selection.
> > If application want to configure encode area and crop meta-data, it
> > should set crop info to OUTPUT queue, is that right?
> > if user space still use g/s_crop ioctl, in
> > v4l_g_crop and v4l_s_crop, it set target to V4L2_SEL_TGT_COMPOSE_ACTIVE
> > when buf type is V4L2_TYPE_IS_OUTPUT.
> >
> > It looks like when work with g/s_crop ioctl, command set to OUTPUT
> > buffer will use target V4L2_SEL_TGT_COMPOSE_ACTIVE.
>
> Correct. The semantics of g/s_crop for output devices is really weird
> and g/s_crop is generally useless for mem2mem devices.
>
> You should completely ignore the old g/s_crop and only look at g/s_selection.
>
> > When work with g/s_selection ictol, command set to OUTPUT buffer will
> > use V4L2_SEL_TGT_CROP_ACTIVE.
> > Is this correct behavior?
>
> Yes. What this means is that userspace has to use g/s_selection for
> mem2mem devices since g/s_crop changes the wrong thing: compose instead
> of crop for OUTPUT and crop instead of compose for CAPTURE.
>
> The g/s_selection ioctls were added to solve this problem with g/s_crop.
> It always confuses people and it was due to a lack of foresight when the
> old crop API was designed: it was made for video capture where you
> crop on the hardware side (in the video receiver), and for video output it
> would compose the image in the video transmitter's total frame (usually
> 720x480/576). None of this applies in general to memory-to-memory devices.
>
Understood now.
Now I am trying to figure out how to make this function right.
Our encoder only support crop range from (0, 0) to (width, height), so
if s->r.top and s->r.left not 0, I will return -EINVAL.
Another thing is that in v4l2-compliance test, it has testLegacyCrop.
It looks like we still need to support
V4L2_SEL_TGT_COMPOSE_DEFAULT:
V4L2_SEL_TGT_COMPOSE_BOUNDS:
V4L2_SEL_TGT_COMPOSE:
to pass v4l2 compliance test, Or it will fail in
fail: v4l2-test-formats.cpp(1318): !doioctl(node, VIDIOC_G_SELECTION,
&sel)
fail: v4l2-test-formats.cpp(1336): testLegacyCrop(node)
test Cropping: FAIL
I don't understand the following testing code.
/*
* If either CROPCAP or G_CROP works, then G_SELECTION should
* work as well.
* If neither CROPCAP nor G_CROP work, then G_SELECTION
shouldn't
* work either.
*/
if (!doioctl(node, VIDIOC_CROPCAP, &cap)) {
fail_on_test(doioctl(node, VIDIOC_G_SELECTION, &sel));
// Checks for invalid types
if (cap.type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
cap.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
else
cap.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
fail_on_test(doioctl(node, VIDIOC_CROPCAP, &cap) !=
EINVAL);
cap.type = 0xff;
fail_on_test(doioctl(node, VIDIOC_CROPCAP, &cap) !=
EINVAL);
} else {
fail_on_test(!doioctl(node, VIDIOC_G_SELECTION, &sel));
-> fail here
}
When test OUTPUT queue, it fail because v4l_cropcap will fail when
s.target = V4L2_SEL_TGT_COMPOSE_BOUNDS.
If VIDIOC_CROPCAP ioctl fail, VIDIOC_G_SELECTION should fail.
But VIDIOC_G_SELECTION target on CROP not COMPOSE and it success.
best regards,
Tiffany
> Regards,
>
> Hans
>
> >
> >
> > static int v4l_g_crop(const struct v4l2_ioctl_ops *ops,
> > struct file *file, void *fh, void *arg)
> > {
> > struct v4l2_crop *p = arg;
> > struct v4l2_selection s = {
> > .type = p->type,
> > };
> > int ret;
> >
> > if (ops->vidioc_g_crop)
> > return ops->vidioc_g_crop(file, fh, p);
> > /* simulate capture crop using selection api */
> >
> > /* crop means compose for output devices */
> > if (V4L2_TYPE_IS_OUTPUT(p->type))
> > s.target = V4L2_SEL_TGT_COMPOSE_ACTIVE;
> > else
> > s.target = V4L2_SEL_TGT_CROP_ACTIVE;
> >
> > ret = ops->vidioc_g_selection(file, fh, &s);
> >
> > /* copying results to old structure on success */
> > if (!ret)
> > p->c = s.r;
> > return ret;
> > }
> >
> > static int v4l_s_crop(const struct v4l2_ioctl_ops *ops,
> > struct file *file, void *fh, void *arg)
> > {
> > struct v4l2_crop *p = arg;
> > struct v4l2_selection s = {
> > .type = p->type,
> > .r = p->c,
> > };
> >
> > if (ops->vidioc_s_crop)
> > return ops->vidioc_s_crop(file, fh, p);
> > /* simulate capture crop using selection api */
> >
> > /* crop means compose for output devices */
> > if (V4L2_TYPE_IS_OUTPUT(p->type))
> > s.target = V4L2_SEL_TGT_COMPOSE_ACTIVE;
> > else
> > s.target = V4L2_SEL_TGT_CROP_ACTIVE;
> >
> > return ops->vidioc_s_selection(file, fh, &s);
> > }
> >
> >
> > best regards,
> > Tiffany
> >
> >
> >
> >
> >> Regards,
> >>
> >> Hans
> >>
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> static int vidioc_venc_qbuf(struct file *file, void *priv,
> >>> struct v4l2_buffer *buf)
> >>> {
> >>> @@ -688,6 +759,9 @@ const struct v4l2_ioctl_ops mtk_venc_ioctl_ops = {
> >>>
> >>> .vidioc_create_bufs = v4l2_m2m_ioctl_create_bufs,
> >>> .vidioc_prepare_buf = v4l2_m2m_ioctl_prepare_buf,
> >>> +
> >>> + .vidioc_g_selection = vidioc_venc_g_selection,
> >>> + .vidioc_s_selection = vidioc_venc_s_selection,
> >>> };
> >>>
> >>> static int vb2ops_venc_queue_setup(struct vb2_queue *vq,
> >>>
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
