Em 08-12-2010 12:11, Hans de Goede escreveu:
> Hi,
>
> On 12/08/2010 01:54 PM, Mauro Carvalho Chehab wrote:
>> Em 08-12-2010 08:01, Hans de Goede escreveu:
>>> Hi,
>>>
>>> On 12/07/2010 09:01 PM, Mauro Carvalho Chehab wrote:
>>>> Hi Jean-Fronçois,
>>>>
>>>> git commit 35680baa6822df98a6ed602e2380aa0a04e18b07 (see enclosed) caused not only a regression
>>>> at PS/3 Eye webcam (git commit f43402fa55bf5e7e190c176343015122f694857c), but also at sn9c201 driver,
>>>> when used on SXGA mode. What happens is that only the highest alternate mode is enough for the
>>>> maximum resolution.
>>>>
>>>> I suspect that other drivers broke due to that change. So, IMO, the better is to revert and work
>>>> on another alternative.
>>>>
>>>
>>> I have to agree with Mauro here, dropping back to a lower alt setting for all devices with a usb
>>> audio interface is the wrong thing to do.
>>
>>
>> Btw, I tested here with other hardware. It completely broke stv06xx driver for devices with audio
>> (046d:08f6 Logitech, Inc. QuickCam Messenger Plus), as there's just one alternate config:
>>
>> gspca: no transfer endpoint found
>>
>> My proposal, for now, is to just revert the logic, enabling the hack only in the specific case where
>> doing alt-1 work (see enclosed patch). I tested the enclosed patch and it fixed for PS/3, Quickcam
>> and Gigaware (the 3 cams where I noticed the breakage).
>>
>
> Hmm, looking at your patch I notice that the current patch is completely wrong. Not only does it
> lower the endpoint to use wrongly in many cases, it also does the lowering *each* time we try
> to start streaming, so if we have a cam with alt settings 0 - 8 and there is not enough bandwidth
> we used to try: 8, 7, 6, ... and now we will try 7, 5, 3, ... Notice the skipping of an alt setting
> each time.
>
> Before this broken, should be reverted, patch in some cases we used to lower the nbalt setting in some
> subdrivers where the highest alt setting causes issues for audio, which would result in trying 7, 6, 5
> ... in the example, iow work as intended where as 7, 5, 3, ... clearly is not work as intended.
Agreed. Let's then just revert the broken patch.
>> Of course, it will break the specific model(s) where the audio hack work, so we need to do:
>> gspca_dev->audio_hack = 1;
>> On the specific model(s) tested with the hack.
>>
>>> For example I know multiple devices where the highest alt setting does not have a wMaxPacketSize
>>> of 1023, but use something lower instead, to make sure there is enough bandwidth left for usb audio
>>> even if the highest alt setting get used.
>>>
>>>> Btw, no matter what resolution is used, sn9c201 is setting the same alternate for all modes,
>>>> spending more USB bandwidth than needed. Why gspca is just getting the highest value for
>>>> wMaxPacketSize? It should, instead, seek for the minimum packet size that is needed for a
>>>> given resolution.
>>>
>>> There are 2 reasons for this:
>>> 1) We do not know the minimum packetsize for a given resolution, unlike with uvc where this
>>> is arranged in the protocol, for proprietary apps we can only guess
>>
>> Ok, but, during the driver development, someone could test it.
>>
>
> Right, but many of the drivers come from other sources (old v4l1 drivers) and often we only
> have one model of many supported models when porting these drivers to the new gspca framework.
Ok, so, for some drivers, we might never be able to have a proper fix, due to the lack of
resources, but we should try to fix it where possible.
>>> 2) Almost all devices supported by gspca are usb-1.1 and thus the framerate is bandwidth
>>> limited. We want to deliver the highest framerate possible, and thus get as high an alt
>>> setting as possible.
>>
>> This is a bogus argument. Core shouldn't assume that all devices are usb 1.1.
>
> You're right about the should not assume part, but that does not make this a bogus argument
> usb-1.1 webcams are bandwidth limited in their framerate, people don't like low framerates
> so clearly trying to get the highest alt setting and thus allow for the highest framerate
> is the right thing to do for 1.1 cameras.
Allowing the highest framerate/resolution is the right thing to do also for 2.0 cameras.
The point is that, if the user is selecting a lower resolution/framerate, drivers should
use lower sizes, to save bandwidth.
>> In the specific case of sn9c20x, it has 8 alternates. This probably means that all those alternates
>> could be used, depending on the bandwidth requirements.
>>
>> At the maximum packet size (alt 8) means to spend 60% of the USB 2.0 traffic for isoc, being
>> required for SXGA. Alt 7 spends 36% and it is enough for VGA resolution.
>>
>> If someone wants to have 2 webcams at VGA resolution, wasting bandwith using alt8 would prevent it,
>> as core don't provide 120% bandwidth ;)
>>
>
> I agree that the highest setting should not be used when not needed.
Yes, that's my point ;)
>> The alternate selection should be based on the desired resolution. It is a function of
>> the imagesize, the frame rate, and the line width. On em28xx, we've made a function that estimates
>> the needed alternate. This works pretty well.
>>
>
> When the framerate is a fixed number this works well, but it is not a fixed number with webcams. More
> over some 1.1 webcams use "dynamic" compression algorithms where the bridge changes the compression
> quality setting based on available bandwidth. So here we may be able to do the same framerate at a
> lower alt setting, but this will result in a lower quality picture.
Well, for devices that have just jpeg (or other compressed formats), and has just one resolution,
the algorithm of just getting the highest available packet size seems to work well.
This covers several usb 1.1 cameras.
But, on devices that have multiple resolutions (e. g. USB 2.0 devices) and/or provide raw image
formats, this algorithm is sub-optimal.
Perhaps the core should have different strategies to handle compressed and uncompressed formats.
>>> I most note that many gspca subdrivers are defective wrt how this is all handled though, in
>>> that they don't vary the framerate when the available bandwidth changes. So in many cases if
>>> the gspca core cannot get the highest alt setting things may not function at all (because the
>>> subdriver fails to select a lower framerate, resulting in parts of frames getting dropped
>>> due to bandwidth limitations).
>>
>> Ok, but gspca shouldn't assume that subdrivers won't do it. I did a "grep alt" and discovered that
>> most drivers just force the number of alternates to something else, like:
>> drivers/media/video/gspca/spca561.c: gspca_dev->nbalt = 7 + 1; /* choose alternate 7 first */
>>
>
> "most" drivers ? I know of a few that do this, and they do this usually to make sure that the highest
> setting does not get used as that conflicts with using the microphone at the same time (on some types).
I mean, most drivers that do something with alternates ;)
> > Despite the comment, what the above does is to change the alternates count to 8 (0 to 7), instead of
>> asking the core to use alternate 7. Ah, and, due to the audio hack, the above hack won't work anymore,
>> if the device has audio, as the core will decrement it to 6.
>>
>
> Right, this likely was a variant of the same hack. Combining the 2 is a bit overkill.
>
>> The only driver that seems to have a logic to work with different alts per-resolution is xirlink_cit.c,
>> but it is based on a try-and-error approach.
>
> Right, because its max framerate at 320x240 is something like 8 fps. So we try the highest alt setting
> first, as 8 fps is not exactly a framerate to write home about. Then if we fail there, we try the next
> which is something like 6 fps, etc.
>
>
>> Drivers shouldn't be allowed to change the number of alternates. Instead, the core should provide a standard
>> way to use an alternate number provided by the sub-driver, or, otherwise, fallback to an auto-select
>> mode. Something like:
>>
>>
>> if (gspca_dev->force_alt&& gspca_dev->force_alt< gspca_dev->nbalt)
>> gspca_dev->alt = gspca_dev->force_alt;
>> else {
>> /* Current way */
>> }
>>
>
> Drivers should provide a wmaxpacketsize range which they need / can deal with for a given resolution. This
> way we can fix your does not need highest alt setting at lower resolutions scenario (which is a very valid
> one), while still allowing fallback to a lower alt setting if the driver can deal with lower settings too,
> be it by lowering framerate or by increasing compression (decreasing the image quality).
It seems a good strategy.
Cheers,
Mauro.
--
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
