Re: [PATCH v2 3/8] media: vidc: decoder: add video decoder files

[Hans Verkuil <hverkuil@xs4all.nl>]

On 03/11/16 11:45, Stanimir Varbanov wrote:
> Hi Hans,
>
> On 09/19/2016 01:04 PM, Hans Verkuil wrote:
>> On 09/07/2016 01:37 PM, Stanimir Varbanov wrote:
>>> This consists of video decoder implementation plus decoder
>>> controls.
>>>
>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>> ---
>>>  drivers/media/platform/qcom/vidc/vdec.c       | 1091 +++++++++++++++++++++++++
>>>  drivers/media/platform/qcom/vidc/vdec.h       |   29 +
>>>  drivers/media/platform/qcom/vidc/vdec_ctrls.c |  200 +++++
>>>  drivers/media/platform/qcom/vidc/vdec_ctrls.h |   21 +
>>>  4 files changed, 1341 insertions(+)
>>>  create mode 100644 drivers/media/platform/qcom/vidc/vdec.c
>>>  create mode 100644 drivers/media/platform/qcom/vidc/vdec.h
>>>  create mode 100644 drivers/media/platform/qcom/vidc/vdec_ctrls.c
>>>  create mode 100644 drivers/media/platform/qcom/vidc/vdec_ctrls.h
>>>
>
> <cut>
>
>>> +
>>> +static int
>>> +vdec_g_selection(struct file *file, void *priv, struct v4l2_selection *s)
>>> +{
>>> +	struct vidc_inst *inst = to_inst(file);
>>> +
>>> +	if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
>>> +		return -EINVAL;
>>> +
>>> +	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:
>>
>> This is almost certainly wrong.
>>
>> For capture I would expect that you can do compose, but not crop.
>>
>> This would likely explain that v4l2-compliance thinks that the driver can
>> scale:
>>
>>                 test Scaling: OK
>>
>
> Maybe I need some help to implement correctly g_selection.
>
> Lets say that the resolution of the compressed stream is 1280x720, and
> that resolution is set with s_fmt(OUTPUT queue), then I calculate the
> output resolution which I will return by g_fmt(CAPTURE queue) and it
> will be 1280x736 (hardware wants height to be multiple of 32 lines). So
> the result will be 16 lines of vertical padding which should be exposed
> to client (think of gstreamer v4l2videodec element) via g_crop
> (g_selection) as 1280x720 because this is the actual image.
>
> So from what I understood while read Selection API, I need to support
> only composing on CAPTURE queue, no scaling and no cropping.
>
> OUTPUT buffer type, data source
> TGT_CROP_BOUNDS = TGT_CROP_DEFAULT = TGT_CROP = 1280x720
> TGT_COMPOSE_BOUNDS = TGT_COMPOSE_DEFAULT = TGT_COMPOSE = 1280x720

The output buffer type doesn't do composition, only crop. So you shouldn't
support the compose targets.

>
> CAPTURE buffer type, data sink
> TGT_CROP_BOUNDS = TGT_CROP_DEFAULT = TGT_CROP = EINVAL
> TGT_COMPOSE_BOUNDS = 1280x736
> TGT_COMPOSE_DEFAULT = TGT_COMPOSE = 1280x720

This looks good.

>
> With this logic in g_selection the output of v4l2-compliance test
> application is:
> 	test Cropping: OK
> 	test Composing: OK
> 	test Scaling: OK (Not Supported)
>
> So why v4l2-compliance still thinks that the driver supports Cropping?

Hmm, v4l2-compliance could be improved. The problem is that it doesn't
show for m2m devices for which side (capture or output) cropping, composing
or scaling is supported. It does test both sides, but you can't tell from
the output.

It's a bit confusing in this case.

Regards,

	Hans

>
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	s->r.top = 0;
>>> +	s->r.left = 0;
>>> +	s->r.width = inst->out_width;
>>> +	s->r.height = inst->out_height;
>>> +
>>> +	return 0;
>>> +}
>
> <cut>
>