[PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE

[PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE

[Wu-Cheng Li]

Some drivers also need a control like
V4L2_CID_MPEG_MFC51_VIDEO_FORCE_FRAME_TYPE to force an encoder frame
type. This patch adds a general V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.

This control only affects the next queued buffer. There's no need to
clear the value after requesting an I frame. But all controls are set
in v4l2_ctrl_handler_setup. So a default DISABLED value is required.
Basically this control is like V4L2_CTRL_TYPE_BUTTON with parameters.
How to prevent a control from being set in v4l2_ctrl_handler_setup so
DISABLED value is not needed? Does it make sense not to set a control
if it is EXECUTE_ON_WRITE?

Wu-Cheng Li (1):
  v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.

 Documentation/DocBook/media/v4l/controls.xml | 23 +++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c         | 13 +++++++++++++
 include/uapi/linux/v4l2-controls.h           |  5 +++++
 3 files changed, 41 insertions(+)

-- 
2.6.0.rc2.230.g3dd15c0

[PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.

[Wu-Cheng Li]

Some drivers also need a control like
V4L2_CID_MPEG_MFC51_VIDEO_FORCE_FRAME_TYPE to force an encoder
frame type. Add a general V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE
so the new drivers and applications can use it.

Signed-off-by: Wu-Cheng Li <wuchengli@chromium.org>
---
 Documentation/DocBook/media/v4l/controls.xml | 23 +++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c         | 13 +++++++++++++
 include/uapi/linux/v4l2-controls.h           |  5 +++++
 3 files changed, 41 insertions(+)

diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index f13a429..326947c 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -2330,6 +2330,29 @@ vertical search range for motion estimation module in video encoder.</entry>
 	      </row>
 
 	      <row><entry></entry></row>
+	      <row id="v4l2-mpeg-video-force-frame-type">
+		<entry spanname="id"><constant>V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE</constant>&nbsp;</entry>
+		<entry>enum&nbsp;v4l2_mpeg_video_force_frame_type</entry>
+	      </row>
+	      <row><entry spanname="descr">Force a frame type for the next queued buffer. Applicable to encoders.
+This is a general, codec-agnostic keyframe control. This is a write-only and execute-on-write control. Possible values are:</entry>
+	      </row>
+	      <row>
+		<entrytbl spanname="descr" cols="2">
+		  <tbody valign="top">
+		    <row>
+		      <entry><constant>V4L2_MPEG_FORCE_FRAME_TYPE_DISABLED</constant>&nbsp;</entry>
+		      <entry>Force a specific frame type disabled.</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_MPEG_FORCE_FRAME_TYPE_I_FRAME</constant>&nbsp;</entry>
+		      <entry>Force an I-frame.</entry>
+		    </row>
+		  </tbody>
+		</entrytbl>
+	      </row>
+
+	      <row><entry></entry></row>
 	      <row>
 		<entry spanname="id"><constant>V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE</constant>&nbsp;</entry>
 		<entry>integer</entry>
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index c9d5537..53a8f72 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -315,6 +315,11 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		"Max Bytes",
 		NULL,
 	};
+	static const char * const force_frame_type[] = {
+		"Disabled",
+		"I Frame",
+		NULL,
+	};
 	static const char * const entropy_mode[] = {
 		"CAVLC",
 		"CABAC",
@@ -533,6 +538,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		return header_mode;
 	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
 		return multi_slice;
+	case V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE:
+		return force_frame_type;
 	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
 		return entropy_mode;
 	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
@@ -747,6 +754,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE:		return "Horizontal MV Search Range";
 	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:		return "Vertical MV Search Range";
 	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:		return "Repeat Sequence Header";
+	case V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE:		return "Force an encoded frame type";
 
 	/* VPX controls */
 	case V4L2_CID_MPEG_VIDEO_VPX_NUM_PARTITIONS:		return "VPX Number of Partitions";
@@ -1045,6 +1053,11 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_DETECT_MD_MODE:
 		*type = V4L2_CTRL_TYPE_MENU;
 		break;
+	case V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE:
+		*type = V4L2_CTRL_TYPE_MENU;
+		*flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
+			  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
+		break;
 	case V4L2_CID_LINK_FREQ:
 		*type = V4L2_CTRL_TYPE_INTEGER_MENU;
 		break;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 2d225bc..c2be60c 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -390,6 +390,11 @@ enum v4l2_mpeg_video_multi_slice_mode {
 #define V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER		(V4L2_CID_MPEG_BASE+226)
 #define V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE		(V4L2_CID_MPEG_BASE+227)
 #define V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE		(V4L2_CID_MPEG_BASE+228)
+#define V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE		(V4L2_CID_MPEG_BASE+229)
+enum v4l2_mpeg_video_force_frame_type {
+	V4L2_MPEG_VIDEO_FORCE_FRAME_TYPE_DISABLED	= 0,
+	V4L2_MPEG_VIDEO_FORCE_FRAME_TYPE_I_FRAME	= 1,
+};
 
 #define V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP		(V4L2_CID_MPEG_BASE+300)
 #define V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP		(V4L2_CID_MPEG_BASE+301)
-- 
2.6.0.rc2.230.g3dd15c0

RE: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.

[Kamil Debski]

Hi,

> From: Wu-Cheng Li [mailto:wuchengli@chromium.org]
> Sent: Wednesday, January 13, 2016 1:04 PM
> To: pawel@osciak.com; mchehab@osg.samsung.com; hverkuil@xs4all.nl;
> k.debski@samsung.com; crope@iki.fi; standby24x7@gmail.com;
> wuchengli@chromium.org; nicolas.dufresne@collabora.com;
> ricardo.ribalda@gmail.com; ao2@ao2.it; bparrot@ti.com
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> api@vger.kernel.org
> Subject: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.
> 
> Some drivers also need a control like
> V4L2_CID_MPEG_MFC51_VIDEO_FORCE_FRAME_TYPE to force an encoder
> frame type. Add a general V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE
> so the new drivers and applications can use it.

Good to hear that there are new codecs to use the V4L2 codec API :)

My two cents are following - when you add a control that does the same work
as a driver specific control then it would be great if you modified the
driver that
uses the specific control to also support the newly added control.
This way future applications  could use the control you added for both new
and
old drivers.

In future versions of this patch set please include modification of the
s5p-mfc
diver, this shouldn't be much work. 

Regarding the changes in the API I would like to hear also Hans Verkuil's
opinion, as he was a great help when I introduced the changes in the codec
API.

Any comments Hans?

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

> 
> Signed-off-by: Wu-Cheng Li <wuchengli@chromium.org>
> ---
>  Documentation/DocBook/media/v4l/controls.xml | 23
> +++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c         | 13 +++++++++++++
>  include/uapi/linux/v4l2-controls.h           |  5 +++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> b/Documentation/DocBook/media/v4l/controls.xml
> index f13a429..326947c 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -2330,6 +2330,29 @@ vertical search range for motion estimation
> module in video encoder.</entry>
>  	      </row>
> 
>  	      <row><entry></entry></row>
> +	      <row id="v4l2-mpeg-video-force-frame-type">
> +		<entry
> spanname="id"><constant>V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE<
> /constant>&nbsp;</entry>
> +
> 	<entry>enum&nbsp;v4l2_mpeg_video_force_frame_type</entry>
> +	      </row>
> +	      <row><entry spanname="descr">Force a frame type for the next
> queued buffer. Applicable to encoders.
> +This is a general, codec-agnostic keyframe control. This is a write-only
and
> execute-on-write control. Possible values are:</entry>
> +	      </row>
> +	      <row>
> +		<entrytbl spanname="descr" cols="2">
> +		  <tbody valign="top">
> +		    <row>
> +
> <entry><constant>V4L2_MPEG_FORCE_FRAME_TYPE_DISABLED</constant
> >&nbsp;</entry>
> +		      <entry>Force a specific frame type disabled.</entry>
> +		    </row>
> +		    <row>
> +
> <entry><constant>V4L2_MPEG_FORCE_FRAME_TYPE_I_FRAME</constant>
> &nbsp;</entry>
> +		      <entry>Force an I-frame.</entry>
> +		    </row>
> +		  </tbody>
> +		</entrytbl>
> +	      </row>
> +
> +	      <row><entry></entry></row>
>  	      <row>
>  		<entry
> spanname="id"><constant>V4L2_CID_MPEG_VIDEO_H264_CPB_SIZE</cons
> tant>&nbsp;</entry>
>  		<entry>integer</entry>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-
> core/v4l2-ctrls.c
> index c9d5537..53a8f72 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -315,6 +315,11 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"Max Bytes",
>  		NULL,
>  	};
> +	static const char * const force_frame_type[] = {
> +		"Disabled",
> +		"I Frame",
> +		NULL,
> +	};
>  	static const char * const entropy_mode[] = {
>  		"CAVLC",
>  		"CABAC",
> @@ -533,6 +538,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return header_mode;
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
>  		return multi_slice;
> +	case V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE:
> +		return force_frame_type;
>  	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
>  		return entropy_mode;
>  	case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
> @@ -747,6 +754,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE:
> 	return "Horizontal MV Search Range";
>  	case V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE:
> 	return "Vertical MV Search Range";
>  	case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER:
> 	return "Repeat Sequence Header";
> +	case V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE:		return
> "Force an encoded frame type";
> 
>  	/* VPX controls */
>  	case V4L2_CID_MPEG_VIDEO_VPX_NUM_PARTITIONS:
> 	return "VPX Number of Partitions";
> @@ -1045,6 +1053,11 @@ void v4l2_ctrl_fill(u32 id, const char **name,
> enum v4l2_ctrl_type *type,
>  	case V4L2_CID_DETECT_MD_MODE:
>  		*type = V4L2_CTRL_TYPE_MENU;
>  		break;
> +	case V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE:
> +		*type = V4L2_CTRL_TYPE_MENU;
> +		*flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
> +			  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> +		break;
>  	case V4L2_CID_LINK_FREQ:
>  		*type = V4L2_CTRL_TYPE_INTEGER_MENU;
>  		break;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-
> controls.h
> index 2d225bc..c2be60c 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -390,6 +390,11 @@ enum v4l2_mpeg_video_multi_slice_mode {
>  #define V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER
> 	(V4L2_CID_MPEG_BASE+226)
>  #define V4L2_CID_MPEG_VIDEO_MV_H_SEARCH_RANGE
> 	(V4L2_CID_MPEG_BASE+227)
>  #define V4L2_CID_MPEG_VIDEO_MV_V_SEARCH_RANGE
> 	(V4L2_CID_MPEG_BASE+228)
> +#define V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE
> 	(V4L2_CID_MPEG_BASE+229)
> +enum v4l2_mpeg_video_force_frame_type {
> +	V4L2_MPEG_VIDEO_FORCE_FRAME_TYPE_DISABLED	= 0,
> +	V4L2_MPEG_VIDEO_FORCE_FRAME_TYPE_I_FRAME	= 1,
> +};
> 
>  #define V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP
> 	(V4L2_CID_MPEG_BASE+300)
>  #define V4L2_CID_MPEG_VIDEO_H263_P_FRAME_QP
> 	(V4L2_CID_MPEG_BASE+301)
> --
> 2.6.0.rc2.230.g3dd15c0

Re: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

Hi Kamil,

Le mercredi 13 janvier 2016 à 17:43 +0100, Kamil Debski a écrit :
> Good to hear that there are new codecs to use the V4L2 codec API :)
> 
> My two cents are following - when you add a control that does the same work
> as a driver specific control then it would be great if you modified the
> driver that
> uses the specific control to also support the newly added control.
> This way future applications  could use the control you added for both new
> and
> old drivers.

When i first notice this MFC specific control, I believed it was use to
produce I-Frame only streams (rather then a toggle, to produce one key-
frame on demand). So I wanted to verify the implementation to make sure
what Wu-Cheng is doing make sense. Though, I found that we set:

  ctx->force_frame_type = ctrl->val;

And never use that value anywhere else in the driver code. Hopefully
I'm just missing something, but if it's not implemented, maybe it's
better not to expose it. Otherwise, this may lead to hard to find
streaming issues. I do hope we can add this feature though, as it's
very useful feature for real time encoding.

cheers,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

Hi Kamil,

Le mercredi 13 janvier 2016 à 17:43 +0100, Kamil Debski a écrit :
> Good to hear that there are new codecs to use the V4L2 codec API :)
> 
> My two cents are following - when you add a control that does the same work
> as a driver specific control then it would be great if you modified the
> driver that
> uses the specific control to also support the newly added control.
> This way future applications  could use the control you added for both new
> and
> old drivers.

When i first notice this MFC specific control, I believed it was use to
produce I-Frame only streams (rather then a toggle, to produce one key-
frame on demand). So I wanted to verify the implementation to make sure
what Wu-Cheng is doing make sense. Though, I found that we set:

  ctx->force_frame_type = ctrl->val;

And never use that value anywhere else in the driver code. Hopefully
I'm just missing something, but if it's not implemented, maybe it's
better not to expose it. Otherwise, this may lead to hard to find
streaming issues. I do hope we can add this feature though, as it's
very useful feature for real time encoding.

cheers,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

RE: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.

[Kamil Debski]

Hi,

> From: Nicolas Dufresne [mailto:nicolas.dufresne-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org]
> Sent: Thursday, January 14, 2016 4:02 PM
> To: Kamil Debski; 'Wu-Cheng Li'; pawel-FA/gS7QP4orQT0dZR+AlfA@public.gmane.org;
> mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org; hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org; crope-X3B1VOXEql0@public.gmane.org;
> standby24x7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; ricardo.ribalda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; ao2-qKGr9MkilAE@public.gmane.org;
> bparrot-l0cyMroinI0@public.gmane.org
> Cc: linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] v4l: add
> V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.
> 
> Hi Kamil,
> 
> Le mercredi 13 janvier 2016 à 17:43 +0100, Kamil Debski a écrit :
> > Good to hear that there are new codecs to use the V4L2 codec API :)
> >
> > My two cents are following - when you add a control that does the same
> > work as a driver specific control then it would be great if you
> > modified the driver that uses the specific control to also support the
> > newly added control.
> > This way future applications  could use the control you added for both
> > new and old drivers.
> 
> When i first notice this MFC specific control, I believed it was use to produce
> I-Frame only streams (rather then a toggle, to produce one key- frame on
> demand). So I wanted to verify the implementation to make sure what Wu-
> Cheng is doing make sense. Though, I found that we set:
> 
>   ctx->force_frame_type = ctrl->val;
> 
> And never use that value anywhere else in the driver code. Hopefully I'm just
> missing something, but if it's not implemented, maybe it's better not to
> expose it. Otherwise, this may lead to hard to find streaming issues. I do
> hope we can add this feature though, as it's very useful feature for real time
> encoding.

Ooops, you're right. It's not implemented. I am adding Andrzej Hajda to the CC loop, he may know more about this. I think it may have been implemented in some of our development branches, but somehow did not make it into the mainline kernel. That's one thing.

The other one is about your previous comment. I will quote it below, as it is in another email.

> I don't like the way it's implemented. I don't know any decoder that have
> a frame type forcing feature other they I-Frame. It would be much more
> natural to use a toggle button control (and add more controls for other
> types when needed) then trying to merge hypothetical toggles into
>something that manually need to be set and disabled.

I had a look into the documentation of MFC. It is possible to force two types of a frame to be coded.
The one is a keyframe, the other is a not coded frame. As I understand this is a type of frame that means that image did not change from previous frame. I am sure I seen it in an MPEG4 stream in a movie trailer. The initial board with the age rating is displayed for a couple of seconds and remains static. Thus there is one I-frame and a number of non-coded frames.

That is the reason why the control was implemented in MFC as a menu and not a button. Thus the question remains - is it better to leave it as a menu, or should there be two (maybe more in the future) buttons? 

Wu-Cheng, does your hardware also supports inserting such a non-coded frame? I can imagine that there could be hardware (in the future or some current hardware that I am not aware of other than MFC) that could support this.

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

RE: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.

[Kamil Debski]

Hi,

> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> Sent: Thursday, January 14, 2016 4:02 PM
> To: Kamil Debski; 'Wu-Cheng Li'; pawel@osciak.com;
> mchehab@osg.samsung.com; hverkuil@xs4all.nl; crope@iki.fi;
> standby24x7@gmail.com; ricardo.ribalda@gmail.com; ao2@ao2.it;
> bparrot@ti.com
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> api@vger.kernel.org
> Subject: Re: [PATCH] v4l: add
> V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.
> 
> Hi Kamil,
> 
> Le mercredi 13 janvier 2016 à 17:43 +0100, Kamil Debski a écrit :
> > Good to hear that there are new codecs to use the V4L2 codec API :)
> >
> > My two cents are following - when you add a control that does the same
> > work as a driver specific control then it would be great if you
> > modified the driver that uses the specific control to also support the
> > newly added control.
> > This way future applications  could use the control you added for both
> > new and old drivers.
> 
> When i first notice this MFC specific control, I believed it was use to produce
> I-Frame only streams (rather then a toggle, to produce one key- frame on
> demand). So I wanted to verify the implementation to make sure what Wu-
> Cheng is doing make sense. Though, I found that we set:
> 
>   ctx->force_frame_type = ctrl->val;
> 
> And never use that value anywhere else in the driver code. Hopefully I'm just
> missing something, but if it's not implemented, maybe it's better not to
> expose it. Otherwise, this may lead to hard to find streaming issues. I do
> hope we can add this feature though, as it's very useful feature for real time
> encoding.

Ooops, you're right. It's not implemented. I am adding Andrzej Hajda to the CC loop, he may know more about this. I think it may have been implemented in some of our development branches, but somehow did not make it into the mainline kernel. That's one thing.

The other one is about your previous comment. I will quote it below, as it is in another email.

> I don't like the way it's implemented. I don't know any decoder that have
> a frame type forcing feature other they I-Frame. It would be much more
> natural to use a toggle button control (and add more controls for other
> types when needed) then trying to merge hypothetical toggles into
>something that manually need to be set and disabled.

I had a look into the documentation of MFC. It is possible to force two types of a frame to be coded.
The one is a keyframe, the other is a not coded frame. As I understand this is a type of frame that means that image did not change from previous frame. I am sure I seen it in an MPEG4 stream in a movie trailer. The initial board with the age rating is displayed for a couple of seconds and remains static. Thus there is one I-frame and a number of non-coded frames.

That is the reason why the control was implemented in MFC as a menu and not a button. Thus the question remains - is it better to leave it as a menu, or should there be two (maybe more in the future) buttons? 

Wu-Cheng, does your hardware also supports inserting such a non-coded frame? I can imagine that there could be hardware (in the future or some current hardware that I am not aware of other than MFC) that could support this.

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

Re: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

Le jeudi 14 janvier 2016 à 18:21 +0100, Kamil Debski a écrit :
> I had a look into the documentation of MFC. It is possible to force
> two types of a frame to be coded.
> The one is a keyframe, the other is a not coded frame. As I
> understand this is a type of frame that means that image did not
> change from previous frame. I am sure I seen it in an MPEG4 stream in
> a movie trailer. The initial board with the age rating is displayed
> for a couple of seconds and remains static. Thus there is one I-frame 
> and a number of non-coded frames.
> 
> That is the reason why the control was implemented in MFC as a menu
> and not a button. Thus the question remains - is it better to leave
> it as a menu, or should there be two (maybe more in the future)
> buttons?

Then I believe we need both. Because with the menu, setting I-Frame, I
would expect to only receive keyframes from now-on. While the useful
feature we are looking for here, is to get the next buffer (or nearby)
to be a keyframe. It's the difference between creating an I-Frame only
stream and requesting a key-frame manually for recovery (RTP use case).
In this end, we should probably take that time to review the features
we have as we need:

- A way to trigger a key frame to be produce
- A way to produce a I-Frame only stream
- A way to set the key-frame distance (in frames) even though this
could possibly be emulated using the trigger.

cheers,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

Le jeudi 14 janvier 2016 à 18:21 +0100, Kamil Debski a écrit :
> I had a look into the documentation of MFC. It is possible to force
> two types of a frame to be coded.
> The one is a keyframe, the other is a not coded frame. As I
> understand this is a type of frame that means that image did not
> change from previous frame. I am sure I seen it in an MPEG4 stream in
> a movie trailer. The initial board with the age rating is displayed
> for a couple of seconds and remains static. Thus there is one I-frame 
> and a number of non-coded frames.
> 
> That is the reason why the control was implemented in MFC as a menu
> and not a button. Thus the question remains - is it better to leave
> it as a menu, or should there be two (maybe more in the future)
> buttons?

Then I believe we need both. Because with the menu, setting I-Frame, I
would expect to only receive keyframes from now-on. While the useful
feature we are looking for here, is to get the next buffer (or nearby)
to be a keyframe. It's the difference between creating an I-Frame only
stream and requesting a key-frame manually for recovery (RTP use case).
In this end, we should probably take that time to review the features
we have as we need:

- A way to trigger a key frame to be produce
- A way to produce a I-Frame only stream
- A way to set the key-frame distance (in frames) even though this
could possibly be emulated using the trigger.

cheers,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

RE: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.

[Kamil Debski]

Hi,

> From: Nicolas Dufresne [mailto:nicolas.dufresne@collabora.com]
> Sent: Thursday, January 14, 2016 8:02 PM
> To: Kamil Debski; 'Wu-Cheng Li'; pawel@osciak.com;
> mchehab@osg.samsung.com; hverkuil@xs4all.nl; crope@iki.fi;
> standby24x7@gmail.com; ricardo.ribalda@gmail.com; ao2@ao2.it;
> bparrot@ti.com; 'Andrzej Hajda'
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> api@vger.kernel.org
> Subject: Re: [PATCH] v4l: add
> V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.
> 
> Le jeudi 14 janvier 2016 à 18:21 +0100, Kamil Debski a écrit :
> > I had a look into the documentation of MFC. It is possible to force
> > two types of a frame to be coded.
> > The one is a keyframe, the other is a not coded frame. As I understand
> > this is a type of frame that means that image did not change from
> > previous frame. I am sure I seen it in an MPEG4 stream in a movie
> > trailer. The initial board with the age rating is displayed for a
> > couple of seconds and remains static. Thus there is one I-frame and a
> > number of non-coded frames.
> >
> > That is the reason why the control was implemented in MFC as a menu
> > and not a button. Thus the question remains - is it better to leave it
> > as a menu, or should there be two (maybe more in the future) buttons?
> 
> Then I believe we need both. Because with the menu, setting I-Frame, I
> would expect to only receive keyframes from now-on. While the useful
> feature we are looking for here, is to get the next buffer (or nearby) to be a
> keyframe. It's the difference between creating an I-Frame only stream and
> requesting a key-frame manually for recovery (RTP use case).
> In this end, we should probably take that time to review the features we
> have as we need:

I think we had a discussion about this long, long time ago. Should it be
deterministic which frame Is encoded as a key frame? Should it be the
next queued frame, or the next processed frame? How to achieve this?
I vaguely remember that we discussed per buffer controls on the mailing
list, but I am not sure where the discussion went from there.

> 
> - A way to trigger a key frame to be produce
> - A way to produce a I-Frame only stream

This control can be used to do this:
- V4L2_CID_MPEG_VIDEO_GOP_SIZE (It is not well documented as I can see ;) )
	+ If set to 0 the encoder produces a stream with P only frames
	+ if set to 1 the encoder produces a stream with I only frames
	+ other values indicate the GOP size (I-frame interval)

> - A way to set the key-frame distance (in frames) even though this could
> possibly be emulated using the trigger.

As described above V4L2_CID_MPEG_VIDEO_GOP_SIZE can be used to achieve this.

> 
> cheers,
>Nicolas
 
Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

Re: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1893 bytes --]

Le vendredi 15 janvier 2016 à 18:01 +0100, Kamil Debski a écrit :
> I think we had a discussion about this long, long time ago. Should it
> be
> deterministic which frame Is encoded as a key frame? Should it be the
> next queued frame, or the next processed frame? How to achieve this?
> I vaguely remember that we discussed per buffer controls on the
> mailing
> list, but I am not sure where the discussion went from there.

In RTP use cases (and most streaming cases), all we care is that we
have a key-frame produced somewhere nearby after the request. In those
cases we use P frame only stream to reduce the bandwidth and only
request a key frame for recovery. This I believe that most common case.

Many decoders though also offer what they called an "automatic" key
frame mode. In the case of x264, there is also hints you can give to
the encoder about what type of video (film, anime, etc.) so it can
optimize all this. I believe this is very advance and there is no
particular need for it.

> 
> > 
> > - A way to trigger a key frame to be produce
> > - A way to produce a I-Frame only stream
> 
> This control can be used to do this:
> - V4L2_CID_MPEG_VIDEO_GOP_SIZE (It is not well documented as I can
> see ;) )
>         + If set to 0 the encoder produces a stream with P only
> frames
>         + if set to 1 the encoder produces a stream with I only
> frames
>         + other values indicate the GOP size (I-frame interval)
> 
> > - A way to set the key-frame distance (in frames) even though this
> could
> > possibly be emulated using the trigger.
> 
> As described above V4L2_CID_MPEG_VIDEO_GOP_SIZE can be used to
> achieve this.

Great, my memory failed on me, should have checked. This is exactly
what I was thinking. So Wu-Cheng Li patch is really what we need in the
immediate.

regards,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]

Le mercredi 13 janvier 2016 à 20:03 +0800, Wu-Cheng Li a écrit :
> Some drivers also need a control like
> V4L2_CID_MPEG_MFC51_VIDEO_FORCE_FRAME_TYPE to force an encoder frame
> type. This patch adds a general V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.
> 
> This control only affects the next queued buffer. There's no need to
> clear the value after requesting an I frame. But all controls are set
> in v4l2_ctrl_handler_setup. So a default DISABLED value is required.
> Basically this control is like V4L2_CTRL_TYPE_BUTTON with parameters.
> How to prevent a control from being set in v4l2_ctrl_handler_setup so
> DISABLED value is not needed? Does it make sense not to set a control
> if it is EXECUTE_ON_WRITE?

I don't like the way it's implemented. I don't know any decoder that
have a frame type forcing feature other they I-Frame. It would be much
more natural to use a toggle button control (and add more controls for
other types when needed) then trying to merge hypothetical toggles into
something that manually need to be set and disabled.

> 
> Wu-Cheng Li (1):
>   v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.
> 
>  Documentation/DocBook/media/v4l/controls.xml | 23
> +++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c         | 13 +++++++++++++
>  include/uapi/linux/v4l2-controls.h           |  5 +++++
>  3 files changed, 41 insertions(+)
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE

[Wu-Cheng Li (李務誠)]

On Wed, Jan 13, 2016 at 11:02 PM, Nicolas Dufresne
<nicolas.dufresne-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
> Le mercredi 13 janvier 2016 à 20:03 +0800, Wu-Cheng Li a écrit :
>> Some drivers also need a control like
>> V4L2_CID_MPEG_MFC51_VIDEO_FORCE_FRAME_TYPE to force an encoder frame
>> type. This patch adds a general V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.
>>
>> This control only affects the next queued buffer. There's no need to
>> clear the value after requesting an I frame. But all controls are set
>> in v4l2_ctrl_handler_setup. So a default DISABLED value is required.
>> Basically this control is like V4L2_CTRL_TYPE_BUTTON with parameters.
>> How to prevent a control from being set in v4l2_ctrl_handler_setup so
>> DISABLED value is not needed? Does it make sense not to set a control
>> if it is EXECUTE_ON_WRITE?
>
> I don't like the way it's implemented. I don't know any decoder that
> have a frame type forcing feature other they I-Frame. It would be much
> more natural to use a toggle button control (and add more controls for
> other types when needed) then trying to merge hypothetical toggles into
> something that manually need to be set and disabled.
Using a button control sounds like a good idea. I'll discuss with other people
and reply tomorrow.
>
>>
>> Wu-Cheng Li (1):
>>   v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.
>>
>>  Documentation/DocBook/media/v4l/controls.xml | 23
>> +++++++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-ctrls.c         | 13 +++++++++++++
>>  include/uapi/linux/v4l2-controls.h           |  5 +++++
>>  3 files changed, 41 insertions(+)
>>

Re: [PATCH] v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE

[Wu-Cheng Li (李務誠)]

On Wed, Jan 13, 2016 at 11:02 PM, Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
> Le mercredi 13 janvier 2016 à 20:03 +0800, Wu-Cheng Li a écrit :
>> Some drivers also need a control like
>> V4L2_CID_MPEG_MFC51_VIDEO_FORCE_FRAME_TYPE to force an encoder frame
>> type. This patch adds a general V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.
>>
>> This control only affects the next queued buffer. There's no need to
>> clear the value after requesting an I frame. But all controls are set
>> in v4l2_ctrl_handler_setup. So a default DISABLED value is required.
>> Basically this control is like V4L2_CTRL_TYPE_BUTTON with parameters.
>> How to prevent a control from being set in v4l2_ctrl_handler_setup so
>> DISABLED value is not needed? Does it make sense not to set a control
>> if it is EXECUTE_ON_WRITE?
>
> I don't like the way it's implemented. I don't know any decoder that
> have a frame type forcing feature other they I-Frame. It would be much
> more natural to use a toggle button control (and add more controls for
> other types when needed) then trying to merge hypothetical toggles into
> something that manually need to be set and disabled.
Using a button control sounds like a good idea. I'll discuss with other people
and reply tomorrow.
>
>>
>> Wu-Cheng Li (1):
>>   v4l: add V4L2_CID_MPEG_VIDEO_FORCE_FRAME_TYPE.
>>
>>  Documentation/DocBook/media/v4l/controls.xml | 23
>> +++++++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-ctrls.c         | 13 +++++++++++++
>>  include/uapi/linux/v4l2-controls.h           |  5 +++++
>>  3 files changed, 41 insertions(+)
>>