Hi Sylwester,
On Mon, Jun 17, 2013 at 2:34 PM, Sylwester Nawrocki
<s.nawrocki@xxxxxxxxxxx> wrote:
> On 06/17/2013 06:25 AM, Arun Kumar K wrote:
>> On Sat, Jun 15, 2013 at 1:01 AM, Sylwester Nawrocki
>> <s.nawrocki@xxxxxxxxxxx> wrote:
>>> On 06/14/2013 03:21 PM, Arun Kumar K wrote:
>>>> On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
>>>> <s.nawrocki@xxxxxxxxxxx> 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,
> 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_PARTITION 0
> #define V4L2_VPX_2_PARTITIONS 1
> #define V4L2_VPX_4_PARTITIONS 2
> #define V4L2_VPX_8_PARTITIONS 3
>
> Each driver could then map enum v4l2_vp8_num_partitions onto its
> required H/W register values, without having to translate from
> the MFC specific values. :)
Ok got it now. Thanks for the wonderful explanation :)
Regards
Arun
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
