Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6ee01cf1-5a8b-081f-e218-8c7da39343bc@xs4all.nl>
Date:   Tue, 11 Apr 2023 09:26:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v1] media: vivid: Add webcam parameter for (un)limited
 bandwidth
Content-Language: en-US
To:     Max Staudt <mstaudt@chromium.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>
Cc:     linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Ricardo Ribalda <ribalda@chromium.org>,
        Yunke Cao <yunkec@chromium.org>,
        Tomasz Figa <tfiga@chromium.org>
References: <20230410063356.3894767-1-mstaudt@chromium.org>
 <20230410102350.382f7d02@sal.lan>
 <6aafad18-13a2-ef45-48a1-1f094554af31@chromium.org>
From:   Hans Verkuil <hverkuil@xs4all.nl>
In-Reply-To: <6aafad18-13a2-ef45-48a1-1f094554af31@chromium.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 11/04/2023 08:51, Max Staudt wrote:
> Thank you Mauro for having a first look!
> 
> Questions below.
> 
> 
> On 4/10/23 18:23, Mauro Carvalho Chehab wrote:
>> IMO, instead of a parameter that just enables/disables the bandwidth
>> limit, the best would be to have a parameter specifying the bandwidth
>> (with 0 meaning unlimited).
>>
>> If not used, vivid would initialize it to dev->webcam_bandwidth_limit,
>> so a read operation will show the current limit.
> Up until now, the bandwidth limit is a rather arbitrary reduction of two interval sizes per frame size.
> 
> How would you prefer to define a limited bandwidth in this parameter? How would it affect the simulated camera, do you have a suggestion for a formula from bandwidth to frame/interval sizes offered?
> 
> 
>>> +/* Default: limited webcam bandwidth */
>>> +static bool webcam_bandwidth_limit[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS - 1)] = true };
>>> +module_param_array(webcam_bandwidth_limit, bool, NULL, 0444);
>>
>> I would also use 0666, to allow changing this on runtime.
> 
> I guess that's possible, though it would add complexity.
> 
> Currently we can ask for two instances, each with a different setting:
> 
>   n_devs=2 webcam_bandwidth_limit=1,0
> 
> This creates /dev/video0 which is limited, and /dev/video4 which is unlimited.
> 
> Maybe this already sufficiently covers the case you are looking for, and we can keep the complexity low? A real webcam won't suddenly offer new frame rates either...

I think we either use this bandwidth option and calculate the max fps based on
that (basically the bandwidth divided by (image_size + some blanking factor)),
or we keep it simple and instead of going down two steps in fps we allow up to
60 fps up to 720p, then 30 fps for 1080p and 15 fps for 4k.

The fps values currently used are a bit outdated w.r.t. modern webcams, so
upgrading it wouldn't hurt. And this is a lot simpler than doing bandwidth
calculations.

Regards,

	Hans