Re: partial revert of "uvcvideo: set error_idx properly"

Re: partial revert of "uvcvideo: set error_idx properly"

[Hans de Goede]

<modified the CC list to be more appropriate>

Hi,

On 12/25/2012 05:56 AM, Mauro Carvalho Chehab wrote:

> The pwc driver can currently return -ENOENT at VIDIOC_S_FMT ioctl. This
> doesn't seem right. Instead, it should be getting the closest format to
> the requested one and return 0, passing the selected format back to
> userspace, just like the other drivers do. I'm c/c Hans de Goede for him
> to take a look on it.

I've been looking into this today, and the ENOENT gets returned by
pwc_set_video_mode and through that by:
1) Device init
2) VIDIOC_STREAMON
3) VIDIOC_S_PARM
4) VIDIOC_S_FMT

But only when the requested width + height + pixelformat is an
unsupported combination, and it being a supported combination
already gets enforced by a call to pwc_get_size in
pwc_vidioc_try_fmt, which also gets called from pwc_s_fmt_vid_cap
before it does anything else.

So the ENOENT can only happen on some internal driver error,
I'm open for suggestions for a better error code to return in
this case.

What I did notice is that pwc_vidioc_try_fmt returns EINVAL when
an unsupported pixelformat is requested. IIRC we agreed that the
correct behavior in this case is to instead just change the
pixelformat to a default format, so I'll write a patch fixing
this.

Regards,

Hans

Re: partial revert of "uvcvideo: set error_idx properly"

[Hans Verkuil]

On Fri January 25 2013 10:51:57 Hans de Goede wrote:
> <modified the CC list to be more appropriate>
> 
> Hi,
> 
> On 12/25/2012 05:56 AM, Mauro Carvalho Chehab wrote:
> 
> > The pwc driver can currently return -ENOENT at VIDIOC_S_FMT ioctl. This
> > doesn't seem right. Instead, it should be getting the closest format to
> > the requested one and return 0, passing the selected format back to
> > userspace, just like the other drivers do. I'm c/c Hans de Goede for him
> > to take a look on it.
> 
> I've been looking into this today, and the ENOENT gets returned by
> pwc_set_video_mode and through that by:
> 1) Device init
> 2) VIDIOC_STREAMON
> 3) VIDIOC_S_PARM
> 4) VIDIOC_S_FMT
> 
> But only when the requested width + height + pixelformat is an
> unsupported combination, and it being a supported combination
> already gets enforced by a call to pwc_get_size in
> pwc_vidioc_try_fmt, which also gets called from pwc_s_fmt_vid_cap
> before it does anything else.
> 
> So the ENOENT can only happen on some internal driver error,
> I'm open for suggestions for a better error code to return in
> this case.

Perhaps returning EINVAL but adding a WARN_ON would be a good compromise.

> What I did notice is that pwc_vidioc_try_fmt returns EINVAL when
> an unsupported pixelformat is requested. IIRC we agreed that the
> correct behavior in this case is to instead just change the
> pixelformat to a default format, so I'll write a patch fixing
> this.

There are issues with that idea in the case of TV capture cards, since
some important apps (tvtime and mythtv to a lesser extent) assume -EINVAL
in the case of unsupported pixelformats.

Webcam apps can't assume that since gspca never returned -EINVAL, so I
think it should be OK to fix this in pwc, but Mauro may disagree.

Regards,

	Hans

Re: partial revert of "uvcvideo: set error_idx properly"

[Hans de Goede]

Hi,

On 01/25/2013 11:40 AM, Hans Verkuil wrote:

<snip>

>> What I did notice is that pwc_vidioc_try_fmt returns EINVAL when
>> an unsupported pixelformat is requested. IIRC we agreed that the
>> correct behavior in this case is to instead just change the
>> pixelformat to a default format, so I'll write a patch fixing
>> this.
>
> There are issues with that idea in the case of TV capture cards, since
> some important apps (tvtime and mythtv to a lesser extent) assume -EINVAL
> in the case of unsupported pixelformats.

Oh, I thought we agreed on never returning EINVAL accept for on invalid
buffer types in Barcelona ?

Regards,

Hans

Re: partial revert of "uvcvideo: set error_idx properly"

[Hans Verkuil]

On Fri January 25 2013 14:40:45 Hans de Goede wrote:
> Hi,
> 
> On 01/25/2013 11:40 AM, Hans Verkuil wrote:
> 
> <snip>
> 
> >> What I did notice is that pwc_vidioc_try_fmt returns EINVAL when
> >> an unsupported pixelformat is requested. IIRC we agreed that the
> >> correct behavior in this case is to instead just change the
> >> pixelformat to a default format, so I'll write a patch fixing
> >> this.
> >
> > There are issues with that idea in the case of TV capture cards, since
> > some important apps (tvtime and mythtv to a lesser extent) assume -EINVAL
> > in the case of unsupported pixelformats.
> 
> Oh, I thought we agreed on never returning EINVAL accept for on invalid
> buffer types in Barcelona ?

We did, but then it was discovered that apps like tvtime *rely* on such an
error code.

All TV capture drivers that do stream I/O return EINVAL for unsupported
formats today. There are exceptions (cx18/ivtv/pvrusb2 (?)), but those
have a read() API only.

Very annoying...

Regards,

	Hans

Re: partial revert of "uvcvideo: set error_idx properly"

[Mauro Carvalho Chehab]

Em Fri, 25 Jan 2013 14:42:59 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Fri January 25 2013 14:40:45 Hans de Goede wrote:
> > Hi,
> > 
> > On 01/25/2013 11:40 AM, Hans Verkuil wrote:
> > 
> > <snip>
> > 
> > >> What I did notice is that pwc_vidioc_try_fmt returns EINVAL when
> > >> an unsupported pixelformat is requested. IIRC we agreed that the
> > >> correct behavior in this case is to instead just change the
> > >> pixelformat to a default format, so I'll write a patch fixing
> > >> this.
> > >
> > > There are issues with that idea in the case of TV capture cards, since
> > > some important apps (tvtime and mythtv to a lesser extent) assume -EINVAL
> > > in the case of unsupported pixelformats.
> > 
> > Oh, I thought we agreed on never returning EINVAL accept for on invalid
> > buffer types in Barcelona ?
> 
> We did, but then it was discovered that apps like tvtime *rely* on such an
> error code.

Yes. Basically, tvtime and MythTV rely on receiving an error when the format
is not supported (I think they accept if the driver changes resolution and
interleaving mode, though). Xawtv (and likely the other apps that use its
code as a reference) will accept if the driver would change the video format
to one that it is actually supported.

> 
> All TV capture drivers that do stream I/O return EINVAL for unsupported
> formats today. There are exceptions (cx18/ivtv/pvrusb2 (?)), but those
> have a read() API only.
> 
> Very annoying...
> 
> Regards,
> 
> 	Hans


-- 

Cheers,
Mauro

Re: partial revert of "uvcvideo: set error_idx properly"

[Mauro Carvalho Chehab]

Em Fri, 25 Jan 2013 11:40:13 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Fri January 25 2013 10:51:57 Hans de Goede wrote:
> > <modified the CC list to be more appropriate>
> > 
> > Hi,
> > 
> > On 12/25/2012 05:56 AM, Mauro Carvalho Chehab wrote:
> > 
> > > The pwc driver can currently return -ENOENT at VIDIOC_S_FMT ioctl. This
> > > doesn't seem right. Instead, it should be getting the closest format to
> > > the requested one and return 0, passing the selected format back to
> > > userspace, just like the other drivers do. I'm c/c Hans de Goede for him
> > > to take a look on it.
> > 
> > I've been looking into this today, and the ENOENT gets returned by
> > pwc_set_video_mode and through that by:
> > 1) Device init
> > 2) VIDIOC_STREAMON
> > 3) VIDIOC_S_PARM
> > 4) VIDIOC_S_FMT
> > 
> > But only when the requested width + height + pixelformat is an
> > unsupported combination, and it being a supported combination
> > already gets enforced by a call to pwc_get_size in
> > pwc_vidioc_try_fmt, which also gets called from pwc_s_fmt_vid_cap
> > before it does anything else.
> > 
> > So the ENOENT can only happen on some internal driver error,
> > I'm open for suggestions for a better error code to return in
> > this case.
> 
> Perhaps returning EINVAL but adding a WARN_ON would be a good compromise.
> 
> > What I did notice is that pwc_vidioc_try_fmt returns EINVAL when
> > an unsupported pixelformat is requested. IIRC we agreed that the
> > correct behavior in this case is to instead just change the
> > pixelformat to a default format, so I'll write a patch fixing
> > this.
> 
> There are issues with that idea in the case of TV capture cards, since
> some important apps (tvtime and mythtv to a lesser extent) assume -EINVAL
> in the case of unsupported pixelformats.
> 
> Webcam apps can't assume that since gspca never returned -EINVAL, so I
> think it should be OK to fix this in pwc, but Mauro may disagree.

It is known that both MythTV and tvtime have issues.

Well, I don't think that MythTV has webcam support. So, it will likely
fail with pwc anyway, as it doesn't have a tuner. So, webcam drivers don't
need to care with breaking anything on it.

Tvtime can work with webcams, if they provide a resolution that it is
compatible with it and if it supports UVYV or YUYV. This is not the case
of pwc, that seems to support only pwc proprietary formats and yuv420.

So, neither tvtime or MythTV currently works with pwc cameras.

However, the issue is a little more complex, as we don't really know if
there aren't any other applications that use a code similar to tvtime
or MythYV.

Regards,
Mauro

Re: partial revert of "uvcvideo: set error_idx properly"

[Hans de Goede]

Hi,

On 01/27/2013 03:06 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 25 Jan 2013 11:40:13 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> On Fri January 25 2013 10:51:57 Hans de Goede wrote:
>>> <modified the CC list to be more appropriate>
>>>
>>> Hi,
>>>
>>> On 12/25/2012 05:56 AM, Mauro Carvalho Chehab wrote:
>>>
>>>> The pwc driver can currently return -ENOENT at VIDIOC_S_FMT ioctl. This
>>>> doesn't seem right. Instead, it should be getting the closest format to
>>>> the requested one and return 0, passing the selected format back to
>>>> userspace, just like the other drivers do. I'm c/c Hans de Goede for him
>>>> to take a look on it.
>>>
>>> I've been looking into this today, and the ENOENT gets returned by
>>> pwc_set_video_mode and through that by:
>>> 1) Device init
>>> 2) VIDIOC_STREAMON
>>> 3) VIDIOC_S_PARM
>>> 4) VIDIOC_S_FMT
>>>
>>> But only when the requested width + height + pixelformat is an
>>> unsupported combination, and it being a supported combination
>>> already gets enforced by a call to pwc_get_size in
>>> pwc_vidioc_try_fmt, which also gets called from pwc_s_fmt_vid_cap
>>> before it does anything else.
>>>
>>> So the ENOENT can only happen on some internal driver error,
>>> I'm open for suggestions for a better error code to return in
>>> this case.
>>
>> Perhaps returning EINVAL but adding a WARN_ON would be a good compromise.
>>
>>> What I did notice is that pwc_vidioc_try_fmt returns EINVAL when
>>> an unsupported pixelformat is requested. IIRC we agreed that the
>>> correct behavior in this case is to instead just change the
>>> pixelformat to a default format, so I'll write a patch fixing
>>> this.
>>
>> There are issues with that idea in the case of TV capture cards, since
>> some important apps (tvtime and mythtv to a lesser extent) assume -EINVAL
>> in the case of unsupported pixelformats.
>>
>> Webcam apps can't assume that since gspca never returned -EINVAL, so I
>> think it should be OK to fix this in pwc, but Mauro may disagree.
>
> It is known that both MythTV and tvtime have issues.
>
> Well, I don't think that MythTV has webcam support. So, it will likely
> fail with pwc anyway, as it doesn't have a tuner. So, webcam drivers don't
> need to care with breaking anything on it.
>
> Tvtime can work with webcams, if they provide a resolution that it is
> compatible with it and if it supports UVYV or YUYV. This is not the case
> of pwc, that seems to support only pwc proprietary formats and yuv420.
>
> So, neither tvtime or MythTV currently works with pwc cameras.
>
> However, the issue is a little more complex, as we don't really know if
> there aren't any other applications that use a code similar to tvtime
> or MythYV.

I understand. A patch to change pwc to the behavior discussed in Barcelona
(so changing the format to a supported one rather then returning -EINVAL),
is part of my last pull-req, I'll leave it up to you whether you will take
it or not :)

If you don't take it I'll drop it from my tree, so that it does not
show up again in my next pull-req.

Regards,

Hans



>
> Regards,
> Mauro
>