Re: [PATCH] media: vivid: Support 480p for webcam capture

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

On 06/10/18 11:29, Keiichi Watanabe wrote:
> Hi all,
> 
> On Fri, Oct 5, 2018 at 6:18 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 10/03/18 09:08, Keiichi Watanabe wrote:
>>> I think 480p is a common frame size and it's worth supporting in vivid.
>>> But, my patch might be ad-hoc. Actually, I'm not sure which values are
>>> suitable for the intervals.
>>
>> I can apply this ad-hoc patch as-is.
>>
>> Or do you want to postpone this and work on a more generic solution?
>> Although I am not sure what that would look like.
> 
> I prefer providing a more flexible way rather than this ad-hoc patch.
> It would be helpful if there is a way of changing supported frame sizes easily.
> Perhaps, Kieran and me would use it, at least:)
> 

o/ <raising hand in agreement>

>>
>> Proposals are welcome!
>>
>> The main purpose of this code is to have something that kind of acts like
>> a real webcam that has various resolutions, and a slower framerate for
>> higher resolutions (as you would expect).
> 
> This sounds reasonable, so I guess we can keep this way as default and
> provide a way for specifying extra frame sizes as an option.
> 
> For example, how about a module option like this?
> "webcam_sizes=640x480@15,320x240@30"
> 
> If this parameter is passed to vivid, vivid works like a webcam that
> supports the following 7 pairs of frame size and fps:
> - 5 pairs of frame sizes and fps, as with the current implementation
> - 640x480 (15fps)
> - 320x240 (30fps)

I like the concept of being able to specify as a module parameter.

Perhaps we could have a magic marker on the string to define if the
existing frame sizes should be kept - or if this is just a complete
override ?

 vivid.webcam=640x480@15,320x240@30  # Specify sizes explicitly
 vivid.webcam=+640x480@15,320x240@30  # Append to existing frames
              ^ Magic Marker

We might of course want multiple rates per frame,

 vivid.webcam=640x480@15-25-30-90-120, # Separator to be defined...


> 
> If this parameter is not passed, vivid's behavior won't change from
> the current one.
> 
> How do you think?
> 
> We might want to stop using the default framesizes, i.e. vivid only
> supports framesize/fps that passed as the option.
> But, if we do so, the parameter will become mandatory and it would be annoying.

I think mandatory would be annoying yes.

Thus my suggestion for a magic marker above :)

--
Kieran


> So, I personally like to keep the default framesizes.
> 
> Best regards,
> Kei
> 
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> We might want to add a more flexible/extensible way to specify frame sizes.
>>> e.g. passing frame sizes and intervals as module parameters
>>>
>>> Kei
>>>
>>> On Wed, Oct 3, 2018 at 4:06 PM, Keiichi Watanabe <keiichiw@chromium.org> wrote:
>>>> Support 640x480 as a frame size for video input devices of vivid.
>>>>
>>>> Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
>>>> ---
>>>>  drivers/media/platform/vivid/vivid-vid-cap.c | 5 ++++-
>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/platform/vivid/vivid-vid-cap.c b/drivers/media/platform/vivid/vivid-vid-cap.c
>>>> index 58e14dd1dcd3..da80bf4bc365 100644
>>>> --- a/drivers/media/platform/vivid/vivid-vid-cap.c
>>>> +++ b/drivers/media/platform/vivid/vivid-vid-cap.c
>>>> @@ -51,7 +51,7 @@ static const struct vivid_fmt formats_ovl[] = {
>>>>  };
>>>>
>>>>  /* The number of discrete webcam framesizes */
>>>> -#define VIVID_WEBCAM_SIZES 5
>>>> +#define VIVID_WEBCAM_SIZES 6
>>>>  /* The number of discrete webcam frameintervals */
>>>>  #define VIVID_WEBCAM_IVALS (VIVID_WEBCAM_SIZES * 2)
>>>>
>>>> @@ -59,6 +59,7 @@ static const struct vivid_fmt formats_ovl[] = {
>>>>  static const struct v4l2_frmsize_discrete webcam_sizes[VIVID_WEBCAM_SIZES] = {
>>>>         {  320, 180 },
>>>>         {  640, 360 },
>>>> +       {  640, 480 },
>>>>         { 1280, 720 },
>>>>         { 1920, 1080 },
>>>>         { 3840, 2160 },
>>>> @@ -75,6 +76,8 @@ static const struct v4l2_fract webcam_intervals[VIVID_WEBCAM_IVALS] = {
>>>>         {  1, 5 },
>>>>         {  1, 10 },
>>>>         {  1, 15 },
>>>> +       {  1, 15 },
>>>> +       {  1, 25 },
>>>>         {  1, 25 },
>>>>         {  1, 30 },
>>>>         {  1, 50 },
>>>> --
>>>> 2.19.0.605.g01d371f741-goog
>>>>
>>


-- 
Regards
--
Kieran