[PATCH 10/11] [media] v4l2: Add v4l2 control IDs for HEVC encoder
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"; + 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"; + 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"; + 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"; + case V4L2_CID_MPEG_VIDEO_HEVC_REFRESH_TYPE: return "HEVC refresh type"; + 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"; + case V4L2_CID_MPEG_VIDEO_HEVC_LF_DISABLE: return "HEVC Filter disable"; + case V4L2_CID_MPEG_VIDEO_HEVC_LF_SLICE_BOUNDARY:return "across or not slice boundary"; + 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"; + case V4L2_CID_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_LAYER:return "Hierarchical Coding Layer"; + 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_
Re: [PATCH 10/11] [media] v4l2: Add v4l2 control IDs for HEVC encoder
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:r
Re: [PATCH 10/11] [media] v4l2: Add v4l2 control IDs for HEVC encoder
On Mon, 2017-02-06 at 15:54 +0100, Andrzej Hajda wrote: > 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. > Thank you so much for the review :) I will compare it with the software HEVC encoders for the naming convention. Basically I was following the convention used for other codecs in the same file. > > 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 :) > The I,P,B frame naming convention for other codecs is like "V4L2_CID_MPEG_VIDEO_H263_I_FRAME_QP" and "V4L2_CID_MPEG_VIDEO_H264_I_FRAME_QP" so I followed the same for HEVC codec too. Yes they use the word "value" for other codecs too. > > + 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" ? > I will correct it to 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. > Yes 0 is for Main tier and 1 is for High tier. Since the flag by default is main tier and it can be used for both the tiers I just kept the name as "TIER_FLAG" > > + 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. > Sorry the description should have been "Number of reference frames for P-Frame". P-frame can use 1 or 2 frames for reference. > > + 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. > There are 3 refresh types : None, CRA, IDR. I will add more details in the Documentation patch and in the menu on this. > > + 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:
