| 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
| ||
|
Message-ID: <3d08da09653034128c48e97c1f6fad59c2ff5f35.camel@ndufresne.ca>
Date: Mon, 16 Jun 2025 15:37:42 -0400
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: Ricardo Ribalda <ribalda@...omium.org>, Vikash Garodia
<quic_vgarodia@...cinc.com>, Dikshita Agarwal <quic_dikshita@...cinc.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>, Mauro Carvalho Chehab
<mchehab@...nel.org>, Stanimir Varbanov <stanimir.varbanov@...aro.org>,
Hans Verkuil <hans.verkuil@...co.com>
Cc: linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 3/5] media: venus: Remove timeperframe from inst
Hi Ricardo,
Le lundi 16 juin 2025 à 15:29 +0000, Ricardo Ribalda a écrit :
> The driver only cares about whole fps. We can infer the timeperframe
> from the fps field. Remove the redundant field.
I do have reserved about this change. Video standards commonly uses fractional
rates for videos. If my memory is correct, venus uses Q16 ... So with this change,
we now round all frame rate passed to encoders to an integer, which will introduce
error in the resulting bitrate.
Perhaps it was already broken, but if so, it should be fixed instead ?
regards,
Nicolas
>
> Reviewed-by: Vikash Garodia <quic_vgarodia@...cinc.com>
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
> drivers/media/platform/qcom/venus/core.h | 2 --
> drivers/media/platform/qcom/venus/vdec.c | 15 ++++++++-------
> drivers/media/platform/qcom/venus/venc.c | 16 ++++++++--------
> 3 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 5b1ba1c69adba14c3560a4bc6d09435529f295a6..9cfb860e01e752bf9856a3550f59c8c7b43647d2 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -413,7 +413,6 @@ enum venus_inst_modes {
> * @tss: timestamp metadata
> * @payloads: cache plane payload to use it for clock/BW scaling
> * @fps: holds current FPS
> - * @timeperframe: holds current time per frame structure
> * @fmt_out: a reference to output format structure
> * @fmt_cap: a reference to capture format structure
> * @num_input_bufs: holds number of input buffers
> @@ -484,7 +483,6 @@ struct venus_inst {
> struct venus_ts_metadata tss[VIDEO_MAX_FRAME];
> unsigned long payloads[VIDEO_MAX_FRAME];
> u64 fps;
> - struct v4l2_fract timeperframe;
> const struct venus_format *fmt_out;
> const struct venus_format *fmt_cap;
> unsigned int num_input_bufs;
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index fca27be61f4b869840904cc0577949635bc63cab..7d6612234d18a49573dc502d48ee61a900b63194 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -471,10 +471,12 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> return -EINVAL;
>
> memset(cap->reserved, 0, sizeof(cap->reserved));
> - if (!timeperframe->denominator)
> - timeperframe->denominator = inst->timeperframe.denominator;
> - if (!timeperframe->numerator)
> - timeperframe->numerator = inst->timeperframe.numerator;
> +
> + if (!timeperframe->numerator || !timeperframe->denominator) {
> + timeperframe->numerator = 1;
> + timeperframe->denominator = inst->fps;
> + }
> +
> cap->readbuffers = 0;
> cap->extendedmode = 0;
> cap->capability = V4L2_CAP_TIMEPERFRAME;
> @@ -487,7 +489,8 @@ static int vdec_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> fps = min(VENUS_MAX_FPS, fps);
>
> inst->fps = fps;
> - inst->timeperframe = *timeperframe;
> + timeperframe->numerator = 1;
> + timeperframe->denominator = inst->fps;
>
> return 0;
> }
> @@ -1622,8 +1625,6 @@ static void vdec_inst_init(struct venus_inst *inst)
> inst->out_width = frame_width_min(inst);
> inst->out_height = frame_height_min(inst);
> inst->fps = 30;
> - inst->timeperframe.numerator = 1;
> - inst->timeperframe.denominator = 30;
> inst->opb_buftype = HFI_BUFFER_OUTPUT;
> }
>
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index b9ccee870c3d1238e04cef5e9344bd992d86d737..4979392aa20b6dc94895c7089878531b92b57754 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -401,10 +401,10 @@ static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
>
> memset(out->reserved, 0, sizeof(out->reserved));
>
> - if (!timeperframe->denominator)
> - timeperframe->denominator = inst->timeperframe.denominator;
> - if (!timeperframe->numerator)
> - timeperframe->numerator = inst->timeperframe.numerator;
> + if (!timeperframe->numerator || !timeperframe->denominator) {
> + timeperframe->numerator = 1;
> + timeperframe->denominator = inst->fps;
> + }
>
> out->capability = V4L2_CAP_TIMEPERFRAME;
>
> @@ -416,8 +416,9 @@ static int venc_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> do_div(fps, us_per_frame);
> fps = min(VENUS_MAX_FPS, fps);
>
> - inst->timeperframe = *timeperframe;
> inst->fps = fps;
> + timeperframe->numerator = 1;
> + timeperframe->denominator = inst->fps;
>
> return 0;
> }
> @@ -431,7 +432,8 @@ static int venc_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
> return -EINVAL;
>
> a->parm.output.capability |= V4L2_CAP_TIMEPERFRAME;
> - a->parm.output.timeperframe = inst->timeperframe;
> + a->parm.output.timeperframe.numerator = 1;
> + a->parm.output.timeperframe.denominator = inst->fps;
>
> return 0;
> }
> @@ -1454,8 +1456,6 @@ static void venc_inst_init(struct venus_inst *inst)
> inst->out_width = 1280;
> inst->out_height = 720;
> inst->fps = 15;
> - inst->timeperframe.numerator = 1;
> - inst->timeperframe.denominator = 15;
> inst->hfi_codec = HFI_VIDEO_CODEC_H264;
> }
>
Powered by blists - more mailing lists