Re: [git:xawtv3/master] xawtv: reenable its usage with webcam's

Re: [git:xawtv3/master] xawtv: reenable its usage with webcam's

[Hans de Goede]

Hi,

On 06/28/2011 07:32 PM, Mauro Carvalho Chehab wrote:
> This is an automatic generated email to let you know that the following patch were queued at the
> http://git.linuxtv.org/xawtv3.git tree:
>
> Subject: xawtv: reenable its usage with webcam's
> Author:  Mauro Carvalho Chehab<mchehab@redhat.com>
> Date:    Tue Jun 28 14:22:55 2011 -0300
>
> git changeset c28978f3693bc0f40607d0b3e589774b9452608d was requiring that
> tuner would be available, in order to allow it to run. Relax the restriction,
> in order to allow using xawtv to test webcams, restoring the previous
> behavior.
>
> Signed-off-by: Mauro Carvalho Chehab<mchehab@redhat.com>
>
>   libng/grab-ng.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> ---
>
> http://git.linuxtv.org/xawtv3.git?a=commitdiff;h=2238f79d9fb2801a3acd114242b437686fa2f0c8
>
> diff --git a/libng/grab-ng.c b/libng/grab-ng.c
> index f5203cc..94f31e8 100644
> --- a/libng/grab-ng.c
> +++ b/libng/grab-ng.c
> @@ -563,9 +563,9 @@ static void *ng_vid_open_auto(struct ng_vid_driver *drv, char *devpath)
>   	    continue;
>   	}
>
> -	/* Check caps return this device if it can capture and has a tuner */
> +	/* Check caps return this device if it can capture */
>   	caps = drv->capabilities(handle);
> -	if ((caps&  CAN_CAPTURE)&&  (caps&  CAN_TUNE))
> +	if (caps&  CAN_CAPTURE)
>   	    break;
>
>   	drv->close(handle);
>

Hmm, this changes the behavior from what I intended, the idea was to select the
first *tv-card*, without checking for a tuner, there is little value in the auto
device feature. Granted it will still skip v4l2 output only devices but those are
very rare.

Note that only the xawtv binary is using a device value of "auto" by default,
the webcam tool still defaults to /dev/video0

Given that xawtv is specifically meant for tv-cards (unlike the webcam tool)
failing if it cannot find a tv-card and no device is explicitly specified seems
reasonable.

Alternatively we could make the desired caps a param too ng_vid_open_auto
and first try with (CAN_CAPTURE | CAN_TUNE) and then retry with only
CAN_CAPTURE.

The above patch definitely is not what I had in mind. My system has a
bt878 tv card, and a varying number of webcams connected, thus constantly
changing the /dev/video# for the tv-card. The intent of my "auto" device
patches was to make xawtv automatically pick the tvcard.

Regards,

Hans

p.s.

I intented to mail you about my get_media_devices fixes as well as my
auto device patches, and suggest that we do a new release soon. But first
we need to sort out the auto device thingie. If you could fix it to
first look for cards with a tuner and if none is available fall back
to just looking for capture capable cards that would be great, I'm a
bit busy atm I'm afraid.

Re: [git:xawtv3/master] xawtv: reenable its usage with webcam's

[Mauro Carvalho Chehab]

Em 29-06-2011 08:01, Hans de Goede escreveu:
> Hi,
> 
> On 06/28/2011 07:32 PM, Mauro Carvalho Chehab wrote:
>> This is an automatic generated email to let you know that the following patch were queued at the
>> http://git.linuxtv.org/xawtv3.git tree:
>>
>> Subject: xawtv: reenable its usage with webcam's
>> Author:  Mauro Carvalho Chehab<mchehab@redhat.com>
>> Date:    Tue Jun 28 14:22:55 2011 -0300
>>
>> git changeset c28978f3693bc0f40607d0b3e589774b9452608d was requiring that
>> tuner would be available, in order to allow it to run. Relax the restriction,
>> in order to allow using xawtv to test webcams, restoring the previous
>> behavior.
>>
>> Signed-off-by: Mauro Carvalho Chehab<mchehab@redhat.com>
>>
>>   libng/grab-ng.c |    4 ++--
>>   1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> ---
>>
>> http://git.linuxtv.org/xawtv3.git?a=commitdiff;h=2238f79d9fb2801a3acd114242b437686fa2f0c8
>>
>> diff --git a/libng/grab-ng.c b/libng/grab-ng.c
>> index f5203cc..94f31e8 100644
>> --- a/libng/grab-ng.c
>> +++ b/libng/grab-ng.c
>> @@ -563,9 +563,9 @@ static void *ng_vid_open_auto(struct ng_vid_driver *drv, char *devpath)
>>           continue;
>>       }
>>
>> -    /* Check caps return this device if it can capture and has a tuner */
>> +    /* Check caps return this device if it can capture */
>>       caps = drv->capabilities(handle);
>> -    if ((caps&  CAN_CAPTURE)&&  (caps&  CAN_TUNE))
>> +    if (caps&  CAN_CAPTURE)
>>           break;
>>
>>       drv->close(handle);
>>
> 
> Hmm, this changes the behavior from what I intended, the idea was to select the
> first *tv-card*, without checking for a tuner, there is little value in the auto
> device feature. Granted it will still skip v4l2 output only devices but those are
> very rare.

Your patch broke support for vivi and for video grabber devices. Those devices don't
have a tuner.

> Note that only the xawtv binary is using a device value of "auto" by default,
> the webcam tool still defaults to /dev/video0

It is not "by default", as there's was way to override the CAN_TUNE check logic.
calling xawtv without my changes with vivi or a webcam would simply return as if
there's no supported V4L device.

> Given that xawtv is specifically meant for tv-cards (unlike the webcam tool)
> failing if it cannot find a tv-card and no device is explicitly specified seems
> reasonable.

Well, xawtv ever worked also with webcams, and with the libv4l, it is now
working even better. I don't see any reason why removing such support for it.

Btw, with the changes I've made, the TV-specific controls are now removed if
the device is a grabber or a webcam. There's currently just one detail to be
fixed: the window title will be changed to "???" on those devices. This is an
old bug, as changing from Television to S-Video or Composite, on a device that
has both tuner and grabber capabilities, it will still keep the channel name
there. It probably makes sense to print there the input name instead, if the
input is not Television.

> Alternatively we could make the desired caps a param too ng_vid_open_auto
> and first try with (CAN_CAPTURE | CAN_TUNE) and then retry with only
> CAN_CAPTURE.

A logic like that would be better, although, IMO, we should print a message
saying that we're not using video0. Also, if we're willing to do such logic,
it makes sense to implement the auto mode also for "scantv".

> The above patch definitely is not what I had in mind. My system has a
> bt878 tv card, and a varying number of webcams connected, thus constantly
> changing the /dev/video# for the tv-card. The intent of my "auto" device
> patches was to make xawtv automatically pick the tvcard.

Well, a varying device for /dev/video is something that we need to fix at udev.
There are some ways to create persistent rules for that.

In a matter of fact, IMO, we should change the V4L2 device nodes reported via
udev, to be more intuitive, e. g. instead of creating /dev/video for everything,
create /dev/webcam? /dev/grabber? /dev/analog_tv? device nodes, while creating
a symlink to /dev/video, in order to not break existing applications that have
it hardcoded.

Cheers,
Mauro

> I intented to mail you about my get_media_devices fixes as well as my
> auto device patches, and suggest that we do a new release soon. 

Yes, I think we should make a release for it soon. There are enough features
added on xawtv that justifies doing a new release.

> But first
> we need to sort out the auto device thingie. If you could fix it to
> first look for cards with a tuner and if none is available fall back
> to just looking for capture capable cards that would be great, I'm a
> bit busy atm I'm afraid.

I'll seek for some time. I'm also busy atm, trying to check what applications
would break if we change the error value for unimplemented calls to ENOTTY
(that was basically the reason for me to fix xawtv, as grabber devices and webcams
have less ioctl's implemented).

Cheers,
Mauro.

Re: [git:xawtv3/master] xawtv: reenable its usage with webcam's

[Hans de Goede]

Hi,

On 06/29/2011 02:01 PM, Mauro Carvalho Chehab wrote:
> Em 29-06-2011 08:01, Hans de Goede escreveu:
>> Hi,
>>
>> On 06/28/2011 07:32 PM, Mauro Carvalho Chehab wrote:
>>> This is an automatic generated email to let you know that the following patch were queued at the
>>> http://git.linuxtv.org/xawtv3.git tree:
>>>
>>> Subject: xawtv: reenable its usage with webcam's
>>> Author:  Mauro Carvalho Chehab<mchehab@redhat.com>
>>> Date:    Tue Jun 28 14:22:55 2011 -0300
>>>
>>> git changeset c28978f3693bc0f40607d0b3e589774b9452608d was requiring that
>>> tuner would be available, in order to allow it to run. Relax the restriction,
>>> in order to allow using xawtv to test webcams, restoring the previous
>>> behavior.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab<mchehab@redhat.com>
>>>
>>>    libng/grab-ng.c |    4 ++--
>>>    1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> ---
>>>
>>> http://git.linuxtv.org/xawtv3.git?a=commitdiff;h=2238f79d9fb2801a3acd114242b437686fa2f0c8
>>>
>>> diff --git a/libng/grab-ng.c b/libng/grab-ng.c
>>> index f5203cc..94f31e8 100644
>>> --- a/libng/grab-ng.c
>>> +++ b/libng/grab-ng.c
>>> @@ -563,9 +563,9 @@ static void *ng_vid_open_auto(struct ng_vid_driver *drv, char *devpath)
>>>            continue;
>>>        }
>>>
>>> -    /* Check caps return this device if it can capture and has a tuner */
>>> +    /* Check caps return this device if it can capture */
>>>        caps = drv->capabilities(handle);
>>> -    if ((caps&   CAN_CAPTURE)&&   (caps&   CAN_TUNE))
>>> +    if (caps&   CAN_CAPTURE)
>>>            break;
>>>
>>>        drv->close(handle);
>>>
>>
>> Hmm, this changes the behavior from what I intended, the idea was to select the
>> first *tv-card*, without checking for a tuner, there is little value in the auto
>> device feature. Granted it will still skip v4l2 output only devices but those are
>> very rare.
>
> Your patch broke support for vivi and for video grabber devices. Those devices don't
> have a tuner.
>

I did no such thing as "break support". The new xawtv will still work fine with
them with an explicit "-c /dev/video#" argument.

>> Note that only the xawtv binary is using a device value of "auto" by default,
>> the webcam tool still defaults to /dev/video0
>
> It is not "by default", as there's was way to override the CAN_TUNE check logic.
> calling xawtv without my changes with vivi or a webcam would simply return as if
> there's no supported V4L device.
>

One can override the CAN_TUNE logic quite simply, revert your patch and do:
xawtv -c /dev/video#

On a device without a tuner, and it will work fine, if you don't specify the
device and don't have a device with a tuner you will get the following error:
vid-open: could not find a suitable videodev

Which sounds like a sane error for a tv viewing app when it cannot find
a tv-card. Granted, maybe the error should be changed to:
vid-open: could not find a suitable tv-card

To make things even more clear.

This is only the default behavior, and in *no way* have I *broken* use
with a webcam, simply doing:
xawtv -c /dev/video#

Will work fine with any device which worked before as can been clearly
seen from the new code:

+#ifdef __linux__
+    if (!strcmp(*device, "auto")) {
+        char devpath[PATH_MAX];
+	*handle = ng_vid_open_auto(drv, devpath);
+	if (*handle == NULL) {
+	    fprintf(stderr, "vid-open: could not find a suitable videodev\n");
+	    return NULL;
+	}
+	*device = strdup(devpath);
+    } else
+#endif
+    {
+	if (ng_debug)
+	    fprintf(stderr,"vid-open: trying: %s... \n", drv->name);
+	if (!(*handle = (drv->open)(*device))) {
+	    fprintf(stderr,"vid-open: failed: %s\n", drv->name);
+	    return NULL;
+	}
+	if (ng_debug)
+	    fprintf(stderr,"vid-open: ok: %s\n", drv->name);
      }


Specify anything other then auto and the code you've changed simply does
not get called.

>> Given that xawtv is specifically meant for tv-cards (unlike the webcam tool)
>> failing if it cannot find a tv-card and no device is explicitly specified seems
>> reasonable.
>
> Well, xawtv ever worked also with webcams, and with the libv4l, it is now
> working even better. I don't see any reason why removing such support for it.
>

Again I did not remove support for that, you should have studied my patch /
the problem a bit better before making such claims.

> Btw, with the changes I've made, the TV-specific controls are now removed if
> the device is a grabber or a webcam.

Nice!

> There's currently just one detail to be
> fixed: the window title will be changed to "???" on those devices. This is an
> old bug, as changing from Television to S-Video or Composite, on a device that
> has both tuner and grabber capabilities, it will still keep the channel name
> there. It probably makes sense to print there the input name instead, if the
> input is not Television.

Agreed.

>> Alternatively we could make the desired caps a param too ng_vid_open_auto
>> and first try with (CAN_CAPTURE | CAN_TUNE) and then retry with only
>> CAN_CAPTURE.
>
> A logic like that would be better, although, IMO, we should print a message
> saying that we're not using video0. Also, if we're willing to do such logic,
> it makes sense to implement the auto mode also for "scantv".
>

Why should we print a message that we're not using video0? In the modern
dynamic device discovery world, the # in /dev/video# is as good as meaningless.

>> The above patch definitely is not what I had in mind. My system has a
>> bt878 tv card, and a varying number of webcams connected, thus constantly
>> changing the /dev/video# for the tv-card. The intent of my "auto" device
>> patches was to make xawtv automatically pick the tvcard.
>
> Well, a varying device for /dev/video is something that we need to fix at udev.
> There are some ways to create persistent rules for that.
>
> In a matter of fact, IMO, we should change the V4L2 device nodes reported via
> udev, to be more intuitive, e. g. instead of creating /dev/video for everything,
> create /dev/webcam? /dev/grabber? /dev/analog_tv? device nodes, while creating
> a symlink to /dev/video, in order to not break existing applications that have
> it hardcoded.

That sounds like a good idea, but first needs to be written and then make its
way into distributions, I wanted something which would improve the user experience
right now, rather then in 2 years.

>> I intented to mail you about my get_media_devices fixes as well as my
>> auto device patches, and suggest that we do a new release soon.
>
> Yes, I think we should make a release for it soon. There are enough features
> added on xawtv that justifies doing a new release.
>

Agreed,

Regards,

Hans

Re: [git:xawtv3/master] xawtv: reenable its usage with webcam's

[Mauro Carvalho Chehab]

Em 29-06-2011 09:24, Hans de Goede escreveu:
> Hi,
> 
> On 06/29/2011 02:01 PM, Mauro Carvalho Chehab wrote:
>> Em 29-06-2011 08:01, Hans de Goede escreveu:
>>> Hmm, this changes the behavior from what I intended, the idea was to select the
>>> first *tv-card*, without checking for a tuner, there is little value in the auto
>>> device feature. Granted it will still skip v4l2 output only devices but those are
>>> very rare.
>>
>> Your patch broke support for vivi and for video grabber devices. Those devices don't
>> have a tuner.
>>
> 
> I did no such thing as "break support". The new xawtv will still work fine with
> them with an explicit "-c /dev/video#" argument.

Ok. I got confused by your patch, and the error message didn't help.

Anyway, it is fixed. I also made scantv to force for a TV device at auto mode, as it
doesn't sense to scan for TV channels on devices without tuner.
> Granted, maybe the error should be changed to:
> vid-open: could not find a suitable tv-card
> 
> To make things even more clear.

Yes, I've changed it to a message similar to that.

>> There's currently just one detail to be
>> fixed: the window title will be changed to "???" on those devices. This is an
>> old bug, as changing from Television to S-Video or Composite, on a device that
>> has both tuner and grabber capabilities, it will still keep the channel name
>> there. It probably makes sense to print there the input name instead, if the
>> input is not Television.
> 
> Agreed.

Fixed.

>>> The above patch definitely is not what I had in mind. My system has a
>>> bt878 tv card, and a varying number of webcams connected, thus constantly
>>> changing the /dev/video# for the tv-card. The intent of my "auto" device
>>> patches was to make xawtv automatically pick the tvcard.
>>
>> Well, a varying device for /dev/video is something that we need to fix at udev.
>> There are some ways to create persistent rules for that.
>>
>> In a matter of fact, IMO, we should change the V4L2 device nodes reported via
>> udev, to be more intuitive, e. g. instead of creating /dev/video for everything,
>> create /dev/webcam? /dev/grabber? /dev/analog_tv? device nodes, while creating
>> a symlink to /dev/video, in order to not break existing applications that have
>> it hardcoded.
> 
> That sounds like a good idea, but first needs to be written and then make its
> way into distributions, I wanted something which would improve the user experience
> right now, rather then in 2 years.

I see. Yet, I think we should or ping someone from udev team or add it on our TODO
lists. The needed information is probably already there (and there's an udev name
retrieving the information from querycap). I suspect that all it needs to be 
persistent is to add some udev rules.

>>> I intented to mail you about my get_media_devices fixes as well as my
>>> auto device patches, and suggest that we do a new release soon.
>>
>> Yes, I think we should make a release for it soon. There are enough features
>> added on xawtv that justifies doing a new release.
>>
> 
> Agreed,

>From my side, I don't intend to touch on xawtv any time soon. So, maybe we can wait
for a couple days and release version 1.101.

Cheers,
Mauro

Re: [git:xawtv3/master] xawtv: reenable its usage with webcam's

[Hans de Goede]

Hi,

On 06/29/2011 09:27 PM, Mauro Carvalho Chehab wrote:

<snip>

>
> Anyway, it is fixed. I also made scantv to force for a TV device at auto mode, as it
> doesn't sense to scan for TV channels on devices without tuner.

Thanks for fixing this, 2 remarks wrt the auto patch for
scantv:

1) This bit should be #ifdef __linux__ since we only support
auto* on linux because of the sysfs dep:

@@ -149,6 +149,9 @@ main(int argc, char **argv)

      /* parse options */
      ng_init();
+    /* Autodetect devices */
+    ng_dev.video = "auto_tv";
+
      for (;;) {
  	if (-1 == (c = getopt(argc, argv, "hsadi:n:f:o:c:C:D:")))
  	    break;

2) The added return NULL in case no device can be found lacks
printing an error message:

@@ -568,6 +569,8 @@ static void *ng_vid_open_auto(struct ng_vid_driver *drv, char *devpath)

      /* Step 2: try grabber devices and webcams */
      if (!handle) {
+	if (!allow_grabber)
+	    return NULL;
  	device = NULL;
  	while (1) {
  	    device = get_associated_device(md, device, MEDIA_V4L_VIDEO, NULL, NONE);

I propose changing the return NULL, with a goto to the error print further down.

>  From my side, I don't intend to touch on xawtv any time soon. So, maybe we can wait
> for a couple days and release version 1.101.

Assuming the 2 things mentioned above get fixed that sounds like a good plan to me.

Regards,

Hans

Re: [git:xawtv3/master] xawtv: reenable its usage with webcam's

[Mauro Carvalho Chehab]

Em 30-06-2011 07:55, Hans de Goede escreveu:
> Hi,
> 
> On 06/29/2011 09:27 PM, Mauro Carvalho Chehab wrote:
> 
> <snip>
> 
>>
>> Anyway, it is fixed. I also made scantv to force for a TV device at auto mode, as it
>> doesn't sense to scan for TV channels on devices without tuner.
> 
> Thanks for fixing this, 2 remarks wrt the auto patch for
> scantv:
> 
> 1) This bit should be #ifdef __linux__ since we only support
> auto* on linux because of the sysfs dep:
> 
> @@ -149,6 +149,9 @@ main(int argc, char **argv)
> 
>      /* parse options */
>      ng_init();
> +    /* Autodetect devices */
> +    ng_dev.video = "auto_tv";
> +
>      for (;;) {
>      if (-1 == (c = getopt(argc, argv, "hsadi:n:f:o:c:C:D:")))
>          break;
> 

True, but instead of adding it on every place, the better would be to replace auto/auto_tv
at the library, instead of adding the test at each place we change to auto mode.

BTW, does the bsd driver actually work? I remember they asked us to release the videodev2.h
file as dual licensing, in order to allow BSD to use V4L2 API also, a few years ago. If they
actually changed, all those bsd compat stuff is wrong.

> 2) The added return NULL in case no device can be found lacks
> printing an error message:
> 
> @@ -568,6 +569,8 @@ static void *ng_vid_open_auto(struct ng_vid_driver *drv, char *devpath)
> 
>      /* Step 2: try grabber devices and webcams */
>      if (!handle) {
> +    if (!allow_grabber)
> +        return NULL;
>      device = NULL;
>      while (1) {
>          device = get_associated_device(md, device, MEDIA_V4L_VIDEO, NULL, NONE);
> 
> I propose changing the return NULL, with a goto to the error print further down.

Yes, that sounds better to me.
> 
>>  From my side, I don't intend to touch on xawtv any time soon. So, maybe we can wait
>> for a couple days and release version 1.101.
> 
> Assuming the 2 things mentioned above get fixed that sounds like a good plan to me.
> 
> Regards,
> 
> Hans

Re: [git:xawtv3/master] xawtv: reenable its usage with webcam's

[Mauro Carvalho Chehab]

Em 30-06-2011 09:35, Mauro Carvalho Chehab escreveu:
> Em 30-06-2011 07:55, Hans de Goede escreveu:

>> 1) This bit should be #ifdef __linux__ since we only support
>> auto* on linux because of the sysfs dep:
> 
> True, but instead of adding it on every place, the better would be to replace auto/auto_tv
> at the library, instead of adding the test at each place we change to auto mode.

I fixed it using this approach.

>> 2) The added return NULL in case no device can be found lacks
>> printing an error message:

>> I propose changing the return NULL, with a goto to the error print further down.
> 
> Yes, that sounds better to me.

The error message didn't look good, so I added an specific message for it.

Yet, IMO, we're being too verbose:

$ scantv 
vid-open-auto: failed to open an analog TV device at /dev/video0
vid-open: could not find a suitable videodev
no analog TV device available

Cheers,
Mauro

Re: [git:xawtv3/master] xawtv: reenable its usage with webcam's

[Mauro Carvalho Chehab]

Em 30-06-2011 10:20, Mauro Carvalho Chehab escreveu:
> Em 30-06-2011 09:35, Mauro Carvalho Chehab escreveu:
>> Em 30-06-2011 07:55, Hans de Goede escreveu:
> 
>>> 1) This bit should be #ifdef __linux__ since we only support
>>> auto* on linux because of the sysfs dep:
>>
>> True, but instead of adding it on every place, the better would be to replace auto/auto_tv
>> at the library, instead of adding the test at each place we change to auto mode.
> 
> I fixed it using this approach.
> 
>>> 2) The added return NULL in case no device can be found lacks
>>> printing an error message:
> 
>>> I propose changing the return NULL, with a goto to the error print further down.
>>
>> Yes, that sounds better to me.
> 
> The error message didn't look good, so I added an specific message for it.
> 
> Yet, IMO, we're being too verbose:
> 
> $ scantv 
> vid-open-auto: failed to open an analog TV device at /dev/video0
> vid-open: could not find a suitable videodev
> no analog TV device available

Ah, calling it without any media driver is also verbose and wrong:

$ xawtv
This is xawtv-, running on Linux/x86_64 (2.6.32-131.0.15.el6.x86_64)
vid-open-auto: failed to open a capture device at \x03
vid-open: could not find a suitable videodev
no video grabber device available

$ scantv
vid-open-auto: failed to open an analog TV device at �7
                                                       \x01
vid-open: could not find a suitable videodev
no analog TV device available

Mauro