Hi Arun,
On 06/18/2013 02:56 PM, Arun Kumar K wrote:
This patch adds new V4L controls for VP8 encoding.
Signed-off-by: Arun Kumar K<arun.kk@xxxxxxxxxxx>
Signed-off-by: Kiran AVND<avnd.kiran@xxxxxxxxxxx>
I think your signed-off-by should be last one, since you're submitting
the patch.
---
Documentation/DocBook/media/v4l/controls.xml | 151 ++++++++++++++++++++++++++
drivers/media/v4l2-core/v4l2-ctrls.c | 36 ++++++
include/uapi/linux/v4l2-controls.h | 28 ++++-
3 files changed, 213 insertions(+), 2 deletions(-)
diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index 8d7a779..cd87000 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -3009,6 +3009,156 @@ in by the application. 0 = do not insert, 1 = insert packets.</entry>
</tgroup>
</table>
</section>
+
+<section>
+<title>VPX Control Reference</title>
+
+<para>The VPX controls include controls for encoding parameters
+ of VPx video codec.</para>
+
+<table pgwide="1" frame="none" id="vpx-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> </entry>
What is this ' ' at the end of an entry needed for ? I can see lots
of similar ones elsewhere in this patch.
+ <entry>enum 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> </entry>
+ <entry>1 coefficient partition</entry>
+ </row>
+ <row>
+ <entry><constant>V4L2_VPX_2_PARTITIONS</constant> </entry>
+ <entry>2 partitions</entry>
+ </row>
+ <row>
+ <entry><constant>V4L2_VPX_4_PARTITIONS</constant> </entry>
+ <entry>4 partitions</entry>
+ </row>
+ <row>
+ <entry><constant>V4L2_VPX_8_PARTITIONS</constant> </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> </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> </entry>
+ <entry>enum 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> </entry>
+ <entry>Last encoded frame will be searched</entry>
+ </row>
+ <row>
+ <entry><constant>V4L2_VPX_2_REF_FRAME</constant> </entry>
+ <entry>Two frames would be searched among last encoded frame, golden frame
+and altref frame. Encoder implementation can decide which two are chosen.</entry>
+ </row>
+ <row>
+ <entry><constant>V4L2_VPX_3_REF_FRAME</constant> </entry>
+ <entry>The last encoded frame, golden frame and altref frame will be searched.</entry>
+ </row>
+</tbody>
+ </entrytbl>
+ </row>
+
+ <row><entry></entry></row>
+ <row>
+ <entry spanname="id"><constant>V4L2_CID_VPX_FILTER_LEVEL</constant> </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> </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> </entry>
+ <entry>integer</entry>
+ </row>
+ <row><entry spanname="descr">Sets the refresh period for golden frame. Period is defined
+in number of frames. For a value of 'n', every nth frame will be taken as 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> </entry>
+ <entry>enum 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> </entry>
+ <entry>Use the previous second frame (last to last frame) as a golden frame</entry>
I can't understand what this means exactly. But this could be just my bad
English skills... ;)
+ </row>
+ <row>
+ <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD</constant> </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>
<section id="camera-controls">
@@ -4772,4 +4922,5 @@ defines possible values for de-emphasis. Here they are:</entry>
</table>
</section>
+
Unnecessary change ?
</section>
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 3cb1cff..2a4413b 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -424,6 +424,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
NULL,
};
+ static const char * const vpx_golden_frame_sel[] = {
+ "Use Previous Frame",
+ "Use Frame Indicated By GOLDEN_FRAME_REF_PERIOD",
That name is too long, look at VIDIOC_QUERYMENU ioctl specification [1] what
a capacity of the menu item name storage is.
[1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-queryctrl.html
+ NULL,
+ };
+
static const char * const flash_led_mode[] = {
"Off",
"Flash",
@@ -538,6 +544,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
return mpeg_mpeg4_level;
case V4L2_CID_MPEG_VIDEO_MPEG4_PROFILE:
return mpeg4_profile;
+ case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
+ return vpx_golden_frame_sel;
case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
return jpeg_chroma_subsampling;
case V4L2_CID_DV_TX_MODE:
@@ -558,7 +566,23 @@ EXPORT_SYMBOL(v4l2_ctrl_get_menu);
*/
const s64 const *v4l2_ctrl_get_int_menu(u32 id, u32 *len)
{
+#define V4L2_INT_MENU_RETURN(qmenu) \
+ do { *len = ARRAY_SIZE(qmenu); return qmenu; } while (0)
How about using something along the lines of:
#define __v4l2_qmenu_int_len(arr, len) ({ *(len) = ARRAY_SIZE(arr); arr; })
? And also moving it out of the function, above ?
The main problem with your macro is that contains a return statement. But
also relies on a specific variable name.
In Documentation/CodingStyle we read:
" ...
Things to avoid when using macros:
1) macros that affect control flow:
#define FOO(x) \
do { \
if (blah(x) < 0) \
return -EBUGGERED; \
} while(0)
is a _very_ bad idea. It looks like a function call but exits the "calling"
function; don't break the internal parsers of those who will read the code.
2) macros that depend on having a local variable with a magic name:
#define FOO(val) bar(index, val)
might look like a good thing, but it's confusing as hell when one reads the
code and it's prone to breakage from seemingly innocent changes.
...
"
+ static const s64 const qmenu_int_vpx_num_partitions[] = {
+ 1, 2, 4, 8,
+ };
+
+ static const s64 const qmenu_int_vpx_num_ref_frames[] = {
+ 1, 2, 3,
+ };
+
switch (id) {
+ case V4L2_CID_VPX_NUM_PARTITIONS:
+ V4L2_INT_MENU_RETURN(qmenu_int_vpx_num_partitions);
Then this would became:
return __v4l2_qmenu_int_len(qmenu_int_vpx_num_partitions, &len);
+ case V4L2_CID_VPX_NUM_REF_FRAMES:
+ V4L2_INT_MENU_RETURN(qmenu_int_vpx_num_ref_frames);
Ditto.
default:
*len = 0;
return NULL;
@@ -714,6 +738,15 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_MPEG_VIDEO_VBV_DELAY: return "Initial Delay for VBV Control";
case V4L2_CID_MPEG_VIDEO_REPEAT_SEQ_HEADER: return "Repeat Sequence Header";
+ /* VPX 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 No. of refs for P frame";
+ 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";
+
/* CAMERA controls */
/* Keep the order of the 'case's the same as in videodev2.h! */
case V4L2_CID_CAMERA_CLASS: return "Camera Controls";
@@ -929,6 +962,7 @@ 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_GOLDEN_FRAME_SEL:
*type = V4L2_CTRL_TYPE_MENU;
break;
case V4L2_CID_LINK_FREQ:
@@ -940,6 +974,8 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
break;
case V4L2_CID_ISO_SENSITIVITY:
case V4L2_CID_AUTO_EXPOSURE_BIAS:
+ case V4L2_CID_VPX_NUM_PARTITIONS:
+ case V4L2_CID_VPX_NUM_REF_FRAMES:
*type = V4L2_CTRL_TYPE_INTEGER_MENU;
break;
case V4L2_CID_USER_CLASS:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 69bd5bb..a1f6036 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -522,6 +522,32 @@ enum v4l2_mpeg_video_mpeg4_profile {
};
#define V4L2_CID_MPEG_VIDEO_MPEG4_QPEL (V4L2_CID_MPEG_BASE+407)
+/* Control IDs for VP8 streams
+ * Though VP8 is not part of MPEG, adding it here as MPEG class is
+ * already handling other video compression standards */
Please use proper comment style, i.e.
/*
* Control IDs for VP8 streams.
* ...
*/
+#define V4L2_CID_VPX_NUM_PARTITIONS (V4L2_CID_MPEG_BASE+500)
+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,
+};
+#define V4L2_CID_VPX_IMD_DISABLE_4X4 (V4L2_CID_MPEG_BASE+501)
+#define V4L2_CID_VPX_NUM_REF_FRAMES (V4L2_CID_MPEG_BASE+502)
+enum v4l2_vp8_num_ref_frames {
+ V4L2_VPX_1_REF_FRAME = 0,
+ V4L2_VPX_2_REF_FRAME = 1,
+ V4L2_VPX_3_REF_FRAME = 2,
+};
+#define V4L2_CID_VPX_FILTER_LEVEL (V4L2_CID_MPEG_BASE+503)
+#define V4L2_CID_VPX_FILTER_SHARPNESS (V4L2_CID_MPEG_BASE+504)
+#define V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD (V4L2_CID_MPEG_BASE+505)
+#define V4L2_CID_VPX_GOLDEN_FRAME_SEL (V4L2_CID_MPEG_BASE+506)
+enum v4l2_vp8_golden_frame_sel {
+ V4L2_VPX_GOLDEN_FRAME_USE_PREV = 0,
+ V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD = 1,
+};
Otherwise looks good to me.
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
