Re: [PATCH] vcodec: mediatek: Add g/s_selection support for V4L2 Encoder

[tiffany lin]

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
> >