Re: [QCAP] Final patch

[Dimi Paun]

On Fri, 2005-05-20 at 00:42 +0200, Maarten Lankhorst wrote:
> m3h, v4l driver for vfwcapture.. please leave the ERR notice on 
> initialisation intact, thank you :)

I think you've overusing ERR(). ERR() should be called
to signal an internal error such as inconsistent state,
not something that the function is designed to handle.
> -
>        capBox->grab_buf = CoTaskMemAlloc(sizeof(struct video_mmap) * capBox->buffers);
> -
>        if(!capBox->grab_buf) {
> -
>            ERR("Out of memory?\n");
> -
>            munmap(capBox->pmap, capBox->gb_buffers.size);
> -
>            return E_OUTOFMEMORY;
> -
>        }

The function knows how to deal with this condition gracefully,
just get rid of the ERR().
> -
>     if (!capBox->grab_data)
> -
>     {
> -
>        ERR("%p: Out of memory?\n", capBox);
> -
>        return E_OUTOFMEMORY;
> -
>     }

Ditto. Also the code is more clear if you just do:
            if (!capBox->grab_data) return E_OUTOFMEMORY;
> - if (!capBox) {
> -
>  ERR("Out of memory\n");
> -
>  return E_OUTOFMEMORY;
> - }

Ditto.
> - if (stat (device, &st) == -1) {
> -
>  ERR("%s: %s\n", device, strerror(errno));
> -
>  CoTaskMemFree(capBox);
> -
>  return E_FAIL;
> - }

Why is this a ERR()?
> -
> - if (!S_ISCHR (st.st_mode)) {
> -
>  ERR("%s: Not a device\n", device);
> -
>  CoTaskMemFree(capBox);
> -
>  return E_FAIL;
> - }

And why this?
> -
> - capBox->fd = open(device, O_RDWR | O_NONBLOCK);
> - if (capBox->fd == -1) {
> -
>  ERR("%s: Failed to open: %s\n", device, strerror(errno));
> -
>  CoTaskMemFree(capBox);
> -
>  return E_FAIL;
> - }

This is a TRACE or WARN at most.
> -
>  ERR("Tinkerer detected? Thou shalt not define HAVE_V4L2\n");
> -
>  CoTaskMemFree(capBox);
> -
>  close(capBox->fd);
> -
>  return E_FAIL;

This is a FIXME.

And so on...

[QCAP] Final patch

[Andreas Mohr]

Hi,

On Thu, May 19, 2005 at 09:20:25PM -0400, Dimi Paun wrote:
> On Fri, 2005-05-20 at 00:42 +0200, Maarten Lankhorst wrote:
> > -
> > - capBox->fd = open(device, O_RDWR | O_NONBLOCK);
> > - if (capBox->fd == -1) {
> > -
> >  ERR("%s: Failed to open: %s\n", device, strerror(errno));
> > -
> >  CoTaskMemFree(capBox);
> > -
> >  return E_FAIL;
> > - }
>
> This is a TRACE or WARN at most.

This should not be a TRACE, since it is an anomaly.
It should perhaps even be a MESSAGE (since the user might want to
know about broken permissions). Or just WARN instead.
> > -
> >  ERR("Tinkerer detected? Thou shalt not define HAVE_V4L2\n");
> > -
> >  CoTaskMemFree(capBox);
> > -
> >  close(capBox->fd);
> > -
> >  return E_FAIL;
>
> This is a FIXME.

It's not. His comments indicate that he has no plans whatsoever
to add V4L2 support since it's entirely unneeded, thus there's nothing
to be "fixed".

Andreas

[QCAP] Final patch

[Dimi Paun]

On Fri, 2005-05-20 at 10:37 +0200, Andreas Mohr wrote:
> > > -
> > >  ERR("Tinkerer detected? Thou shalt not define HAVE_V4L2
>
> \n");
> > > -
> > >  CoTaskMemFree(capBox);
> > > -
> > >  close(capBox->fd);
> > > -
> > >  return E_FAIL;
> >
> > This is a FIXME.
>
> It's not. His comments indicate that he has no plans whatsoever
> to add V4L2 support since it's entirely unneeded, thus there's nothing
> to be "fixed".

Yes, it is. His comments indicate he doesn't care much to do it,
but if someone else wants to, he is more than welcomed.

Also, we should never generate an ERR() only based on a user setting.
This is a reasonable user configuration, it should be handled
gracefully. Maarten doesn't care for it, and that OK, but someone
else might.

[QCAP] Final patch

[Maarten Lankhorst]

Dimi Paun wrote:
> On Fri, 2005-05-20 at 00:42 +0200, Maarten Lankhorst wrote:
> > m3h, v4l driver for vfwcapture.. please leave the ERR notice on 
> > initialisation intact, thank you :)
>
> I think you've overusing ERR(). ERR() should be called
> to signal an internal error such as inconsistent state,
> not something that the function is designed to handle.

What do you suggest then ... fprintf(stderr?
> > -
> >        capBox->grab_buf = CoTaskMemAlloc(sizeof(struct video_mmap) * capBox->buffers);
> > -
> >        if(!capBox->grab_buf) {
> > -
> >            ERR("Out of memory?\n");
> > -
> >            munmap(capBox->pmap, capBox->gb_buffers.size);
> > -
> >            return E_OUTOFMEMORY;
> > -
> >        }
>
> The function knows how to deal with this condition gracefully,
> just get rid of the ERR().
> > -
> >     if (!capBox->grab_data)
> > -
> >     {
> > -
> >        ERR("%p: Out of memory?\n", capBox);
> > -
> >        return E_OUTOFMEMORY;
> > -
> >     }
>
> Ditto. Also the code is more clear if you just do:
>            if (!capBox->grab_data) return E_OUTOFMEMORY;
> > - if (!capBox) {
> > -
> >  ERR("Out of memory\n");
> > -
> >  return E_OUTOFMEMORY;
> > - }
>
> Ditto.

Ok,  I'll remove the out of memory errors.
> > - if (stat (device, &st) == -1) {
> > -
> >  ERR("%s: %s\n", device, strerror(errno));
> > -
> >  CoTaskMemFree(capBox);
> > -
> >  return E_FAIL;
> > - }
>
> Why is this a ERR()?
> > -
> > - if (!S_ISCHR (st.st_mode)) {
> > -
> >  ERR("%s: Not a device\n", device);
> > -
> >  CoTaskMemFree(capBox);
> > -
> >  return E_FAIL;
> > - }
>
> And why this?
> > -
> > - capBox->fd = open(device, O_RDWR | O_NONBLOCK);
> > - if (capBox->fd == -1) {
> > -
> >  ERR("%s: Failed to open: %s\n", device, strerror(errno));
> > -
> >  CoTaskMemFree(capBox);
> > -
> >  return E_FAIL;
> > - }
>
> This is a TRACE or WARN at most.

Basically, I made those an error because before getting to this point, 
the device has existed (It gets detected by devenum using 
avicap32->getdriverdescription, if something is wrong now, it will be 
much clearer why it happened, I don't believe most people that are going 
to use this webcam patch will be able to find out what happened 
themself, so I tried being as verbose as possible, especially since MSN 
will tell NOTHING, and most people wil be guessing in the dark. There's 
a small bug in our devenum, which causes it not to rescan for devices. 
The NATIVE version does, and since that gets installed by internet 
explorer, it's used. In that case, most of the errors I described 
shouldn't happen, because if it happens, something REALLY weird is going 
on... avicap runs the same stuff and works fine, but in qcap it fails... 
sounds like an 'inconsistent state' to me........ but who am I to tell O_O
> > -
> >  ERR("Tinkerer detected? Thou shalt not define HAVE_V4L2\n");
> > -
> >  CoTaskMemFree(capBox);
> > -
> >  close(capBox->fd);
> > -
> >  return E_FAIL;
>
> This is a FIXME.
>
> And so on...

Fine :/

[QCAP] Final patch

[Dimi Paun]

On Fri, 2005-05-20 at 12:26 +0200, Maarten Lankhorst wrote:
> What do you suggest then ... fprintf(stderr?

Absolutely not. :) But there are other options available:
TRACE/WARN/FIXME. There's also MESSAGE, but it should be
used *very* little. Also remember that most people will
not see MESSAGE()s anyway because they only go to console.
> > > - if (stat (device, &st) == -1) {
> > > -
> > >  ERR("%s: %s\n", device, strerror(errno));
> > > -
> > >  CoTaskMemFree(capBox);
> > > -
> > >  return E_FAIL;
> > > - }
> >
> > Why is this a ERR()?

This is more like a WARN or MESSAGE.
> > > -
> > > - if (!S_ISCHR (st.st_mode)) {
> > > -
> > >  ERR("%s: Not a device\n", device);
> > > -
> > >  CoTaskMemFree(capBox);
> > > -
> > >  return E_FAIL;
> > > - }

Same here.
> > > -
> > > - capBox->fd = open(device, O_RDWR | O_NONBLOCK);
> > > - if (capBox->fd == -1) {
> > > -
> > >  ERR("%s: Failed to open: %s\n", device, strerror(errno));
> > > -
> > >  CoTaskMemFree(capBox);
> > > -
> > >  return E_FAIL;
> > > - }

Ditto.

[QCAP] Final patch

[Maarten Lankhorst]

Dimi Paun wrote:
> On Fri, 2005-05-20 at 12:26 +0200, Maarten Lankhorst wrote:
> > What do you suggest then ... fprintf(stderr?
>
> Absolutely not. :) But there are other options available:
> TRACE/WARN/FIXME. There's also MESSAGE, but it should be
> used *very* little. Also remember that most people will
> not see MESSAGE()s anyway because they only go to console.

Ah well, it doesn't matter much, wrote it so II could keep video contact 
in the first place
> > > > - if (stat (device, &st) == -1) {
> > > > -
> > > >  ERR("%s: %s\n", device, strerror(errno));
> > > > -
> > > >  CoTaskMemFree(capBox);
> > > > -
> > > >  return E_FAIL;
> > > > - }
> > >
> > > Why is this a ERR()?
>
> This is more like a WARN or MESSAGE.

You are partially right, see below.
> > > > -
> > > > - if (!S_ISCHR (st.st_mode)) {
> > > > -
> > > >  ERR("%s: Not a device\n", device);
> > > > -
> > > >  CoTaskMemFree(capBox);
> > > > -
> > > >  return E_FAIL;
> > > > - }
>
> Same here.

No, /dev/video* should never be an ordinary file, and see below.
> > > > -
> > > > - capBox->fd = open(device, O_RDWR | O_NONBLOCK);
> > > > - if (capBox->fd == -1) {
> > > > -
> > > >  ERR("%s: Failed to open: %s\n", device, strerror(errno));
> > > > -
> > > >  CoTaskMemFree(capBox);
> > > > -
> > > >  return E_FAIL;
> > > > - }
>
> Ditto.

Like I said before, it shouldn't fail at this point, if it fails it 
should be at *capgetdriverdescription*, because it executes the same 
stuff, only if that function succeeds it will try to open the device 
here, the only time I can imagine it fails here, is when using the 
builtin devenum, which only rescans when the registry key 
currentuser/microsoft/activemovie doesn't exist any more, the video 
class should be rescanned every time the constructor gets called, so if 
a webcam is plugged in and/or modprobed it will be found inmediately, 
and when it is disconnected/rmmod'd it will disappear inmediately, 
instead of trying to initialise it anyway.  Since for msn messenger I 
recommended to install Internet Explorer using winetools, the windows 
version of devenum and quartz will be used, so that these ERR's should 
never occur, and if they occur, it will be at avicap32, not here...

To be honest, I expected more arguments about my reading thread, since 
that doesn't do much error checking, and doesn't call ifiltergraph::stop 
if an error occurs... :/

[QCAP] Final patch

[Alexandre Julliard]

Maarten Lankhorst  writes:
> > > > > -
> > > > > - if (!S_ISCHR (st.st_mode)) {
> > > > > -
> > > > >  ERR("%s: Not a device\n", device);
> > > > > -
> > > > >  CoTaskMemFree(capBox);
> > > > > -
> > > > >  return E_FAIL;
> > > > > - }
> >
> > Same here.
>
> No, /dev/video* should never be an ordinary file, and see below.

There is absolutely no reason to check for that, it just hurts
performance for no reason. Just open it and do the ioctl, if it's not
a device it will fail, and you have to handle that case properly
anyway.