Created attachment 63735 [details]
interactive test program

compile with:

gcc -Wall -g3 -O0 `pkg-config --cflags --libs gstreamer-base-0.10` -lgstinterfaces-0.10 v4l2test.c

Just updating the summary; new elements don't get added to -base. As discussed on irc, you'll need to go through the entire moving-plugins checklist. Also, this test program should just get committed to cvs (in -bad, with the plugin, for now, of course) now, no need for it to languish in bugzilla.

interactive test program added to gst-plugins-bad/tests/icles/

It seems you chose one of the cruftiest elements to port :-)

First of all gstv4l2element was made so that the source and sink
elements could share code. Therefore it doesn't make sense for it to be
a GstPushSrc -- if the need for the intermediate layer is gone, then
everything can be in gstv4l2src itself.

However, it's true that in the future we will want to support sinks as
well, and share code between the two. This suggests that we need to
create helper objects to hold the shared functionality -- it can't be
part of the class hierarchy, because we want to use basesrc and
basesink.

So, I suggest an approach more like the mixer interface in sound device
plugins. See the alsasrc or osssrc code to see how that is done. I think
that the tuner interface could be factored into a helper object in this
way. Some of the raw device handling will have to be duplicated though,
I think.

The end will be a gstv4l2src.c that descends from GstPushSrc, no
gstv4l2element or gstv4l2element.c, and a much thicker tuner object that
can be instantiated via gst_v4l2_tuner_new (int fd) or something. Then
to implement the tuner interface, there should be a macro like the one
that exists for GstMixer in gstalsamixer.h.

Now some specific comments:

* gstv4l2element.c: bad indentation -- fix the comment block, and put a
  semicolon after the GST_BOILERPLATE_FULL so that the static methods
  are indented properly

* gstv4l2element.c: the propertyprobe stuff isn't threadsafe. why do
  those static variable caching tricks at all? it just brings problems
  with memory management and threadsafety. Better to stick the list of
  probe-able properties in the instance, and free them when the instance
  is freed, and for the devices, probe every time the application asks.

* gstv4l2element.c: no need to g_object_notify from within set_property,
  that's done by gobject.

* suggestions for making the tuner interface a more capable object:

  * merge gst_v4l2_get_input and gst_v4l2_set_input from v4l2_calls.c
    into gst_v4l2_tuner_get_channel and gst_v4l2_tuner_set_channel in
    gstv4l2tuner.c, respectively. They are called from nowhere else.

  * actually, factor out the functionality from get_channel and
    set_channel such that only calls from the GstTuner interface will
    result in a g_object_notify; calls from the set_property will
    already have gobject doing notifies

  * move gst_v4l2_set_defaults from v4l2_calls.c to gstv4ltuner.c, fold
    into gst_v4l2_tuner_new

  * remove the gst_v4l_tuner_is_sink function for now, it's useless;
    also fold the code from gst_v4l2_{get,set}_output into
    tuner_set_channel

* v4l2_calls.c should be factored into gstv4l2src.c and gstv4l2tuner.c,
  and then removed

* v4l2src_calls.c should be factored into gstv4l2src.c

* perhaps you can factor the caps code into a helper object as well?
  dunno -- would allow you to absorb code from
  v4l2src_calls.c:gst_v4l2src_get_fps, get_fps_list as well

* instead of marking the debugging functions
  gst_v4l2_tuner_contains_channel and gst_v4l2_tuner_contains_norm as
  unused, they should only be compiled when they could be called, i.e.
  when G_DISABLE_ASSERTS or whatever it is isn't defined

* *.[ch]: You should be in the copyrights of these.

Well, this is just how it looks to me. I've spent a few hours now
looking at this code -- tell me what you think. I think that due to all
of this v4l2src will only hit -good on the next cycle.

ps. Clever program!

Hi Wingo,

Ive just commited the changes you requested.

Please let me know what more to do and if it is like you had in mind.

some points:
I didnt remove v4l2_calls and v4l2src_call. I like this because it is like a layer that has v4l2 knowledge (you will find ioctls and open just here)

Sorry I didnt understand the point bellow

* instead of marking the debugging functions
  gst_v4l2_tuner_contains_channel and gst_v4l2_tuner_contains_norm as
  unused, they should only be compiled when they could be called, i.e.
  when G_DISABLE_ASSERTS or whatever it is isn't defined


BR,

Some new, non-comprehensive comments... My brain is not working right now :/

* fine to keep the _calls files, use your best judgement :)

* Check your copyrights -- the ones with your name should all be 2006

* header on gstv4l2object badly formatted -- just copy from gstv4l2.c

* OPEN_V4L2OBJECT_PROPS/CLOSE_V4L2_PROPS nonstandard and mess with
  indentation; just define an enum in gstv4l2object.h or something:

enum {
  PROP_0,
  PROP_DEVICE,
  PROP_DEVICE_NAME,
  PROP_FLAGS,
  PROP_STD,
  PROP_INPUT,
  PROP_FREQUENCY,
  PROP_V4L2_LAST = PROP_FREQUENCY
};

  and then in e.g. gstv4l2src.c:

enum {
  PROP_FOO = PROP_V4L2_LAST + 1,
  ...
};

* In general any of these magical macros should be made in such a way
  that you can add a semicolon at the end, so that gst-indent is happy.

* functions that operate on GstV4l2Object should be called
  gst_v4l2_object_*.

* you should comment a bit on the rationale for all of this macro
  trickery :)

* any reason to have properyprobe in gstv4l2object and not in its own
  file?

Created attachment 65769 [details] [review]
code cleanups

some of the requested code cleanups

and this is the only lauch line that currently works for me:

gst-launch-0.10 v4l2src ! videoscale ! videorate ! video/x-raw-rgb,width=320,height=240,framerate='(fraction)'15/1 ! ximagesink

any idea how to pick the native resoltion of the cam?

Hi Wingo,

Could you please check v4l2 again and see if something should be fixed?

> any reason to have properyprobe in gstv4l2object and not in its own file?

may be a future gstv4l2xxx element could use this? I don't know, what do you think?

Hi Stefan,

does

gst-launch-0.10 v4l2src ! ffmpegcolorspace ! ximagesink

or 

gst-launch-0.10 v4l2src ! xvimagesink

works for you?

I sponsor the move

Stefan,

You mention a problem yourself in comment 9, but did not get a reply - does that mean you don't think this is a problem that needs to be solved first ?

To sponsor the move you need to take the checklist from moving-plugins and verify that these items are taken care of.


Since this was lacking from this bug report, I took a look at it myself.


The first thing I did was the same as you:

[gst-head] [thomas@otto v4l2]$ gst-launch v4l2src ! videoscale ! videorate ! video/x-raw-rgb,width=320,height=240,framerate='(fraction)'15/1 ! ximagesink
Setting pipeline to PAUSED ...
ERROR: Pipeline doesn't want to pause.
ERROR: from element /pipeline0/v4l2src0: Error getting capabilities '/dev/video0': 22, Invalid argument. It isn't a v4l2 driver. Check if it is a v4l1 driver

Additional debug info:
v4l2_calls.c(58): gst_v4l2_get_capabilities (): /pipeline0/v4l2src0:
system error: Invalid argument
Setting pipeline to NULL ...
FREEING pipeline ...

This is a downright terrible error message.  Imagine a recording application throwing up a dialog box with this text.  What does 22 mean ? What does Invalid argument mean ? Who cares ?

So I looked through the code and saw a bunch more like these.  Since the first thing I try with this plugin is something that fails the checklist, I assume no one has done a full review yet, and this should be done first before it can be approved for move.

Anyone willing ?

Thomas,

Comment #9 is not longer a concern. Edgard fixed the aps nego a while ago.

I've checked all GST_ELEMENT_ERROR statements and unified them a bit. No more errno values in user-messages.
The copyright header in all files are now also in the format of the rest of GStreamer.
The object property names whave been dashified.
The element has docs. I am a bit unsure about unit tests for such an element.

Stefan,

there is a test at 

gst-plugins-bad/tests/icles/v4l2src-test.c

I will have a look again and see if I improved it. Do you already have any sugestions?

btw: Thomas, just to let you know, your web-cam driver is not in v4l2 version (it is on v4l1 version yet)....you can't test it unless you install a newer version of  your web-cam hardware.

AS Edgard pointed out, for the future it might be a good idea to implement the GstMixer interface for cameras with audio input (see http://bugzilla.gnome.org/show_bug.cgi?id=354908).

Add to your todo list the fact that GstImplementsInterface isn't properly implemented (see #340338)

Hi Stefan,

from <a href="show_bug.cgi?id=338818#c15">Comment #15</a>
bug #354908 already solved as you know. 

Hi Edward,

from <a href="show_bug.cgi?id=338818#c16">Comment #16</a>
I don't think #340338 is a v4l2src bug, please let me know if I'm wrong and, if possible, how can I fix that.

BR,
Edgard

trying to approve this now.

Marked some lines in the code with FIXME in current CVS, please fix or clarify in comments.

since no indication that all items on the checklist are verified and fixed, I'll go over them myself now.

What about the use-fixed-fps property? It does not seem to be used except to enforce a framerate be set by the device. The property description should reflect that.

Minor issue, but any chance to change the "std" property to something more descriptive like "standard" or "norm"?

Also, does the gst_tuner_list_norms() implementation actually work? A quick grep -e '->stds' makes it seem like it never gets assigned to anything but NULL.

The flags listing in gst-inspect also doesn't seem quite right, at least different compared to cdparanoiasrc for example (which lists flag nicks rather than descriptions). Might mostly be a gst-inspect issue though, it probably should list both flag nicks and descriptions. Still, seems like nicks/descriptions are mixed up somewhere.

we have a history of wrongly specifying flags and enums.. The correct layout is like:

    static const GFlagsValue values[] = {
      {V4L2_CAP_VIDEO_CAPTURE, "Device supports video capture", "capture"},
      {V4L2_CAP_VIDEO_OUTPUT, "Device supports video playback", "output"},
      ...
    }

The nick is used when setting properties, the name is the long, readable description.

I just fixed -inspect for flags too.

It's pretty ok now for me except for the renaming of the property names:

 std ->   norm (term used by the tuner interface)
 input -> channel (term used by the tuner interface)

Tim: ->std is assigned in gst_v4l2_fill_lists(), like ->channels.

> It's pretty ok now for me except for the renaming of the property names:
> 
>  std ->   norm (term used by the tuner interface)
>  input -> channel (term used by the tuner interface)

Could we also rename "use-undef-fps", either into "use-undefined-fps" or preferably even something better?


> Tim: ->std is assigned in gst_v4l2_fill_lists(), like ->channels.

Sorry, my fault, grepped against gst*.c ;)

Framerate is set t random values (fix for that is comming)
Trying to stop it deadlocks on my webcam.

Top stack of deadlock, seems to be blocked in an ioctl.

+ Trace 73486

I still see some obvious problems with the error stuff.

Here's an example:
fmt_failed:
  {
    GST_ELEMENT_ERROR (v4l2src, RESOURCE, SETTINGS,
        (_("Failed to set pixelformat to %s @ %dx%d for device %s: %s."),
            fmt->description, *width, *height, v4l2src->v4l2object->videodev),
        GST_ERROR_SYSTEM);
    return FALSE;
  }
pixfmt_failed:
  {
    GST_ELEMENT_ERROR (v4l2src, RESOURCE, SETTINGS,
        (_("Failed to set pixelformat to %s @ %dx%d for device %s: %s."),
            fmt->description, *width, *height, v4l2src->v4l2object->videodev),
        GST_ERROR_SYSTEM);
    return FALSE;
  }

What is wrong here ?

a) count the number of %'s and the number of actual arguments.  Then rub your eyes and count them again.

b) Please, *imagine* you get this error message as a user.  *a user*.  This is not a debugging message, this should be a readable message.  It should not contain @, x, and stuff like that.  It should not contain a word like pixelformat.  What is a pixelformat ? All that stuff needs to go in a debug string.  Does the width and height matter in this case ? Please explain in human terms what the problem is.

There are more like these:
    GST_ELEMENT_ERROR (v4l2src, RESOURCE, READ,
        (_("Could not mmap video buffer %d."), n), GST_ERROR_SYSTEM);

What is mmap ? Why does it matter *which* value of n it failed for ? All of that -> debug info.

Really, *read* the error messages that will get shown and think about them in the perspective of the application.  It is not that hard, it just requires putting yourself in that position.

Another problem I ran into.  Right now v4l2src tries to negotiate a 32768x32768 frame size.  I haven't yet encountered a v4l2 device that can do this.  I also think it is not the behaviour you expect, and it is definately different from v4lsrc.

So I tried changing it to a standard format: CIF, which is 352x288.

This does not work on my pwc camera however - the log shows that it requests 352x288, but gets 320x240 instead.  That should be fine; however the pipeline then stops with a negotiation error.

[gst-head] [thomas@otto v4l2]$ GST_DEBUG=v4l*:4 gst-launch v4l2src ! video/x-raw-yuv,width=352,height=288 ! xvimagesink sync=FALSE
0:00:01.183184000 12969 0x8b22a18 DEBUG              v4l2src gstv4l2src.c:558:gst_v4l2src_v4l2fourcc_to_caps: Unknown fourcc 0x31384142 BA81
0:00:01.183436000 12969 0x8b22a18 DEBUG              v4l2src gstv4l2src.c:558:gst_v4l2src_v4l2fourcc_to_caps: Unknown fourcc 0x30313953 S910
Setting pipeline to PAUSED ...
0:00:01.405691000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:349:gst_v4l2_open:<v4l2src0> Trying to open device /dev/video0
0:00:01.780812000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:54:gst_v4l2_get_capabilities:<v4l2src0> getting capabilities
0:00:01.781064000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:86:gst_v4l2_fill_lists:<v4l2src0> getting enumerations
0:00:01.781167000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:89:gst_v4l2_fill_lists:<v4l2src0>   channels
0:00:01.781268000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:108:gst_v4l2_fill_lists:<v4l2src0>     'usb'
0:00:01.781446000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:148:gst_v4l2_fill_lists:<v4l2src0>   norms
0:00:01.781552000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:172:gst_v4l2_fill_lists:<v4l2src0>     'webcam', fps: 0/1
0:00:01.781853000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:184:gst_v4l2_fill_lists:<v4l2src0>   controls+menus
0:00:01.781956000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:259:gst_v4l2_fill_lists:<v4l2src0> Adding ControlID Brightness (980900)
0:00:01.782088000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:259:gst_v4l2_fill_lists:<v4l2src0> Adding ControlID Contrast (980901)
0:00:01.782196000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:259:gst_v4l2_fill_lists:<v4l2src0> Adding ControlID Saturation (980902)
0:00:01.782310000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:259:gst_v4l2_fill_lists:<v4l2src0> Adding ControlID Auto White Balance (98090c)
0:00:01.782418000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:259:gst_v4l2_fill_lists:<v4l2src0> Adding ControlID Red Gain (98090e)
0:00:01.782524000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:259:gst_v4l2_fill_lists:<v4l2src0> Adding ControlID Blue Gain (98090f)
0:00:01.782629000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:259:gst_v4l2_fill_lists:<v4l2src0> Adding ControlID Gamma (980910)
0:00:01.782733000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:259:gst_v4l2_fill_lists:<v4l2src0> Adding ControlID Shutter Speed (Exposure) (980911)
0:00:01.782840000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:259:gst_v4l2_fill_lists:<v4l2src0> Adding ControlID Auto Gain Enabled (980912)
0:00:01.782945000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:259:gst_v4l2_fill_lists:<v4l2src0> Adding ControlID Gain Level (980913)
0:00:01.783376000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:193:gst_v4l2_fill_lists:<v4l2src0> checking private CIDs
0:00:01.783482000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:315:gst_v4l2_fill_lists:<v4l2src0> done
0:00:01.783581000 12969 0x8b22a18 INFO                  v4l2 v4l2_calls.c:387:gst_v4l2_open:<v4l2src0> Opened device 'Logitech QuickCam Orbit' (/dev/video0) successfully
0:00:01.783705000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:469:gst_v4l2_get_norm:<v4l2src0> getting norm
0:00:01.783828000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:705:gst_v4l2_get_input:<v4l2src0> trying to get input
0:00:01.783948000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:70:gst_v4l2src_fill_format_list:<v4l2src0> getting src format enumerations
0:00:01.784104000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:789:gst_v4l2src_get_fps:<v4l2src0> failed to get PARM: Invalid argument
0:00:01.784205000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:469:gst_v4l2_get_norm:<v4l2src0> getting norm
0:00:01.784305000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:823:gst_v4l2src_get_fps:<v4l2src0> frame rate returned by get_norm: 0/1 fps
0:00:01.784429000 12969 0x8b22a18 DEBUG              v4l2src gstv4l2src.c:558:gst_v4l2src_v4l2fourcc_to_caps: Unknown fourcc 0x32435750 PWC2
0:00:01.785124000 12969 0x8b22a18 DEBUG              v4l2src gstv4l2src.c:372:gst_v4l2src_fixate:<v4l2src0> fixating caps video/x-raw-yuv, format=(fourcc)I420, width=(int)352, height=(int)288, framerate=(fraction)[ 0/1, 100/1 ]
0:00:01.785260000 12969 0x8b22a18 DEBUG              v4l2src gstv4l2src.c:797:gst_v4l2src_set_caps:<v4l2src0> trying to set_capture 352x288, format 4:2:0, planar, Y-Cb-Cr
0:00:01.785368000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:285:gst_v4l2src_set_capture:<v4l2src0> V4L2SRC: Setting capture format to 352x288, format 4:2:0, planar, Y-Cb-Cr
0:00:01.785471000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:248:gst_v4l2src_get_capture:<v4l2src0> V4L2SRC: Getting capture format
0:00:01.943879000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:307:gst_v4l2src_set_capture:<v4l2src0> V4L2SRC: Updating size from 352x288 to 320x240, format 4:2:0, planar, Y-Cb-Cr
0:00:01.944120000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:248:gst_v4l2src_get_capture:<v4l2src0> V4L2SRC: Getting capture format
0:00:01.944252000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:757:gst_v4l2src_set_fps:<v4l2src0> failed to get PARM: Invalid argument
0:00:01.944379000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:375:gst_v4l2src_capture_init:<v4l2src0> initting the capture system
0:00:01.944490000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:789:gst_v4l2src_get_fps:<v4l2src0> failed to get PARM: Invalid argument
0:00:01.944590000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:469:gst_v4l2_get_norm:<v4l2src0> getting norm
0:00:01.944692000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:823:gst_v4l2src_get_fps:<v4l2src0> frame rate returned by get_norm: 0/1 fps
0:00:01.944849000 12969 0x8b22a18 INFO               v4l2src v4l2src_calls.c:420:gst_v4l2src_capture_init:<v4l2src0> Got 2 buffers (YU12) of size 112 KB
0:00:01.945007000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:528:gst_v4l2src_capture_start:<v4l2src0> starting the capturing
0:00:01.945109000 12969 0x8b22a18 WARN               v4l2src gstv4l2src.c:832:gst_v4l2src_set_caps:<v4l2src0> framerate changed after start capturing from 15/2 to 0/1
Pipeline is live and does not need PREROLL ...
Setting pipeline to PLAYING ...
New clock: GstSystemClock
ERROR: from element /pipeline0/v4l2src0: Internal data flow error.
Additional debug info:
gstbasesrc.c(1610): gst_base_src_loop (): /pipeline0/v4l2src0:
streaming task paused, reason not-negotiated (-4)
Execution ended after 861552000 ns.
Setting pipeline to PAUSED ...
Setting pipeline to READY ...
0:00:02.808439000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:564:gst_v4l2src_capture_stop:<v4l2src0> stopping capturing
0:00:02.808472000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:619:gst_v4l2src_capture_deinit:<v4l2src0> deinitting capture system
0:00:02.906883000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:528:gst_v4l2src_capture_start:<v4l2src0> starting the capturing
0:00:02.906938000 12969 0x8b22a18 DEBUG              v4l2src v4l2src_calls.c:564:gst_v4l2src_capture_stop:<v4l2src0> stopping capturing
0:00:02.906962000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:445:gst_v4l2_close:<v4l2src0> Trying to close /dev/video0
0:00:02.913273000 12969 0x8b22a18 DEBUG                 v4l2 v4l2_calls.c:323:gst_v4l2_empty_lists:<v4l2src0> deleting enumerations
Setting pipeline to NULL ...
FREEING pipeline ...

You sure that should be fine? Looks like a valid error in this case (when trying to set caps on capsfilter's sink, obviously that will fail)

Regarding b) in #27, I added a G_GNUC_PRINTF in core on Andy's suggestion, and now the wrong number of arguments gets caught properly.  Too bad I can't actually commit it, because it doesn't understand things like GST_PTR_FORMAT :/

> I added a G_GNUC_PRINTF in core, and now the wrong number of arguments 
> gets caught properly.  Too bad I can't actually commit it, because it
> doesn't understand things like GST_PTR_FORMAT :/

Why not have two declarations, one without G_GNUC_PRINTF (if HAVE_PRINTF_EXTENSION is set) and one with G_GNUC_PRINTF (when GST_PTR_FORMAT and GST_SEGMENT_FORMAT just map to "p" anyway)? That way mistakes like this would at least be caught by folks building on/for platforms where the printf extension is disabled or unavailable. We could even wrap this into a GST_GNUC_PRINTF, since the same ussye exists with the log functions IIRC.

yeah, we could do that, but we would get very limited testing on it - for example all of our slaves build with gcc atm.

So in practice you would get a build failure on a platform used by someone that traditionally doesn't give us much feedback.

Dunno :)

The printf extension can be turned off by setting ac_cv_func_register_printf_function=no in the environment prior to running configure.  It might be wise to set up a build slave that builds with this parameter.  Or actually, one that rotates through a list of outlying build options.

I like that idea, Tim, think you can cook up a patch for that ?

A new problem I found after moving.  Apparently the check for XVIDEO was busted and so the plugin was never built with HAVE_XVIDEO.  Now that it does get built that way, it doesn't build properly.

cc1: warnings being treated as errors
gstv4l2src.c: In function 'gst_v4l2src_xoverlay_set_xwindow_id':
gstv4l2src.c:132: warning: implicit declaration of function 'gst_v4l2_xoverlay_set_xwindow_id'
gstv4l2src.c: In function 'gst_v4l2src_xoverlay_interface_init':
gstv4l2src.c:132: error: expected expression before 'GstXOverlayClass'
make: *** [libgstvideo4linux2_la-gstv4l2src.lo] Error 1

probably an easy fix for someone that knows his way around.

Created attachment 74288 [details] [review]
Fixes build error mentioned in comment #35

I removed the "static", and that made the build go through, but I don't know if that was the correct solution to the problem. Please review.

Yeah, that's the right fix. Committed with a slight change to make the GST_IMPLEMENT_V4L2_XOVERLAY_METHODS macro only 80 columns wide too.

        * sys/v4l2/gstv4l2xoverlay.c:
        * sys/v4l2/gstv4l2xoverlay.h:
        Fix build as per the patch in #338818 comment 36.

Created attachment 74343 [details] [review]
Fixes problem where set_channel could not be called.

The internal function gst_v4l2_tuner_set_channel_and_notify is obsolete since channel is no longer a property, but handled through the tuner interface. However, this function was still called internally and it still treated "channel" as a property. This patch fixes this by removing the function and redirecting all calls to it to gst_v4l2_tuner_set_channel instead.

Thanks, comitted:

  2006-10-18  Tim-Philipp Müller  <tim at centricular dot net>

        Patch by: Fredrik Persson  <frepe at broadband net>

        * sys/v4l2/gstv4l2tuner.c:
        * sys/v4l2/gstv4l2tuner.h:
          Fix _set_channel(): remove useless g_object_notify() for "channel"
          property that doesn't exist any longer and therefore now also
          useless redirect (#338818).




So, is anyone going to fix up the error messages?

for comments #35, #36 and #37, I have add #if 0 to xovelay code because it is still not implemented in v4l2src. (I hope implement it some day)

for comment #28 and #29. If the device can't work in the size specified in filter caps it will try to work in the closest possible size, and I have just added a GST_ELEMENT_WARNING about that. If no size is specified the driver will work in the biggest possible size (that should be something like 640x480 or 1024x956... depending on device) (that is why the v4l2src code asks for 32768x32768).

wtay, what about comment #25 and #26, has it been fixed for you?

Thomas, please review the messages and tell me what more I need to do to get v4l2src out of --enable-experimental

I would like also to see all other v4l2src bugs closed...it would be good to ask people for NEED info

http://bugzilla.gnome.org/buglist.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=v4l2src&product=GStreamer&long_desc_type=substring&long_desc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&bug_status=NEEDINFO&emailassigned_to1=1&bugidtype=include&chfieldto=Now&cmdtype=doit
Bug #340338, Bug #314160 and Bug #347806

Edgard: the first critical two ones seem to have all the info that is needed.

Egard: there are some ""Error read()ing ..." messages in gstv4l2src.c. Simply say "Error reading ...". The one in line 911 should only give the number of bytes read in the debug.

Apart the messages are better now. Thanks!

I would like to close this. I'll set it to NEEDINFO and if nobody complains within two week close as OBSOLETE.