Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls
Hi Sylwester,
Also for number of ref frames, the standard allows only the options 1,
2 and 3 which
cannot be extended more. So is it correct to use INTEGER_MENU control here
and
let the driver define the values?
If this is standard then the core should define available menu items. But
it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
opinions though.
Here even though 1,2 and 3 are standard, the interpretation is
1 - 1 reference frame (previous frame)
2 - previous frame + golden frame
3 - previous frame + golden frame + altref frame.
OK, then perhaps for this parameter a standard menu control would be better.
However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
You probably want to change the menu strings to reflect this more precisely,
but there might be not enough room for any creative names anyway. Maybe
something like:
static const char * const vpx_num_ref_frames[] = {
Previous Frame,
Previous + Golden Frame,
Prev + Golden + Altref Frame,
NULL,
};
On a more detailed inspection, the standard says maximum of 3 reference
frames. So in case of 2, it can be any of the permutation combination
possible. So rather I will stick to integer menu items saying 1, 2 and 3 where
on setting value 2, the encoder can decide on which frames to refer based
on its implementation but keeping the searching limit to 2 frames only.
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 5/6] [media] V4L: Add VP8 encoder controls
Hi Arun,
On 06/17/2013 06:25 AM, Arun Kumar K wrote:
On Sat, Jun 15, 2013 at 1:01 AM, Sylwester Nawrocki
s.nawro...@samsung.com wrote:
On 06/14/2013 03:21 PM, Arun Kumar K wrote:
On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
s.nawro...@samsung.com wrote:
On 06/14/2013 11:26 AM, Arun Kumar K wrote:
+ static const char * const vpx_num_partitions[] = {
+ 1 partition,
+ 2 partitions,
+ 4 partitions,
+ 8 partitions,
+ NULL,
+ };
+ static const char * const vpx_num_ref_frames[] = {
+ 1 reference frame,
+ 2 reference frame,
+ NULL,
+ };
Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for
this ?
One example is V4L2_CID_ISO_SENSITIVITY control.
If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
controls where
the driver / IP can support different values depending on its
capabilities.
No, not really, it just happens there is no INTEGER_MENU control with
standard
values yet. I think there are some (minor) changes needed in the v4l2-ctrls
code to support INTEGER_MENU control with standard menu items.
But here VP8 standard supports only 4 options for no. of partitions
that is 1, 2, 4 and 8.
I think such a standard menu list should be defined in v4l2-ctrls.c then.
One more concern here is these integer values 1, 2, 4 and 8 may not hold
much significance while setting to the registers. Some IPs may need these
values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
values that are given by the user may not be directly applicable to the
register setting.
The INTEGER_MENU control is not much different than MENU control from
driver POV. s_ctrl() op is called with similarly with the an index to
the menu option. In addition to standard menu applications can query
real value corresponding to a menu option, rather than a character
string only.
With each new control a nw strings are added, that cumulate in the
videodev module and make it bigger. Actually __s64 is not much smaller
than 1 partition in your case. But it's a bit smaller. :)
Yes that makes sense. But there will be a few extra functions that
would be needed in the v4l2-ctrls.c like may be
v4l2_ctrl_new_int_menu_fixed() which doesnt take qmenu arg from driver.
Will try to make this change.
I wouldn't be adding another helper like this, IMHO v4l2_ctrl_new_menu()
could well handle such entirely standard control type. I looked into that
over the weekend, let me send you my work-in-progress patch. Maybe you find
it useful.
That said I'm not either a codec expert or the v4l2 controls maintainer.
I think last words belongs to Hans. I just express my suggestions of
what I though we be more optimal (but not necessarily less work!). :)
Also for number of ref frames, the standard allows only the options 1,
2 and 3 which
cannot be extended more. So is it correct to use INTEGER_MENU control
here and
let the driver define the values?
If this is standard then the core should define available menu items. But
it seems more appropriate for me to use INTEGER_MENU. I'd like to hear
other
opinions though.
Here even though 1,2 and 3 are standard, the interpretation is
1 - 1 reference frame (previous frame)
2 - previous frame + golden frame
3 - previous frame + golden frame + altref frame.
OK, then perhaps for this parameter a standard menu control would be better.
However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
Thats because MFCv7 doesnt support the 3rd option yet. But still I would
add this in the control to make it generic.
I see. Yes, I think it makes more sense to specify the control fully,
according to the standard.
You probably want to change the menu strings to reflect this more precisely,
but there might be not enough room for any creative names anyway. Maybe
something like:
static const char * const vpx_num_ref_frames[] = {
Previous Frame,
Previous + Golden Frame,
Prev + Golden + Altref Frame,
NULL,
};
Anyway raw numbers might be better for the control and details could be
described in the documentation.
Ok will do like this.
Just one more note, I think I might have confused you with my comment
on the enum v4l2_vp8_num_partitions. Presumably we just need to define
contiguous enumeration for all options of V4L2_CID_VPX_NUM_PARTITIONS
control. And the actual values would be defined only on the integer
menu values array, e.g.
copnst s64 qmenu_int_vpx_num_partitions[] [
1, 2, 4, 8
};
and
#define V4L2_CID_VPX_NUM_PARTITIONS (V4L2_CID_MPEG_BASE + ?)
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,
};
or
#define V4L2_CID_VPX_NUM_PARTITIONS (V4L2_CID_MPEG_BASE + ?)
#define V4L2_VPX_1_PARTITION0
#define
Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls
Hi Sylwester,
On Mon, Jun 17, 2013 at 2:34 PM, Sylwester Nawrocki
s.nawro...@samsung.com wrote:
On 06/17/2013 06:25 AM, Arun Kumar K wrote:
On Sat, Jun 15, 2013 at 1:01 AM, Sylwester Nawrocki
s.nawro...@samsung.com wrote:
On 06/14/2013 03:21 PM, Arun Kumar K wrote:
On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
s.nawro...@samsung.com wrote:
On 06/14/2013 11:26 AM, Arun Kumar K wrote:
+ static const char * const vpx_num_partitions[] = {
+ 1 partition,
+ 2 partitions,
+ 4 partitions,
+ 8 partitions,
+ NULL,
+ };
+ static const char * const vpx_num_ref_frames[] = {
+ 1 reference frame,
+ 2 reference frame,
+ NULL,
+ };
Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for
this ?
One example is V4L2_CID_ISO_SENSITIVITY control.
If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
controls where
the driver / IP can support different values depending on its
capabilities.
No, not really, it just happens there is no INTEGER_MENU control with
standard
values yet. I think there are some (minor) changes needed in the
v4l2-ctrls
code to support INTEGER_MENU control with standard menu items.
But here VP8 standard supports only 4 options for no. of partitions
that is 1, 2, 4 and 8.
I think such a standard menu list should be defined in v4l2-ctrls.c then.
One more concern here is these integer values 1, 2, 4 and 8 may not hold
much significance while setting to the registers. Some IPs may need these
values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
values that are given by the user may not be directly applicable to the
register setting.
The INTEGER_MENU control is not much different than MENU control from
driver POV. s_ctrl() op is called with similarly with the an index to
the menu option. In addition to standard menu applications can query
real value corresponding to a menu option, rather than a character
string only.
With each new control a nw strings are added, that cumulate in the
videodev module and make it bigger. Actually __s64 is not much smaller
than 1 partition in your case. But it's a bit smaller. :)
Yes that makes sense. But there will be a few extra functions that
would be needed in the v4l2-ctrls.c like may be
v4l2_ctrl_new_int_menu_fixed() which doesnt take qmenu arg from driver.
Will try to make this change.
I wouldn't be adding another helper like this, IMHO v4l2_ctrl_new_menu()
could well handle such entirely standard control type. I looked into that
over the weekend, let me send you my work-in-progress patch. Maybe you find
it useful.
Ok that would be really helpful. Will check that patch.
That said I'm not either a codec expert or the v4l2 controls maintainer.
I think last words belongs to Hans. I just express my suggestions of
what I though we be more optimal (but not necessarily less work!). :)
Also for number of ref frames, the standard allows only the options 1,
2 and 3 which
cannot be extended more. So is it correct to use INTEGER_MENU control
here and
let the driver define the values?
If this is standard then the core should define available menu items. But
it seems more appropriate for me to use INTEGER_MENU. I'd like to hear
other
opinions though.
Here even though 1,2 and 3 are standard, the interpretation is
1 - 1 reference frame (previous frame)
2 - previous frame + golden frame
3 - previous frame + golden frame + altref frame.
OK, then perhaps for this parameter a standard menu control would be better.
However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
Thats because MFCv7 doesnt support the 3rd option yet. But still I would
add this in the control to make it generic.
I see. Yes, I think it makes more sense to specify the control fully,
according to the standard.
You probably want to change the menu strings to reflect this more precisely,
but there might be not enough room for any creative names anyway. Maybe
something like:
static const char * const vpx_num_ref_frames[] = {
Previous Frame,
Previous + Golden Frame,
Prev + Golden + Altref Frame,
NULL,
};
Anyway raw numbers might be better for the control and details could be
described in the documentation.
Ok will do like this.
Just one more note, I think I might have confused you with my comment
on the enum v4l2_vp8_num_partitions. Presumably we just need to define
contiguous enumeration for all options of V4L2_CID_VPX_NUM_PARTITIONS
control. And the actual values would be defined only on the integer
menu values array, e.g.
copnst s64 qmenu_int_vpx_num_partitions[] [
1, 2, 4, 8
};
and
#define V4L2_CID_VPX_NUM_PARTITIONS (V4L2_CID_MPEG_BASE + ?)
enum v4l2_vp8_num_partitions {
V4L2_VPX_1_PARTITION= 0,
V4L2_VPX_2_PARTITIONS = 1,
Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls
Hi Sylwester,
On Sat, Jun 15, 2013 at 1:01 AM, Sylwester Nawrocki
s.nawro...@samsung.com wrote:
Hi Arun,
On 06/14/2013 03:21 PM, Arun Kumar K wrote:
On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
s.nawro...@samsung.com wrote:
On 06/14/2013 11:26 AM, Arun Kumar K wrote:
+ static const char * const vpx_num_partitions[] = {
+ 1 partition,
+ 2 partitions,
+ 4 partitions,
+ 8 partitions,
+ NULL,
+ };
+ static const char * const vpx_num_ref_frames[] = {
+ 1 reference frame,
+ 2 reference frame,
+ NULL,
+ };
Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for
this ?
One example is V4L2_CID_ISO_SENSITIVITY control.
If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
controls where
the driver / IP can support different values depending on its capabilities.
No, not really, it just happens there is no INTEGER_MENU control with
standard
values yet. I think there are some (minor) changes needed in the v4l2-ctrls
code to support INTEGER_MENU control with standard menu items.
But here VP8 standard supports only 4 options for no. of partitions
that is 1, 2, 4 and 8.
I think such a standard menu list should be defined in v4l2-ctrls.c then.
One more concern here is these integer values 1, 2, 4 and 8 may not hold
much significance while setting to the registers. Some IPs may need these
values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
values that are given by the user may not be directly applicable to the
register setting.
The INTEGER_MENU control is not much different than MENU control from
driver POV. s_ctrl() op is called with similarly with the an index to
the menu option. In addition to standard menu applications can query
real value corresponding to a menu option, rather than a character
string only.
With each new control a nw strings are added, that cumulate in the
videodev module and make it bigger. Actually __s64 is not much smaller
than 1 partition in your case. But it's a bit smaller. :)
Yes that makes sense. But there will be a few extra functions that
would be needed in the v4l2-ctrls.c like may be
v4l2_ctrl_new_int_menu_fixed() which doesnt take qmenu arg from driver.
Will try to make this change.
That said I'm not either a codec expert or the v4l2 controls maintainer.
I think last words belongs to Hans. I just express my suggestions of
what I though we be more optimal (but not necessarily less work!). :)
Also for number of ref frames, the standard allows only the options 1,
2 and 3 which
cannot be extended more. So is it correct to use INTEGER_MENU control here
and
let the driver define the values?
If this is standard then the core should define available menu items. But
it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
opinions though.
Here even though 1,2 and 3 are standard, the interpretation is
1 - 1 reference frame (previous frame)
2 - previous frame + golden frame
3 - previous frame + golden frame + altref frame.
OK, then perhaps for this parameter a standard menu control would be better.
However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
Thats because MFCv7 doesnt support the 3rd option yet. But still I would
add this in the control to make it generic.
You probably want to change the menu strings to reflect this more precisely,
but there might be not enough room for any creative names anyway. Maybe
something like:
static const char * const vpx_num_ref_frames[] = {
Previous Frame,
Previous + Golden Frame,
Prev + Golden + Altref Frame,
NULL,
};
Anyway raw numbers might be better for the control and details could be
described in the documentation.
Ok will do like this.
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 5/6] [media] V4L: Add VP8 encoder controls
Hi Sylwester,
+ static const char * const vpx_num_partitions[] = {
+ 1 partition,
+ 2 partitions,
+ 4 partitions,
+ 8 partitions,
+ NULL,
+ };
+ static const char * const vpx_num_ref_frames[] = {
+ 1 reference frame,
+ 2 reference frame,
+ NULL,
+ };
Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
One example is V4L2_CID_ISO_SENSITIVITY control.
If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
controls where
the driver / IP can support different values depending on its capabilities.
But here VP8 standard supports only 4 options for no. of partitions
that is 1, 2, 4 and 8.
Also for number of ref frames, the standard allows only the options 1,
2 and 3 which
cannot be extended more. So is it correct to use INTEGER_MENU control here and
let the driver define the values?
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 5/6] [media] V4L: Add VP8 encoder controls
Hi Arun,
On 06/14/2013 11:26 AM, Arun Kumar K wrote:
Hi Sylwester,
+ static const char * const vpx_num_partitions[] = {
+ 1 partition,
+ 2 partitions,
+ 4 partitions,
+ 8 partitions,
+ NULL,
+ };
+ static const char * const vpx_num_ref_frames[] = {
+ 1 reference frame,
+ 2 reference frame,
+ NULL,
+ };
Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
One example is V4L2_CID_ISO_SENSITIVITY control.
If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
controls where
the driver / IP can support different values depending on its capabilities.
No, not really, it just happens there is no INTEGER_MENU control with standard
values yet. I think there are some (minor) changes needed in the v4l2-ctrls
code to support INTEGER_MENU control with standard menu items.
But here VP8 standard supports only 4 options for no. of partitions
that is 1, 2, 4 and 8.
I think such a standard menu list should be defined in v4l2-ctrls.c then.
Also for number of ref frames, the standard allows only the options 1,
2 and 3 which
cannot be extended more. So is it correct to use INTEGER_MENU control here and
let the driver define the values?
If this is standard then the core should define available menu items. But
it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
opinions though.
Regards,
Sylwester
--
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 5/6] [media] V4L: Add VP8 encoder controls
Hi Sylwester,
On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
s.nawro...@samsung.com wrote:
Hi Arun,
On 06/14/2013 11:26 AM, Arun Kumar K wrote:
Hi Sylwester,
+ static const char * const vpx_num_partitions[] = {
+ 1 partition,
+ 2 partitions,
+ 4 partitions,
+ 8 partitions,
+ NULL,
+ };
+ static const char * const vpx_num_ref_frames[] = {
+ 1 reference frame,
+ 2 reference frame,
+ NULL,
+ };
Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this
?
One example is V4L2_CID_ISO_SENSITIVITY control.
If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
controls where
the driver / IP can support different values depending on its capabilities.
No, not really, it just happens there is no INTEGER_MENU control with standard
values yet. I think there are some (minor) changes needed in the v4l2-ctrls
code to support INTEGER_MENU control with standard menu items.
But here VP8 standard supports only 4 options for no. of partitions
that is 1, 2, 4 and 8.
I think such a standard menu list should be defined in v4l2-ctrls.c then.
One more concern here is these integer values 1, 2, 4 and 8 may not hold
much significance while setting to the registers. Some IPs may need these
values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
values that are given by the user may not be directly applicable to the
register setting.
Also for number of ref frames, the standard allows only the options 1,
2 and 3 which
cannot be extended more. So is it correct to use INTEGER_MENU control here
and
let the driver define the values?
If this is standard then the core should define available menu items. But
it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
opinions though.
Here even though 1,2 and 3 are standard, the interpretation is
1 - 1 reference frame (previous frame)
2 - previous frame + golden frame
3 - previous frame + golden frame + altref frame.
Again the driver may need to set different registers based on these and the
integer values as such may not be used.
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 5/6] [media] V4L: Add VP8 encoder controls
Hi Arun,
On 06/14/2013 03:21 PM, Arun Kumar K wrote:
On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
s.nawro...@samsung.com wrote:
On 06/14/2013 11:26 AM, Arun Kumar K wrote:
+ static const char * const vpx_num_partitions[] = {
+ 1 partition,
+ 2 partitions,
+ 4 partitions,
+ 8 partitions,
+ NULL,
+ };
+ static const char * const vpx_num_ref_frames[] = {
+ 1 reference frame,
+ 2 reference frame,
+ NULL,
+ };
Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for
this ?
One example is V4L2_CID_ISO_SENSITIVITY control.
If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
controls where
the driver / IP can support different values depending on its capabilities.
No, not really, it just happens there is no INTEGER_MENU control with
standard
values yet. I think there are some (minor) changes needed in the v4l2-ctrls
code to support INTEGER_MENU control with standard menu items.
But here VP8 standard supports only 4 options for no. of partitions
that is 1, 2, 4 and 8.
I think such a standard menu list should be defined in v4l2-ctrls.c then.
One more concern here is these integer values 1, 2, 4 and 8 may not hold
much significance while setting to the registers. Some IPs may need these
values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
values that are given by the user may not be directly applicable to the
register setting.
The INTEGER_MENU control is not much different than MENU control from
driver POV. s_ctrl() op is called with similarly with the an index to
the menu option. In addition to standard menu applications can query
real value corresponding to a menu option, rather than a character
string only.
With each new control a nw strings are added, that cumulate in the
videodev module and make it bigger. Actually __s64 is not much smaller
than 1 partition in your case. But it's a bit smaller. :)
That said I'm not either a codec expert or the v4l2 controls maintainer.
I think last words belongs to Hans. I just express my suggestions of
what I though we be more optimal (but not necessarily less work!). :)
Also for number of ref frames, the standard allows only the options 1,
2 and 3 which
cannot be extended more. So is it correct to use INTEGER_MENU control here
and
let the driver define the values?
If this is standard then the core should define available menu items. But
it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
opinions though.
Here even though 1,2 and 3 are standard, the interpretation is
1 - 1 reference frame (previous frame)
2 - previous frame + golden frame
3 - previous frame + golden frame + altref frame.
OK, then perhaps for this parameter a standard menu control would be better.
However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
You probably want to change the menu strings to reflect this more precisely,
but there might be not enough room for any creative names anyway. Maybe
something like:
static const char * const vpx_num_ref_frames[] = {
Previous Frame,
Previous + Golden Frame,
Prev + Golden + Altref Frame,
NULL,
};
Anyway raw numbers might be better for the control and details could be
described in the documentation.
Again the driver may need to set different registers based on these and the
integer values as such may not be used.
This is really not relevant, the driver can map index of the menu to any
value that is needed by the hardware.
Regards,
Sylwester
--
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 5/6] [media] V4L: Add VP8 encoder controls
Hi Hans,
Thank you for the review.
+
+section id=vpx-controls
+ titleVPX Control Reference/title
+
+ paraThe VPX control class includes controls for encoding parameters
+ of VPx video codec./para
Are these controls defined by the VPx standard, or are they specific to the
Samsung hardware?
These controls are part of VP8 standard and even the reference VP8 encoder
supports these.
I am not certain whether a separate class should be created for these. Adding
it to the MPEG control class might be a better approach (yes, I know MPEG is
a bit of a misnomer, but it already handles other compressions standards as
well).
Ok. I added them as a separate control class as VP8 is not from MPEG.
I shall add it along with MPEG in the v2 version..
+ rowentry/entry/row
+ row
+ entry
spanname=idconstantV4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD/constantnbsp;/entry
+ entryinteger/entry
+ /row
+ rowentry spanname=descrSets the refresh period for golden
frame./entry
The period is in number of frames I assume? And is the golden frame taken at
the start or at the end of the period?
Yes its in number of frames.
If we set refresh period as 5, then every 5th frame starting from 1st
frame is made as golden frame.
Will update to make it more clear.
+ rowentry/entry/row
+ row id=v4l2-vpx-golden-frame-sel
+ entry
spanname=idconstantV4L2_CID_VPX_GOLDEN_FRAME_SEL/constantnbsp;/entry
+ entryenumnbsp;v4l2_vp8_golden_frame_sel/entry
+ /row
+ rowentry spanname=descrSelects the golden frame for
encoding.
+Possible values are:/entry
+ /row
+ row
+ entrytbl spanname=descr cols=2
+ tbody valign=top
+ row
+
entryconstantV4L2_VPX_GOLDEN_FRAME_USE_PREV/constantnbsp;/entry
+ entryUse the previous second frame as a golden
frame/entry
Second frame of what? It's not clear to me how I should interpret this.
This means the second last frame.
When user selects 2 reference frames for encoding, the last frame and
the golden frame is searched
for reference. With the V4L2_VPX_GOLDEN_FRAME_USE_PREV option, the
last to last frame is
selected as the golden frame. Will update the description for more clarity.
+ /row
+ row
+
entryconstantV4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD/constantnbsp;/entry
+ entryUse the previous specific frame indicated by
V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD as a golden frame/entry
+ /row
+ /tbody
+ /entrytbl
+ /row
@@ -456,6 +456,23 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
RGB full range (0-255),
NULL,
};
+ static const char * const vpx_num_partitions[] = {
+ 1 partition,
+ 2 partitions,
+ 4 partitions,
+ 8 partitions,
Please use a capital P for Partition.
Ok.
+ NULL,
+ };
+ static const char * const vpx_num_ref_frames[] = {
+ 1 reference frame,
+ 2 reference frame,
frame - Frames
Capitalize these strings. Same for all the others.
Ok.
+ NULL,
+ };
+ static const char * const vpx_golden_frame_sel[] = {
+ Use previous frame,
+ Use frame indicated by GOLDEN_FRAME_REF_PERIOD,
+ NULL,
+ };
switch (id) {
@@ -545,6 +562,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
case V4L2_CID_DV_TX_RGB_RANGE:
case V4L2_CID_DV_RX_RGB_RANGE:
return dv_rgb_range;
+ case V4L2_CID_VPX_NUM_PARTITIONS:
+ return vpx_num_partitions;
+ case V4L2_CID_VPX_NUM_REF_FRAMES:
+ return vpx_num_ref_frames;
+ case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
+ return vpx_golden_frame_sel;
default:
return NULL;
@@ -806,6 +829,17 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_FM_RX_CLASS: return FM Radio Receiver
Controls;
case V4L2_CID_TUNE_DEEMPHASIS: return De-Emphasis;
case V4L2_CID_RDS_RECEPTION:return RDS Reception;
+
+ /* VPX controls */
+ case V4L2_CID_VPX_CLASS:return VPX Encoder 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 Number of
reference pictures for P frames;
This string is too long: only 31 characters (excluding the trailing \0) can
be used.
Ok will correct it.
Thanks and regards
Arun
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls
Hi Sylwester,
Thank you for the review.
+ static const char * const vpx_num_partitions[] = {
+ 1 partition,
+ 2 partitions,
+ 4 partitions,
+ 8 partitions,
+ NULL,
+ };
+ static const char * const vpx_num_ref_frames[] = {
+ 1 reference frame,
+ 2 reference frame,
+ NULL,
+ };
Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
One example is V4L2_CID_ISO_SENSITIVITY control.
Ok will change it to V4L2_CTRL_TYPE_INTEGER_MENU.
+/* VPX streams, specific to multiplexed streams */
+#define V4L2_CID_VPX_NUM_PARTITIONS (V4L2_CID_VPX_BASE+0)
+enum v4l2_vp8_num_partitions {
+ V4L2_VPX_1_PARTITION= 0,
+ V4L2_VPX_2_PARTITIONS = (1 1),
+ V4L2_VPX_4_PARTITIONS = (1 2),
+ V4L2_VPX_8_PARTITIONS = (1 3),
+};
I think we could still have such standard value definitions if needed,
but rather in form of:
#define V4L2_VPX_1_PARTITION1
#define V4L2_VPX_2_PARTITIONS 2
#define V4L2_VPX_4_PARTITIONS 4
#define V4L2_VPX_8_PARTITIONS 8
Ok will change.
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 5/6] [media] V4L: Add VP8 encoder controls
This patch adds new V4L controls for VP8 encoding. Signed-off-by: Arun Kumar K arun...@samsung.com Signed-off-by: Kiran AVND avnd.ki...@samsung.com --- Documentation/DocBook/media/v4l/controls.xml | 145 ++ drivers/media/v4l2-core/v4l2-ctrls.c | 38 +++ include/uapi/linux/v4l2-controls.h | 30 +- 3 files changed, 212 insertions(+), 1 deletion(-) diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml index 8d7a779..db614c7 100644 --- a/Documentation/DocBook/media/v4l/controls.xml +++ b/Documentation/DocBook/media/v4l/controls.xml @@ -4772,4 +4772,149 @@ defines possible values for de-emphasis. Here they are:/entry /table /section + +section id=vpx-controls + titleVPX Control Reference/title + + paraThe VPX control class includes controls for encoding parameters + of VPx video codec./para + + table pgwide=1 frame=none id=fm-rx-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/constantnbsp;/entry + entryenumnbsp;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/constantnbsp;/entry + entry1 coefficient partition/entry + /row + row + entryconstantV4L2_VPX_2_PARTITIONS/constantnbsp;/entry + entry2 partitions/entry + /row + row + entryconstantV4L2_VPX_4_PARTITIONS/constantnbsp;/entry + entry4 partitions/entry + /row + row + entryconstantV4L2_VPX_8_PARTITIONS/constantnbsp;/entry + entry8 partitions/entry + /row + /tbody + /entrytbl + /row + + rowentry/entry/row + row + entry spanname=idconstantV4L2_CID_VPX_IMD_DISABLE_4X4/constantnbsp;/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/constantnbsp;/entry + entryenumnbsp;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/constantnbsp;/entry + entryLast encoded frame will be searched/entry + /row + row + entryconstantV4L2_VPX_2_REF_FRAME/constantnbsp;/entry + entryLast encoded frame and the Golden frame will be searched/entry + /row + /tbody + /entrytbl + /row + + rowentry/entry/row + row + entry spanname=idconstantV4L2_CID_VPX_FILTER_LEVEL/constantnbsp;/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/constantnbsp;/entry + entryinteger/entry + /row + rowentry spanname=descrThis parameter affects the loop filter. Anything above +zero weakens the deblocking effect on loop filter./entry + /row + + rowentry/entry/row + row +
Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls
Hi Arun, Some review comments below... On Mon June 10 2013 15:23:05 Arun Kumar K wrote: This patch adds new V4L controls for VP8 encoding. Signed-off-by: Arun Kumar K arun...@samsung.com Signed-off-by: Kiran AVND avnd.ki...@samsung.com --- Documentation/DocBook/media/v4l/controls.xml | 145 ++ drivers/media/v4l2-core/v4l2-ctrls.c | 38 +++ include/uapi/linux/v4l2-controls.h | 30 +- 3 files changed, 212 insertions(+), 1 deletion(-) diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml index 8d7a779..db614c7 100644 --- a/Documentation/DocBook/media/v4l/controls.xml +++ b/Documentation/DocBook/media/v4l/controls.xml @@ -4772,4 +4772,149 @@ defines possible values for de-emphasis. Here they are:/entry /table /section + +section id=vpx-controls + titleVPX Control Reference/title + + paraThe VPX control class includes controls for encoding parameters + of VPx video codec./para Are these controls defined by the VPx standard, or are they specific to the Samsung hardware? I am not certain whether a separate class should be created for these. Adding it to the MPEG control class might be a better approach (yes, I know MPEG is a bit of a misnomer, but it already handles other compressions standards as well). + + table pgwide=1 frame=none id=fm-rx-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/constantnbsp;/entry + entryenumnbsp;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/constantnbsp;/entry + entry1 coefficient partition/entry + /row + row + entryconstantV4L2_VPX_2_PARTITIONS/constantnbsp;/entry + entry2 partitions/entry + /row + row + entryconstantV4L2_VPX_4_PARTITIONS/constantnbsp;/entry + entry4 partitions/entry + /row + row + entryconstantV4L2_VPX_8_PARTITIONS/constantnbsp;/entry + entry8 partitions/entry + /row + /tbody + /entrytbl + /row + + rowentry/entry/row + row + entry spanname=idconstantV4L2_CID_VPX_IMD_DISABLE_4X4/constantnbsp;/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/constantnbsp;/entry + entryenumnbsp;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/constantnbsp;/entry + entryLast encoded frame will be searched/entry + /row + row + entryconstantV4L2_VPX_2_REF_FRAME/constantnbsp;/entry + entryLast encoded frame and the Golden frame will be searched/entry + /row + /tbody + /entrytbl + /row + + rowentry/entry/row + row + entry spanname=idconstantV4L2_CID_VPX_FILTER_LEVEL/constantnbsp;/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 +
Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls
Hi Arun,
On 06/10/2013 03:23 PM, Arun Kumar K wrote:
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index fccd08b..2cf17d4 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -456,6 +456,23 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
RGB full range (0-255),
NULL,
};
+ static const char * const vpx_num_partitions[] = {
+ 1 partition,
+ 2 partitions,
+ 4 partitions,
+ 8 partitions,
+ NULL,
+ };
+ static const char * const vpx_num_ref_frames[] = {
+ 1 reference frame,
+ 2 reference frame,
+ NULL,
+ };
Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
One example is V4L2_CID_ISO_SENSITIVITY control.
+/* VPX streams, specific to multiplexed streams */
+#define V4L2_CID_VPX_NUM_PARTITIONS (V4L2_CID_VPX_BASE+0)
+enum v4l2_vp8_num_partitions {
+ V4L2_VPX_1_PARTITION= 0,
+ V4L2_VPX_2_PARTITIONS = (1 1),
+ V4L2_VPX_4_PARTITIONS = (1 2),
+ V4L2_VPX_8_PARTITIONS = (1 3),
+};
I think we could still have such standard value definitions if needed,
but rather in form of:
#define V4L2_VPX_1_PARTITION1
#define V4L2_VPX_2_PARTITIONS 2
#define V4L2_VPX_4_PARTITIONS 4
#define V4L2_VPX_8_PARTITIONS 8
+#define V4L2_CID_VPX_IMD_DISABLE_4X4 (V4L2_CID_VPX_BASE+1)
+#define V4L2_CID_VPX_NUM_REF_FRAMES (V4L2_CID_VPX_BASE+2)
+enum v4l2_vp8_num_ref_frames {
+ V4L2_VPX_1_REF_FRAME= 0,
+ V4L2_VPX_2_REF_FRAME= 1,
+};
Regards,
Sylwester
--
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
