From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-bk0-f45.google.com ([209.85.214.45]:43366 "EHLO mail-bk0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752287Ab3FWUxQ (ORCPT ); Sun, 23 Jun 2013 16:53:16 -0400 Received: by mail-bk0-f45.google.com with SMTP id je9so3953688bkc.32 for ; Sun, 23 Jun 2013 13:53:15 -0700 (PDT) Message-ID: <51C76037.8050106@gmail.com> Date: Sun, 23 Jun 2013 22:53:11 +0200 From: Sylwester Nawrocki MIME-Version: 1.0 To: Arun Kumar K CC: linux-media@vger.kernel.org, k.debski@samsung.com, jtp.park@samsung.com, s.nawrocki@samsung.com, hverkuil@xs4all.nl, avnd.kiran@samsung.com, arunkk.samsung@gmail.com Subject: Re: [PATCH v2 7/8] [media] V4L: Add VP8 encoder controls References: <1371560183-23244-1-git-send-email-arun.kk@samsung.com> <1371560183-23244-8-git-send-email-arun.kk@samsung.com> In-Reply-To: <1371560183-23244-8-git-send-email-arun.kk@samsung.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: 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 > Signed-off-by: Kiran AVND 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. > > > > + > +
> +VPX Control Reference > + > +The VPX controls include controls for encoding parameters > + of VPx video codec. > + > + > +VPX Control IDs > + > + > + > + > + > + > + > + > + > + > +ID > +Type > +Description > + > + > + > + > + > + > + > + V4L2_CID_VPX_NUM_PARTITIONS  What is this ' ' at the end of an entry needed for ? I can see lots of similar ones elsewhere in this patch. > + enum v4l2_vp8_num_partitions > + > + The number of token partitions to use in VP8 encoder. > +Possible values are: > + > + > + > + > + > + V4L2_VPX_1_PARTITION  > + 1 coefficient partition > + > + > + V4L2_VPX_2_PARTITIONS  > + 2 partitions > + > + > + V4L2_VPX_4_PARTITIONS  > + 4 partitions > + > + > + V4L2_VPX_8_PARTITIONS  > + 8 partitions > + > + > + > + > + > + > + > + V4L2_CID_VPX_IMD_DISABLE_4X4  > + boolean > + > + Setting this prevents intra 4x4 mode in the intra mode decision. > + > + > + > + > + V4L2_CID_VPX_NUM_REF_FRAMES  > + enum v4l2_vp8_num_ref_frames > + > + The number of reference pictures for encoding P frames. > +Possible values are: > + > + > + > + > + > + V4L2_VPX_1_REF_FRAME  > + Last encoded frame will be searched > + > + > + V4L2_VPX_2_REF_FRAME  > + Two frames would be searched among last encoded frame, golden frame > +and altref frame. Encoder implementation can decide which two are chosen. > + > + > + V4L2_VPX_3_REF_FRAME  > + The last encoded frame, golden frame and altref frame will be searched. > + > + > + > + > + > + > + > + V4L2_CID_VPX_FILTER_LEVEL  > + integer > + > + Indicates the loop filter level. The adjustment of loop > +filter level is done via a delta value against a baseline loop filter value. > + > + > + > + > + V4L2_CID_VPX_FILTER_SHARPNESS  > + integer > + > + This parameter affects the loop filter. Anything above > +zero weakens the deblocking effect on loop filter. > + > + > + > + > + V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD  > + integer > + > + 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. > + > + > + > + > + V4L2_CID_VPX_GOLDEN_FRAME_SEL  > + enum v4l2_vp8_golden_frame_sel > + > + Selects the golden frame for encoding. > +Possible values are: > + > + > + > + > + > + V4L2_VPX_GOLDEN_FRAME_USE_PREV  > + Use the previous second frame (last to last frame) as a golden frame I can't understand what this means exactly. But this could be just my bad English skills... ;) > + > + > + V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD  > + Use the previous specific frame indicated by V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD as a golden frame > + > + > + > + > + > + > + > + > +
> + > +
> > >
> @@ -4772,4 +4922,5 @@ defines possible values for de-emphasis. Here they are: > > >
> + Unnecessary change ? > > 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