All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] Make Frame Skip Mode control a standard
@ 2020-07-05 12:11 ` Stanimir Varbanov
  0 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-05 12:11 UTC (permalink / raw)
  To: linux-media, linux-kernel, linux-arm-msm, linux-arm-kernel
  Cc: Kyungmin Park, Kamil Debski, Jeongtae Park, Andrzej Hajda,
	Hans Verkuil, Maheshwar Ajja, Stanimir Varbanov

Hello,

Suggested by Hans at [1], this patchset is promoting a standard control
for frame skip mode.

The original private V4L2_CID_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE control is
applicable and can be used by Venus driver too (and probably other drivers).
In order to make that possible make a new V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE
standard menu control (a copy of the private one).

Also, to keep mfc driver backward compatible kept the private one, and mark
it as depricated in docs.

regards,
Stan

[1] https://lkml.org/lkml/2020/5/19/122

Stanimir Varbanov (4):
  media: v4l2-ctrl: Add frame-skip std encoder control
  venus: venc: Add support for frame-skip mode v4l2 control
  media: s5p-mfc: Use standard frame skip mode control
  media: docs: Depricate mfc frame skip control

 .../media/v4l/ext-ctrls-codec.rst             | 37 +++++++++++++++++++
 drivers/media/platform/qcom/venus/core.h      |  1 +
 drivers/media/platform/qcom/venus/venc.c      |  6 ++-
 .../media/platform/qcom/venus/venc_ctrls.c    | 12 +++++-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c  |  6 +++
 drivers/media/v4l2-core/v4l2-ctrls.c          | 10 +++++
 include/uapi/linux/v4l2-controls.h            |  6 +++
 7 files changed, 75 insertions(+), 3 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 0/4] Make Frame Skip Mode control a standard
@ 2020-07-05 12:11 ` Stanimir Varbanov
  0 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-05 12:11 UTC (permalink / raw)
  To: linux-media, linux-kernel, linux-arm-msm, linux-arm-kernel
  Cc: Maheshwar Ajja, Andrzej Hajda, Kamil Debski, Stanimir Varbanov,
	Jeongtae Park, Kyungmin Park, Hans Verkuil

Hello,

Suggested by Hans at [1], this patchset is promoting a standard control
for frame skip mode.

The original private V4L2_CID_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE control is
applicable and can be used by Venus driver too (and probably other drivers).
In order to make that possible make a new V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE
standard menu control (a copy of the private one).

Also, to keep mfc driver backward compatible kept the private one, and mark
it as depricated in docs.

regards,
Stan

[1] https://lkml.org/lkml/2020/5/19/122

Stanimir Varbanov (4):
  media: v4l2-ctrl: Add frame-skip std encoder control
  venus: venc: Add support for frame-skip mode v4l2 control
  media: s5p-mfc: Use standard frame skip mode control
  media: docs: Depricate mfc frame skip control

 .../media/v4l/ext-ctrls-codec.rst             | 37 +++++++++++++++++++
 drivers/media/platform/qcom/venus/core.h      |  1 +
 drivers/media/platform/qcom/venus/venc.c      |  6 ++-
 .../media/platform/qcom/venus/venc_ctrls.c    | 12 +++++-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c  |  6 +++
 drivers/media/v4l2-core/v4l2-ctrls.c          | 10 +++++
 include/uapi/linux/v4l2-controls.h            |  6 +++
 7 files changed, 75 insertions(+), 3 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control
  2020-07-05 12:11 ` Stanimir Varbanov
@ 2020-07-05 12:11   ` Stanimir Varbanov
  -1 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-05 12:11 UTC (permalink / raw)
  To: linux-media, linux-kernel, linux-arm-msm, linux-arm-kernel
  Cc: Kyungmin Park, Kamil Debski, Jeongtae Park, Andrzej Hajda,
	Hans Verkuil, Maheshwar Ajja, Stanimir Varbanov

Adds encoders standard v4l2 control for frame-skip. The control
is a copy of a custom encoder control so that other v4l2 encoder
drivers can use it.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
 include/uapi/linux/v4l2-controls.h            |  6 ++++
 3 files changed, 48 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index d0d506a444b1..a8b4c0b40747 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -592,6 +592,38 @@ enum v4l2_mpeg_video_bitrate_mode -
     the average video bitrate. It is ignored if the video bitrate mode
     is set to constant bitrate.
 
+``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE (enum)``
+
+enum v4l2_mpeg_video_frame_skip_mode -
+    Indicates in what conditions the encoder should skip frames. If
+    encoding a frame would cause the encoded stream to be larger then a
+    chosen data limit then the frame will be skipped. Possible values
+    are:
+
+
+.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
+
+.. raw:: latex
+
+    \small
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
+      - Frame skip mode is disabled.
+    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
+      - Frame skip mode enabled and buffer limit is set by the chosen
+	level and is defined by the standard.
+    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
+      - Frame skip mode enabled and buffer limit is set by the VBV
+	(MPEG1/2/4) or CPB (H264) buffer size control.
+
+.. raw:: latex
+
+    \normalsize
+
 ``V4L2_CID_MPEG_VIDEO_TEMPORAL_DECIMATION (integer)``
     For every captured frame, skip this many subsequent frames (default
     0).
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 3f3fbcd60cc6..d088acfa6dd8 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		"External",
 		NULL,
 	};
+	static const char * const mpeg_video_frame_skip[] = {
+		"Disabled",
+		"Level Limit",
+		"VBV/CPB Limit",
+		NULL,
+	};
 
 	switch (id) {
 	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
@@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		return flash_strobe_source;
 	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
 		return header_mode;
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
+		return mpeg_video_frame_skip;
 	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
 		return multi_slice;
 	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
@@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
 	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:			return "Sequence Header Mode";
 	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
 	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
 	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
 	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
@@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_FLASH_LED_MODE:
 	case V4L2_CID_FLASH_STROBE_SOURCE:
 	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
 	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
 	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
 	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 62271418c1be..4e1526175a4c 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -742,6 +742,12 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
 #define V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR	(V4L2_CID_MPEG_BASE + 642)
 #define V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES	(V4L2_CID_MPEG_BASE + 643)
 #define V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR	(V4L2_CID_MPEG_BASE + 644)
+#define V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)
+enum v4l2_mpeg_video_frame_skip_mode {
+	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
+	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
+	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
+};
 
 /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
 #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control
@ 2020-07-05 12:11   ` Stanimir Varbanov
  0 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-05 12:11 UTC (permalink / raw)
  To: linux-media, linux-kernel, linux-arm-msm, linux-arm-kernel
  Cc: Maheshwar Ajja, Andrzej Hajda, Kamil Debski, Stanimir Varbanov,
	Jeongtae Park, Kyungmin Park, Hans Verkuil

Adds encoders standard v4l2 control for frame-skip. The control
is a copy of a custom encoder control so that other v4l2 encoder
drivers can use it.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
 include/uapi/linux/v4l2-controls.h            |  6 ++++
 3 files changed, 48 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index d0d506a444b1..a8b4c0b40747 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -592,6 +592,38 @@ enum v4l2_mpeg_video_bitrate_mode -
     the average video bitrate. It is ignored if the video bitrate mode
     is set to constant bitrate.
 
+``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE (enum)``
+
+enum v4l2_mpeg_video_frame_skip_mode -
+    Indicates in what conditions the encoder should skip frames. If
+    encoding a frame would cause the encoded stream to be larger then a
+    chosen data limit then the frame will be skipped. Possible values
+    are:
+
+
+.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
+
+.. raw:: latex
+
+    \small
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
+      - Frame skip mode is disabled.
+    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
+      - Frame skip mode enabled and buffer limit is set by the chosen
+	level and is defined by the standard.
+    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
+      - Frame skip mode enabled and buffer limit is set by the VBV
+	(MPEG1/2/4) or CPB (H264) buffer size control.
+
+.. raw:: latex
+
+    \normalsize
+
 ``V4L2_CID_MPEG_VIDEO_TEMPORAL_DECIMATION (integer)``
     For every captured frame, skip this many subsequent frames (default
     0).
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 3f3fbcd60cc6..d088acfa6dd8 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		"External",
 		NULL,
 	};
+	static const char * const mpeg_video_frame_skip[] = {
+		"Disabled",
+		"Level Limit",
+		"VBV/CPB Limit",
+		NULL,
+	};
 
 	switch (id) {
 	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
@@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		return flash_strobe_source;
 	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
 		return header_mode;
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
+		return mpeg_video_frame_skip;
 	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
 		return multi_slice;
 	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
@@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
 	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:			return "Sequence Header Mode";
 	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
 	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
 	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
 	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
@@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_FLASH_LED_MODE:
 	case V4L2_CID_FLASH_STROBE_SOURCE:
 	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
 	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
 	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
 	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 62271418c1be..4e1526175a4c 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -742,6 +742,12 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
 #define V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR	(V4L2_CID_MPEG_BASE + 642)
 #define V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES	(V4L2_CID_MPEG_BASE + 643)
 #define V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR	(V4L2_CID_MPEG_BASE + 644)
+#define V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)
+enum v4l2_mpeg_video_frame_skip_mode {
+	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
+	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
+	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
+};
 
 /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
 #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/4] venus: venc: Add support for frame-skip mode v4l2 control
  2020-07-05 12:11 ` Stanimir Varbanov
@ 2020-07-05 12:11   ` Stanimir Varbanov
  -1 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-05 12:11 UTC (permalink / raw)
  To: linux-media, linux-kernel, linux-arm-msm, linux-arm-kernel
  Cc: Kyungmin Park, Kamil Debski, Jeongtae Park, Andrzej Hajda,
	Hans Verkuil, Maheshwar Ajja, Stanimir Varbanov

This adds support for frame-skip-mode standard v4l2 control in
encoder driver. The control is implemented based on the
combination of client selected bitrate-mode and frame-skip-mode.
Once The client selected bitrate-mode (constant or variable) and
the frame-skip-mode is not disabled we set variable framerate for
rate controller.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/core.h       |  1 +
 drivers/media/platform/qcom/venus/venc.c       |  6 ++++--
 drivers/media/platform/qcom/venus/venc_ctrls.c | 12 +++++++++++-
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 7118612673c9..5e74d0441592 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -201,6 +201,7 @@ struct venc_controls {
 	u32 bitrate;
 	u32 bitrate_peak;
 	u32 rc_enable;
+	u32 frame_skip_mode;
 
 	u32 h264_i_period;
 	u32 h264_entropy_mode;
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 513bbc07f7bc..2279596d4d60 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -739,9 +739,11 @@ static int venc_set_properties(struct venus_inst *inst)
 	if (!ctr->rc_enable)
 		rate_control = HFI_RATE_CONTROL_OFF;
 	else if (ctr->bitrate_mode == V4L2_MPEG_VIDEO_BITRATE_MODE_VBR)
-		rate_control = HFI_RATE_CONTROL_VBR_CFR;
+		rate_control = ctr->frame_skip_mode ? HFI_RATE_CONTROL_VBR_VFR :
+						      HFI_RATE_CONTROL_VBR_CFR;
 	else
-		rate_control = HFI_RATE_CONTROL_CBR_CFR;
+		rate_control = ctr->frame_skip_mode ? HFI_RATE_CONTROL_CBR_VFR :
+						      HFI_RATE_CONTROL_CBR_CFR;
 
 	ptype = HFI_PROPERTY_PARAM_VENC_RATE_CONTROL;
 	ret = hfi_session_set_property(inst, ptype, &rate_control);
diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 8362dde7949e..a418d7d6db0c 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -202,6 +202,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
 		ctr->rc_enable = ctrl->val;
 		break;
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
+		ctr->frame_skip_mode = ctrl->val;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -217,7 +220,7 @@ int venc_ctrl_init(struct venus_inst *inst)
 {
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 31);
+	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 32);
 	if (ret)
 		return ret;
 
@@ -357,6 +360,13 @@ int venc_ctrl_init(struct venus_inst *inst)
 	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
 			  V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE, 0, 1, 1, 1);
 
+	v4l2_ctrl_new_std_menu(&inst->ctrl_handler, &venc_ctrl_ops,
+			V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE,
+			V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT,
+			~((1 << V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED) |
+			  (1 << V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT)),
+			V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED);
+
 	ret = inst->ctrl_handler.error;
 	if (ret)
 		goto err;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/4] venus: venc: Add support for frame-skip mode v4l2 control
@ 2020-07-05 12:11   ` Stanimir Varbanov
  0 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-05 12:11 UTC (permalink / raw)
  To: linux-media, linux-kernel, linux-arm-msm, linux-arm-kernel
  Cc: Maheshwar Ajja, Andrzej Hajda, Kamil Debski, Stanimir Varbanov,
	Jeongtae Park, Kyungmin Park, Hans Verkuil

This adds support for frame-skip-mode standard v4l2 control in
encoder driver. The control is implemented based on the
combination of client selected bitrate-mode and frame-skip-mode.
Once The client selected bitrate-mode (constant or variable) and
the frame-skip-mode is not disabled we set variable framerate for
rate controller.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/core.h       |  1 +
 drivers/media/platform/qcom/venus/venc.c       |  6 ++++--
 drivers/media/platform/qcom/venus/venc_ctrls.c | 12 +++++++++++-
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 7118612673c9..5e74d0441592 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -201,6 +201,7 @@ struct venc_controls {
 	u32 bitrate;
 	u32 bitrate_peak;
 	u32 rc_enable;
+	u32 frame_skip_mode;
 
 	u32 h264_i_period;
 	u32 h264_entropy_mode;
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 513bbc07f7bc..2279596d4d60 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -739,9 +739,11 @@ static int venc_set_properties(struct venus_inst *inst)
 	if (!ctr->rc_enable)
 		rate_control = HFI_RATE_CONTROL_OFF;
 	else if (ctr->bitrate_mode == V4L2_MPEG_VIDEO_BITRATE_MODE_VBR)
-		rate_control = HFI_RATE_CONTROL_VBR_CFR;
+		rate_control = ctr->frame_skip_mode ? HFI_RATE_CONTROL_VBR_VFR :
+						      HFI_RATE_CONTROL_VBR_CFR;
 	else
-		rate_control = HFI_RATE_CONTROL_CBR_CFR;
+		rate_control = ctr->frame_skip_mode ? HFI_RATE_CONTROL_CBR_VFR :
+						      HFI_RATE_CONTROL_CBR_CFR;
 
 	ptype = HFI_PROPERTY_PARAM_VENC_RATE_CONTROL;
 	ret = hfi_session_set_property(inst, ptype, &rate_control);
diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 8362dde7949e..a418d7d6db0c 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -202,6 +202,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE:
 		ctr->rc_enable = ctrl->val;
 		break;
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
+		ctr->frame_skip_mode = ctrl->val;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -217,7 +220,7 @@ int venc_ctrl_init(struct venus_inst *inst)
 {
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 31);
+	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 32);
 	if (ret)
 		return ret;
 
@@ -357,6 +360,13 @@ int venc_ctrl_init(struct venus_inst *inst)
 	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
 			  V4L2_CID_MPEG_VIDEO_FRAME_RC_ENABLE, 0, 1, 1, 1);
 
+	v4l2_ctrl_new_std_menu(&inst->ctrl_handler, &venc_ctrl_ops,
+			V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE,
+			V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT,
+			~((1 << V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED) |
+			  (1 << V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT)),
+			V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED);
+
 	ret = inst->ctrl_handler.error;
 	if (ret)
 		goto err;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 3/4] media: s5p-mfc: Use standard frame skip mode control
  2020-07-05 12:11 ` Stanimir Varbanov
@ 2020-07-05 12:11   ` Stanimir Varbanov
  -1 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-05 12:11 UTC (permalink / raw)
  To: linux-media, linux-kernel, linux-arm-msm, linux-arm-kernel
  Cc: Kyungmin Park, Kamil Debski, Jeongtae Park, Andrzej Hajda,
	Hans Verkuil, Maheshwar Ajja, Stanimir Varbanov

Use the standard menu control for frame skip mode in the MFC
driver. The legacy private menu control is kept for backward
compatibility.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 912fe0c5ab18..3092eb6777a5 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -261,6 +261,11 @@ static struct mfc_control controls[] = {
 		.menu_skip_mask = 0,
 		.default_value = V4L2_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE_DISABLED,
 	},
+	{
+		.id = V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE,
+		.maximum = V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT,
+		.default_value = V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED,
+	},
 	{
 		.id = V4L2_CID_MPEG_MFC51_VIDEO_RC_FIXED_TARGET_BIT,
 		.type = V4L2_CTRL_TYPE_BOOLEAN,
@@ -1849,6 +1854,7 @@ static int s5p_mfc_enc_s_ctrl(struct v4l2_ctrl *ctrl)
 		p->seq_hdr_mode = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE:
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
 		p->frame_skip_mode = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_MFC51_VIDEO_RC_FIXED_TARGET_BIT:
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 3/4] media: s5p-mfc: Use standard frame skip mode control
@ 2020-07-05 12:11   ` Stanimir Varbanov
  0 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-05 12:11 UTC (permalink / raw)
  To: linux-media, linux-kernel, linux-arm-msm, linux-arm-kernel
  Cc: Maheshwar Ajja, Andrzej Hajda, Kamil Debski, Stanimir Varbanov,
	Jeongtae Park, Kyungmin Park, Hans Verkuil

Use the standard menu control for frame skip mode in the MFC
driver. The legacy private menu control is kept for backward
compatibility.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 912fe0c5ab18..3092eb6777a5 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -261,6 +261,11 @@ static struct mfc_control controls[] = {
 		.menu_skip_mask = 0,
 		.default_value = V4L2_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE_DISABLED,
 	},
+	{
+		.id = V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE,
+		.maximum = V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT,
+		.default_value = V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED,
+	},
 	{
 		.id = V4L2_CID_MPEG_MFC51_VIDEO_RC_FIXED_TARGET_BIT,
 		.type = V4L2_CTRL_TYPE_BOOLEAN,
@@ -1849,6 +1854,7 @@ static int s5p_mfc_enc_s_ctrl(struct v4l2_ctrl *ctrl)
 		p->seq_hdr_mode = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE:
+	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
 		p->frame_skip_mode = ctrl->val;
 		break;
 	case V4L2_CID_MPEG_MFC51_VIDEO_RC_FIXED_TARGET_BIT:
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 4/4] media: docs: Depricate mfc frame skip control
  2020-07-05 12:11 ` Stanimir Varbanov
@ 2020-07-05 12:11   ` Stanimir Varbanov
  -1 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-05 12:11 UTC (permalink / raw)
  To: linux-media, linux-kernel, linux-arm-msm, linux-arm-kernel
  Cc: Kyungmin Park, Kamil Debski, Jeongtae Park, Andrzej Hajda,
	Hans Verkuil, Maheshwar Ajja, Stanimir Varbanov

Depricate mfc private frame skip mode control for new
clients and use the standard one instead.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index a8b4c0b40747..c0760bfc54d4 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -2805,6 +2805,11 @@ MFC 5.1 Control IDs
 ``V4L2_CID_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE``
     (enum)
 
+    .. note::
+
+       This control is depricated. Use the standard one
+       ``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE`` instead.
+
 enum v4l2_mpeg_mfc51_video_frame_skip_mode -
     Indicates in what conditions the encoder should skip frames. If
     encoding a frame would cause the encoded stream to be larger then a
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 4/4] media: docs: Depricate mfc frame skip control
@ 2020-07-05 12:11   ` Stanimir Varbanov
  0 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-05 12:11 UTC (permalink / raw)
  To: linux-media, linux-kernel, linux-arm-msm, linux-arm-kernel
  Cc: Maheshwar Ajja, Andrzej Hajda, Kamil Debski, Stanimir Varbanov,
	Jeongtae Park, Kyungmin Park, Hans Verkuil

Depricate mfc private frame skip mode control for new
clients and use the standard one instead.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index a8b4c0b40747..c0760bfc54d4 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -2805,6 +2805,11 @@ MFC 5.1 Control IDs
 ``V4L2_CID_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE``
     (enum)
 
+    .. note::
+
+       This control is depricated. Use the standard one
+       ``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE`` instead.
+
 enum v4l2_mpeg_mfc51_video_frame_skip_mode -
     Indicates in what conditions the encoder should skip frames. If
     encoding a frame would cause the encoded stream to be larger then a
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] media: docs: Depricate mfc frame skip control
  2020-07-05 12:11   ` Stanimir Varbanov
@ 2020-07-05 15:29     ` Randy Dunlap
  -1 siblings, 0 replies; 24+ messages in thread
From: Randy Dunlap @ 2020-07-05 15:29 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media, linux-kernel, linux-arm-msm,
	linux-arm-kernel
  Cc: Kyungmin Park, Kamil Debski, Jeongtae Park, Andrzej Hajda,
	Hans Verkuil, Maheshwar Ajja

On 7/5/20 5:11 AM, Stanimir Varbanov wrote:
> Depricate mfc private frame skip mode control for new

  Deprecate

Also in $subject.

> clients and use the standard one instead.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index a8b4c0b40747..c0760bfc54d4 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -2805,6 +2805,11 @@ MFC 5.1 Control IDs
>  ``V4L2_CID_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE``
>      (enum)
>  
> +    .. note::
> +
> +       This control is depricated. Use the standard one

                          deprecated.


> +       ``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE`` instead.
> +
>  enum v4l2_mpeg_mfc51_video_frame_skip_mode -
>      Indicates in what conditions the encoder should skip frames. If
>      encoding a frame would cause the encoded stream to be larger then a
> 


-- 
~Randy


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] media: docs: Depricate mfc frame skip control
@ 2020-07-05 15:29     ` Randy Dunlap
  0 siblings, 0 replies; 24+ messages in thread
From: Randy Dunlap @ 2020-07-05 15:29 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media, linux-kernel, linux-arm-msm,
	linux-arm-kernel
  Cc: Maheshwar Ajja, Andrzej Hajda, Kamil Debski, Jeongtae Park,
	Kyungmin Park, Hans Verkuil

On 7/5/20 5:11 AM, Stanimir Varbanov wrote:
> Depricate mfc private frame skip mode control for new

  Deprecate

Also in $subject.

> clients and use the standard one instead.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index a8b4c0b40747..c0760bfc54d4 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -2805,6 +2805,11 @@ MFC 5.1 Control IDs
>  ``V4L2_CID_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE``
>      (enum)
>  
> +    .. note::
> +
> +       This control is depricated. Use the standard one

                          deprecated.


> +       ``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE`` instead.
> +
>  enum v4l2_mpeg_mfc51_video_frame_skip_mode -
>      Indicates in what conditions the encoder should skip frames. If
>      encoding a frame would cause the encoded stream to be larger then a
> 


-- 
~Randy


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control
  2020-07-05 12:11   ` Stanimir Varbanov
@ 2020-07-07 20:53     ` Nicolas Dufresne
  -1 siblings, 0 replies; 24+ messages in thread
From: Nicolas Dufresne @ 2020-07-07 20:53 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media, linux-kernel, linux-arm-msm,
	linux-arm-kernel
  Cc: Kyungmin Park, Kamil Debski, Jeongtae Park, Andrzej Hajda,
	Hans Verkuil, Maheshwar Ajja

Le dimanche 05 juillet 2020 à 15:11 +0300, Stanimir Varbanov a écrit :
> Adds encoders standard v4l2 control for frame-skip. The control
> is a copy of a custom encoder control so that other v4l2 encoder
> drivers can use it.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
>  include/uapi/linux/v4l2-controls.h            |  6 ++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index d0d506a444b1..a8b4c0b40747 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -592,6 +592,38 @@ enum v4l2_mpeg_video_bitrate_mode -
>      the average video bitrate. It is ignored if the video bitrate mode
>      is set to constant bitrate.
>  
> +``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE (enum)``
> +
> +enum v4l2_mpeg_video_frame_skip_mode -
> +    Indicates in what conditions the encoder should skip frames. If
> +    encoding a frame would cause the encoded stream to be larger then a
> +    chosen data limit then the frame will be skipped. Possible values
> +    are:

I have nothing against this API, in fact it's really nice to generalize
as this is very common. Though, I think we are missing two things. This
documentation refer to the "chosen data limit". Is there controls to
configure these *chosen* limit ? The other issue is the vagueness of
the documented mode, see lower...

> +
> +
> +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
> +
> +.. raw:: latex
> +
> +    \small
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
> +      - Frame skip mode is disabled.
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
> +      - Frame skip mode enabled and buffer limit is set by the chosen
> +	level and is defined by the standard.

At least for H.264, a level is compose of 3 limits. One is the maximum
number of macroblocks, this is is evidently not use for frame skipping
and already constrained in V4L2 (assuming the driver does not ignore
the level control of course). The two other limits are decoded
macroblocks/s and encoded kbits/s. Both are measure over time, which
means the M2M encoder needs to be timing aware. I think the time source
should be documented. Perhaps it is mandatory to set a frame interval
for this to work ? Or we need some timestamp to allow variable frame
interval ? (I don't think the second is really an option without
extending the API again, and confusingly, since I think we have used
the timestamp for other purpose already)

> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
> +      - Frame skip mode enabled and buffer limit is set by the VBV
> +	(MPEG1/2/4) or CPB (H264) buffer size control.

The notion of VBV an CPB is unlikely well known. If my memory is right,
these are constrained in buffering: in bytes (VBV) or bits per frame
over a window of n-frames (or the gop size for some less flexible
encoder) (CPB). I think these should be somehow chosen by application
(with controls), directly or indirectly, and documented here to ensure
we get consistent implementation across drivers.

> +
> +.. raw:: latex
> +
> +    \normalsize
> +
>  ``V4L2_CID_MPEG_VIDEO_TEMPORAL_DECIMATION (integer)``
>      For every captured frame, skip this many subsequent frames (default
>      0).
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 3f3fbcd60cc6..d088acfa6dd8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"External",
>  		NULL,
>  	};
> +	static const char * const mpeg_video_frame_skip[] = {
> +		"Disabled",
> +		"Level Limit",
> +		"VBV/CPB Limit",
> +		NULL,
> +	};
>  
>  	switch (id) {
>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> @@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return flash_strobe_source;
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
>  		return header_mode;
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
> +		return mpeg_video_frame_skip;
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  		return multi_slice;
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
> @@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:			return "Sequence Header Mode";
>  	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
>  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
> @@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_FLASH_LED_MODE:
>  	case V4L2_CID_FLASH_STROBE_SOURCE:
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 62271418c1be..4e1526175a4c 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -742,6 +742,12 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
>  #define V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR	(V4L2_CID_MPEG_BASE + 642)
>  #define V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES	(V4L2_CID_MPEG_BASE + 643)
>  #define V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR	(V4L2_CID_MPEG_BASE + 644)
> +#define V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)
> +enum v4l2_mpeg_video_frame_skip_mode {
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
> +};
>  
>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control
@ 2020-07-07 20:53     ` Nicolas Dufresne
  0 siblings, 0 replies; 24+ messages in thread
From: Nicolas Dufresne @ 2020-07-07 20:53 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media, linux-kernel, linux-arm-msm,
	linux-arm-kernel
  Cc: Maheshwar Ajja, Andrzej Hajda, Kamil Debski, Jeongtae Park,
	Kyungmin Park, Hans Verkuil

Le dimanche 05 juillet 2020 à 15:11 +0300, Stanimir Varbanov a écrit :
> Adds encoders standard v4l2 control for frame-skip. The control
> is a copy of a custom encoder control so that other v4l2 encoder
> drivers can use it.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
>  include/uapi/linux/v4l2-controls.h            |  6 ++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index d0d506a444b1..a8b4c0b40747 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -592,6 +592,38 @@ enum v4l2_mpeg_video_bitrate_mode -
>      the average video bitrate. It is ignored if the video bitrate mode
>      is set to constant bitrate.
>  
> +``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE (enum)``
> +
> +enum v4l2_mpeg_video_frame_skip_mode -
> +    Indicates in what conditions the encoder should skip frames. If
> +    encoding a frame would cause the encoded stream to be larger then a
> +    chosen data limit then the frame will be skipped. Possible values
> +    are:

I have nothing against this API, in fact it's really nice to generalize
as this is very common. Though, I think we are missing two things. This
documentation refer to the "chosen data limit". Is there controls to
configure these *chosen* limit ? The other issue is the vagueness of
the documented mode, see lower...

> +
> +
> +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
> +
> +.. raw:: latex
> +
> +    \small
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
> +      - Frame skip mode is disabled.
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
> +      - Frame skip mode enabled and buffer limit is set by the chosen
> +	level and is defined by the standard.

At least for H.264, a level is compose of 3 limits. One is the maximum
number of macroblocks, this is is evidently not use for frame skipping
and already constrained in V4L2 (assuming the driver does not ignore
the level control of course). The two other limits are decoded
macroblocks/s and encoded kbits/s. Both are measure over time, which
means the M2M encoder needs to be timing aware. I think the time source
should be documented. Perhaps it is mandatory to set a frame interval
for this to work ? Or we need some timestamp to allow variable frame
interval ? (I don't think the second is really an option without
extending the API again, and confusingly, since I think we have used
the timestamp for other purpose already)

> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
> +      - Frame skip mode enabled and buffer limit is set by the VBV
> +	(MPEG1/2/4) or CPB (H264) buffer size control.

The notion of VBV an CPB is unlikely well known. If my memory is right,
these are constrained in buffering: in bytes (VBV) or bits per frame
over a window of n-frames (or the gop size for some less flexible
encoder) (CPB). I think these should be somehow chosen by application
(with controls), directly or indirectly, and documented here to ensure
we get consistent implementation across drivers.

> +
> +.. raw:: latex
> +
> +    \normalsize
> +
>  ``V4L2_CID_MPEG_VIDEO_TEMPORAL_DECIMATION (integer)``
>      For every captured frame, skip this many subsequent frames (default
>      0).
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 3f3fbcd60cc6..d088acfa6dd8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"External",
>  		NULL,
>  	};
> +	static const char * const mpeg_video_frame_skip[] = {
> +		"Disabled",
> +		"Level Limit",
> +		"VBV/CPB Limit",
> +		NULL,
> +	};
>  
>  	switch (id) {
>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> @@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return flash_strobe_source;
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
>  		return header_mode;
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
> +		return mpeg_video_frame_skip;
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  		return multi_slice;
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
> @@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:			return "Sequence Header Mode";
>  	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
>  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
> @@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_FLASH_LED_MODE:
>  	case V4L2_CID_FLASH_STROBE_SOURCE:
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 62271418c1be..4e1526175a4c 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -742,6 +742,12 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
>  #define V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR	(V4L2_CID_MPEG_BASE + 642)
>  #define V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES	(V4L2_CID_MPEG_BASE + 643)
>  #define V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR	(V4L2_CID_MPEG_BASE + 644)
> +#define V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)
> +enum v4l2_mpeg_video_frame_skip_mode {
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
> +};
>  
>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control
  2020-07-07 20:53     ` Nicolas Dufresne
@ 2020-07-15 15:42       ` Stanimir Varbanov
  -1 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-15 15:42 UTC (permalink / raw)
  To: Nicolas Dufresne, Stanimir Varbanov, linux-media, linux-kernel,
	linux-arm-msm, linux-arm-kernel
  Cc: Kyungmin Park, Kamil Debski, Jeongtae Park, Andrzej Hajda,
	Hans Verkuil, Maheshwar Ajja

Hi Nicolas,

On 7/7/20 11:53 PM, Nicolas Dufresne wrote:
> Le dimanche 05 juillet 2020 à 15:11 +0300, Stanimir Varbanov a écrit :
>> Adds encoders standard v4l2 control for frame-skip. The control
>> is a copy of a custom encoder control so that other v4l2 encoder
>> drivers can use it.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
>>  include/uapi/linux/v4l2-controls.h            |  6 ++++
>>  3 files changed, 48 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> index d0d506a444b1..a8b4c0b40747 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -592,6 +592,38 @@ enum v4l2_mpeg_video_bitrate_mode -
>>      the average video bitrate. It is ignored if the video bitrate mode
>>      is set to constant bitrate.
>>  
>> +``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE (enum)``
>> +
>> +enum v4l2_mpeg_video_frame_skip_mode -
>> +    Indicates in what conditions the encoder should skip frames. If
>> +    encoding a frame would cause the encoded stream to be larger then a
>> +    chosen data limit then the frame will be skipped. Possible values
>> +    are:
> 
> I have nothing against this API, in fact it's really nice to generalize
> as this is very common. Though, I think we are missing two things. This
> documentation refer to the "chosen data limit". Is there controls to
> configure these *chosen* limit ? The other issue is the vagueness of
> the documented mode, see lower...
> 
>> +
>> +
>> +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
>> +
>> +.. raw:: latex
>> +
>> +    \small
>> +
>> +.. flat-table::
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
>> +      - Frame skip mode is disabled.
>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
>> +      - Frame skip mode enabled and buffer limit is set by the chosen
>> +	level and is defined by the standard.
> 
> At least for H.264, a level is compose of 3 limits. One is the maximum
> number of macroblocks, this is is evidently not use for frame skipping
> and already constrained in V4L2 (assuming the driver does not ignore
> the level control of course). The two other limits are decoded
> macroblocks/s and encoded kbits/s. Both are measure over time, which
> means the M2M encoder needs to be timing aware. I think the time source
> should be documented. Perhaps it is mandatory to set a frame interval
> for this to work ? Or we need some timestamp to allow variable frame
> interval ? (I don't think the second is really an option without
> extending the API again, and confusingly, since I think we have used
> the timestamp for other purpose already)

Do you want to say that the encoder input timestamp, bitrate control
(V4L2_CID_MPEG_VIDEO_BITRATE) and S_PARM is not enough to describe
FRAME_SKIP_MODE_LEVEL_LIMIT mode?

> 
>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
>> +      - Frame skip mode enabled and buffer limit is set by the VBV
>> +	(MPEG1/2/4) or CPB (H264) buffer size control.
> 
> The notion of VBV an CPB is unlikely well known. If my memory is right,
> these are constrained in buffering: in bytes (VBV) or bits per frame
> over a window of n-frames (or the gop size for some less flexible
> encoder) (CPB). I think these should be somehow chosen by application
> (with controls), directly or indirectly, and documented here to ensure
> we get consistent implementation across drivers.

I guess you want me to add here references to the following controls:

V4L2_CID_MPEG_VIDEO_VBV_SIZE
V4L2_CID_MPEG_VIDEO_VBV_DELAY
V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE

?

> 
>> +
>> +.. raw:: latex
>> +
>> +    \normalsize
>> +
>>  ``V4L2_CID_MPEG_VIDEO_TEMPORAL_DECIMATION (integer)``
>>      For every captured frame, skip this many subsequent frames (default
>>      0).
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 3f3fbcd60cc6..d088acfa6dd8 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>  		"External",
>>  		NULL,
>>  	};
>> +	static const char * const mpeg_video_frame_skip[] = {
>> +		"Disabled",
>> +		"Level Limit",
>> +		"VBV/CPB Limit",
>> +		NULL,
>> +	};
>>  
>>  	switch (id) {
>>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
>> @@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>  		return flash_strobe_source;
>>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
>>  		return header_mode;
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>> +		return mpeg_video_frame_skip;
>>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>>  		return multi_slice;
>>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>> @@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
>>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:			return "Sequence Header Mode";
>>  	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
>>  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
>>  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
>>  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
>> @@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>  	case V4L2_CID_FLASH_LED_MODE:
>>  	case V4L2_CID_FLASH_STROBE_SOURCE:
>>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>>  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 62271418c1be..4e1526175a4c 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -742,6 +742,12 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
>>  #define V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR	(V4L2_CID_MPEG_BASE + 642)
>>  #define V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES	(V4L2_CID_MPEG_BASE + 643)
>>  #define V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR	(V4L2_CID_MPEG_BASE + 644)
>> +#define V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)
>> +enum v4l2_mpeg_video_frame_skip_mode {
>> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
>> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
>> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
>> +};
>>  
>>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>>  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
> 

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control
@ 2020-07-15 15:42       ` Stanimir Varbanov
  0 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-15 15:42 UTC (permalink / raw)
  To: Nicolas Dufresne, Stanimir Varbanov, linux-media, linux-kernel,
	linux-arm-msm, linux-arm-kernel
  Cc: Maheshwar Ajja, Andrzej Hajda, Kamil Debski, Jeongtae Park,
	Kyungmin Park, Hans Verkuil

Hi Nicolas,

On 7/7/20 11:53 PM, Nicolas Dufresne wrote:
> Le dimanche 05 juillet 2020 à 15:11 +0300, Stanimir Varbanov a écrit :
>> Adds encoders standard v4l2 control for frame-skip. The control
>> is a copy of a custom encoder control so that other v4l2 encoder
>> drivers can use it.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
>>  include/uapi/linux/v4l2-controls.h            |  6 ++++
>>  3 files changed, 48 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> index d0d506a444b1..a8b4c0b40747 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -592,6 +592,38 @@ enum v4l2_mpeg_video_bitrate_mode -
>>      the average video bitrate. It is ignored if the video bitrate mode
>>      is set to constant bitrate.
>>  
>> +``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE (enum)``
>> +
>> +enum v4l2_mpeg_video_frame_skip_mode -
>> +    Indicates in what conditions the encoder should skip frames. If
>> +    encoding a frame would cause the encoded stream to be larger then a
>> +    chosen data limit then the frame will be skipped. Possible values
>> +    are:
> 
> I have nothing against this API, in fact it's really nice to generalize
> as this is very common. Though, I think we are missing two things. This
> documentation refer to the "chosen data limit". Is there controls to
> configure these *chosen* limit ? The other issue is the vagueness of
> the documented mode, see lower...
> 
>> +
>> +
>> +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
>> +
>> +.. raw:: latex
>> +
>> +    \small
>> +
>> +.. flat-table::
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
>> +      - Frame skip mode is disabled.
>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
>> +      - Frame skip mode enabled and buffer limit is set by the chosen
>> +	level and is defined by the standard.
> 
> At least for H.264, a level is compose of 3 limits. One is the maximum
> number of macroblocks, this is is evidently not use for frame skipping
> and already constrained in V4L2 (assuming the driver does not ignore
> the level control of course). The two other limits are decoded
> macroblocks/s and encoded kbits/s. Both are measure over time, which
> means the M2M encoder needs to be timing aware. I think the time source
> should be documented. Perhaps it is mandatory to set a frame interval
> for this to work ? Or we need some timestamp to allow variable frame
> interval ? (I don't think the second is really an option without
> extending the API again, and confusingly, since I think we have used
> the timestamp for other purpose already)

Do you want to say that the encoder input timestamp, bitrate control
(V4L2_CID_MPEG_VIDEO_BITRATE) and S_PARM is not enough to describe
FRAME_SKIP_MODE_LEVEL_LIMIT mode?

> 
>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
>> +      - Frame skip mode enabled and buffer limit is set by the VBV
>> +	(MPEG1/2/4) or CPB (H264) buffer size control.
> 
> The notion of VBV an CPB is unlikely well known. If my memory is right,
> these are constrained in buffering: in bytes (VBV) or bits per frame
> over a window of n-frames (or the gop size for some less flexible
> encoder) (CPB). I think these should be somehow chosen by application
> (with controls), directly or indirectly, and documented here to ensure
> we get consistent implementation across drivers.

I guess you want me to add here references to the following controls:

V4L2_CID_MPEG_VIDEO_VBV_SIZE
V4L2_CID_MPEG_VIDEO_VBV_DELAY
V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE

?

> 
>> +
>> +.. raw:: latex
>> +
>> +    \normalsize
>> +
>>  ``V4L2_CID_MPEG_VIDEO_TEMPORAL_DECIMATION (integer)``
>>      For every captured frame, skip this many subsequent frames (default
>>      0).
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 3f3fbcd60cc6..d088acfa6dd8 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>  		"External",
>>  		NULL,
>>  	};
>> +	static const char * const mpeg_video_frame_skip[] = {
>> +		"Disabled",
>> +		"Level Limit",
>> +		"VBV/CPB Limit",
>> +		NULL,
>> +	};
>>  
>>  	switch (id) {
>>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
>> @@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>  		return flash_strobe_source;
>>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
>>  		return header_mode;
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>> +		return mpeg_video_frame_skip;
>>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>>  		return multi_slice;
>>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>> @@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
>>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:			return "Sequence Header Mode";
>>  	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
>>  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
>>  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
>>  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
>> @@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>  	case V4L2_CID_FLASH_LED_MODE:
>>  	case V4L2_CID_FLASH_STROBE_SOURCE:
>>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>>  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 62271418c1be..4e1526175a4c 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -742,6 +742,12 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
>>  #define V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR	(V4L2_CID_MPEG_BASE + 642)
>>  #define V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES	(V4L2_CID_MPEG_BASE + 643)
>>  #define V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR	(V4L2_CID_MPEG_BASE + 644)
>> +#define V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)
>> +enum v4l2_mpeg_video_frame_skip_mode {
>> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
>> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
>> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
>> +};
>>  
>>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>>  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
> 

-- 
regards,
Stan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control
  2020-07-15 15:42       ` Stanimir Varbanov
@ 2020-07-15 18:12         ` Nicolas Dufresne
  -1 siblings, 0 replies; 24+ messages in thread
From: Nicolas Dufresne @ 2020-07-15 18:12 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media, linux-kernel, linux-arm-msm,
	linux-arm-kernel
  Cc: Kyungmin Park, Kamil Debski, Jeongtae Park, Andrzej Hajda,
	Hans Verkuil, Maheshwar Ajja

Le mercredi 15 juillet 2020 à 18:42 +0300, Stanimir Varbanov a écrit :
> Hi Nicolas,
> 
> On 7/7/20 11:53 PM, Nicolas Dufresne wrote:
> > Le dimanche 05 juillet 2020 à 15:11 +0300, Stanimir Varbanov a écrit :
> > > Adds encoders standard v4l2 control for frame-skip. The control
> > > is a copy of a custom encoder control so that other v4l2 encoder
> > > drivers can use it.
> > > 
> > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> > > ---
> > >  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
> > >  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
> > >  include/uapi/linux/v4l2-controls.h            |  6 ++++
> > >  3 files changed, 48 insertions(+)
> > > 
> > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > index d0d506a444b1..a8b4c0b40747 100644
> > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > @@ -592,6 +592,38 @@ enum v4l2_mpeg_video_bitrate_mode -
> > >      the average video bitrate. It is ignored if the video bitrate mode
> > >      is set to constant bitrate.
> > >  
> > > +``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE (enum)``
> > > +
> > > +enum v4l2_mpeg_video_frame_skip_mode -
> > > +    Indicates in what conditions the encoder should skip frames. If
> > > +    encoding a frame would cause the encoded stream to be larger then a
> > > +    chosen data limit then the frame will be skipped. Possible values
> > > +    are:
> > 
> > I have nothing against this API, in fact it's really nice to generalize
> > as this is very common. Though, I think we are missing two things. This
> > documentation refer to the "chosen data limit". Is there controls to
> > configure these *chosen* limit ? The other issue is the vagueness of
> > the documented mode, see lower...
> > 
> > > +
> > > +
> > > +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
> > > +
> > > +.. raw:: latex
> > > +
> > > +    \small
> > > +
> > > +.. flat-table::
> > > +    :header-rows:  0
> > > +    :stub-columns: 0
> > > +
> > > +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
> > > +      - Frame skip mode is disabled.
> > > +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
> > > +      - Frame skip mode enabled and buffer limit is set by the chosen
> > > +	level and is defined by the standard.
> > 
> > At least for H.264, a level is compose of 3 limits. One is the maximum
> > number of macroblocks, this is is evidently not use for frame skipping
> > and already constrained in V4L2 (assuming the driver does not ignore
> > the level control of course). The two other limits are decoded
> > macroblocks/s and encoded kbits/s. Both are measure over time, which
> > means the M2M encoder needs to be timing aware. I think the time source
> > should be documented. Perhaps it is mandatory to set a frame interval
> > for this to work ? Or we need some timestamp to allow variable frame
> > interval ? (I don't think the second is really an option without
> > extending the API again, and confusingly, since I think we have used
> > the timestamp for other purpose already)
> 
> Do you want to say that the encoder input timestamp, bitrate control
> (V4L2_CID_MPEG_VIDEO_BITRATE) and S_PARM is not enough to describe
> FRAME_SKIP_MODE_LEVEL_LIMIT mode?

I don't think we have spec to give the input timestamp a meaning that
driver can interpret. In fact I think we gave it a meaning that the
driver must not interpret it (aka driver opaque). So remain S_PARM to
give a clue, but some stream don't have a framerate (like RTP streams,
unless written in bitstream).

> 
> > > +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
> > > +      - Frame skip mode enabled and buffer limit is set by the VBV
> > > +	(MPEG1/2/4) or CPB (H264) buffer size control.
> > 
> > The notion of VBV an CPB is unlikely well known. If my memory is right,
> > these are constrained in buffering: in bytes (VBV) or bits per frame
> > over a window of n-frames (or the gop size for some less flexible
> > encoder) (CPB). I think these should be somehow chosen by application
> > (with controls), directly or indirectly, and documented here to ensure
> > we get consistent implementation across drivers.
> 
> I guess you want me to add here references to the following controls:
> 
> V4L2_CID_MPEG_VIDEO_VBV_SIZE
> V4L2_CID_MPEG_VIDEO_VBV_DELAY
> V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE

Perhaps it would be helpful yes.

> 
> ?
> 
> > > +
> > > +.. raw:: latex
> > > +
> > > +    \normalsize
> > > +
> > >  ``V4L2_CID_MPEG_VIDEO_TEMPORAL_DECIMATION (integer)``
> > >      For every captured frame, skip this many subsequent frames (default
> > >      0).
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > index 3f3fbcd60cc6..d088acfa6dd8 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > @@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> > >  		"External",
> > >  		NULL,
> > >  	};
> > > +	static const char * const mpeg_video_frame_skip[] = {
> > > +		"Disabled",
> > > +		"Level Limit",
> > > +		"VBV/CPB Limit",
> > > +		NULL,
> > > +	};
> > >  
> > >  	switch (id) {
> > >  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> > > @@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> > >  		return flash_strobe_source;
> > >  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> > >  		return header_mode;
> > > +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
> > > +		return mpeg_video_frame_skip;
> > >  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
> > >  		return multi_slice;
> > >  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
> > > @@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> > >  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
> > >  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:			return "Sequence Header Mode";
> > >  	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
> > > +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
> > >  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
> > >  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
> > >  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
> > > @@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> > >  	case V4L2_CID_FLASH_LED_MODE:
> > >  	case V4L2_CID_FLASH_STROBE_SOURCE:
> > >  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> > > +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
> > >  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
> > >  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
> > >  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
> > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > index 62271418c1be..4e1526175a4c 100644
> > > --- a/include/uapi/linux/v4l2-controls.h
> > > +++ b/include/uapi/linux/v4l2-controls.h
> > > @@ -742,6 +742,12 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
> > >  #define V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR	(V4L2_CID_MPEG_BASE + 642)
> > >  #define V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES	(V4L2_CID_MPEG_BASE + 643)
> > >  #define V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR	(V4L2_CID_MPEG_BASE + 644)
> > > +#define V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)
> > > +enum v4l2_mpeg_video_frame_skip_mode {
> > > +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
> > > +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
> > > +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
> > > +};
> > >  
> > >  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
> > >  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control
@ 2020-07-15 18:12         ` Nicolas Dufresne
  0 siblings, 0 replies; 24+ messages in thread
From: Nicolas Dufresne @ 2020-07-15 18:12 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media, linux-kernel, linux-arm-msm,
	linux-arm-kernel
  Cc: Maheshwar Ajja, Andrzej Hajda, Kamil Debski, Jeongtae Park,
	Kyungmin Park, Hans Verkuil

Le mercredi 15 juillet 2020 à 18:42 +0300, Stanimir Varbanov a écrit :
> Hi Nicolas,
> 
> On 7/7/20 11:53 PM, Nicolas Dufresne wrote:
> > Le dimanche 05 juillet 2020 à 15:11 +0300, Stanimir Varbanov a écrit :
> > > Adds encoders standard v4l2 control for frame-skip. The control
> > > is a copy of a custom encoder control so that other v4l2 encoder
> > > drivers can use it.
> > > 
> > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> > > ---
> > >  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
> > >  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
> > >  include/uapi/linux/v4l2-controls.h            |  6 ++++
> > >  3 files changed, 48 insertions(+)
> > > 
> > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > index d0d506a444b1..a8b4c0b40747 100644
> > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > > @@ -592,6 +592,38 @@ enum v4l2_mpeg_video_bitrate_mode -
> > >      the average video bitrate. It is ignored if the video bitrate mode
> > >      is set to constant bitrate.
> > >  
> > > +``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE (enum)``
> > > +
> > > +enum v4l2_mpeg_video_frame_skip_mode -
> > > +    Indicates in what conditions the encoder should skip frames. If
> > > +    encoding a frame would cause the encoded stream to be larger then a
> > > +    chosen data limit then the frame will be skipped. Possible values
> > > +    are:
> > 
> > I have nothing against this API, in fact it's really nice to generalize
> > as this is very common. Though, I think we are missing two things. This
> > documentation refer to the "chosen data limit". Is there controls to
> > configure these *chosen* limit ? The other issue is the vagueness of
> > the documented mode, see lower...
> > 
> > > +
> > > +
> > > +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
> > > +
> > > +.. raw:: latex
> > > +
> > > +    \small
> > > +
> > > +.. flat-table::
> > > +    :header-rows:  0
> > > +    :stub-columns: 0
> > > +
> > > +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
> > > +      - Frame skip mode is disabled.
> > > +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
> > > +      - Frame skip mode enabled and buffer limit is set by the chosen
> > > +	level and is defined by the standard.
> > 
> > At least for H.264, a level is compose of 3 limits. One is the maximum
> > number of macroblocks, this is is evidently not use for frame skipping
> > and already constrained in V4L2 (assuming the driver does not ignore
> > the level control of course). The two other limits are decoded
> > macroblocks/s and encoded kbits/s. Both are measure over time, which
> > means the M2M encoder needs to be timing aware. I think the time source
> > should be documented. Perhaps it is mandatory to set a frame interval
> > for this to work ? Or we need some timestamp to allow variable frame
> > interval ? (I don't think the second is really an option without
> > extending the API again, and confusingly, since I think we have used
> > the timestamp for other purpose already)
> 
> Do you want to say that the encoder input timestamp, bitrate control
> (V4L2_CID_MPEG_VIDEO_BITRATE) and S_PARM is not enough to describe
> FRAME_SKIP_MODE_LEVEL_LIMIT mode?

I don't think we have spec to give the input timestamp a meaning that
driver can interpret. In fact I think we gave it a meaning that the
driver must not interpret it (aka driver opaque). So remain S_PARM to
give a clue, but some stream don't have a framerate (like RTP streams,
unless written in bitstream).

> 
> > > +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
> > > +      - Frame skip mode enabled and buffer limit is set by the VBV
> > > +	(MPEG1/2/4) or CPB (H264) buffer size control.
> > 
> > The notion of VBV an CPB is unlikely well known. If my memory is right,
> > these are constrained in buffering: in bytes (VBV) or bits per frame
> > over a window of n-frames (or the gop size for some less flexible
> > encoder) (CPB). I think these should be somehow chosen by application
> > (with controls), directly or indirectly, and documented here to ensure
> > we get consistent implementation across drivers.
> 
> I guess you want me to add here references to the following controls:
> 
> V4L2_CID_MPEG_VIDEO_VBV_SIZE
> V4L2_CID_MPEG_VIDEO_VBV_DELAY
> V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE

Perhaps it would be helpful yes.

> 
> ?
> 
> > > +
> > > +.. raw:: latex
> > > +
> > > +    \normalsize
> > > +
> > >  ``V4L2_CID_MPEG_VIDEO_TEMPORAL_DECIMATION (integer)``
> > >      For every captured frame, skip this many subsequent frames (default
> > >      0).
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > index 3f3fbcd60cc6..d088acfa6dd8 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > @@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> > >  		"External",
> > >  		NULL,
> > >  	};
> > > +	static const char * const mpeg_video_frame_skip[] = {
> > > +		"Disabled",
> > > +		"Level Limit",
> > > +		"VBV/CPB Limit",
> > > +		NULL,
> > > +	};
> > >  
> > >  	switch (id) {
> > >  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> > > @@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> > >  		return flash_strobe_source;
> > >  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> > >  		return header_mode;
> > > +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
> > > +		return mpeg_video_frame_skip;
> > >  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
> > >  		return multi_slice;
> > >  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
> > > @@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> > >  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
> > >  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:			return "Sequence Header Mode";
> > >  	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
> > > +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
> > >  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
> > >  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
> > >  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
> > > @@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> > >  	case V4L2_CID_FLASH_LED_MODE:
> > >  	case V4L2_CID_FLASH_STROBE_SOURCE:
> > >  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> > > +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
> > >  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
> > >  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
> > >  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
> > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > index 62271418c1be..4e1526175a4c 100644
> > > --- a/include/uapi/linux/v4l2-controls.h
> > > +++ b/include/uapi/linux/v4l2-controls.h
> > > @@ -742,6 +742,12 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
> > >  #define V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR	(V4L2_CID_MPEG_BASE + 642)
> > >  #define V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES	(V4L2_CID_MPEG_BASE + 643)
> > >  #define V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR	(V4L2_CID_MPEG_BASE + 644)
> > > +#define V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)
> > > +enum v4l2_mpeg_video_frame_skip_mode {
> > > +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
> > > +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
> > > +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
> > > +};
> > >  
> > >  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
> > >  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control
  2020-07-15 18:12         ` Nicolas Dufresne
@ 2020-07-16 10:50           ` Stanimir Varbanov
  -1 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-16 10:50 UTC (permalink / raw)
  To: Nicolas Dufresne, Stanimir Varbanov, linux-media, linux-kernel,
	linux-arm-msm, linux-arm-kernel
  Cc: Kyungmin Park, Kamil Debski, Jeongtae Park, Andrzej Hajda,
	Hans Verkuil, Maheshwar Ajja



On 7/15/20 9:12 PM, Nicolas Dufresne wrote:
> Le mercredi 15 juillet 2020 à 18:42 +0300, Stanimir Varbanov a écrit :
>> Hi Nicolas,
>>
>> On 7/7/20 11:53 PM, Nicolas Dufresne wrote:
>>> Le dimanche 05 juillet 2020 à 15:11 +0300, Stanimir Varbanov a écrit :
>>>> Adds encoders standard v4l2 control for frame-skip. The control
>>>> is a copy of a custom encoder control so that other v4l2 encoder
>>>> drivers can use it.
>>>>
>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>> ---
>>>>  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
>>>>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
>>>>  include/uapi/linux/v4l2-controls.h            |  6 ++++
>>>>  3 files changed, 48 insertions(+)
>>>>
>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> index d0d506a444b1..a8b4c0b40747 100644
>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> @@ -592,6 +592,38 @@ enum v4l2_mpeg_video_bitrate_mode -
>>>>      the average video bitrate. It is ignored if the video bitrate mode
>>>>      is set to constant bitrate.
>>>>  
>>>> +``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE (enum)``
>>>> +
>>>> +enum v4l2_mpeg_video_frame_skip_mode -
>>>> +    Indicates in what conditions the encoder should skip frames. If
>>>> +    encoding a frame would cause the encoded stream to be larger then a
>>>> +    chosen data limit then the frame will be skipped. Possible values
>>>> +    are:
>>>
>>> I have nothing against this API, in fact it's really nice to generalize
>>> as this is very common. Though, I think we are missing two things. This
>>> documentation refer to the "chosen data limit". Is there controls to
>>> configure these *chosen* limit ? The other issue is the vagueness of
>>> the documented mode, see lower...
>>>
>>>> +
>>>> +
>>>> +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
>>>> +
>>>> +.. raw:: latex
>>>> +
>>>> +    \small
>>>> +
>>>> +.. flat-table::
>>>> +    :header-rows:  0
>>>> +    :stub-columns: 0
>>>> +
>>>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
>>>> +      - Frame skip mode is disabled.
>>>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
>>>> +      - Frame skip mode enabled and buffer limit is set by the chosen
>>>> +	level and is defined by the standard.
>>>
>>> At least for H.264, a level is compose of 3 limits. One is the maximum
>>> number of macroblocks, this is is evidently not use for frame skipping
>>> and already constrained in V4L2 (assuming the driver does not ignore
>>> the level control of course). The two other limits are decoded
>>> macroblocks/s and encoded kbits/s. Both are measure over time, which
>>> means the M2M encoder needs to be timing aware. I think the time source
>>> should be documented. Perhaps it is mandatory to set a frame interval
>>> for this to work ? Or we need some timestamp to allow variable frame
>>> interval ? (I don't think the second is really an option without
>>> extending the API again, and confusingly, since I think we have used
>>> the timestamp for other purpose already)
>>
>> Do you want to say that the encoder input timestamp, bitrate control
>> (V4L2_CID_MPEG_VIDEO_BITRATE) and S_PARM is not enough to describe
>> FRAME_SKIP_MODE_LEVEL_LIMIT mode?
> 
> I don't think we have spec to give the input timestamp a meaning that
> driver can interpret. In fact I think we gave it a meaning that the
> driver must not interpret it (aka driver opaque). So remain S_PARM to

At least for Venus the timestamps are passed to the firmware and used by
encoder rate-controller.

> give a clue, but some stream don't have a framerate (like RTP streams,
> unless written in bitstream).
I think v4l2 clients should be able to guess what would be the frame
rate in such cases, no?

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control
@ 2020-07-16 10:50           ` Stanimir Varbanov
  0 siblings, 0 replies; 24+ messages in thread
From: Stanimir Varbanov @ 2020-07-16 10:50 UTC (permalink / raw)
  To: Nicolas Dufresne, Stanimir Varbanov, linux-media, linux-kernel,
	linux-arm-msm, linux-arm-kernel
  Cc: Maheshwar Ajja, Andrzej Hajda, Kamil Debski, Jeongtae Park,
	Kyungmin Park, Hans Verkuil



On 7/15/20 9:12 PM, Nicolas Dufresne wrote:
> Le mercredi 15 juillet 2020 à 18:42 +0300, Stanimir Varbanov a écrit :
>> Hi Nicolas,
>>
>> On 7/7/20 11:53 PM, Nicolas Dufresne wrote:
>>> Le dimanche 05 juillet 2020 à 15:11 +0300, Stanimir Varbanov a écrit :
>>>> Adds encoders standard v4l2 control for frame-skip. The control
>>>> is a copy of a custom encoder control so that other v4l2 encoder
>>>> drivers can use it.
>>>>
>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>> ---
>>>>  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
>>>>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
>>>>  include/uapi/linux/v4l2-controls.h            |  6 ++++
>>>>  3 files changed, 48 insertions(+)
>>>>
>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> index d0d506a444b1..a8b4c0b40747 100644
>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> @@ -592,6 +592,38 @@ enum v4l2_mpeg_video_bitrate_mode -
>>>>      the average video bitrate. It is ignored if the video bitrate mode
>>>>      is set to constant bitrate.
>>>>  
>>>> +``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE (enum)``
>>>> +
>>>> +enum v4l2_mpeg_video_frame_skip_mode -
>>>> +    Indicates in what conditions the encoder should skip frames. If
>>>> +    encoding a frame would cause the encoded stream to be larger then a
>>>> +    chosen data limit then the frame will be skipped. Possible values
>>>> +    are:
>>>
>>> I have nothing against this API, in fact it's really nice to generalize
>>> as this is very common. Though, I think we are missing two things. This
>>> documentation refer to the "chosen data limit". Is there controls to
>>> configure these *chosen* limit ? The other issue is the vagueness of
>>> the documented mode, see lower...
>>>
>>>> +
>>>> +
>>>> +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
>>>> +
>>>> +.. raw:: latex
>>>> +
>>>> +    \small
>>>> +
>>>> +.. flat-table::
>>>> +    :header-rows:  0
>>>> +    :stub-columns: 0
>>>> +
>>>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
>>>> +      - Frame skip mode is disabled.
>>>> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
>>>> +      - Frame skip mode enabled and buffer limit is set by the chosen
>>>> +	level and is defined by the standard.
>>>
>>> At least for H.264, a level is compose of 3 limits. One is the maximum
>>> number of macroblocks, this is is evidently not use for frame skipping
>>> and already constrained in V4L2 (assuming the driver does not ignore
>>> the level control of course). The two other limits are decoded
>>> macroblocks/s and encoded kbits/s. Both are measure over time, which
>>> means the M2M encoder needs to be timing aware. I think the time source
>>> should be documented. Perhaps it is mandatory to set a frame interval
>>> for this to work ? Or we need some timestamp to allow variable frame
>>> interval ? (I don't think the second is really an option without
>>> extending the API again, and confusingly, since I think we have used
>>> the timestamp for other purpose already)
>>
>> Do you want to say that the encoder input timestamp, bitrate control
>> (V4L2_CID_MPEG_VIDEO_BITRATE) and S_PARM is not enough to describe
>> FRAME_SKIP_MODE_LEVEL_LIMIT mode?
> 
> I don't think we have spec to give the input timestamp a meaning that
> driver can interpret. In fact I think we gave it a meaning that the
> driver must not interpret it (aka driver opaque). So remain S_PARM to

At least for Venus the timestamps are passed to the firmware and used by
encoder rate-controller.

> give a clue, but some stream don't have a framerate (like RTP streams,
> unless written in bitstream).
I think v4l2 clients should be able to guess what would be the frame
rate in such cases, no?

-- 
regards,
Stan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] media: docs: Depricate mfc frame skip control
  2020-07-05 12:11   ` Stanimir Varbanov
@ 2020-07-20  9:33     ` Hans Verkuil
  -1 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2020-07-20  9:33 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media, linux-kernel, linux-arm-msm,
	linux-arm-kernel
  Cc: Kyungmin Park, Kamil Debski, Jeongtae Park, Andrzej Hajda,
	Maheshwar Ajja

On 05/07/2020 14:11, Stanimir Varbanov wrote:
> Depricate mfc private frame skip mode control for new

Depricate -> Deprecate (same in the patch below).

> clients and use the standard one instead.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index a8b4c0b40747..c0760bfc54d4 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -2805,6 +2805,11 @@ MFC 5.1 Control IDs
>  ``V4L2_CID_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE``
>      (enum)
>  
> +    .. note::
> +
> +       This control is depricated. Use the standard one

s/one//

> +       ``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE`` instead.

s/instead/control instead/

> +
>  enum v4l2_mpeg_mfc51_video_frame_skip_mode -
>      Indicates in what conditions the encoder should skip frames. If
>      encoding a frame would cause the encoded stream to be larger then a
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 4/4] media: docs: Depricate mfc frame skip control
@ 2020-07-20  9:33     ` Hans Verkuil
  0 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2020-07-20  9:33 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media, linux-kernel, linux-arm-msm,
	linux-arm-kernel
  Cc: Jeongtae Park, Kyungmin Park, Kamil Debski, Maheshwar Ajja,
	Andrzej Hajda

On 05/07/2020 14:11, Stanimir Varbanov wrote:
> Depricate mfc private frame skip mode control for new

Depricate -> Deprecate (same in the patch below).

> clients and use the standard one instead.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index a8b4c0b40747..c0760bfc54d4 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -2805,6 +2805,11 @@ MFC 5.1 Control IDs
>  ``V4L2_CID_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE``
>      (enum)
>  
> +    .. note::
> +
> +       This control is depricated. Use the standard one

s/one//

> +       ``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE`` instead.

s/instead/control instead/

> +
>  enum v4l2_mpeg_mfc51_video_frame_skip_mode -
>      Indicates in what conditions the encoder should skip frames. If
>      encoding a frame would cause the encoded stream to be larger then a
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control
  2020-07-05 12:11   ` Stanimir Varbanov
@ 2020-07-20  9:36     ` Hans Verkuil
  -1 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2020-07-20  9:36 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media, linux-kernel, linux-arm-msm,
	linux-arm-kernel
  Cc: Kyungmin Park, Kamil Debski, Jeongtae Park, Andrzej Hajda,
	Maheshwar Ajja

On 05/07/2020 14:11, Stanimir Varbanov wrote:
> Adds encoders standard v4l2 control for frame-skip. The control
> is a copy of a custom encoder control so that other v4l2 encoder
> drivers can use it.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

But see note at the end.

> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
>  include/uapi/linux/v4l2-controls.h            |  6 ++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index d0d506a444b1..a8b4c0b40747 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -592,6 +592,38 @@ enum v4l2_mpeg_video_bitrate_mode -
>      the average video bitrate. It is ignored if the video bitrate mode
>      is set to constant bitrate.
>  
> +``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE (enum)``
> +
> +enum v4l2_mpeg_video_frame_skip_mode -
> +    Indicates in what conditions the encoder should skip frames. If
> +    encoding a frame would cause the encoded stream to be larger then a
> +    chosen data limit then the frame will be skipped. Possible values
> +    are:
> +
> +
> +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
> +
> +.. raw:: latex
> +
> +    \small
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
> +      - Frame skip mode is disabled.
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
> +      - Frame skip mode enabled and buffer limit is set by the chosen
> +	level and is defined by the standard.
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
> +      - Frame skip mode enabled and buffer limit is set by the VBV
> +	(MPEG1/2/4) or CPB (H264) buffer size control.
> +
> +.. raw:: latex
> +
> +    \normalsize
> +
>  ``V4L2_CID_MPEG_VIDEO_TEMPORAL_DECIMATION (integer)``
>      For every captured frame, skip this many subsequent frames (default
>      0).
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 3f3fbcd60cc6..d088acfa6dd8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"External",
>  		NULL,
>  	};
> +	static const char * const mpeg_video_frame_skip[] = {
> +		"Disabled",
> +		"Level Limit",
> +		"VBV/CPB Limit",
> +		NULL,
> +	};
>  
>  	switch (id) {
>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> @@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return flash_strobe_source;
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
>  		return header_mode;
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
> +		return mpeg_video_frame_skip;
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  		return multi_slice;
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
> @@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:			return "Sequence Header Mode";
>  	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
>  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
> @@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_FLASH_LED_MODE:
>  	case V4L2_CID_FLASH_STROBE_SOURCE:
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 62271418c1be..4e1526175a4c 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -742,6 +742,12 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
>  #define V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR	(V4L2_CID_MPEG_BASE + 642)
>  #define V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES	(V4L2_CID_MPEG_BASE + 643)
>  #define V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR	(V4L2_CID_MPEG_BASE + 644)
> +#define V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)

I think this now clashes with "media: v4l2-ctrls: Add encoder constant quality control".

I recommend making a new series that combines both series. That avoid this problem.

Regards,

	Hans

> +enum v4l2_mpeg_video_frame_skip_mode {
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
> +};
>  
>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control
@ 2020-07-20  9:36     ` Hans Verkuil
  0 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2020-07-20  9:36 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media, linux-kernel, linux-arm-msm,
	linux-arm-kernel
  Cc: Jeongtae Park, Kyungmin Park, Kamil Debski, Maheshwar Ajja,
	Andrzej Hajda

On 05/07/2020 14:11, Stanimir Varbanov wrote:
> Adds encoders standard v4l2 control for frame-skip. The control
> is a copy of a custom encoder control so that other v4l2 encoder
> drivers can use it.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

But see note at the end.

> ---
>  .../media/v4l/ext-ctrls-codec.rst             | 32 +++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
>  include/uapi/linux/v4l2-controls.h            |  6 ++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index d0d506a444b1..a8b4c0b40747 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -592,6 +592,38 @@ enum v4l2_mpeg_video_bitrate_mode -
>      the average video bitrate. It is ignored if the video bitrate mode
>      is set to constant bitrate.
>  
> +``V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE (enum)``
> +
> +enum v4l2_mpeg_video_frame_skip_mode -
> +    Indicates in what conditions the encoder should skip frames. If
> +    encoding a frame would cause the encoded stream to be larger then a
> +    chosen data limit then the frame will be skipped. Possible values
> +    are:
> +
> +
> +.. tabularcolumns:: |p{9.2cm}|p{8.3cm}|
> +
> +.. raw:: latex
> +
> +    \small
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_DISABLED``
> +      - Frame skip mode is disabled.
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_LEVEL_LIMIT``
> +      - Frame skip mode enabled and buffer limit is set by the chosen
> +	level and is defined by the standard.
> +    * - ``V4L2_MPEG_FRAME_SKIP_MODE_BUF_LIMIT``
> +      - Frame skip mode enabled and buffer limit is set by the VBV
> +	(MPEG1/2/4) or CPB (H264) buffer size control.
> +
> +.. raw:: latex
> +
> +    \normalsize
> +
>  ``V4L2_CID_MPEG_VIDEO_TEMPORAL_DECIMATION (integer)``
>      For every captured frame, skip this many subsequent frames (default
>      0).
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 3f3fbcd60cc6..d088acfa6dd8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -590,6 +590,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"External",
>  		NULL,
>  	};
> +	static const char * const mpeg_video_frame_skip[] = {
> +		"Disabled",
> +		"Level Limit",
> +		"VBV/CPB Limit",
> +		NULL,
> +	};
>  
>  	switch (id) {
>  	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> @@ -651,6 +657,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return flash_strobe_source;
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
>  		return header_mode;
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
> +		return mpeg_video_frame_skip;
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  		return multi_slice;
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
> @@ -844,6 +852,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_MB_RC_ENABLE:			return "H264 MB Level Rate Control";
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:			return "Sequence Header Mode";
>  	case V4L2_CID_MPEG_VIDEO_MAX_REF_PIC:			return "Max Number of Reference Pics";
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:		return "Frame Skip Mode";
>  	case V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP:		return "H263 I-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP:		return "H263 P-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_H263_B_FRAME_QP:		return "H263 B-Frame QP Value";
> @@ -1265,6 +1274,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_FLASH_LED_MODE:
>  	case V4L2_CID_FLASH_STROBE_SOURCE:
>  	case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
> +	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 62271418c1be..4e1526175a4c 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -742,6 +742,12 @@ enum v4l2_cid_mpeg_video_hevc_size_of_length_field {
>  #define V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_L6_BR	(V4L2_CID_MPEG_BASE + 642)
>  #define V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES	(V4L2_CID_MPEG_BASE + 643)
>  #define V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR	(V4L2_CID_MPEG_BASE + 644)
> +#define V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE		(V4L2_CID_MPEG_BASE + 645)

I think this now clashes with "media: v4l2-ctrls: Add encoder constant quality control".

I recommend making a new series that combines both series. That avoid this problem.

Regards,

	Hans

> +enum v4l2_mpeg_video_frame_skip_mode {
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED	= 0,
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_LEVEL_LIMIT	= 1,
> +	V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT	= 2,
> +};
>  
>  /*  MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */
>  #define V4L2_CID_MPEG_CX2341X_BASE				(V4L2_CTRL_CLASS_MPEG | 0x1000)
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2020-07-20  9:37 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-05 12:11 [PATCH 0/4] Make Frame Skip Mode control a standard Stanimir Varbanov
2020-07-05 12:11 ` Stanimir Varbanov
2020-07-05 12:11 ` [PATCH 1/4] media: v4l2-ctrl: Add frame-skip std encoder control Stanimir Varbanov
2020-07-05 12:11   ` Stanimir Varbanov
2020-07-07 20:53   ` Nicolas Dufresne
2020-07-07 20:53     ` Nicolas Dufresne
2020-07-15 15:42     ` Stanimir Varbanov
2020-07-15 15:42       ` Stanimir Varbanov
2020-07-15 18:12       ` Nicolas Dufresne
2020-07-15 18:12         ` Nicolas Dufresne
2020-07-16 10:50         ` Stanimir Varbanov
2020-07-16 10:50           ` Stanimir Varbanov
2020-07-20  9:36   ` Hans Verkuil
2020-07-20  9:36     ` Hans Verkuil
2020-07-05 12:11 ` [PATCH 2/4] venus: venc: Add support for frame-skip mode v4l2 control Stanimir Varbanov
2020-07-05 12:11   ` Stanimir Varbanov
2020-07-05 12:11 ` [PATCH 3/4] media: s5p-mfc: Use standard frame skip mode control Stanimir Varbanov
2020-07-05 12:11   ` Stanimir Varbanov
2020-07-05 12:11 ` [PATCH 4/4] media: docs: Depricate mfc frame skip control Stanimir Varbanov
2020-07-05 12:11   ` Stanimir Varbanov
2020-07-05 15:29   ` Randy Dunlap
2020-07-05 15:29     ` Randy Dunlap
2020-07-20  9:33   ` Hans Verkuil
2020-07-20  9:33     ` Hans Verkuil

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.