[5/6,media] V4L: Add VP8 encoder controls

Commit Message

Arun Kumar K June 10, 2013, 1:23 p.m. UTC

This patch adds new V4L controls for VP8 encoding.

Signed-off-by: Arun Kumar K <[e-mail]>
Signed-off-by: Kiran AVND <[e-mail]>
---
 Documentation/DocBook/media/v4l/controls.xml |  145 ++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c         |   38 +++++++
 include/uapi/linux/v4l2-controls.h           |   30 +++++-
 3 files changed, 212 insertions(+), 1 deletion(-)

Comments

Hans Verkuil June 10, 2013, 1:30 p.m. UTC | #1

Hi Arun,

Some review comments below...

On Mon June 10 2013 15:23:05 Arun Kumar K wrote:
> This patch adds new V4L controls for VP8 encoding.
> 
> Signed-off-by: Arun Kumar K <[e-mail]>
> Signed-off-by: Kiran AVND <[e-mail]>
> ---
>  Documentation/DocBook/media/v4l/controls.xml |  145 ++++++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c         |   38 +++++++
>  include/uapi/linux/v4l2-controls.h           |   30 +++++-
>  3 files changed, 212 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
> index 8d7a779..db614c7 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -4772,4 +4772,149 @@ defines possible values for de-emphasis. Here they are:</entry>
>        </table>
>  
>        </section>
> +
> +    <section id="vpx-controls">
> +      <title>VPX Control Reference</title>
> +
> +      <para>The VPX control class includes controls for encoding parameters
> +      of VPx video codec.</para>

Are these controls defined by the VPx standard, or are they specific to the
Samsung hardware?

I am not certain whether a separate class should be created for these. Adding
it to the MPEG control class might be a better approach (yes, I know MPEG is
a bit of a misnomer, but it already handles other compressions standards as
well).

> +
> +      <table pgwide="1" frame="none" id="fm-rx-control-id">
> +      <title>VPX Control IDs</title>
> +
> +      <tgroup cols="4">
> +        <colspec colname="c1" colwidth="1*" />
> +        <colspec colname="c2" colwidth="6*" />
> +        <colspec colname="c3" colwidth="2*" />
> +        <colspec colname="c4" colwidth="6*" />
> +        <spanspec namest="c1" nameend="c2" spanname="id" />
> +        <spanspec namest="c2" nameend="c4" spanname="descr" />
> +        <thead>
> +          <row>
> +            <entry spanname="id" align="left">ID</entry>
> +            <entry align="left">Type</entry>
> +          </row><row rowsep="1"><entry spanname="descr" align="left">Description</entry>
> +          </row>
> +        </thead>
> +        <tbody valign="top">
> +          <row><entry></entry></row>
> +
> +	      <row><entry></entry></row>
> +	      <row id="v4l2-vpx-num-partitions">
> +		<entry spanname="id"><constant>V4L2_CID_VPX_NUM_PARTITIONS</constant>&nbsp;</entry>
> +		<entry>enum&nbsp;v4l2_vp8_num_partitions</entry>
> +	      </row>
> +	      <row><entry spanname="descr">The number of token partitions to use in VP8 encoder.
> +Possible values are:</entry>
> +	      </row>
> +	      <row>
> +		<entrytbl spanname="descr" cols="2">
> +		  <tbody valign="top">
> +		    <row>
> +		      <entry><constant>V4L2_VPX_1_PARTITION</constant>&nbsp;</entry>
> +		      <entry>1 coefficient partition</entry>
> +		    </row>
> +		    <row>
> +		      <entry><constant>V4L2_VPX_2_PARTITIONS</constant>&nbsp;</entry>
> +		      <entry>2 partitions</entry>
> +		    </row>
> +		    <row>
> +		      <entry><constant>V4L2_VPX_4_PARTITIONS</constant>&nbsp;</entry>
> +		      <entry>4 partitions</entry>
> +		    </row>
> +		    <row>
> +		      <entry><constant>V4L2_VPX_8_PARTITIONS</constant>&nbsp;</entry>
> +		      <entry>8 partitions</entry>
> +	            </row>
> +                  </tbody>
> +		</entrytbl>
> +	      </row>
> +
> +	      <row><entry></entry></row>
> +	      <row>
> +		<entry spanname="id"><constant>V4L2_CID_VPX_IMD_DISABLE_4X4</constant>&nbsp;</entry>
> +		<entry>boolean</entry>
> +	      </row>
> +	      <row><entry spanname="descr">Setting this prevents intra 4x4 mode in the intra mode decision.</entry>
> +	      </row>
> +
> +	      <row><entry></entry></row>
> +	      <row id="v4l2-vpx-num-ref-frames">
> +		<entry spanname="id"><constant>V4L2_CID_VPX_NUM_REF_FRAMES</constant>&nbsp;</entry>
> +		<entry>enum&nbsp;v4l2_vp8_num_ref_frames</entry>
> +	      </row>
> +	      <row><entry spanname="descr">The number of reference pictures for encoding P frames.
> +Possible values are:</entry>
> +	      </row>
> +	      <row>
> +		<entrytbl spanname="descr" cols="2">
> +		  <tbody valign="top">
> +		    <row>
> +		      <entry><constant>V4L2_VPX_1_REF_FRAME</constant>&nbsp;</entry>
> +		      <entry>Last encoded frame will be searched</entry>
> +		    </row>
> +		    <row>
> +		      <entry><constant>V4L2_VPX_2_REF_FRAME</constant>&nbsp;</entry>
> +		      <entry>Last encoded frame and the Golden frame will be searched</entry>
> +		    </row>
> +                  </tbody>
> +		</entrytbl>
> +	      </row>
> +
> +	      <row><entry></entry></row>
> +	      <row>
> +		<entry spanname="id"><constant>V4L2_CID_VPX_FILTER_LEVEL</constant>&nbsp;</entry>
> +		<entry>integer</entry>
> +	      </row>
> +	      <row><entry spanname="descr">Indicates the loop filter level. The adjustment of loop
> +filter level is done via a delta value against a baseline loop filter value.</entry>
> +	      </row>
> +
> +	      <row><entry></entry></row>
> +	      <row>
> +		<entry spanname="id"><constant>V4L2_CID_VPX_FILTER_SHARPNESS</constant>&nbsp;</entry>
> +		<entry>integer</entry>
> +	      </row>
> +	      <row><entry spanname="descr">This parameter affects the loop filter. Anything above
> +zero weakens the deblocking effect on loop filter.</entry>
> +	      </row>
> +
> +	      <row><entry></entry></row>
> +	      <row>
> +		<entry spanname="id"><constant>V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD</constant>&nbsp;</entry>
> +		<entry>integer</entry>
> +	      </row>
> +	      <row><entry spanname="descr">Sets the refresh period for golden frame.</entry>

The period is in number of frames I assume? And is the golden frame taken at
the start or at the end of the period?

> +	      </row>
> +
> +	      <row><entry></entry></row>
> +	      <row id="v4l2-vpx-golden-frame-sel">
> +		<entry spanname="id"><constant>V4L2_CID_VPX_GOLDEN_FRAME_SEL</constant>&nbsp;</entry>
> +		<entry>enum&nbsp;v4l2_vp8_golden_frame_sel</entry>
> +	      </row>
> +	      <row><entry spanname="descr">Selects the golden frame for encoding.
> +Possible values are:</entry>
> +	      </row>
> +	      <row>
> +		<entrytbl spanname="descr" cols="2">
> +		  <tbody valign="top">
> +		    <row>
> +		      <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_PREV</constant>&nbsp;</entry>
> +		      <entry>Use the previous second frame as a golden frame</entry>

Second frame of what? It's not clear to me how I should interpret this.

> +		    </row>
> +		    <row>
> +		      <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD</constant>&nbsp;</entry>
> +		      <entry>Use the previous specific frame indicated by V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD as a golden frame</entry>
> +		    </row>
> +                  </tbody>
> +		</entrytbl>
> +	      </row>
> +
> +          <row><entry></entry></row>
> +        </tbody>
> +      </tgroup>
> +      </table>
> +
> +      </section>
> +
>  </section>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index fccd08b..2cf17d4 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -456,6 +456,23 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"RGB full range (0-255)",
>  		NULL,
>  	};
> +	static const char * const vpx_num_partitions[] = {
> +		"1 partition",
> +		"2 partitions",
> +		"4 partitions",
> +		"8 partitions",

Please use a capital P for Partition.

> +		NULL,
> +	};
> +	static const char * const vpx_num_ref_frames[] = {
> +		"1 reference frame",
> +		"2 reference frame",

frame -> Frames

Capitalize these strings. Same for all the others.

> +		NULL,
> +	};
> +	static const char * const vpx_golden_frame_sel[] = {
> +		"Use previous frame",
> +		"Use frame indicated by GOLDEN_FRAME_REF_PERIOD",
> +		NULL,
> +	};
>  
>  
>  	switch (id) {
> @@ -545,6 +562,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  	case V4L2_CID_DV_TX_RGB_RANGE:
>  	case V4L2_CID_DV_RX_RGB_RANGE:
>  		return dv_rgb_range;
> +	case V4L2_CID_VPX_NUM_PARTITIONS:
> +		return vpx_num_partitions;
> +	case V4L2_CID_VPX_NUM_REF_FRAMES:
> +		return vpx_num_ref_frames;
> +	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
> +		return vpx_golden_frame_sel;
>  
>  	default:
>  		return NULL;
> @@ -806,6 +829,17 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_FM_RX_CLASS:		return "FM Radio Receiver Controls";
>  	case V4L2_CID_TUNE_DEEMPHASIS:		return "De-Emphasis";
>  	case V4L2_CID_RDS_RECEPTION:		return "RDS Reception";
> +
> +	/* VPX controls */
> +	case V4L2_CID_VPX_CLASS:		return "VPX Encoder Controls";
> +	case V4L2_CID_VPX_NUM_PARTITIONS:	return "VPX Number of partitions";
> +	case V4L2_CID_VPX_IMD_DISABLE_4X4:	return "VPX Intra mode decision disable";
> +	case V4L2_CID_VPX_NUM_REF_FRAMES:	return "VPX Number of reference pictures for P frames";

This string is too long: only 31 characters (excluding the trailing \0) can
be used.

> +	case V4L2_CID_VPX_FILTER_LEVEL:		return "VPX Loop filter level range";
> +	case V4L2_CID_VPX_FILTER_SHARPNESS:	return "VPX Deblocking effect control";
> +	case V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD:	return "VPX Golden frame refresh period";
> +	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:	return "VPX Golden frame indicator";
> +
>  	default:
>  		return NULL;
>  	}
> @@ -914,6 +948,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_DV_RX_RGB_RANGE:
>  	case V4L2_CID_TEST_PATTERN:
>  	case V4L2_CID_TUNE_DEEMPHASIS:
> +	case V4L2_CID_VPX_NUM_PARTITIONS:
> +	case V4L2_CID_VPX_NUM_REF_FRAMES:
> +	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
>  		*type = V4L2_CTRL_TYPE_MENU;
>  		break;
>  	case V4L2_CID_LINK_FREQ:
> @@ -937,6 +974,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_IMAGE_PROC_CLASS:
>  	case V4L2_CID_DV_CLASS:
>  	case V4L2_CID_FM_RX_CLASS:
> +	case V4L2_CID_VPX_CLASS:
>  		*type = V4L2_CTRL_TYPE_CTRL_CLASS;
>  		/* You can neither read not write these */
>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 69bd5bb..3d6649c 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -60,6 +60,7 @@
>  #define V4L2_CTRL_CLASS_IMAGE_PROC	0x009f0000	/* Image processing controls */
>  #define V4L2_CTRL_CLASS_DV		0x00a00000	/* Digital Video controls */
>  #define V4L2_CTRL_CLASS_FM_RX		0x00a10000	/* Digital Video controls */
> +#define V4L2_CTRL_CLASS_VPX		0x00a20000	/* VPX-compression controls */
>  
>  /* User-class control IDs */
>  
> @@ -818,7 +819,6 @@ enum v4l2_jpeg_chroma_subsampling {
>  #define V4L2_CID_PIXEL_RATE			(V4L2_CID_IMAGE_PROC_CLASS_BASE + 2)
>  #define V4L2_CID_TEST_PATTERN			(V4L2_CID_IMAGE_PROC_CLASS_BASE + 3)
>  
> -
>  /*  DV-class control IDs defined by V4L2 */
>  #define V4L2_CID_DV_CLASS_BASE			(V4L2_CTRL_CLASS_DV | 0x900)
>  #define V4L2_CID_DV_CLASS			(V4L2_CTRL_CLASS_DV | 1)
> @@ -853,4 +853,32 @@ enum v4l2_deemphasis {
>  
>  #define V4L2_CID_RDS_RECEPTION			(V4L2_CID_FM_RX_CLASS_BASE + 2)
>  
> +/* VP-class control IDs */
> +
> +#define V4L2_CID_VPX_BASE			(V4L2_CTRL_CLASS_VPX | 0x900)
> +#define V4L2_CID_VPX_CLASS			(V4L2_CTRL_CLASS_VPX | 1)
> +
> +/*  VPX streams, specific to multiplexed streams */
> +#define V4L2_CID_VPX_NUM_PARTITIONS		(V4L2_CID_VPX_BASE+0)
> +enum v4l2_vp8_num_partitions {
> +	V4L2_VPX_1_PARTITION	= 0,
> +	V4L2_VPX_2_PARTITIONS	= (1 << 1),
> +	V4L2_VPX_4_PARTITIONS	= (1 << 2),
> +	V4L2_VPX_8_PARTITIONS	= (1 << 3),
> +};
> +#define V4L2_CID_VPX_IMD_DISABLE_4X4		(V4L2_CID_VPX_BASE+1)
> +#define V4L2_CID_VPX_NUM_REF_FRAMES		(V4L2_CID_VPX_BASE+2)
> +enum v4l2_vp8_num_ref_frames {
> +	V4L2_VPX_1_REF_FRAME	= 0,
> +	V4L2_VPX_2_REF_FRAME	= 1,
> +};
> +#define V4L2_CID_VPX_FILTER_LEVEL		(V4L2_CID_VPX_BASE+3)
> +#define V4L2_CID_VPX_FILTER_SHARPNESS		(V4L2_CID_VPX_BASE+4)
> +#define V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD	(V4L2_CID_VPX_BASE+5)
> +#define V4L2_CID_VPX_GOLDEN_FRAME_SEL		(V4L2_CID_VPX_BASE+6)
> +enum v4l2_vp8_golden_frame_sel {
> +	V4L2_VPX_GOLDEN_FRAME_USE_PREV		= 0,
> +	V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD	= 1,
> +};
> +
>  #endif
> 

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics June 10, 2013, 1:45 p.m. UTC | #2

Hi Arun,

On 06/10/2013 03:23 PM, Arun Kumar K wrote:
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index fccd08b..2cf17d4 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -456,6 +456,23 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"RGB full range (0-255)",
>  		NULL,
>  	};
> +	static const char * const vpx_num_partitions[] = {
> +		"1 partition",
> +		"2 partitions",
> +		"4 partitions",
> +		"8 partitions",
> +		NULL,
> +	};
> +	static const char * const vpx_num_ref_frames[] = {
> +		"1 reference frame",
> +		"2 reference frame",
> +		NULL,
> +	};

Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
One example is V4L2_CID_ISO_SENSITIVITY control.

> +/*  VPX streams, specific to multiplexed streams */
> +#define V4L2_CID_VPX_NUM_PARTITIONS		(V4L2_CID_VPX_BASE+0)
> +enum v4l2_vp8_num_partitions {
> +	V4L2_VPX_1_PARTITION	= 0,
> +	V4L2_VPX_2_PARTITIONS	= (1 << 1),
> +	V4L2_VPX_4_PARTITIONS	= (1 << 2),
> +	V4L2_VPX_8_PARTITIONS	= (1 << 3),
> +};

I think we could still have such standard value definitions if needed,
but rather in form of:

#define V4L2_VPX_1_PARTITION	1
#define V4L2_VPX_2_PARTITIONS	2
#define V4L2_VPX_4_PARTITIONS	4
#define V4L2_VPX_8_PARTITIONS	8

> +#define V4L2_CID_VPX_IMD_DISABLE_4X4		(V4L2_CID_VPX_BASE+1)
> +#define V4L2_CID_VPX_NUM_REF_FRAMES		(V4L2_CID_VPX_BASE+2)
> +enum v4l2_vp8_num_ref_frames {
> +	V4L2_VPX_1_REF_FRAME	= 0,
> +	V4L2_VPX_2_REF_FRAME	= 1,
> +};

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arun Kumar K June 11, 2013, 9:13 a.m. UTC | #3

Hi Hans,

Thank you for the review.

>> +
>> +    <section id="vpx-controls">
>> +      <title>VPX Control Reference</title>
>> +
>> +      <para>The VPX control class includes controls for encoding parameters
>> +      of VPx video codec.</para>
>
> Are these controls defined by the VPx standard, or are they specific to the
> Samsung hardware?

These controls are part of VP8 standard and even the reference VP8 encoder
supports these.

>
> I am not certain whether a separate class should be created for these. Adding
> it to the MPEG control class might be a better approach (yes, I know MPEG is
> a bit of a misnomer, but it already handles other compressions standards as
> well).
>

Ok. I added them as a separate control class as VP8 is not from MPEG.
I shall add it along with MPEG in the v2 version..


>> +           <row><entry></entry></row>
>> +           <row>
>> +             <entry spanname="id"><constant>V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD</constant>&nbsp;</entry>
>> +             <entry>integer</entry>
>> +           </row>
>> +           <row><entry spanname="descr">Sets the refresh period for golden frame.</entry>
>
> The period is in number of frames I assume? And is the golden frame taken at
> the start or at the end of the period?
>

Yes its in number of frames.
If we set refresh period as 5, then every 5th frame starting from 1st
frame is made as golden frame.
Will update to make it more clear.


>> +           <row><entry></entry></row>
>> +           <row id="v4l2-vpx-golden-frame-sel">
>> +             <entry spanname="id"><constant>V4L2_CID_VPX_GOLDEN_FRAME_SEL</constant>&nbsp;</entry>
>> +             <entry>enum&nbsp;v4l2_vp8_golden_frame_sel</entry>
>> +           </row>
>> +           <row><entry spanname="descr">Selects the golden frame for encoding.
>> +Possible values are:</entry>
>> +           </row>
>> +           <row>
>> +             <entrytbl spanname="descr" cols="2">
>> +               <tbody valign="top">
>> +                 <row>
>> +                   <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_PREV</constant>&nbsp;</entry>
>> +                   <entry>Use the previous second frame as a golden frame</entry>
>
> Second frame of what? It's not clear to me how I should interpret this.
>

This means the second last frame.
When user selects 2 reference frames for encoding, the last frame and
the golden frame is searched
for reference. With the V4L2_VPX_GOLDEN_FRAME_USE_PREV option, the
last to last frame is
selected as the golden frame. Will update the description for more clarity.

> +                 </row>
> +                 <row>
> +                   <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD</constant>&nbsp;</entry>
> +                   <entry>Use the previous specific frame indicated by V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD as a golden frame</entry>
> +                 </row>
> +                  </tbody>
> +             </entrytbl>
> +           </row>

>> @@ -456,6 +456,23 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>               "RGB full range (0-255)",
>>               NULL,
>>       };
>> +     static const char * const vpx_num_partitions[] = {
>> +             "1 partition",
>> +             "2 partitions",
>> +             "4 partitions",
>> +             "8 partitions",
>
> Please use a capital P for Partition.
>

Ok.

>> +             NULL,
>> +     };
>> +     static const char * const vpx_num_ref_frames[] = {
>> +             "1 reference frame",
>> +             "2 reference frame",
>
> frame -> Frames
>
> Capitalize these strings. Same for all the others.

Ok.

>
>> +             NULL,
>> +     };
>> +     static const char * const vpx_golden_frame_sel[] = {
>> +             "Use previous frame",
>> +             "Use frame indicated by GOLDEN_FRAME_REF_PERIOD",
>> +             NULL,
>> +     };
>>
>>
>>       switch (id) {
>> @@ -545,6 +562,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>       case V4L2_CID_DV_TX_RGB_RANGE:
>>       case V4L2_CID_DV_RX_RGB_RANGE:
>>               return dv_rgb_range;
>> +     case V4L2_CID_VPX_NUM_PARTITIONS:
>> +             return vpx_num_partitions;
>> +     case V4L2_CID_VPX_NUM_REF_FRAMES:
>> +             return vpx_num_ref_frames;
>> +     case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
>> +             return vpx_golden_frame_sel;
>>
>>       default:
>>               return NULL;
>> @@ -806,6 +829,17 @@ const char *v4l2_ctrl_get_name(u32 id)
>>       case V4L2_CID_FM_RX_CLASS:              return "FM Radio Receiver Controls";
>>       case V4L2_CID_TUNE_DEEMPHASIS:          return "De-Emphasis";
>>       case V4L2_CID_RDS_RECEPTION:            return "RDS Reception";
>> +
>> +     /* VPX controls */
>> +     case V4L2_CID_VPX_CLASS:                return "VPX Encoder Controls";
>> +     case V4L2_CID_VPX_NUM_PARTITIONS:       return "VPX Number of partitions";
>> +     case V4L2_CID_VPX_IMD_DISABLE_4X4:      return "VPX Intra mode decision disable";
>> +     case V4L2_CID_VPX_NUM_REF_FRAMES:       return "VPX Number of reference pictures for P frames";
>
> This string is too long: only 31 characters (excluding the trailing \0) can
> be used.

Ok will correct it.


Thanks and regards
Arun
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arun Kumar K June 11, 2013, 9:14 a.m. UTC | #4

Hi Sylwester,

Thank you for the review.

>> +     static const char * const vpx_num_partitions[] = {
>> +             "1 partition",
>> +             "2 partitions",
>> +             "4 partitions",
>> +             "8 partitions",
>> +             NULL,
>> +     };
>> +     static const char * const vpx_num_ref_frames[] = {
>> +             "1 reference frame",
>> +             "2 reference frame",
>> +             NULL,
>> +     };
>
> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
> One example is V4L2_CID_ISO_SENSITIVITY control.

Ok will change it to V4L2_CTRL_TYPE_INTEGER_MENU.

>
>> +/*  VPX streams, specific to multiplexed streams */
>> +#define V4L2_CID_VPX_NUM_PARTITIONS          (V4L2_CID_VPX_BASE+0)
>> +enum v4l2_vp8_num_partitions {
>> +     V4L2_VPX_1_PARTITION    = 0,
>> +     V4L2_VPX_2_PARTITIONS   = (1 << 1),
>> +     V4L2_VPX_4_PARTITIONS   = (1 << 2),
>> +     V4L2_VPX_8_PARTITIONS   = (1 << 3),
>> +};
>
> I think we could still have such standard value definitions if needed,
> but rather in form of:
>
> #define V4L2_VPX_1_PARTITION    1
> #define V4L2_VPX_2_PARTITIONS   2
> #define V4L2_VPX_4_PARTITIONS   4
> #define V4L2_VPX_8_PARTITIONS   8
>

Ok will change.

Regards
Arun
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arun Kumar K June 14, 2013, 9:26 a.m. UTC | #5

Hi Sylwester,

>> +     static const char * const vpx_num_partitions[] = {
>> +             "1 partition",
>> +             "2 partitions",
>> +             "4 partitions",
>> +             "8 partitions",
>> +             NULL,
>> +     };
>> +     static const char * const vpx_num_ref_frames[] = {
>> +             "1 reference frame",
>> +             "2 reference frame",
>> +             NULL,
>> +     };
>
> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
> One example is V4L2_CID_ISO_SENSITIVITY control.
>

If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
controls where
the driver / IP can support different values depending on its capabilities.
But here VP8 standard supports only 4 options for no. of partitions
that is 1, 2, 4 and 8.
Also for number of ref frames, the standard allows only the options 1,
2 and 3 which
cannot be extended more. So is it correct to use INTEGER_MENU control here and
let the driver define the values?

Regards
Arun
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics June 14, 2013, 9:53 a.m. UTC | #6

Hi Arun,

On 06/14/2013 11:26 AM, Arun Kumar K wrote:
> Hi Sylwester,
> 
>>> +     static const char * const vpx_num_partitions[] = {
>>> +             "1 partition",
>>> +             "2 partitions",
>>> +             "4 partitions",
>>> +             "8 partitions",
>>> +             NULL,
>>> +     };
>>> +     static const char * const vpx_num_ref_frames[] = {
>>> +             "1 reference frame",
>>> +             "2 reference frame",
>>> +             NULL,
>>> +     };
>>
>> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
>> One example is V4L2_CID_ISO_SENSITIVITY control.
>>
> 
> If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
> controls where
> the driver / IP can support different values depending on its capabilities.

No, not really, it just happens there is no INTEGER_MENU control with standard
values yet. I think there are some (minor) changes needed in the v4l2-ctrls
code to support INTEGER_MENU control with standard menu items.

> But here VP8 standard supports only 4 options for no. of partitions
> that is 1, 2, 4 and 8.

I think such a standard menu list should be defined in v4l2-ctrls.c then.

> Also for number of ref frames, the standard allows only the options 1,
> 2 and 3 which
> cannot be extended more. So is it correct to use INTEGER_MENU control here and
> let the driver define the values?

If this is standard then the core should define available menu items. But
it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
opinions though.

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arun Kumar K June 14, 2013, 1:21 p.m. UTC | #7

Hi Sylwester,

On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
<[e-mail]> wrote:
> Hi Arun,
>
> On 06/14/2013 11:26 AM, Arun Kumar K wrote:
>> Hi Sylwester,
>>
>>>> +     static const char * const vpx_num_partitions[] = {
>>>> +             "1 partition",
>>>> +             "2 partitions",
>>>> +             "4 partitions",
>>>> +             "8 partitions",
>>>> +             NULL,
>>>> +     };
>>>> +     static const char * const vpx_num_ref_frames[] = {
>>>> +             "1 reference frame",
>>>> +             "2 reference frame",
>>>> +             NULL,
>>>> +     };
>>>
>>> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
>>> One example is V4L2_CID_ISO_SENSITIVITY control.
>>>
>>
>> If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
>> controls where
>> the driver / IP can support different values depending on its capabilities.
>
> No, not really, it just happens there is no INTEGER_MENU control with standard
> values yet. I think there are some (minor) changes needed in the v4l2-ctrls
> code to support INTEGER_MENU control with standard menu items.
>
>> But here VP8 standard supports only 4 options for no. of partitions
>> that is 1, 2, 4 and 8.
>
> I think such a standard menu list should be defined in v4l2-ctrls.c then.

One more concern here is these integer values 1, 2, 4 and 8 may not hold
much significance while setting to the registers. Some IPs may need these
values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
values that are given by the user may not be directly applicable to the
register setting.

>
>> Also for number of ref frames, the standard allows only the options 1,
>> 2 and 3 which
>> cannot be extended more. So is it correct to use INTEGER_MENU control here and
>> let the driver define the values?
>
> If this is standard then the core should define available menu items. But
> it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
> opinions though.


Here even though 1,2 and 3 are standard, the interpretation is
1 - 1 reference frame (previous frame)
2 - previous frame + golden frame
3 - previous frame + golden frame + altref frame.

Again the driver may need to set different registers based on these and the
integer values as such may not be used.


Regards
Arun
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics June 14, 2013, 7:31 p.m. UTC | #8

Hi Arun,

On 06/14/2013 03:21 PM, Arun Kumar K wrote:
> On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
> <[e-mail]> wrote:
>> On 06/14/2013 11:26 AM, Arun Kumar K wrote:
>>>>> +     static const char * const vpx_num_partitions[] = {
>>>>> +             "1 partition",
>>>>> +             "2 partitions",
>>>>> +             "4 partitions",
>>>>> +             "8 partitions",
>>>>> +             NULL,
>>>>> +     };
>>>>> +     static const char * const vpx_num_ref_frames[] = {
>>>>> +             "1 reference frame",
>>>>> +             "2 reference frame",
>>>>> +             NULL,
>>>>> +     };
>>>>
>>>> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
>>>> One example is V4L2_CID_ISO_SENSITIVITY control.
>>>>
>>>
>>> If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
>>> controls where
>>> the driver / IP can support different values depending on its capabilities.
>>
>> No, not really, it just happens there is no INTEGER_MENU control with standard
>> values yet. I think there are some (minor) changes needed in the v4l2-ctrls
>> code to support INTEGER_MENU control with standard menu items.
>>
>>> But here VP8 standard supports only 4 options for no. of partitions
>>> that is 1, 2, 4 and 8.
>>
>> I think such a standard menu list should be defined in v4l2-ctrls.c then.
> 
> One more concern here is these integer values 1, 2, 4 and 8 may not hold
> much significance while setting to the registers. Some IPs may need these
> values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
> values that are given by the user may not be directly applicable to the
> register setting.

The INTEGER_MENU control is not much different than MENU control from
driver POV. s_ctrl() op is called with similarly with the an index to
the menu option. In addition to standard menu applications can query
real value corresponding to a menu option, rather than a character
string only.

With each new control a nw strings are added, that cumulate in the
videodev module and make it bigger. Actually __s64 is not much smaller
than "1 partition" in your case. But it's a bit smaller. :)

That said I'm not either a codec expert or the v4l2 controls maintainer.
I think last words belongs to Hans. I just express my suggestions of
what I though we be more optimal (but not necessarily less work!). :)

>>> Also for number of ref frames, the standard allows only the options 1,
>>> 2 and 3 which
>>> cannot be extended more. So is it correct to use INTEGER_MENU control here and
>>> let the driver define the values?
>>
>> If this is standard then the core should define available menu items. But
>> it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
>> opinions though.
> 
> Here even though 1,2 and 3 are standard, the interpretation is
> 1 - 1 reference frame (previous frame)
> 2 - previous frame + golden frame
> 3 - previous frame + golden frame + altref frame.

OK, then perhaps for this parameter a standard menu control would be better.
However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
You probably want to change the menu strings to reflect this more precisely,
but there might be not enough room for any creative names anyway. Maybe
something like:

static const char * const vpx_num_ref_frames[] = {
	"Previous Frame",
	"Previous + Golden Frame",
	"Prev + Golden + Altref Frame",
	NULL,
};

Anyway raw numbers might be better for the control and details could be
described in the documentation.

> Again the driver may need to set different registers based on these and the
> integer values as such may not be used.

This is really not relevant, the driver can map index of the menu to any
value that is needed by the hardware.

Regards,
Sylwester

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arun Kumar K June 17, 2013, 4:25 a.m. UTC | #9

Hi Sylwester,

On Sat, Jun 15, 2013 at 1:01 AM, Sylwester Nawrocki
<[e-mail]> wrote:
> Hi Arun,
>
> On 06/14/2013 03:21 PM, Arun Kumar K wrote:
>> On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
>> <[e-mail]> wrote:
>>> On 06/14/2013 11:26 AM, Arun Kumar K wrote:
>>>>>> +     static const char * const vpx_num_partitions[] = {
>>>>>> +             "1 partition",
>>>>>> +             "2 partitions",
>>>>>> +             "4 partitions",
>>>>>> +             "8 partitions",
>>>>>> +             NULL,
>>>>>> +     };
>>>>>> +     static const char * const vpx_num_ref_frames[] = {
>>>>>> +             "1 reference frame",
>>>>>> +             "2 reference frame",
>>>>>> +             NULL,
>>>>>> +     };
>>>>>
>>>>> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
>>>>> One example is V4L2_CID_ISO_SENSITIVITY control.
>>>>>
>>>>
>>>> If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
>>>> controls where
>>>> the driver / IP can support different values depending on its capabilities.
>>>
>>> No, not really, it just happens there is no INTEGER_MENU control with standard
>>> values yet. I think there are some (minor) changes needed in the v4l2-ctrls
>>> code to support INTEGER_MENU control with standard menu items.
>>>
>>>> But here VP8 standard supports only 4 options for no. of partitions
>>>> that is 1, 2, 4 and 8.
>>>
>>> I think such a standard menu list should be defined in v4l2-ctrls.c then.
>>
>> One more concern here is these integer values 1, 2, 4 and 8 may not hold
>> much significance while setting to the registers. Some IPs may need these
>> values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
>> values that are given by the user may not be directly applicable to the
>> register setting.
>
> The INTEGER_MENU control is not much different than MENU control from
> driver POV. s_ctrl() op is called with similarly with the an index to
> the menu option. In addition to standard menu applications can query
> real value corresponding to a menu option, rather than a character
> string only.
>
> With each new control a nw strings are added, that cumulate in the
> videodev module and make it bigger. Actually __s64 is not much smaller
> than "1 partition" in your case. But it's a bit smaller. :)
>

Yes that makes sense. But there will be a few extra functions that
would be needed in the v4l2-ctrls.c like may be
v4l2_ctrl_new_int_menu_fixed() which doesnt take qmenu arg from driver.
Will try to make this change.

> That said I'm not either a codec expert or the v4l2 controls maintainer.
> I think last words belongs to Hans. I just express my suggestions of
> what I though we be more optimal (but not necessarily less work!). :)
>
>>>> Also for number of ref frames, the standard allows only the options 1,
>>>> 2 and 3 which
>>>> cannot be extended more. So is it correct to use INTEGER_MENU control here and
>>>> let the driver define the values?
>>>
>>> If this is standard then the core should define available menu items. But
>>> it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
>>> opinions though.
>>
>> Here even though 1,2 and 3 are standard, the interpretation is
>> 1 - 1 reference frame (previous frame)
>> 2 - previous frame + golden frame
>> 3 - previous frame + golden frame + altref frame.
>
> OK, then perhaps for this parameter a standard menu control would be better.
> However, why there are only 2 options in vpx_num_ref_frames[] arrays ?

Thats because MFCv7 doesnt support the 3rd option yet. But still I would
add this in the control to make it generic.

> You probably want to change the menu strings to reflect this more precisely,
> but there might be not enough room for any creative names anyway. Maybe
> something like:
>
> static const char * const vpx_num_ref_frames[] = {
>         "Previous Frame",
>         "Previous + Golden Frame",
>         "Prev + Golden + Altref Frame",
>         NULL,
> };
>
> Anyway raw numbers might be better for the control and details could be
> described in the documentation.

Ok will do like this.

Regards
Arun
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arun Kumar K June 17, 2013, 6:18 a.m. UTC | #10

Hi Sylwester,

>
>>>> Also for number of ref frames, the standard allows only the options 1,
>>>> 2 and 3 which
>>>> cannot be extended more. So is it correct to use INTEGER_MENU control here and
>>>> let the driver define the values?
>>>
>>> If this is standard then the core should define available menu items. But
>>> it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
>>> opinions though.
>>
>> Here even though 1,2 and 3 are standard, the interpretation is
>> 1 - 1 reference frame (previous frame)
>> 2 - previous frame + golden frame
>> 3 - previous frame + golden frame + altref frame.
>
> OK, then perhaps for this parameter a standard menu control would be better.
> However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
> You probably want to change the menu strings to reflect this more precisely,
> but there might be not enough room for any creative names anyway. Maybe
> something like:
>
> static const char * const vpx_num_ref_frames[] = {
>         "Previous Frame",
>         "Previous + Golden Frame",
>         "Prev + Golden + Altref Frame",
>         NULL,
> };
>

On a more detailed inspection, the standard says maximum of 3 reference
frames. So in case of 2, it can be any of the permutation combination
possible. So rather I will stick to integer menu items saying 1, 2 and 3 where
on setting value 2, the encoder can decide on which frames to refer based
on its implementation but keeping the searching limit to 2 frames only.

Regards
Arun
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics June 17, 2013, 9:04 a.m. UTC | #11

Hi Arun,

On 06/17/2013 06:25 AM, Arun Kumar K wrote:
> On Sat, Jun 15, 2013 at 1:01 AM, Sylwester Nawrocki
> <[e-mail]> wrote:
>> On 06/14/2013 03:21 PM, Arun Kumar K wrote:
>>> On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
>>> <[e-mail]> wrote:
>>>> On 06/14/2013 11:26 AM, Arun Kumar K wrote:
>>>>>>> +     static const char * const vpx_num_partitions[] = {
>>>>>>> +             "1 partition",
>>>>>>> +             "2 partitions",
>>>>>>> +             "4 partitions",
>>>>>>> +             "8 partitions",
>>>>>>> +             NULL,
>>>>>>> +     };
>>>>>>> +     static const char * const vpx_num_ref_frames[] = {
>>>>>>> +             "1 reference frame",
>>>>>>> +             "2 reference frame",
>>>>>>> +             NULL,
>>>>>>> +     };
>>>>>>
>>>>>> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
>>>>>> One example is V4L2_CID_ISO_SENSITIVITY control.
>>>>>>
>>>>>
>>>>> If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
>>>>> controls where
>>>>> the driver / IP can support different values depending on its capabilities.
>>>>
>>>> No, not really, it just happens there is no INTEGER_MENU control with standard
>>>> values yet. I think there are some (minor) changes needed in the v4l2-ctrls
>>>> code to support INTEGER_MENU control with standard menu items.
>>>>
>>>>> But here VP8 standard supports only 4 options for no. of partitions
>>>>> that is 1, 2, 4 and 8.
>>>>
>>>> I think such a standard menu list should be defined in v4l2-ctrls.c then.
>>>
>>> One more concern here is these integer values 1, 2, 4 and 8 may not hold
>>> much significance while setting to the registers. Some IPs may need these
>>> values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
>>> values that are given by the user may not be directly applicable to the
>>> register setting.
>>
>> The INTEGER_MENU control is not much different than MENU control from
>> driver POV. s_ctrl() op is called with similarly with the an index to
>> the menu option. In addition to standard menu applications can query
>> real value corresponding to a menu option, rather than a character
>> string only.
>>
>> With each new control a nw strings are added, that cumulate in the
>> videodev module and make it bigger. Actually __s64 is not much smaller
>> than "1 partition" in your case. But it's a bit smaller. :)
> 
> Yes that makes sense. But there will be a few extra functions that
> would be needed in the v4l2-ctrls.c like may be
> v4l2_ctrl_new_int_menu_fixed() which doesnt take qmenu arg from driver.
> Will try to make this change.

I wouldn't be adding another helper like this, IMHO v4l2_ctrl_new_menu()
could well handle such entirely standard control type. I looked into that
over the weekend, let me send you my work-in-progress patch. Maybe you find
it useful.

>> That said I'm not either a codec expert or the v4l2 controls maintainer.
>> I think last words belongs to Hans. I just express my suggestions of
>> what I though we be more optimal (but not necessarily less work!). :)
>>
>>>>> Also for number of ref frames, the standard allows only the options 1,
>>>>> 2 and 3 which
>>>>> cannot be extended more. So is it correct to use INTEGER_MENU control here and
>>>>> let the driver define the values?
>>>>
>>>> If this is standard then the core should define available menu items. But
>>>> it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
>>>> opinions though.
>>>
>>> Here even though 1,2 and 3 are standard, the interpretation is
>>> 1 - 1 reference frame (previous frame)
>>> 2 - previous frame + golden frame
>>> 3 - previous frame + golden frame + altref frame.
>>
>> OK, then perhaps for this parameter a standard menu control would be better.
>> However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
> 
> Thats because MFCv7 doesnt support the 3rd option yet. But still I would
> add this in the control to make it generic.

I see. Yes, I think it makes more sense to specify the control fully,
according to the standard.

>> You probably want to change the menu strings to reflect this more precisely,
>> but there might be not enough room for any creative names anyway. Maybe
>> something like:
>>
>> static const char * const vpx_num_ref_frames[] = {
>>         "Previous Frame",
>>         "Previous + Golden Frame",
>>         "Prev + Golden + Altref Frame",
>>         NULL,
>> };
>>
>> Anyway raw numbers might be better for the control and details could be
>> described in the documentation.
> 
> Ok will do like this.

Just one more note, I think I might have confused you with my comment
on the enum v4l2_vp8_num_partitions. Presumably we just need to define
contiguous enumeration for all options of V4L2_CID_VPX_NUM_PARTITIONS
control. And the actual values would be defined only on the integer
menu values array, e.g.

copnst s64 qmenu_int_vpx_num_partitions[] [
	1, 2, 4, 8
};

and

#define V4L2_CID_VPX_NUM_PARTITIONS	(V4L2_CID_MPEG_BASE + ?)
enum v4l2_vp8_num_partitions {
	V4L2_VPX_1_PARTITION	= 0,
	V4L2_VPX_2_PARTITIONS	= 1,
	V4L2_VPX_4_PARTITIONS	= 2,
	V4L2_VPX_8_PARTITIONS	= 3,
};

or

#define V4L2_CID_VPX_NUM_PARTITIONS	(V4L2_CID_MPEG_BASE + ?)
#define V4L2_VPX_1_PARTITION	0
#define V4L2_VPX_2_PARTITIONS	1
#define V4L2_VPX_4_PARTITIONS	2
#define V4L2_VPX_8_PARTITIONS	3

Each driver could then map enum v4l2_vp8_num_partitions onto its
required H/W register values, without having to translate from
the MFC specific values. :)

Regards,
Sylwester


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arun Kumar K June 17, 2013, 9:25 a.m. UTC | #12

Hi Sylwester,


On Mon, Jun 17, 2013 at 2:34 PM, Sylwester Nawrocki
<[e-mail]> wrote:
> On 06/17/2013 06:25 AM, Arun Kumar K wrote:
>> On Sat, Jun 15, 2013 at 1:01 AM, Sylwester Nawrocki
>> <[e-mail]> wrote:
>>> On 06/14/2013 03:21 PM, Arun Kumar K wrote:
>>>> On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
>>>> <[e-mail]> wrote:
>>>>> On 06/14/2013 11:26 AM, Arun Kumar K wrote:
>>>>>>>> +     static const char * const vpx_num_partitions[] = {
>>>>>>>> +             "1 partition",
>>>>>>>> +             "2 partitions",
>>>>>>>> +             "4 partitions",
>>>>>>>> +             "8 partitions",
>>>>>>>> +             NULL,
>>>>>>>> +     };
>>>>>>>> +     static const char * const vpx_num_ref_frames[] = {
>>>>>>>> +             "1 reference frame",
>>>>>>>> +             "2 reference frame",
>>>>>>>> +             NULL,
>>>>>>>> +     };
>>>>>>>
>>>>>>> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
>>>>>>> One example is V4L2_CID_ISO_SENSITIVITY control.
>>>>>>>
>>>>>>
>>>>>> If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
>>>>>> controls where
>>>>>> the driver / IP can support different values depending on its capabilities.
>>>>>
>>>>> No, not really, it just happens there is no INTEGER_MENU control with standard
>>>>> values yet. I think there are some (minor) changes needed in the v4l2-ctrls
>>>>> code to support INTEGER_MENU control with standard menu items.
>>>>>
>>>>>> But here VP8 standard supports only 4 options for no. of partitions
>>>>>> that is 1, 2, 4 and 8.
>>>>>
>>>>> I think such a standard menu list should be defined in v4l2-ctrls.c then.
>>>>
>>>> One more concern here is these integer values 1, 2, 4 and 8 may not hold
>>>> much significance while setting to the registers. Some IPs may need these
>>>> values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
>>>> values that are given by the user may not be directly applicable to the
>>>> register setting.
>>>
>>> The INTEGER_MENU control is not much different than MENU control from
>>> driver POV. s_ctrl() op is called with similarly with the an index to
>>> the menu option. In addition to standard menu applications can query
>>> real value corresponding to a menu option, rather than a character
>>> string only.
>>>
>>> With each new control a nw strings are added, that cumulate in the
>>> videodev module and make it bigger. Actually __s64 is not much smaller
>>> than "1 partition" in your case. But it's a bit smaller. :)
>>
>> Yes that makes sense. But there will be a few extra functions that
>> would be needed in the v4l2-ctrls.c like may be
>> v4l2_ctrl_new_int_menu_fixed() which doesnt take qmenu arg from driver.
>> Will try to make this change.
>
> I wouldn't be adding another helper like this, IMHO v4l2_ctrl_new_menu()
> could well handle such entirely standard control type. I looked into that
> over the weekend, let me send you my work-in-progress patch. Maybe you find
> it useful.


Ok that would be really helpful. Will check that patch.

>
>>> That said I'm not either a codec expert or the v4l2 controls maintainer.
>>> I think last words belongs to Hans. I just express my suggestions of
>>> what I though we be more optimal (but not necessarily less work!). :)
>>>
>>>>>> Also for number of ref frames, the standard allows only the options 1,
>>>>>> 2 and 3 which
>>>>>> cannot be extended more. So is it correct to use INTEGER_MENU control here and
>>>>>> let the driver define the values?
>>>>>
>>>>> If this is standard then the core should define available menu items. But
>>>>> it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
>>>>> opinions though.
>>>>
>>>> Here even though 1,2 and 3 are standard, the interpretation is
>>>> 1 - 1 reference frame (previous frame)
>>>> 2 - previous frame + golden frame
>>>> 3 - previous frame + golden frame + altref frame.
>>>
>>> OK, then perhaps for this parameter a standard menu control would be better.
>>> However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
>>
>> Thats because MFCv7 doesnt support the 3rd option yet. But still I would
>> add this in the control to make it generic.
>
> I see. Yes, I think it makes more sense to specify the control fully,
> according to the standard.
>
>>> You probably want to change the menu strings to reflect this more precisely,
>>> but there might be not enough room for any creative names anyway. Maybe
>>> something like:
>>>
>>> static const char * const vpx_num_ref_frames[] = {
>>>         "Previous Frame",
>>>         "Previous + Golden Frame",
>>>         "Prev + Golden + Altref Frame",
>>>         NULL,
>>> };
>>>
>>> Anyway raw numbers might be better for the control and details could be
>>> described in the documentation.
>>
>> Ok will do like this.
>
> Just one more note, I think I might have confused you with my comment
> on the enum v4l2_vp8_num_partitions. Presumably we just need to define
> contiguous enumeration for all options of V4L2_CID_VPX_NUM_PARTITIONS
> control. And the actual values would be defined only on the integer
> menu values array, e.g.
>
> copnst s64 qmenu_int_vpx_num_partitions[] [
>         1, 2, 4, 8
> };
>
> and
>
> #define V4L2_CID_VPX_NUM_PARTITIONS     (V4L2_CID_MPEG_BASE + ?)
> enum v4l2_vp8_num_partitions {
>         V4L2_VPX_1_PARTITION    = 0,
>         V4L2_VPX_2_PARTITIONS   = 1,
>         V4L2_VPX_4_PARTITIONS   = 2,
>         V4L2_VPX_8_PARTITIONS   = 3,
> };
>
> or
>
> #define V4L2_CID_VPX_NUM_PARTITIONS     (V4L2_CID_MPEG_BASE + ?)
> #define V4L2_VPX_1_PARTITION    0
> #define V4L2_VPX_2_PARTITIONS   1
> #define V4L2_VPX_4_PARTITIONS   2
> #define V4L2_VPX_8_PARTITIONS   3
>
> Each driver could then map enum v4l2_vp8_num_partitions onto its
> required H/W register values, without having to translate from
> the MFC specific values. :)


Ok got it now. Thanks for the wonderful explanation :)

Regards
Arun
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index 8d7a779..db614c7 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -4772,4 +4772,149 @@  defines possible values for de-emphasis. Here they are:</entry>
       </table>
 
       </section>
+
+    <section id="vpx-controls">
+      <title>VPX Control Reference</title>
+
+      <para>The VPX control class includes controls for encoding parameters
+      of VPx video codec.</para>
+
+      <table pgwide="1" frame="none" id="fm-rx-control-id">
+      <title>VPX Control IDs</title>
+
+      <tgroup cols="4">
+        <colspec colname="c1" colwidth="1*" />
+        <colspec colname="c2" colwidth="6*" />
+        <colspec colname="c3" colwidth="2*" />
+        <colspec colname="c4" colwidth="6*" />
+        <spanspec namest="c1" nameend="c2" spanname="id" />
+        <spanspec namest="c2" nameend="c4" spanname="descr" />
+        <thead>
+          <row>
+            <entry spanname="id" align="left">ID</entry>
+            <entry align="left">Type</entry>
+          </row><row rowsep="1"><entry spanname="descr" align="left">Description</entry>
+          </row>
+        </thead>
+        <tbody valign="top">
+          <row><entry></entry></row>
+
+	      <row><entry></entry></row>
+	      <row id="v4l2-vpx-num-partitions">
+		<entry spanname="id"><constant>V4L2_CID_VPX_NUM_PARTITIONS</constant>&nbsp;</entry>
+		<entry>enum&nbsp;v4l2_vp8_num_partitions</entry>
+	      </row>
+	      <row><entry spanname="descr">The number of token partitions to use in VP8 encoder.
+Possible values are:</entry>
+	      </row>
+	      <row>
+		<entrytbl spanname="descr" cols="2">
+		  <tbody valign="top">
+		    <row>
+		      <entry><constant>V4L2_VPX_1_PARTITION</constant>&nbsp;</entry>
+		      <entry>1 coefficient partition</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_VPX_2_PARTITIONS</constant>&nbsp;</entry>
+		      <entry>2 partitions</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_VPX_4_PARTITIONS</constant>&nbsp;</entry>
+		      <entry>4 partitions</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_VPX_8_PARTITIONS</constant>&nbsp;</entry>
+		      <entry>8 partitions</entry>
+	            </row>
+                  </tbody>
+		</entrytbl>
+	      </row>
+
+	      <row><entry></entry></row>
+	      <row>
+		<entry spanname="id"><constant>V4L2_CID_VPX_IMD_DISABLE_4X4</constant>&nbsp;</entry>
+		<entry>boolean</entry>
+	      </row>
+	      <row><entry spanname="descr">Setting this prevents intra 4x4 mode in the intra mode decision.</entry>
+	      </row>
+
+	      <row><entry></entry></row>
+	      <row id="v4l2-vpx-num-ref-frames">
+		<entry spanname="id"><constant>V4L2_CID_VPX_NUM_REF_FRAMES</constant>&nbsp;</entry>
+		<entry>enum&nbsp;v4l2_vp8_num_ref_frames</entry>
+	      </row>
+	      <row><entry spanname="descr">The number of reference pictures for encoding P frames.
+Possible values are:</entry>
+	      </row>
+	      <row>
+		<entrytbl spanname="descr" cols="2">
+		  <tbody valign="top">
+		    <row>
+		      <entry><constant>V4L2_VPX_1_REF_FRAME</constant>&nbsp;</entry>
+		      <entry>Last encoded frame will be searched</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_VPX_2_REF_FRAME</constant>&nbsp;</entry>
+		      <entry>Last encoded frame and the Golden frame will be searched</entry>
+		    </row>
+                  </tbody>
+		</entrytbl>
+	      </row>
+
+	      <row><entry></entry></row>
+	      <row>
+		<entry spanname="id"><constant>V4L2_CID_VPX_FILTER_LEVEL</constant>&nbsp;</entry>
+		<entry>integer</entry>
+	      </row>
+	      <row><entry spanname="descr">Indicates the loop filter level. The adjustment of loop
+filter level is done via a delta value against a baseline loop filter value.</entry>
+	      </row>
+
+	      <row><entry></entry></row>
+	      <row>
+		<entry spanname="id"><constant>V4L2_CID_VPX_FILTER_SHARPNESS</constant>&nbsp;</entry>
+		<entry>integer</entry>
+	      </row>
+	      <row><entry spanname="descr">This parameter affects the loop filter. Anything above
+zero weakens the deblocking effect on loop filter.</entry>
+	      </row>
+
+	      <row><entry></entry></row>
+	      <row>
+		<entry spanname="id"><constant>V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD</constant>&nbsp;</entry>
+		<entry>integer</entry>
+	      </row>
+	      <row><entry spanname="descr">Sets the refresh period for golden frame.</entry>
+	      </row>
+
+	      <row><entry></entry></row>
+	      <row id="v4l2-vpx-golden-frame-sel">
+		<entry spanname="id"><constant>V4L2_CID_VPX_GOLDEN_FRAME_SEL</constant>&nbsp;</entry>
+		<entry>enum&nbsp;v4l2_vp8_golden_frame_sel</entry>
+	      </row>
+	      <row><entry spanname="descr">Selects the golden frame for encoding.
+Possible values are:</entry>
+	      </row>
+	      <row>
+		<entrytbl spanname="descr" cols="2">
+		  <tbody valign="top">
+		    <row>
+		      <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_PREV</constant>&nbsp;</entry>
+		      <entry>Use the previous second frame as a golden frame</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD</constant>&nbsp;</entry>
+		      <entry>Use the previous specific frame indicated by V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD as a golden frame</entry>
+		    </row>
+                  </tbody>
+		</entrytbl>
+	      </row>
+
+          <row><entry></entry></row>
+        </tbody>
+      </tgroup>
+      </table>
+
+      </section>
+
 </section>
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index fccd08b..2cf17d4 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -456,6 +456,23 @@  const char * const *v4l2_ctrl_get_menu(u32 id)
 		"RGB full range (0-255)",
 		NULL,
 	};
+	static const char * const vpx_num_partitions[] = {
+		"1 partition",
+		"2 partitions",
+		"4 partitions",
+		"8 partitions",
+		NULL,
+	};
+	static const char * const vpx_num_ref_frames[] = {
+		"1 reference frame",
+		"2 reference frame",
+		NULL,
+	};
+	static const char * const vpx_golden_frame_sel[] = {
+		"Use previous frame",
+		"Use frame indicated by GOLDEN_FRAME_REF_PERIOD",
+		NULL,
+	};
 
 
 	switch (id) {
@@ -545,6 +562,12 @@  const char * const *v4l2_ctrl_get_menu(u32 id)
 	case V4L2_CID_DV_TX_RGB_RANGE:
 	case V4L2_CID_DV_RX_RGB_RANGE:
 		return dv_rgb_range;
+	case V4L2_CID_VPX_NUM_PARTITIONS:
+		return vpx_num_partitions;
+	case V4L2_CID_VPX_NUM_REF_FRAMES:
+		return vpx_num_ref_frames;
+	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
+		return vpx_golden_frame_sel;
 
 	default:
 		return NULL;
@@ -806,6 +829,17 @@  const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_FM_RX_CLASS:		return "FM Radio Receiver Controls";
 	case V4L2_CID_TUNE_DEEMPHASIS:		return "De-Emphasis";
 	case V4L2_CID_RDS_RECEPTION:		return "RDS Reception";
+
+	/* VPX controls */
+	case V4L2_CID_VPX_CLASS:		return "VPX Encoder Controls";
+	case V4L2_CID_VPX_NUM_PARTITIONS:	return "VPX Number of partitions";
+	case V4L2_CID_VPX_IMD_DISABLE_4X4:	return "VPX Intra mode decision disable";
+	case V4L2_CID_VPX_NUM_REF_FRAMES:	return "VPX Number of reference pictures for P frames";
+	case V4L2_CID_VPX_FILTER_LEVEL:		return "VPX Loop filter level range";
+	case V4L2_CID_VPX_FILTER_SHARPNESS:	return "VPX Deblocking effect control";
+	case V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD:	return "VPX Golden frame refresh period";
+	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:	return "VPX Golden frame indicator";
+
 	default:
 		return NULL;
 	}
@@ -914,6 +948,9 @@  void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_DV_RX_RGB_RANGE:
 	case V4L2_CID_TEST_PATTERN:
 	case V4L2_CID_TUNE_DEEMPHASIS:
+	case V4L2_CID_VPX_NUM_PARTITIONS:
+	case V4L2_CID_VPX_NUM_REF_FRAMES:
+	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
 		*type = V4L2_CTRL_TYPE_MENU;
 		break;
 	case V4L2_CID_LINK_FREQ:
@@ -937,6 +974,7 @@  void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_IMAGE_PROC_CLASS:
 	case V4L2_CID_DV_CLASS:
 	case V4L2_CID_FM_RX_CLASS:
+	case V4L2_CID_VPX_CLASS:
 		*type = V4L2_CTRL_TYPE_CTRL_CLASS;
 		/* You can neither read not write these */
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 69bd5bb..3d6649c 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -60,6 +60,7 @@ 
 #define V4L2_CTRL_CLASS_IMAGE_PROC	0x009f0000	/* Image processing controls */
 #define V4L2_CTRL_CLASS_DV		0x00a00000	/* Digital Video controls */
 #define V4L2_CTRL_CLASS_FM_RX		0x00a10000	/* Digital Video controls */
+#define V4L2_CTRL_CLASS_VPX		0x00a20000	/* VPX-compression controls */
 
 /* User-class control IDs */
 
@@ -818,7 +819,6 @@  enum v4l2_jpeg_chroma_subsampling {
 #define V4L2_CID_PIXEL_RATE			(V4L2_CID_IMAGE_PROC_CLASS_BASE + 2)
 #define V4L2_CID_TEST_PATTERN			(V4L2_CID_IMAGE_PROC_CLASS_BASE + 3)
 
-
 /*  DV-class control IDs defined by V4L2 */
 #define V4L2_CID_DV_CLASS_BASE			(V4L2_CTRL_CLASS_DV | 0x900)
 #define V4L2_CID_DV_CLASS			(V4L2_CTRL_CLASS_DV | 1)
@@ -853,4 +853,32 @@  enum v4l2_deemphasis {
 
 #define V4L2_CID_RDS_RECEPTION			(V4L2_CID_FM_RX_CLASS_BASE + 2)
 
+/* VP-class control IDs */
+
+#define V4L2_CID_VPX_BASE			(V4L2_CTRL_CLASS_VPX | 0x900)
+#define V4L2_CID_VPX_CLASS			(V4L2_CTRL_CLASS_VPX | 1)
+
+/*  VPX streams, specific to multiplexed streams */
+#define V4L2_CID_VPX_NUM_PARTITIONS		(V4L2_CID_VPX_BASE+0)
+enum v4l2_vp8_num_partitions {
+	V4L2_VPX_1_PARTITION	= 0,
+	V4L2_VPX_2_PARTITIONS	= (1 << 1),
+	V4L2_VPX_4_PARTITIONS	= (1 << 2),
+	V4L2_VPX_8_PARTITIONS	= (1 << 3),
+};
+#define V4L2_CID_VPX_IMD_DISABLE_4X4		(V4L2_CID_VPX_BASE+1)
+#define V4L2_CID_VPX_NUM_REF_FRAMES		(V4L2_CID_VPX_BASE+2)
+enum v4l2_vp8_num_ref_frames {
+	V4L2_VPX_1_REF_FRAME	= 0,
+	V4L2_VPX_2_REF_FRAME	= 1,
+};
+#define V4L2_CID_VPX_FILTER_LEVEL		(V4L2_CID_VPX_BASE+3)
+#define V4L2_CID_VPX_FILTER_SHARPNESS		(V4L2_CID_VPX_BASE+4)
+#define V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD	(V4L2_CID_VPX_BASE+5)
+#define V4L2_CID_VPX_GOLDEN_FRAME_SEL		(V4L2_CID_VPX_BASE+6)
+enum v4l2_vp8_golden_frame_sel {
+	V4L2_VPX_GOLDEN_FRAME_USE_PREV		= 0,
+	V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD	= 1,
+};
+
 #endif