From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754399AbdBFOy6 (ORCPT ); Mon, 6 Feb 2017 09:54:58 -0500 Received: from mailout4.w1.samsung.com ([210.118.77.14]:51220 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751533AbdBFOyz (ORCPT ); Mon, 6 Feb 2017 09:54:55 -0500 X-AuditID: cbfec7ef-f79d26d00000420c-af-58988e3dd272 Subject: Re: [PATCH 10/11] [media] v4l2: Add v4l2 control IDs for HEVC encoder To: Smitha T Murthy , linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org Cc: kyungmin.park@samsung.com, kamil@wypas.org, jtp.park@samsung.com, mchehab@kernel.org, pankaj.dubey@samsung.com, krzk@kernel.org, m.szyprowski@samsung.com, s.nawrocki@samsung.com, Hans Verkuil , Wu-Cheng Li , Kieran Bingham , Vladimir Zapolskiy , Laurent Pinchart From: Andrzej Hajda Message-id: Date: Mon, 06 Feb 2017 15:54:48 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-version: 1.0 In-reply-to: <1484733729-25371-11-git-send-email-smitha.t@samsung.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA01SaUhUYRTle8u85+DIa7S8WCkO+cPASUH0azOFkPcrDLIkiBryoZYbMyrZ j3IUXActtRyXbDEMJEnHFZcMx1RcmkxbDMckF8wlK7U0l3LmWfjv3HvPOfeej48l5WbaiY2I jhPU0apIhURK1XeumjyOZ+tDPPN6PPHj1SYCd1S+JXBhdxqNdWWtCJtMVQzuT55j8O3kEQob xt/ReLCpRIJ1VXU0ruwwM7jcsE7gR3VLDDbObcnMLfUI//mdQuK+rH7Cfxf/qHiC4IuTBig+ f62a5ovTC2neUJEh4Wse3+Q3H+ZTfHZtBeIXDc78UIeWCZKelx4LFSIjEgT1Ib9L0nCtsQHF FoRfK33TQyShX8GZyIYFzhuej/aRIt4Dr0efSTKRlJVz5QiMbZ2kWCwiyG5+ymQi1qpoeEn/ J6UNjiGxmEIw8rOVsljZc0HQNlFrtXLgchA01X20WpFcLglfFwwSC0vCucNGzbAVyzg/uLcw zlgwxblBz6cc61G7uRBo/KDb5uyClbxRynKGDRcIJffdLW2S8wR9vpEQsQvUPJ237gKukIXm 7HFCPHs/GF5s5zwJC3UtEhHbw0xXLSPifTCYl0WJ2iwEP3K6GbHIR7D5Tb+tPgrGrgFa3GYH ufUFpLhABumpcpHCw+y8fts0ACY2VxjxiYYQmDZmmFvIpWhHnqIdIYp2hHiAyArkIMRrosIE jZdSo4rSxEeHKS/HRBnQ1qfr3exaaESTKWfaEcciha0sPF0fIqdVCZrEqHYELKlwkPlmbLVk oarE64I65qI6PlLQtKO9LKVwlLU+GDon58JUccJVQYgV1P+mBGvjlIS4V65TY9WruS5Xum2c KKXjcN8BdtmPPd82qTv8HtaSzzZ2eATqAwN8TuHTN8bkBczwaFnwRtnU0vK6OSlVoRw3eae7 lGmD7yvK3SrOKOPvGAZnlpefKJ05J/O061mtxL/TTn9BZ5sbXDubUfr5hK/2+13v6d4jZmkv iU5/8VFQmnCV10FSrVH9BbLvtABwAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrFIsWRmVeSWpSXmKPExsVy+t/xq7rWfTMiDL59V7NY8nMXk8WRtVeZ LGaeaGe16Fm8l9Hi/PkN7BZnm96wW0xsusNisenxNVaLy7vmsFn0bNjKarH2yF12i2Wb/jBZ LNr6hd3i8Bugtrt7tjFa/P/VzGxxpvssk4Ogx6LZT5g8ZjdcZPGY8nsjq8fsjpmsHptWdbJ5 bF5S7/Fv4RQWj74tqxg9Pm+S87hypJE9gCvKzSYjNTEltUghNS85PyUzL91WKTTETddCSSEv MTfVVilC1zckSEmhLDGnFMgzMkADDs4B7sFK+nYJbhmNh7czFkzPqJh36RRTA+P30C5GDg4J AROJ7UdZuxg5gUwxiQv31rN1MXJxCAksYZT4+3QXE4TzjFHi+5cJjCBVwgJ+ErN+HGAFSYgI 9DNKHHjfzg5RdYVR4vmeWWAOs8AkZomnW1+wg7SwCWhK/N18kw3E5hWwk5j7/jFYnEVAVeLU /X5mEFtUIEJi/tNVTBA1ghI/Jt9jAbmPU8BNYs58TRCTWUBP4v5FLZAKZgF5ic1r3jJPYBSY haRhFkLVLCRVCxiZVzGKpJYW56bnFhvpFSfmFpfmpesl5+duYgTG+7ZjP7fsYOx6F3yIUYCD UYmHN6NjRoQQa2JZcWXuIUYJDmYlEV7zTqAQb0piZVVqUX58UWlOavEhRlOgDyYyS4km5wNT UV5JvKGJobmloZGxhYW5kZGSOO/UD1fChQTSE0tSs1NTC1KLYPqYODilGhgrJfJTZl9q0Hz6 4uOb/4z7faJ5q2QZ4iKi1//mOdPRMcUzIrdxfmKRtsfqHtt1i5VO7Vu4V9h238TDjn/6Dyuv 462bKOL00SRZ9Frgw8w5rV8s1y71kN/FlStiODVEKnDhs4Kpt+uLv/rtXHZ93VGWh2Fi0Xp2 vL63s2sVH02eOquo5sQhvgglluKMREMt5qLiRAA3FaLdDQMAAA== X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170206145450eucas1p190d2454f3f5ad468a5c77246d5cbc594 X-Msg-Generator: CA X-Sender-IP: 182.198.249.180 X-Local-Sender: =?UTF-8?B?QW5kcnplaiBIYWpkYRtTUlBPTC1LZXJuZWwgKFRQKRvsgrw=?= =?UTF-8?B?7ISx7KCE7J6QG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Global-Sender: =?UTF-8?B?QW5kcnplaiBIYWpkYRtTUlBPTC1LZXJuZWwgKFRQKRtTYW1z?= =?UTF-8?B?dW5nIEVsZWN0cm9uaWNzG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjczOTI=?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20170118100818epcas5p1c7153a6fe9d93f96269008f42f736b90 X-RootMTR: 20170118100818epcas5p1c7153a6fe9d93f96269008f42f736b90 References: <1484733729-25371-1-git-send-email-smitha.t@samsung.com> <1484733729-25371-11-git-send-email-smitha.t@samsung.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Smitha, I have no big experience with HEVC, so it is hard to review it appropriately but I will try do my best. As these control names goes to user space you should be very careful about it. I guess it could be good to compare these controls with other HEVC encoders including software ones (ffmpeg, intel, ...) to find some similarities, common naming convention. On 18.01.2017 11:02, Smitha T Murthy wrote: > Add v4l2 controls for HEVC encoder > > CC: Hans Verkuil > CC: Wu-Cheng Li > CC: Kieran Bingham > CC: Vladimir Zapolskiy > CC: Laurent Pinchart > Signed-off-by: Smitha T Murthy > --- > drivers/media/v4l2-core/v4l2-ctrls.c | 51 ++++++++++++++++ > include/uapi/linux/v4l2-controls.h | 109 ++++++++++++++++++++++++++++++++++ > 2 files changed, 160 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index 47001e2..387439d 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -775,6 +775,57 @@ static bool is_new_manual(const struct v4l2_ctrl *master) > case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP: return "VPX P-Frame QP Value"; > case V4L2_CID_MPEG_VIDEO_VPX_PROFILE: return "VPX Profile"; > > + /* HEVC controls */ > + case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP: return "HEVC Frame QP value"; Should be "HEVC I-Frame", it looks like the convention is to upper-case first letter of all words, and the convention is I-Frame, B-Frame, P-Frame, here and in the next controls. I would drop also the word "value", but it is already used in other controls so I do not know :) > + case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP: return "HEVC P frame QP value"; > + case V4L2_CID_MPEG_VIDEO_HEVC_B_FRAME_QP: return "HEVC B frame QP value"; > + case V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP: return "HEVC Minimum QP value"; > + case V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP: return "HEVC Maximum QP value"; > + case V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_DARK: return "HEVC dark region adaptive"; > + case V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_SMOOTH: return "HEVC smooth region adaptive"; > + case V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_STATIC: return "HEVC static region adaptive"; > + case V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_ACTIVITY: return "HEVC activity adaptive"; Shouldn't it be "... Region Adaptive RC", or "... Region Adaptive Rate Control" ? > + case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE: return "HEVC Profile"; > + case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL: return "HEVC level"; > + case V4L2_CID_MPEG_VIDEO_HEVC_TIER_FLAG: return "HEVC tier_flag default is Main"; I guess 0 - means main tier, 1 means high tier, am I right? In such case it should be named "HEVC high tier" or sth similar. > + case V4L2_CID_MPEG_VIDEO_HEVC_RC_FRAME_RATE: return "HEVC Frame rate"; > + case V4L2_CID_MPEG_VIDEO_HEVC_MAX_PARTITION_DEPTH: return "HEVC Maximum coding unit depth"; > + case V4L2_CID_MPEG_VIDEO_HEVC_REF_NUMBER_FOR_PFRAMES: return "HEVC Number of reference picture"; What is purpose of this control? Macro name suggest sth different than string. > + case V4L2_CID_MPEG_VIDEO_HEVC_REFRESH_TYPE: return "HEVC refresh type"; Could you enumerate these refresh types, in patch 9 and documentation, maybe it would be worth to make it menu. > + case V4L2_CID_MPEG_VIDEO_HEVC_CONST_INTRA_PRED_ENABLE: return "HEVC constant intra prediction enabled"; > + case V4L2_CID_MPEG_VIDEO_HEVC_LOSSLESS_CU_ENABLE: return "HEVC lossless encoding select"; > + case V4L2_CID_MPEG_VIDEO_HEVC_WAVEFRONT_ENABLE: return "HEVC Wavefront enable"; I see: enable, enabled, select. Let it be consistent. > + case V4L2_CID_MPEG_VIDEO_HEVC_LF_DISABLE: return "HEVC Filter disable"; There is LF in macro name. > + case V4L2_CID_MPEG_VIDEO_HEVC_LF_SLICE_BOUNDARY: return "across or not slice boundary"; What does it mean? > + case V4L2_CID_MPEG_VIDEO_HEVC_LTR_ENABLE: return "long term reference enable"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_QP_ENABLE: return "QP values for temporal layer"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_TYPE: return "Hierarchical Coding Type"; Please enumerate types. > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER:return "Hierarchical Coding Layer"; Please enumerate layers. > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_QP:return "Hierarchical Coding Layer QP"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT0:return "Hierarchical Coding Layer BIT0"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT1:return "Hierarchical Coding Layer BIT1"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT2:return "Hierarchical Coding Layer BIT2"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT3:return "Hierarchical Coding Layer BIT3"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT4:return "Hierarchical Coding Layer BIT4"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT5:return "Hierarchical Coding Layer BIT5"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT6:return "Hierarchical Coding Layer BIT6"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_CH:return "Hierarchical Coding Layer Change"; > + case V4L2_CID_MPEG_VIDEO_HEVC_SIGN_DATA_HIDING: return "HEVC Sign data hiding"; > + case V4L2_CID_MPEG_VIDEO_HEVC_GENERAL_PB_ENABLE: return "HEVC General pb enable"; > + case V4L2_CID_MPEG_VIDEO_HEVC_TEMPORAL_ID_ENABLE: return "HEVC Temporal id enable"; > + case V4L2_CID_MPEG_VIDEO_HEVC_STRONG_SMOTHING_FLAG: return "HEVC Strong intra smoothing flag"; > + case V4L2_CID_MPEG_VIDEO_HEVC_DISABLE_INTRA_PU_SPLIT: return "HEVC disable intra pu split"; > + case V4L2_CID_MPEG_VIDEO_HEVC_DISABLE_TMV_PREDICTION: return "HEVC disable tmv prediction"; > + case V4L2_CID_MPEG_VIDEO_HEVC_MAX_NUM_MERGE_MV_MINUS1: return "max number of candidate MVs"; > + case V4L2_CID_MPEG_VIDEO_HEVC_WITHOUT_STARTCODE_ENABLE: return "ENC without startcode enable"; > + case V4L2_CID_MPEG_VIDEO_HEVC_REFRESH_PERIOD: return "HEVC Number of reference picture"; > + case V4L2_CID_MPEG_VIDEO_HEVC_LF_BETA_OFFSET_DIV2: return "HEVC loop filter beta offset"; > + case V4L2_CID_MPEG_VIDEO_HEVC_LF_TC_OFFSET_DIV2: return "HEVC loop filter tc offset"; > + case V4L2_CID_MPEG_VIDEO_HEVC_SIZE_OF_LENGTH_FIELD: return "HEVC size of length field"; > + case V4L2_CID_MPEG_VIDEO_HEVC_USER_REF: return "user long term reference frame"; > + case V4L2_CID_MPEG_VIDEO_HEVC_STORE_REF: return "store long term reference frame"; > + case V4L2_CID_MPEG_VIDEO_HEVC_PREPEND_SPSPPS_TO_IDR: return "Prepend SPS/PPS to every IDR"; You sometimes add HEVC prefix sometimes not, why? Could you describe more these controls in documentation (patch 9), it is hard to guess what they do. Regards Andrzej > + > /* CAMERA controls */ > /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > case V4L2_CID_CAMERA_CLASS: return "Camera Controls"; > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > index 0d2e1e0..a2a1c5d 100644 > --- a/include/uapi/linux/v4l2-controls.h > +++ b/include/uapi/linux/v4l2-controls.h > @@ -579,6 +579,115 @@ enum v4l2_vp8_golden_frame_sel { > #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP (V4L2_CID_MPEG_BASE+510) > #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE (V4L2_CID_MPEG_BASE+511) > > +/* CIDs for HEVC encoding. Number gaps are for compatibility */ > + > +#define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP \ > + (V4L2_CID_MPEG_BASE + 512) > +#define V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP \ > + (V4L2_CID_MPEG_BASE + 513) > +#define V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP \ > + (V4L2_CID_MPEG_BASE + 514) > +#define V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP \ > + (V4L2_CID_MPEG_BASE + 515) > +#define V4L2_CID_MPEG_VIDEO_HEVC_B_FRAME_QP \ > + (V4L2_CID_MPEG_BASE + 516) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_QP_ENABLE \ > + (V4L2_CID_MPEG_BASE + 517) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_TYPE \ > + (V4L2_CID_MPEG_BASE + 518) > +enum v4l2_mpeg_video_hevc_hier_coding_type { > + V4L2_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_B = 0, > + V4L2_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_P = 1, > +}; > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER \ > + (V4L2_CID_MPEG_BASE + 519) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_QP \ > + (V4L2_CID_MPEG_BASE + 520) > +#define V4L2_CID_MPEG_VIDEO_HEVC_PROFILE \ > + (V4L2_CID_MPEG_BASE + 521) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LEVEL \ > + (V4L2_CID_MPEG_BASE + 522) > +#define V4L2_CID_MPEG_VIDEO_HEVC_RC_FRAME_RATE \ > + (V4L2_CID_MPEG_BASE + 523) > +#define V4L2_CID_MPEG_VIDEO_HEVC_TIER_FLAG \ > + (V4L2_CID_MPEG_BASE + 524) > +#define V4L2_CID_MPEG_VIDEO_HEVC_MAX_PARTITION_DEPTH \ > + (V4L2_CID_MPEG_BASE + 525) > +#define V4L2_CID_MPEG_VIDEO_HEVC_REF_NUMBER_FOR_PFRAMES \ > + (V4L2_CID_MPEG_BASE + 526) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LF_DISABLE \ > + (V4L2_CID_MPEG_BASE + 527) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LF_SLICE_BOUNDARY \ > + (V4L2_CID_MPEG_BASE + 528) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LF_BETA_OFFSET_DIV2 \ > + (V4L2_CID_MPEG_BASE + 529) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LF_TC_OFFSET_DIV2 \ > + (V4L2_CID_MPEG_BASE + 530) > +#define V4L2_CID_MPEG_VIDEO_HEVC_REFRESH_TYPE \ > + (V4L2_CID_MPEG_BASE + 531) > +#define V4L2_CID_MPEG_VIDEO_HEVC_REFRESH_PERIOD \ > + (V4L2_CID_MPEG_BASE + 532) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LOSSLESS_CU_ENABLE \ > + (V4L2_CID_MPEG_BASE + 533) > +#define V4L2_CID_MPEG_VIDEO_HEVC_CONST_INTRA_PRED_ENABLE \ > + (V4L2_CID_MPEG_BASE + 534) > +#define V4L2_CID_MPEG_VIDEO_HEVC_WAVEFRONT_ENABLE \ > + (V4L2_CID_MPEG_BASE + 535) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LTR_ENABLE \ > + (V4L2_CID_MPEG_BASE + 536) > +#define V4L2_CID_MPEG_VIDEO_HEVC_USER_REF \ > + (V4L2_CID_MPEG_BASE + 537) > +#define V4L2_CID_MPEG_VIDEO_HEVC_STORE_REF \ > + (V4L2_CID_MPEG_BASE + 538) > +#define V4L2_CID_MPEG_VIDEO_HEVC_SIGN_DATA_HIDING \ > + (V4L2_CID_MPEG_BASE + 539) > +#define V4L2_CID_MPEG_VIDEO_HEVC_GENERAL_PB_ENABLE \ > + (V4L2_CID_MPEG_BASE + 540) > +#define V4L2_CID_MPEG_VIDEO_HEVC_TEMPORAL_ID_ENABLE \ > + (V4L2_CID_MPEG_BASE + 541) > +#define V4L2_CID_MPEG_VIDEO_HEVC_STRONG_SMOTHING_FLAG \ > + (V4L2_CID_MPEG_BASE + 542) > +#define V4L2_CID_MPEG_VIDEO_HEVC_MAX_NUM_MERGE_MV_MINUS1 \ > + (V4L2_CID_MPEG_BASE + 543) > +#define V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_DARK \ > + (V4L2_CID_MPEG_BASE + 544) > +#define V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_SMOOTH \ > + (V4L2_CID_MPEG_BASE + 545) > +#define V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_STATIC \ > + (V4L2_CID_MPEG_BASE + 546) > +#define V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_ACTIVITY \ > + (V4L2_CID_MPEG_BASE + 547) > +#define V4L2_CID_MPEG_VIDEO_HEVC_DISABLE_INTRA_PU_SPLIT \ > + (V4L2_CID_MPEG_BASE + 548) > +#define V4L2_CID_MPEG_VIDEO_HEVC_DISABLE_TMV_PREDICTION \ > + (V4L2_CID_MPEG_BASE + 549) > +#define V4L2_CID_MPEG_VIDEO_HEVC_WITHOUT_STARTCODE_ENABLE \ > + (V4L2_CID_MPEG_BASE + 550) > +#define V4L2_CID_MPEG_VIDEO_HEVC_QP_INDEX_CR \ > + (V4L2_CID_MPEG_BASE + 551) > +#define V4L2_CID_MPEG_VIDEO_HEVC_QP_INDEX_CB \ > + (V4L2_CID_MPEG_BASE + 552) > +#define V4L2_CID_MPEG_VIDEO_HEVC_SIZE_OF_LENGTH_FIELD \ > + (V4L2_CID_MPEG_BASE + 553) > +#define V4L2_CID_MPEG_VIDEO_HEVC_PREPEND_SPSPPS_TO_IDR \ > + (V4L2_CID_MPEG_BASE + 554) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_CH \ > + (V4L2_CID_MPEG_BASE + 555) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT0 \ > + (V4L2_CID_MPEG_BASE + 556) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT1 \ > + (V4L2_CID_MPEG_BASE + 557) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT2 \ > + (V4L2_CID_MPEG_BASE + 558) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT3 \ > + (V4L2_CID_MPEG_BASE + 559) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT4 \ > + (V4L2_CID_MPEG_BASE + 560) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT5 \ > + (V4L2_CID_MPEG_BASE + 561) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT6 \ > + (V4L2_CID_MPEG_BASE + 562) > + > /* MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */ > #define V4L2_CID_MPEG_CX2341X_BASE (V4L2_CTRL_CLASS_MPEG | 0x1000) > #define V4L2_CID_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE (V4L2_CID_MPEG_CX2341X_BASE+0) From mboxrd@z Thu Jan 1 00:00:00 1970 From: a.hajda@samsung.com (Andrzej Hajda) Date: Mon, 06 Feb 2017 15:54:48 +0100 Subject: [PATCH 10/11] [media] v4l2: Add v4l2 control IDs for HEVC encoder In-Reply-To: <1484733729-25371-11-git-send-email-smitha.t@samsung.com> References: <1484733729-25371-1-git-send-email-smitha.t@samsung.com> <1484733729-25371-11-git-send-email-smitha.t@samsung.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Smitha, I have no big experience with HEVC, so it is hard to review it appropriately but I will try do my best. As these control names goes to user space you should be very careful about it. I guess it could be good to compare these controls with other HEVC encoders including software ones (ffmpeg, intel, ...) to find some similarities, common naming convention. On 18.01.2017 11:02, Smitha T Murthy wrote: > Add v4l2 controls for HEVC encoder > > CC: Hans Verkuil > CC: Wu-Cheng Li > CC: Kieran Bingham > CC: Vladimir Zapolskiy > CC: Laurent Pinchart > Signed-off-by: Smitha T Murthy > --- > drivers/media/v4l2-core/v4l2-ctrls.c | 51 ++++++++++++++++ > include/uapi/linux/v4l2-controls.h | 109 ++++++++++++++++++++++++++++++++++ > 2 files changed, 160 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index 47001e2..387439d 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -775,6 +775,57 @@ static bool is_new_manual(const struct v4l2_ctrl *master) > case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP: return "VPX P-Frame QP Value"; > case V4L2_CID_MPEG_VIDEO_VPX_PROFILE: return "VPX Profile"; > > + /* HEVC controls */ > + case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP: return "HEVC Frame QP value"; Should be "HEVC I-Frame", it looks like the convention is to upper-case first letter of all words, and the convention is I-Frame, B-Frame, P-Frame, here and in the next controls. I would drop also the word "value", but it is already used in other controls so I do not know :) > + case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP: return "HEVC P frame QP value"; > + case V4L2_CID_MPEG_VIDEO_HEVC_B_FRAME_QP: return "HEVC B frame QP value"; > + case V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP: return "HEVC Minimum QP value"; > + case V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP: return "HEVC Maximum QP value"; > + case V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_DARK: return "HEVC dark region adaptive"; > + case V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_SMOOTH: return "HEVC smooth region adaptive"; > + case V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_STATIC: return "HEVC static region adaptive"; > + case V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_ACTIVITY: return "HEVC activity adaptive"; Shouldn't it be "... Region Adaptive RC", or "... Region Adaptive Rate Control" ? > + case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE: return "HEVC Profile"; > + case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL: return "HEVC level"; > + case V4L2_CID_MPEG_VIDEO_HEVC_TIER_FLAG: return "HEVC tier_flag default is Main"; I guess 0 - means main tier, 1 means high tier, am I right? In such case it should be named "HEVC high tier" or sth similar. > + case V4L2_CID_MPEG_VIDEO_HEVC_RC_FRAME_RATE: return "HEVC Frame rate"; > + case V4L2_CID_MPEG_VIDEO_HEVC_MAX_PARTITION_DEPTH: return "HEVC Maximum coding unit depth"; > + case V4L2_CID_MPEG_VIDEO_HEVC_REF_NUMBER_FOR_PFRAMES: return "HEVC Number of reference picture"; What is purpose of this control? Macro name suggest sth different than string. > + case V4L2_CID_MPEG_VIDEO_HEVC_REFRESH_TYPE: return "HEVC refresh type"; Could you enumerate these refresh types, in patch 9 and documentation, maybe it would be worth to make it menu. > + case V4L2_CID_MPEG_VIDEO_HEVC_CONST_INTRA_PRED_ENABLE: return "HEVC constant intra prediction enabled"; > + case V4L2_CID_MPEG_VIDEO_HEVC_LOSSLESS_CU_ENABLE: return "HEVC lossless encoding select"; > + case V4L2_CID_MPEG_VIDEO_HEVC_WAVEFRONT_ENABLE: return "HEVC Wavefront enable"; I see: enable, enabled, select. Let it be consistent. > + case V4L2_CID_MPEG_VIDEO_HEVC_LF_DISABLE: return "HEVC Filter disable"; There is LF in macro name. > + case V4L2_CID_MPEG_VIDEO_HEVC_LF_SLICE_BOUNDARY: return "across or not slice boundary"; What does it mean? > + case V4L2_CID_MPEG_VIDEO_HEVC_LTR_ENABLE: return "long term reference enable"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_QP_ENABLE: return "QP values for temporal layer"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_TYPE: return "Hierarchical Coding Type"; Please enumerate types. > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER:return "Hierarchical Coding Layer"; Please enumerate layers. > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_QP:return "Hierarchical Coding Layer QP"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT0:return "Hierarchical Coding Layer BIT0"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT1:return "Hierarchical Coding Layer BIT1"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT2:return "Hierarchical Coding Layer BIT2"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT3:return "Hierarchical Coding Layer BIT3"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT4:return "Hierarchical Coding Layer BIT4"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT5:return "Hierarchical Coding Layer BIT5"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT6:return "Hierarchical Coding Layer BIT6"; > + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_CH:return "Hierarchical Coding Layer Change"; > + case V4L2_CID_MPEG_VIDEO_HEVC_SIGN_DATA_HIDING: return "HEVC Sign data hiding"; > + case V4L2_CID_MPEG_VIDEO_HEVC_GENERAL_PB_ENABLE: return "HEVC General pb enable"; > + case V4L2_CID_MPEG_VIDEO_HEVC_TEMPORAL_ID_ENABLE: return "HEVC Temporal id enable"; > + case V4L2_CID_MPEG_VIDEO_HEVC_STRONG_SMOTHING_FLAG: return "HEVC Strong intra smoothing flag"; > + case V4L2_CID_MPEG_VIDEO_HEVC_DISABLE_INTRA_PU_SPLIT: return "HEVC disable intra pu split"; > + case V4L2_CID_MPEG_VIDEO_HEVC_DISABLE_TMV_PREDICTION: return "HEVC disable tmv prediction"; > + case V4L2_CID_MPEG_VIDEO_HEVC_MAX_NUM_MERGE_MV_MINUS1: return "max number of candidate MVs"; > + case V4L2_CID_MPEG_VIDEO_HEVC_WITHOUT_STARTCODE_ENABLE: return "ENC without startcode enable"; > + case V4L2_CID_MPEG_VIDEO_HEVC_REFRESH_PERIOD: return "HEVC Number of reference picture"; > + case V4L2_CID_MPEG_VIDEO_HEVC_LF_BETA_OFFSET_DIV2: return "HEVC loop filter beta offset"; > + case V4L2_CID_MPEG_VIDEO_HEVC_LF_TC_OFFSET_DIV2: return "HEVC loop filter tc offset"; > + case V4L2_CID_MPEG_VIDEO_HEVC_SIZE_OF_LENGTH_FIELD: return "HEVC size of length field"; > + case V4L2_CID_MPEG_VIDEO_HEVC_USER_REF: return "user long term reference frame"; > + case V4L2_CID_MPEG_VIDEO_HEVC_STORE_REF: return "store long term reference frame"; > + case V4L2_CID_MPEG_VIDEO_HEVC_PREPEND_SPSPPS_TO_IDR: return "Prepend SPS/PPS to every IDR"; You sometimes add HEVC prefix sometimes not, why? Could you describe more these controls in documentation (patch 9), it is hard to guess what they do. Regards Andrzej > + > /* CAMERA controls */ > /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > case V4L2_CID_CAMERA_CLASS: return "Camera Controls"; > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > index 0d2e1e0..a2a1c5d 100644 > --- a/include/uapi/linux/v4l2-controls.h > +++ b/include/uapi/linux/v4l2-controls.h > @@ -579,6 +579,115 @@ enum v4l2_vp8_golden_frame_sel { > #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP (V4L2_CID_MPEG_BASE+510) > #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE (V4L2_CID_MPEG_BASE+511) > > +/* CIDs for HEVC encoding. Number gaps are for compatibility */ > + > +#define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP \ > + (V4L2_CID_MPEG_BASE + 512) > +#define V4L2_CID_MPEG_VIDEO_HEVC_MAX_QP \ > + (V4L2_CID_MPEG_BASE + 513) > +#define V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP \ > + (V4L2_CID_MPEG_BASE + 514) > +#define V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP \ > + (V4L2_CID_MPEG_BASE + 515) > +#define V4L2_CID_MPEG_VIDEO_HEVC_B_FRAME_QP \ > + (V4L2_CID_MPEG_BASE + 516) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_QP_ENABLE \ > + (V4L2_CID_MPEG_BASE + 517) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_TYPE \ > + (V4L2_CID_MPEG_BASE + 518) > +enum v4l2_mpeg_video_hevc_hier_coding_type { > + V4L2_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_B = 0, > + V4L2_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_P = 1, > +}; > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER \ > + (V4L2_CID_MPEG_BASE + 519) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_QP \ > + (V4L2_CID_MPEG_BASE + 520) > +#define V4L2_CID_MPEG_VIDEO_HEVC_PROFILE \ > + (V4L2_CID_MPEG_BASE + 521) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LEVEL \ > + (V4L2_CID_MPEG_BASE + 522) > +#define V4L2_CID_MPEG_VIDEO_HEVC_RC_FRAME_RATE \ > + (V4L2_CID_MPEG_BASE + 523) > +#define V4L2_CID_MPEG_VIDEO_HEVC_TIER_FLAG \ > + (V4L2_CID_MPEG_BASE + 524) > +#define V4L2_CID_MPEG_VIDEO_HEVC_MAX_PARTITION_DEPTH \ > + (V4L2_CID_MPEG_BASE + 525) > +#define V4L2_CID_MPEG_VIDEO_HEVC_REF_NUMBER_FOR_PFRAMES \ > + (V4L2_CID_MPEG_BASE + 526) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LF_DISABLE \ > + (V4L2_CID_MPEG_BASE + 527) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LF_SLICE_BOUNDARY \ > + (V4L2_CID_MPEG_BASE + 528) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LF_BETA_OFFSET_DIV2 \ > + (V4L2_CID_MPEG_BASE + 529) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LF_TC_OFFSET_DIV2 \ > + (V4L2_CID_MPEG_BASE + 530) > +#define V4L2_CID_MPEG_VIDEO_HEVC_REFRESH_TYPE \ > + (V4L2_CID_MPEG_BASE + 531) > +#define V4L2_CID_MPEG_VIDEO_HEVC_REFRESH_PERIOD \ > + (V4L2_CID_MPEG_BASE + 532) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LOSSLESS_CU_ENABLE \ > + (V4L2_CID_MPEG_BASE + 533) > +#define V4L2_CID_MPEG_VIDEO_HEVC_CONST_INTRA_PRED_ENABLE \ > + (V4L2_CID_MPEG_BASE + 534) > +#define V4L2_CID_MPEG_VIDEO_HEVC_WAVEFRONT_ENABLE \ > + (V4L2_CID_MPEG_BASE + 535) > +#define V4L2_CID_MPEG_VIDEO_HEVC_LTR_ENABLE \ > + (V4L2_CID_MPEG_BASE + 536) > +#define V4L2_CID_MPEG_VIDEO_HEVC_USER_REF \ > + (V4L2_CID_MPEG_BASE + 537) > +#define V4L2_CID_MPEG_VIDEO_HEVC_STORE_REF \ > + (V4L2_CID_MPEG_BASE + 538) > +#define V4L2_CID_MPEG_VIDEO_HEVC_SIGN_DATA_HIDING \ > + (V4L2_CID_MPEG_BASE + 539) > +#define V4L2_CID_MPEG_VIDEO_HEVC_GENERAL_PB_ENABLE \ > + (V4L2_CID_MPEG_BASE + 540) > +#define V4L2_CID_MPEG_VIDEO_HEVC_TEMPORAL_ID_ENABLE \ > + (V4L2_CID_MPEG_BASE + 541) > +#define V4L2_CID_MPEG_VIDEO_HEVC_STRONG_SMOTHING_FLAG \ > + (V4L2_CID_MPEG_BASE + 542) > +#define V4L2_CID_MPEG_VIDEO_HEVC_MAX_NUM_MERGE_MV_MINUS1 \ > + (V4L2_CID_MPEG_BASE + 543) > +#define V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_DARK \ > + (V4L2_CID_MPEG_BASE + 544) > +#define V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_SMOOTH \ > + (V4L2_CID_MPEG_BASE + 545) > +#define V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_STATIC \ > + (V4L2_CID_MPEG_BASE + 546) > +#define V4L2_CID_MPEG_VIDEO_HEVC_ADAPTIVE_RC_ACTIVITY \ > + (V4L2_CID_MPEG_BASE + 547) > +#define V4L2_CID_MPEG_VIDEO_HEVC_DISABLE_INTRA_PU_SPLIT \ > + (V4L2_CID_MPEG_BASE + 548) > +#define V4L2_CID_MPEG_VIDEO_HEVC_DISABLE_TMV_PREDICTION \ > + (V4L2_CID_MPEG_BASE + 549) > +#define V4L2_CID_MPEG_VIDEO_HEVC_WITHOUT_STARTCODE_ENABLE \ > + (V4L2_CID_MPEG_BASE + 550) > +#define V4L2_CID_MPEG_VIDEO_HEVC_QP_INDEX_CR \ > + (V4L2_CID_MPEG_BASE + 551) > +#define V4L2_CID_MPEG_VIDEO_HEVC_QP_INDEX_CB \ > + (V4L2_CID_MPEG_BASE + 552) > +#define V4L2_CID_MPEG_VIDEO_HEVC_SIZE_OF_LENGTH_FIELD \ > + (V4L2_CID_MPEG_BASE + 553) > +#define V4L2_CID_MPEG_VIDEO_HEVC_PREPEND_SPSPPS_TO_IDR \ > + (V4L2_CID_MPEG_BASE + 554) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_CH \ > + (V4L2_CID_MPEG_BASE + 555) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT0 \ > + (V4L2_CID_MPEG_BASE + 556) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT1 \ > + (V4L2_CID_MPEG_BASE + 557) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT2 \ > + (V4L2_CID_MPEG_BASE + 558) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT3 \ > + (V4L2_CID_MPEG_BASE + 559) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT4 \ > + (V4L2_CID_MPEG_BASE + 560) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT5 \ > + (V4L2_CID_MPEG_BASE + 561) > +#define V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER_BIT6 \ > + (V4L2_CID_MPEG_BASE + 562) > + > /* MPEG-class control IDs specific to the CX2341x driver as defined by V4L2 */ > #define V4L2_CID_MPEG_CX2341X_BASE (V4L2_CTRL_CLASS_MPEG | 0x1000) > #define V4L2_CID_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE (V4L2_CID_MPEG_CX2341X_BASE+0)