From: Sylwester Nawrocki <s.nawrocki@samsung.com>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Kamil Debski <k.debski@samsung.com>,
'Laurent Pinchart' <laurent.pinchart@ideasonboard.com>,
linux-media@vger.kernel.org,
Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: Codec controls question
Date: Wed, 18 May 2011 17:57:37 +0200 [thread overview]
Message-ID: <4DD3EC71.5040100@samsung.com> (raw)
In-Reply-To: <16ed9ac8f44869af2d6ff7cded1c0023.squirrel@webmail.xs4all.nl>
Hi Hans,
On 05/18/2011 05:22 PM, Hans Verkuil wrote:
>>> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
>>> Sent: 18 May 2011 16:10
>>> Subject: Re: Codec controls question
>>> On Tuesday 17 May 2011 18:23:19 Kamil Debski wrote:
>>>> Hi,
>> Hi,
>>
>>>>
>>>> Some time ago we were discussing the set of controls that should be
>>>> implemented for codec support.
>>>>
>>>> I remember that the result of this discussion was that the controls
>>> should
>>>> be as "integrated" as possible. This included the V4L2_CID_MPEG_LEVEL
>>> and
>>>> all controls related to the quantization parameter.
>>>> The problem with such approach is that the levels are different for
>>> MPEG4,
>>>> H264 and H263. Same for quantization parameter - it ranges from 1 to
>>> 31
>>>> for MPEG4/H263 and from 0 to 51 for H264.
>>>>
>>>> Having single controls for the more than one codec seemed as a good
>>>> solution. Unfortunately I don't see a good option to implement it,
>>>> especially with the control framework. My idea was to have the min/max
>>>> values for QP set in the S_FMT call on the CAPTURE. For MPEG_LEVEL it
>>>> would be checked in the S_CTRL callback and if it did not fit the
>>> chosen
>>>> format it failed.
>>>>
>>>> So I see three solutions to this problem and I wanted to ask about
>>> your
>>>> opinion.
>>>>
>>>> 1) Have a separate controls whenever the range or valid value range
>>>> differs.
>>>>
>>>> This is the simplest and in my opinion the best solution I can think
>>> of.
>>>> This way we'll have different set of controls if the valid values are
>>>> different (e.g. V4L2_CID_MPEG_MPEG4_LEVEL, V4L2_CID_MPEG_H264_LEVEL).
>>>> User can set the controls at any time. The only con of this approach
>>> is
>>>> having more controls.
>>>>
>>>> 2) Permit the user to set the control only after running S_FMT on the
>>>> CAPTURE. This approach would enable us to keep less controls, but
>>> would
>>>> require to set the min/max values for controls in the S_FMT. This
>>> could be
>>>> done by adding controls in S_FMT or by manipulating their range and
>>>> disabling unused controls. In case of MPEG_LEVEL it would require
>>> s_ctrl
>>>> callback to check whether the requested level is valid for the chosen
>>>> codec.
>>>>
>>>> This would be somehow against the spec, but if we allow the "codec
>>>> interface" to have some differences this would be ok.
>>>>
>>>> 3) Let the user set the controls whenever and check them during the
>>>> STREAMON call.
>>>>
>>>> The controls could be set anytime, and the control range supplied to
>>> the
>>>> control framework would cover values possible for all supported
>>> codecs.
>>>>
>>>> This approach is more difficult than first approach. It is worse in
>>> case
>>> of
>>>> user space than the second approach - the user is unaware of any
>>> mistakes
>>>> until the STREAMON call. The argument for this approach is the
>>> possibility
>>>> to have a few controls less.
>>>>
>>>> So I would like to hear a comment about the above propositions.
>>> Personally
>>>> I would opt for the first solution.
>>>
>>> I think the question boils down to whether we want to support controls
>>> that
>>> have different valid ranges depending on formats, or even other
>>> controls. I
>>> think the issue isn't specific to codoc controls.
>>>
>>
>> So what is your opinion on this? If there are more controls where the
>> valid
>> range could depend on other controls or the chosen format then it might be
>> worth
>> implementing such functionality. If there would be only a few such
>> controls then
>> it might be better to just have separate controls (with the codec controls
>> - only
>> *_MPEG_LEVEL and quantization parameter related controls would have
>> different
>> valid range depending on the format).
>
> I have experimented with control events to change ranges and while it can
> be done technically it is in practice a bit of a mess. I think personally
> it is just easier to have separate controls.
>
> We are going to have similar problems if different video inputs are
> controlled by different i2c devices with different (but partially
> overlapping) controls. So switching an input also changes the controls. I
> have experimented with this while working on control events and it became
> very messy indeed. I won't do this for the first version of control
> events.
>
> One subtle but real problem with changing control ranges on the fly is
> that it makes it next to impossible to save all control values to a file
> and restore them later. That is a desirable feature that AFAIK is actually
> in use already.
What are your views on creating controls in subdev s_power operation ?
Some sensors/ISPs have control ranges dependant on a firmware revision.
So before creating the controls min/max/step values need to be read from them
over I2C. We chose to postpone enabling ISP's power until a corresponding video
(or subdev) device node is opened. And thus controls are not created during
driver probing, because there is no enough information to do this.
I don't see a possibility for the applications to be able to access the controls
before they are created as this happens during a first device (either video
or subdev) open(). And they are destroyed only in video/subdev device relase().
Do you see any potential issues with this scheme ?
Regards,
--
Sylwester Nawrocki
Samsung Poland R&D Center
next prev parent reply other threads:[~2011-05-18 15:57 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-05-17 16:23 Codec controls question Kamil Debski
2011-05-18 14:10 ` Laurent Pinchart
2011-05-18 14:39 ` Kamil Debski
2011-05-18 15:22 ` Hans Verkuil
2011-05-18 15:57 ` Sylwester Nawrocki [this message]
2011-05-18 16:02 ` Hans Verkuil
2011-05-18 16:03 ` Laurent Pinchart
2011-05-18 16:34 ` Sylwester Nawrocki
2011-05-18 16:27 ` Kamil Debski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4DD3EC71.5040100@samsung.com \
--to=s.nawrocki@samsung.com \
--cc=hverkuil@xs4all.nl \
--cc=k.debski@samsung.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.