On Thu, Oct 25, 2018 at 4:23 PM Vikash Garodia <vgarodia@xxxxxxxxxxxxxx> wrote: > > On 2018-10-24 20:02, Tomasz Figa wrote: > > On Wed, Oct 24, 2018 at 10:52 PM Malathi Gottam > > <mgottam@xxxxxxxxxxxxxx> wrote: > >> > >> When client requests for a keyframe, set the property > >> to hardware to generate the sync frame. > >> > >> Signed-off-by: Malathi Gottam <mgottam@xxxxxxxxxxxxxx> > >> --- > >> 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