Re: [PATCH v1 16/19] v4l: Add encoding camera controls.

Pawel Osciak
2013-09-09T08:00:11Z

On Mon, Sep 9, 2013 at 4:52 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 09/09/2013 05:48 AM, Pawel Osciak wrote:
>> Hi Hans,
>> Thanks for the comments, one question inline.
>>
>> On Fri, Aug 30, 2013 at 3:48 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> On 08/30/2013 04:17 AM, Pawel Osciak wrote:
>>>> Add defines for controls found in UVC 1.5 encoding cameras.
>>>>
>>>> Signed-off-by: Pawel Osciak <posciak@chromium.org>
>>>> ---
>>>> drivers/media/v4l2-core/v4l2-ctrls.c | 29 +++++++++++++++++++++++++++++
>>>> include/uapi/linux/v4l2-controls.h | 31 +++++++++++++++++++++++++++++++
>>>> 2 files changed, 60 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>> index c3f0803..0b3a632 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>> @@ -781,6 +781,35 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>> case V4L2_CID_AUTO_FOCUS_STATUS: return "Auto Focus, Status";
>>>> case V4L2_CID_AUTO_FOCUS_RANGE: return "Auto Focus, Range";
>>>>
>>>> + case V4L2_CID_ENCODER_MIN_FRAME_INTERVAL: return "Encoder, min. frame interval";
>>>> + case V4L2_CID_ENCODER_RATE_CONTROL_MODE: return "Encoder, rate control mode";
>>>> + case V4L2_CID_ENCODER_AVERAGE_BITRATE: return "Encoder, average bitrate";
>>>> + case V4L2_CID_ENCODER_CPB_SIZE: return "Encoder, CPB size";
>>>> + case V4L2_CID_ENCODER_PEAK_BIT_RATE: return "Encoder, peak bit rate";
>>>> + case V4L2_CID_ENCODER_QP_PARAM_I: return "Encoder, QP param for I frames";
>>>> + case V4L2_CID_ENCODER_QP_PARAM_P: return "Encoder, QP param for P frames";
>>>> + case V4L2_CID_ENCODER_QP_PARAM_BG: return "Encoder, QP param for B/G frames";
>>>
>>> A lot of these exist already. E.g. V4L2_CID_MPEG_VIDEO_MPEG4_I/P/B_FRAME_QP.
>>>
>>> Samsung added support for many of these parameters for their MFC encoder (including
>>> VP8 support) so you should use them as well. As mentioned in v4l2-controls.h the
>>> MPEG part of the control name is historical. Interpret it as 'CODEC', not MPEG.
>>>
>>
>> We have QP controls separately for H264, H263 and MPEG4. Why is that?
>> Which one should I use for VP8? Shouldn't we unify them instead?
>
> I can't quite remember the details, so I've CCed Kamil since he added those controls.
> At least the H264 QP controls are different from the others as they have a different
> range. What's the range for VP8?
>

Yes, it differs, 0-127.
But I feel this is pretty unfortunate, is it a good idea to multiply
controls to have one per format when they have different ranges
depending on the selected format in general? Perhaps a custom handler
would be better?

> I'm not sure why the H263/MPEG4 controls weren't unified: it might be that since the
> H264 range was different we decided to split it up per codec. But I seem to remember
> that there was another reason as well.
>
> Regards,
>
> Hans