Created attachment 135124 [details] [review]
Patch for NetBSD video support and making HAL optional

Sorry, Jared's email is jmcneill@NetBSD.org, not jared

I'm a bit curious about this argument.
HAL is going to be deprecated sooner or later in the new DeviceKit world.
Devices who will need some helper userspace stuff like batteries and disks at them moment will have their DeviceKit-thing. The other devices like video4linux and other stuff (phones, modems, music players) will just use plain libudev.

So either gstreamer will provide some kind of device probing thing or we will move to libudev sooner or later.
Does netbsd support udev? I'd have said it didn't support video4*linux* either but apparently it does, so...

Anyway, I'm usually a fan of "everything upstream" idea, but this is a bit of a corner case. Do we want to explicitly support non linux platforms?
I mean accepting a patch is easy and doesn't cost anything but it adds code that needs to be maintained. I don't think neither of us uses or is going to ever use netbsd so that piece of code will stay unmaintained. So isn't this a case where maintaining a downstream patch would be better?

I'm not clear on what part(s) of the patch you're objecting too.

I see it as having two parts: optional hal support; and video4linux adaptions for NetBSD. I'd definitely like to see the v4l part accepted. The hal part was IMHO mostly because the NetBSD hal port wasn't quite advanced enough yet at the time the patch was done.

I'm a bit astounded about the part of your comment where you talk of only supporting Linux. The gnome package in pkgsrc are quite up-to-date, and many don't need any patches.

Jared just mentioned to me that "gstreamer already provides a device probing mechanism; that's why gstreamer-properties can pick up webcams"

(In reply to comment #4)
> I'm not clear on what part(s) of the patch you're objecting too.

I wasn't reviewing the patch. I was expressing a more general concern, and most of all asking if netbsd supports udev/libudev (at least that was the part directed to you, the rest was for the other Cheese developers).

> I see it as having two parts: optional hal support; and video4linux adaptions
> for NetBSD. I'd definitely like to see the v4l part accepted. The hal part was
> IMHO mostly because the NetBSD hal port wasn't quite advanced enough yet at the
> time the patch was done.

As I said I wasn't reviewing the patch :) 

> I'm a bit astounded about the part of your comment where you talk of only
> supporting Linux. The gnome package in pkgsrc are quite up-to-date, and many
> don't need any patches.

Sure, most of gnome stuff just uses glib/gtk+ stack that it's intrinsecally multiplatform. But I suppose that it's more difficult with software that depend highly on hardware interaction (e.g. networkmanager, gnome-power-manager, gnome-bluetooth, totem dvb support, ...). Do they work out of the box without any patch?
I doubt any of those explicitly "support" non Linux stuff, it just works for most of them with no extra effort.

(In reply to comment #5)
> Jared just mentioned to me that "gstreamer already provides a device probing
> mechanism; that's why gstreamer-properties can pick up webcams"
> 

See http://sourceforge.net/mailarchive/forum.php?thread_name=8ceb98f20905090204m56b54e9ax4183f2092a17a697%40mail.gmail.com&forum_name=gstreamer-devel

(In reply to comment #7)
> See
> http://sourceforge.net/mailarchive/forum.php?thread_name=8ceb98f20905090204m56b54e9ax4183f2092a17a697%40mail.gmail.com&forum_name=gstreamer-devel
> 

Or if you don't want to go through the whole mail: gstreamer doesn't support device detection, it just blindly scans /dev/ dir looking for video* stuff.
It doesn't even check which api they support nor device capabilities.

Thanks for the information.
To answer your question: AFAIK there is no udev/libudev in/for NetBSD yet.
Let me know if you need anything else.

FYI, I'm trying to get some better detection code in gstreamer. It will use libudev if enabled at compile time and fallback to plain /dev/ dir scanning if not.
If it gets included I will remove HAL based detection code from our codebase and will use a gstreamer only approach. So this patch won't be needed anymore.
See http://bugzilla.gnome.org/show_bug.cgi?id=583640

That sounds interesting.

Please note that there is a second part of code in this patch, enabling support for the NetBSD video API -- basically, all the stuff that doesn't match [hH][aA][lL] in the patch.

would it be possible for you to split up the patch into those two parts? that would be awesome, as i would like to add at least the netbsd specific code.

(In reply to comment #11)
> That sounds interesting.
> 
> Please note that there is a second part of code in this patch, enabling support
> for the NetBSD video API -- basically, all the stuff that doesn't match
> [hH][aA][lL] in the patch.

I'm sorry, I wasn't clear enough. If that patch will go in gstreamer I'll remove the whole device detection code from cheese. This includes also the ioctls to detect camera cababilities since that will be done in gstreamer.
I'm not sure if v4l/v4l2src aready includes support for netbsd, they don't include videoio.h as far as I can tell, but probably you already have patches for it since you're able to use it.

(In reply to comment #13)
> This includes also the
> ioctls to detect camera cababilities since that will be done in gstreamer.

So there won't be any need to include low level v4l headers in cheese and no need for the second part of the patch too.

Created attachment 135459 [details] [review]
Patch only for the videoio part (no HAL changes)

@daniel: as requested, compile-tested patch of only the videoio changes.

Filippo: gstreamer support for the v4l2 plugin are just header changes; pkgsrc already contains patches for that.

I'll leave it up to you to decide if you want to integrate this patch before that gstreamer change or not, the patches have been in pkgsrc long enough anyway :)

Sure, I guess we will get a reply from gst guys before 2.28 so we'll either include your patch(es) or completely drop that code, time will tell.

filippo, do you want to have this patch in 2.28?

Please only use NEEDINFO against reporters (http://bugzilla.gnome.org/page.cgi?id=bug-status.html#status).

I have a patch almost ready, it needs latest v4lsrc from gstreamer-plugins-base and v4l2src from the yet to be released gstreamer-plugins-good (planned for next week). It completely drops hal and uses gstreamer propertyprobe interface only.
It also takes advantage of a fix that recently entered v4l plugins (open the device in READY state instead of PLAYING) that should speed up a little bit cheese startup.
I'll let you know when it will be ready, you know, I don't have so much time for hacking... What's the timeframe I have to get it included in 2.28?

Do you have anything against a hard *runtime* dependency on latest gstreamer releases? (it's mandatory to get that fix included, I could maintain two code paths but I'm not sure it's worth).

this is a bug fix, so you have time until september 7th.

however, we have to use the gstreamer version, gnome is using. i currently do not have a clue, what this version is...

Basically GNOME uses the latest. :-P
GNOME 2.27.5 shipped gstreamer 0.10.23, see http://ftp.gnome.org/pub/GNOME/desktop/2.27/2.27.5/sources/ .
As gstreamer 0.10.24 is out now I expect GNOME 2.27.90 to ship gstreamer 0.10.24.

thanks for the hint!

Thomas, the device discovery is now using udev on Linux platforms.

Could you please rework your patch to use your old cheese_webcam_get_video_devices_from_probe(), and put that into cheese_camera_device_monitor_coldplug() in libcheese/cheese-camera-device-monitor.c

Cheers

Thanks for looking at this. I've lost my interest to work on this though. Feel free to close the bug report if you like (though I'd of course prefer inclusion of a version of this patch, just on general principles :) ).

Well, I don't have a NetBSD machine to test it, and left you the stubs to implement in the current master code.

any news on this?

Daniel: Who do you ask? A NEEDINFO without an addressee always goes wrong.

Reopening as I can't see any open question.

I do not have NetBSD installed to test, and there does not seem to be any interest in adding NetBSD support.

I have NetBSD installed to test... Is there something to test? If not what do I need to write?

There is no code to test (but it might be possible to look at the obsolete patches in this bug report and port them to the latest Cheese code). CheeseCameraDeviceMonitor would need to me modified to work with whatever device autodetection mechanism NetBSD has. camerabin2 from GStreamer would need to be tested on NetBSD, but it might work without any modification.