Re: [PATCH v3 7/8] [media] V4L: Add VP8 encoder controls
Hi Hans On Fri, Jun 28, 2013 at 7:55 PM, Hans Verkuil hverk...@xs4all.nl wrote: Hi Arun, As promised, here is my review. Thank you :). Sorry for the delay in response. I have been thinking a bit more about whether or not a VPX control class should be added, and in my opinion it shouldn't. These controls should be part of the MPEG control class, as the VPX encoder shares a lot of general encoding parameters, just like h264 and mpeg4. It is unfortunate that all the defines contain the MPEG name, and I take the blame for that since I came up with these defines originally. That said, there are some things that can be done to make it less confusing: - Clearly state in v4l2-controls.h and v4l2-ctrls.c that the MPEG controls are really Codec Controls, so not MPEG specific, and that the 'MPEG' part of the define is historical. Ok will do that. - Currently the V4L2_CID_MPEG_CLASS name in v4l2-ctrls.c is MPEG Encoder Controls. This should be changed to Codec Controls, since the controls in this class are neither MPEG specific, nor are they encoder specific as there are also controls related to the decoder. - Update the DocBook section for the MPEG controls accordingly: change 'MPEG' in the text to 'Codec' and add a note explaining why all the defines are prefixed with V4L2_CID_MPEG/V4L2_MPEG instead of _CODEC. Ok will do these changes. I did toy with the idea of adding aliases in v4l2-controls.h replacing MPEG with CODEC, but that really is too messy. I think if you can take care of the three points mentioned above we should be OK. This also means that in this patch the V4L2_CID_VPX_ prefix changes to V4L2_CID_MPEG_VIDEO_VPX_ as that is consistent with the current naming convention in v4l2-controls.h: V4L2_CID_MPEG_VIDEO_H264_ASO, V4L2_CID_MPEG_VIDEO_MPEG4_LEVEL. Ok. Enums use V4L2_MPEG_VIDEO_VPX_ prefix. Yes, I know, this will make the names quite a bit longer, but it is important for consistency. Codecs are likely to have lots of controls since there are lots of knobs you can tweak. So using a systematic naming scheme will prevent it from descending into chaos... On Tue June 25 2013 12:57:14 Arun Kumar K wrote: This patch adds new V4L controls for VP8 encoding. Signed-off-by: Kiran AVND avnd.ki...@samsung.com Signed-off-by: Arun Kumar K arun...@samsung.com --- Documentation/DocBook/media/v4l/controls.xml | 150 ++ drivers/media/v4l2-core/v4l2-ctrls.c | 33 ++ include/uapi/linux/v4l2-controls.h | 29 - 3 files changed, 210 insertions(+), 2 deletions(-) diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml index 8d7a779..736c991 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 + titleVPX Control Reference/title + + paraThe VPX controls include controls for encoding parameters + of VPx video codec./para + + table pgwide=1 frame=none id=vpx-control-id + titleVPX 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=leftID/entry +entry align=leftType/entry + /rowrow rowsep=1entry spanname=descr align=leftDescription/entry + /row +/thead +tbody valign=top + rowentry/entry/row + + rowentry/entry/row + row id=v4l2-vpx-num-partitions + entry spanname=idconstantV4L2_CID_VPX_NUM_PARTITIONS/constant/entry + entryenum v4l2_vp8_num_partitions/entry + /row + rowentry spanname=descrThe number of token partitions to use in VP8 encoder. +Possible values are:/entry + /row + row + entrytbl spanname=descr cols=2 + tbody valign=top + row + entryconstantV4L2_VPX_1_PARTITION/constant/entry + entry1 coefficient partition/entry + /row + row + entryconstantV4L2_VPX_2_PARTITIONS/constant/entry + entry2 partitions/entry Add 'coefficient' for the other cases as well in the description. At least, I think this should be '2 coefficient partitions'. Ok. + /row + row + entryconstantV4L2_VPX_4_PARTITIONS/constant/entry + entry4 partitions/entry + /row +
Re: [PATCH v3 7/8] [media] V4L: Add VP8 encoder controls
Hi Arun, As promised, here is my review. I have been thinking a bit more about whether or not a VPX control class should be added, and in my opinion it shouldn't. These controls should be part of the MPEG control class, as the VPX encoder shares a lot of general encoding parameters, just like h264 and mpeg4. It is unfortunate that all the defines contain the MPEG name, and I take the blame for that since I came up with these defines originally. That said, there are some things that can be done to make it less confusing: - Clearly state in v4l2-controls.h and v4l2-ctrls.c that the MPEG controls are really Codec Controls, so not MPEG specific, and that the 'MPEG' part of the define is historical. - Currently the V4L2_CID_MPEG_CLASS name in v4l2-ctrls.c is MPEG Encoder Controls. This should be changed to Codec Controls, since the controls in this class are neither MPEG specific, nor are they encoder specific as there are also controls related to the decoder. - Update the DocBook section for the MPEG controls accordingly: change 'MPEG' in the text to 'Codec' and add a note explaining why all the defines are prefixed with V4L2_CID_MPEG/V4L2_MPEG instead of _CODEC. I did toy with the idea of adding aliases in v4l2-controls.h replacing MPEG with CODEC, but that really is too messy. I think if you can take care of the three points mentioned above we should be OK. This also means that in this patch the V4L2_CID_VPX_ prefix changes to V4L2_CID_MPEG_VIDEO_VPX_ as that is consistent with the current naming convention in v4l2-controls.h: V4L2_CID_MPEG_VIDEO_H264_ASO, V4L2_CID_MPEG_VIDEO_MPEG4_LEVEL. Enums use V4L2_MPEG_VIDEO_VPX_ prefix. Yes, I know, this will make the names quite a bit longer, but it is important for consistency. Codecs are likely to have lots of controls since there are lots of knobs you can tweak. So using a systematic naming scheme will prevent it from descending into chaos... On Tue June 25 2013 12:57:14 Arun Kumar K wrote: This patch adds new V4L controls for VP8 encoding. Signed-off-by: Kiran AVND avnd.ki...@samsung.com Signed-off-by: Arun Kumar K arun...@samsung.com --- Documentation/DocBook/media/v4l/controls.xml | 150 ++ drivers/media/v4l2-core/v4l2-ctrls.c | 33 ++ include/uapi/linux/v4l2-controls.h | 29 - 3 files changed, 210 insertions(+), 2 deletions(-) diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml index 8d7a779..736c991 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 + titleVPX Control Reference/title + + paraThe VPX controls include controls for encoding parameters + of VPx video codec./para + + table pgwide=1 frame=none id=vpx-control-id + titleVPX 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=leftID/entry +entry align=leftType/entry + /rowrow rowsep=1entry spanname=descr align=leftDescription/entry + /row +/thead +tbody valign=top + rowentry/entry/row + + rowentry/entry/row + row id=v4l2-vpx-num-partitions + entry spanname=idconstantV4L2_CID_VPX_NUM_PARTITIONS/constant/entry + entryenum v4l2_vp8_num_partitions/entry + /row + rowentry spanname=descrThe number of token partitions to use in VP8 encoder. +Possible values are:/entry + /row + row + entrytbl spanname=descr cols=2 + tbody valign=top + row + entryconstantV4L2_VPX_1_PARTITION/constant/entry + entry1 coefficient partition/entry + /row + row + entryconstantV4L2_VPX_2_PARTITIONS/constant/entry + entry2 partitions/entry Add 'coefficient' for the other cases as well in the description. At least, I think this should be '2 coefficient partitions'. + /row + row + entryconstantV4L2_VPX_4_PARTITIONS/constant/entry + entry4 partitions/entry + /row + row + entryconstantV4L2_VPX_8_PARTITIONS/constant/entry + entry8 partitions/entry + /row + /tbody + /entrytbl + /row +
Re: [PATCH v3 7/8] [media] V4L: Add VP8 encoder controls
On Tue June 25 2013 12:57:14 Arun Kumar K wrote: This patch adds new V4L controls for VP8 encoding. FYI: I plan on reviewing this as soon as I have some time (should be this weekend at the latest). Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 7/8] [media] V4L: Add VP8 encoder controls
Hi Kamil,
--- 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 Previous Specific Frame,
Use Previous Specific Frame seems unclear.
Maybe Use Previous Golden Frame or Use Periodic Golden Frame?
I'm not sure if I get the description right.
Use Periodic Golden Frame seems more reasonable. WIll change it.
Regards
Arun
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 7/8] [media] V4L: Add VP8 encoder controls
Hi Hans, On Wed, Jun 26, 2013 at 12:23 PM, Hans Verkuil hverk...@xs4all.nl wrote: On Tue June 25 2013 12:57:14 Arun Kumar K wrote: This patch adds new V4L controls for VP8 encoding. FYI: I plan on reviewing this as soon as I have some time (should be this weekend at the latest). Thank you. Will wait for you review before posting next version. Regards Arun -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 7/8] [media] V4L: Add VP8 encoder controls
This patch adds new V4L controls for VP8 encoding. Signed-off-by: Kiran AVND avnd.ki...@samsung.com Signed-off-by: Arun Kumar K arun...@samsung.com --- Documentation/DocBook/media/v4l/controls.xml | 150 ++ drivers/media/v4l2-core/v4l2-ctrls.c | 33 ++ include/uapi/linux/v4l2-controls.h | 29 - 3 files changed, 210 insertions(+), 2 deletions(-) diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml index 8d7a779..736c991 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 + titleVPX Control Reference/title + + paraThe VPX controls include controls for encoding parameters + of VPx video codec./para + + table pgwide=1 frame=none id=vpx-control-id + titleVPX 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=leftID/entry +entry align=leftType/entry + /rowrow rowsep=1entry spanname=descr align=leftDescription/entry + /row +/thead +tbody valign=top + rowentry/entry/row + + rowentry/entry/row + row id=v4l2-vpx-num-partitions + entry spanname=idconstantV4L2_CID_VPX_NUM_PARTITIONS/constant/entry + entryenum v4l2_vp8_num_partitions/entry + /row + rowentry spanname=descrThe number of token partitions to use in VP8 encoder. +Possible values are:/entry + /row + row + entrytbl spanname=descr cols=2 + tbody valign=top + row + entryconstantV4L2_VPX_1_PARTITION/constant/entry + entry1 coefficient partition/entry + /row + row + entryconstantV4L2_VPX_2_PARTITIONS/constant/entry + entry2 partitions/entry + /row + row + entryconstantV4L2_VPX_4_PARTITIONS/constant/entry + entry4 partitions/entry + /row + row + entryconstantV4L2_VPX_8_PARTITIONS/constant/entry + entry8 partitions/entry + /row + /tbody + /entrytbl + /row + + rowentry/entry/row + row + entry spanname=idconstantV4L2_CID_VPX_IMD_DISABLE_4X4/constant/entry + entryboolean/entry + /row + rowentry spanname=descrSetting this prevents intra 4x4 mode in the intra mode decision./entry + /row + + rowentry/entry/row + row id=v4l2-vpx-num-ref-frames + entry spanname=idconstantV4L2_CID_VPX_NUM_REF_FRAMES/constant/entry + entryenum v4l2_vp8_num_ref_frames/entry + /row + rowentry spanname=descrThe number of reference pictures for encoding P frames. +Possible values are:/entry + /row + row + entrytbl spanname=descr cols=2 + tbody valign=top + row + entryconstantV4L2_VPX_1_REF_FRAME/constant/entry + entryLast encoded frame will be searched/entry + /row + row + entryconstantV4L2_VPX_2_REF_FRAME/constant/entry + entryTwo frames would be searched among last encoded frame, golden frame +and altref frame. Encoder implementation can decide which two are chosen./entry + /row + row + entryconstantV4L2_VPX_3_REF_FRAME/constant/entry + entryThe last encoded frame, golden frame and altref frame will be searched./entry + /row + /tbody + /entrytbl + /row + + rowentry/entry/row + row + entry spanname=idconstantV4L2_CID_VPX_FILTER_LEVEL/constant/entry + entryinteger/entry + /row + rowentry spanname=descrIndicates the loop filter level. The adjustment of loop +filter level is done via a delta value against a baseline loop filter value./entry + /row + + rowentry/entry/row + row + entry spanname=idconstantV4L2_CID_VPX_FILTER_SHARPNESS/constant/entry + entryinteger/entry +
RE: [PATCH v3 7/8] [media] V4L: Add VP8 encoder controls
Hi Arun, This patch sets looks very good. Please check my comments below. From: Arun Kumar K [mailto:arun...@samsung.com] This patch adds new V4L controls for VP8 encoding. Signed-off-by: Kiran AVND avnd.ki...@samsung.com Signed-off-by: Arun Kumar K arun...@samsung.com --- Documentation/DocBook/media/v4l/controls.xml | 150 ++ drivers/media/v4l2-core/v4l2-ctrls.c | 33 ++ include/uapi/linux/v4l2-controls.h | 29 - 3 files changed, 210 insertions(+), 2 deletions(-) diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml index 8d7a779..736c991 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 + titleVPX Control Reference/title + + paraThe VPX controls include controls for encoding parameters + of VPx video codec./para + + table pgwide=1 frame=none id=vpx-control-id + titleVPX 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=leftID/entry +entry align=leftType/entry + /rowrow rowsep=1entry spanname=descr align=leftDescription/entry + /row +/thead +tbody valign=top + rowentry/entry/row + + rowentry/entry/row + row id=v4l2-vpx-num-partitions + entry spanname=idconstantV4L2_CID_VPX_NUM_PARTITIONS/constant/entry + entryenum v4l2_vp8_num_partitions/entry + /row + rowentry spanname=descrThe number of token partitions to use in VP8 encoder. +Possible values are:/entry + /row + row + entrytbl spanname=descr cols=2 + tbody valign=top + row + entryconstantV4L2_VPX_1_PARTITION/constant/entry + entry1 coefficient partition/entry + /row + row + entryconstantV4L2_VPX_2_PARTITIONS/constant/entry + entry2 partitions/entry + /row + row + entryconstantV4L2_VPX_4_PARTITIONS/constant/entry + entry4 partitions/entry + /row + row + entryconstantV4L2_VPX_8_PARTITIONS/constant/entry + entry8 partitions/entry + /row + /tbody + /entrytbl + /row + + rowentry/entry/row + row + entry spanname=idconstantV4L2_CID_VPX_IMD_DISABLE_4X4/constant/entry + entryboolean/entry + /row + rowentry spanname=descrSetting this prevents intra 4x4 mode in the intra mode decision./entry + /row + + rowentry/entry/row + row id=v4l2-vpx-num-ref-frames + entry spanname=idconstantV4L2_CID_VPX_NUM_REF_FRAMES/constant/entry + entryenum v4l2_vp8_num_ref_frames/entry + /row + rowentry spanname=descrThe number of reference pictures for encoding P frames. +Possible values are:/entry + /row + row + entrytbl spanname=descr cols=2 + tbody valign=top + row + entryconstantV4L2_VPX_1_REF_FRAME/constant/entry + entryLast encoded frame will be searched/entry + /row + row + entryconstantV4L2_VPX_2_REF_FRAME/constant/entry + entryTwo frames would be searched among last encoded frame, +golden frame and altref frame. Encoder implementation can decide which two are chosen./entry + /row + row + entryconstantV4L2_VPX_3_REF_FRAME/constant/entry + entryThe last encoded frame, golden frame and altref frame will be searched./entry + /row + /tbody + /entrytbl + /row + + rowentry/entry/row + row + entry spanname=idconstantV4L2_CID_VPX_FILTER_LEVEL/constant/entry + entryinteger/entry + /row + rowentry spanname=descrIndicates the loop filter level. +The adjustment of loop filter level is done via a delta value against a baseline loop filter value./entry + /row + + rowentry/entry/row + row + entry spanname=idconstantV4L2_CID_VPX_FILTER_SHARPNESS/constant/entry + entryinteger/entry +
