* [PATCH v2] media: venus: add support for key frame
@ 2018-10-24 13:52 Malathi Gottam
2018-10-24 14:32 ` Tomasz Figa
0 siblings, 1 reply; 4+ messages in thread
From: Malathi Gottam @ 2018-10-24 13:52 UTC (permalink / raw)
To: stanimir.varbanov, hverkuil, mchehab
Cc: linux-media, linux-kernel, linux-arm-msm, acourbot, vgarodia, mgottam
When client requests for a keyframe, set the property
to hardware to generate the sync frame.
Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
---
drivers/media/platform/qcom/venus/venc_ctrls.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 45910172..6c2655d 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -79,8 +79,10 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
{
struct venus_inst *inst = ctrl_to_inst(ctrl);
struct venc_controls *ctr = &inst->controls.enc;
+ struct hfi_enable en = { .enable = 1 };
u32 bframes;
int ret;
+ u32 ptype;
switch (ctrl->id) {
case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
@@ -173,6 +175,15 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
ctr->num_b_frames = bframes;
break;
+ case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
+ if (inst->streamon_out && inst->streamon_cap) {
+ ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
+ ret = hfi_session_set_property(inst, ptype, &en);
+
+ if (ret)
+ return ret;
+ }
+ break;
default:
return -EINVAL;
}
@@ -188,7 +199,7 @@ int venc_ctrl_init(struct venus_inst *inst)
{
int ret;
- ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 27);
+ ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 28);
if (ret)
return ret;
@@ -309,6 +320,9 @@ int venc_ctrl_init(struct venus_inst *inst)
v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
V4L2_CID_MPEG_VIDEO_H264_I_PERIOD, 0, (1 << 16) - 1, 1, 0);
+ v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
+ V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME, 0, 0, 0, 0);
+
ret = inst->ctrl_handler.error;
if (ret)
goto err;
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2] media: venus: add support for key frame
2018-10-24 13:52 [PATCH v2] media: venus: add support for key frame Malathi Gottam
@ 2018-10-24 14:32 ` Tomasz Figa
2018-10-25 7:23 ` Vikash Garodia
0 siblings, 1 reply; 4+ messages in thread
From: Tomasz Figa @ 2018-10-24 14:32 UTC (permalink / raw)
To: mgottam
Cc: Stanimir Varbanov, Hans Verkuil, Mauro Carvalho Chehab,
Linux Media Mailing List, Linux Kernel Mailing List,
linux-arm-msm, Alexandre Courbot, vgarodia
On Wed, Oct 24, 2018 at 10:52 PM Malathi Gottam <mgottam@codeaurora.org> wrote:
>
> When client requests for a keyframe, set the property
> to hardware to generate the sync frame.
>
> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> ---
> drivers/media/platform/qcom/venus/venc_ctrls.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> index 45910172..6c2655d 100644
> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> @@ -79,8 +79,10 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct venus_inst *inst = ctrl_to_inst(ctrl);
> struct venc_controls *ctr = &inst->controls.enc;
> + struct hfi_enable en = { .enable = 1 };
> u32 bframes;
> int ret;
> + u32 ptype;
>
> switch (ctrl->id) {
> case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
> @@ -173,6 +175,15 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>
> ctr->num_b_frames = bframes;
> break;
> + case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
> + if (inst->streamon_out && inst->streamon_cap) {
> + ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
> + ret = hfi_session_set_property(inst, ptype, &en);
> +
> + if (ret)
> + return ret;
> + }
> + break;
This is still not the right way to handle this.
Please see the documentation of this control [1]:
"V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME (button)
Force a key frame for the next queued buffer. Applicable to encoders.
This is a general, codec-agnostic keyframe control."
Even if the driver is not streaming, it must remember that the
keyframe was requested for next buffer. The next time userspace QBUFs
an OUTPUT buffer, it should ask the hardware to encode that OUTPUT
buffer into a keyframe.
[1] https://www.kernel.org/doc/html/latest/media/uapi/v4l/extended-controls.html?highlight=v4l2_cid_mpeg_video_force_key_frame
But generally, the proper modern way for the userspace to request a
keyframe is to set the V4L2_BUF_FLAG_KEYFRAME flag in the
vb2_buffer_flag when queuing an OUTPUT buffer. It's the only
guaranteed way to ensure that the keyframe will be encoded exactly for
the selected frame. (The V4L2 control API doesn't guarantee any
synchronization between controls and buffers itself.)
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] media: venus: add support for key frame
2018-10-24 14:32 ` Tomasz Figa
@ 2018-10-25 7:23 ` Vikash Garodia
2018-10-25 7:30 ` Tomasz Figa
0 siblings, 1 reply; 4+ messages in thread
From: Vikash Garodia @ 2018-10-25 7:23 UTC (permalink / raw)
To: Tomasz Figa
Cc: mgottam, Stanimir Varbanov, Hans Verkuil, Mauro Carvalho Chehab,
Linux Media Mailing List, Linux Kernel Mailing List,
linux-arm-msm, Alexandre Courbot, linux-media-owner
On 2018-10-24 20:02, Tomasz Figa wrote:
> On Wed, Oct 24, 2018 at 10:52 PM Malathi Gottam
> <mgottam@codeaurora.org> wrote:
>>
>> When client requests for a keyframe, set the property
>> to hardware to generate the sync frame.
>>
>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>> ---
>> drivers/media/platform/qcom/venus/venc_ctrls.c | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c
>> b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> index 45910172..6c2655d 100644
>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> @@ -79,8 +79,10 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>> {
>> struct venus_inst *inst = ctrl_to_inst(ctrl);
>> struct venc_controls *ctr = &inst->controls.enc;
>> + struct hfi_enable en = { .enable = 1 };
>> u32 bframes;
>> int ret;
>> + u32 ptype;
>>
>> switch (ctrl->id) {
>> case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
>> @@ -173,6 +175,15 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>
>> ctr->num_b_frames = bframes;
>> break;
>> + case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
>> + if (inst->streamon_out && inst->streamon_cap) {
>> + ptype =
>> HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
>> + ret = hfi_session_set_property(inst, ptype,
>> &en);
>> +
>> + if (ret)
>> + return ret;
>> + }
>> + break;
>
> This is still not the right way to handle this.
>
> Please see the documentation of this control [1]:
>
> "V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME (button)
> Force a key frame for the next queued buffer. Applicable to encoders.
> This is a general, codec-agnostic keyframe control."
>
> Even if the driver is not streaming, it must remember that the
> keyframe was requested for next buffer. The next time userspace QBUFs
> an OUTPUT buffer, it should ask the hardware to encode that OUTPUT
> buffer into a keyframe.
That's correct. Driver can cache the client request and set it when the
hardware
is capable of accepting the property.
Still the issue having the requested OUTPUT buffer to be encoded as sync
frame will
be there. If there are few frames queued before streamon, driver will
only keep a
note that it has to set the request for keyframe, but not the exact one
which was
requested.
> [1]
> https://www.kernel.org/doc/html/latest/media/uapi/v4l/extended-controls.html?highlight=v4l2_cid_mpeg_video_force_key_frame
>
> But generally, the proper modern way for the userspace to request a
> keyframe is to set the V4L2_BUF_FLAG_KEYFRAME flag in the
> vb2_buffer_flag when queuing an OUTPUT buffer. It's the only
> guaranteed way to ensure that the keyframe will be encoded exactly for
> the selected frame. (The V4L2 control API doesn't guarantee any
> synchronization between controls and buffers itself.)
This is a better way to handle it to ensure exact buffer gets encoded as
sync frame.
> Best regards,
> Tomasz
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] media: venus: add support for key frame
2018-10-25 7:23 ` Vikash Garodia
@ 2018-10-25 7:30 ` Tomasz Figa
0 siblings, 0 replies; 4+ messages in thread
From: Tomasz Figa @ 2018-10-25 7:30 UTC (permalink / raw)
To: vgarodia
Cc: mgottam, Stanimir Varbanov, Hans Verkuil, Mauro Carvalho Chehab,
Linux Media Mailing List, Linux Kernel Mailing List,
linux-arm-msm, Alexandre Courbot, linux-media-owner
On Thu, Oct 25, 2018 at 4:23 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> On 2018-10-24 20:02, Tomasz Figa wrote:
> > On Wed, Oct 24, 2018 at 10:52 PM Malathi Gottam
> > <mgottam@codeaurora.org> wrote:
> >>
> >> When client requests for a keyframe, set the property
> >> to hardware to generate the sync frame.
> >>
> >> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> >> ---
> >> drivers/media/platform/qcom/venus/venc_ctrls.c | 16 +++++++++++++++-
> >> 1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c
> >> b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >> index 45910172..6c2655d 100644
> >> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> >> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >> @@ -79,8 +79,10 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> >> {
> >> struct venus_inst *inst = ctrl_to_inst(ctrl);
> >> struct venc_controls *ctr = &inst->controls.enc;
> >> + struct hfi_enable en = { .enable = 1 };
> >> u32 bframes;
> >> int ret;
> >> + u32 ptype;
> >>
> >> switch (ctrl->id) {
> >> case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
> >> @@ -173,6 +175,15 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> >>
> >> ctr->num_b_frames = bframes;
> >> break;
> >> + case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
> >> + if (inst->streamon_out && inst->streamon_cap) {
> >> + ptype =
> >> HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
> >> + ret = hfi_session_set_property(inst, ptype,
> >> &en);
> >> +
> >> + if (ret)
> >> + return ret;
> >> + }
> >> + break;
> >
> > This is still not the right way to handle this.
> >
> > Please see the documentation of this control [1]:
> >
> > "V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME (button)
> > Force a key frame for the next queued buffer. Applicable to encoders.
> > This is a general, codec-agnostic keyframe control."
> >
> > Even if the driver is not streaming, it must remember that the
> > keyframe was requested for next buffer. The next time userspace QBUFs
> > an OUTPUT buffer, it should ask the hardware to encode that OUTPUT
> > buffer into a keyframe.
>
> That's correct. Driver can cache the client request and set it when the
> hardware
> is capable of accepting the property.
> Still the issue having the requested OUTPUT buffer to be encoded as sync
> frame will
> be there. If there are few frames queued before streamon, driver will
> only keep a
> note that it has to set the request for keyframe, but not the exact one
> which was
> requested.
The description (quoted above) specifies exactly that the control
applies only to the next queued buffer. It's a "button" control, so
when the application sets it (to 1), it triggers a call to driver's
s_ctrl callback and then resets to 0 automatically.
>
> > [1]
> > https://www.kernel.org/doc/html/latest/media/uapi/v4l/extended-controls.html?highlight=v4l2_cid_mpeg_video_force_key_frame
> >
> > But generally, the proper modern way for the userspace to request a
> > keyframe is to set the V4L2_BUF_FLAG_KEYFRAME flag in the
> > vb2_buffer_flag when queuing an OUTPUT buffer. It's the only
> > guaranteed way to ensure that the keyframe will be encoded exactly for
> > the selected frame. (The V4L2 control API doesn't guarantee any
> > synchronization between controls and buffers itself.)
>
> This is a better way to handle it to ensure exact buffer gets encoded as
> sync frame.
It was created later to solve this problem. For compatibility, we have
to keep supporting the control too.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-10-25 7:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-24 13:52 [PATCH v2] media: venus: add support for key frame Malathi Gottam
2018-10-24 14:32 ` Tomasz Figa
2018-10-25 7:23 ` Vikash Garodia
2018-10-25 7:30 ` Tomasz Figa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).