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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20aa8a6d-d1a6-c4e4-761d-71460629ff7f@linaro.org>
Date:   Thu, 3 Nov 2016 12:45:20 +0200
From:   Stanimir Varbanov <stanimir.varbanov@...aro.org>
To:     Hans Verkuil <hverkuil@...all.nl>,
        Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>
Cc:     Andy Gross <andy.gross@...aro.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 3/8] media: vidc: decoder: add video decoder files

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@...aro.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

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

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?

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

-- 
regards,
Stan