[gstreamer-bugs] [Bug 338818] move v4l2src from bad to good

[GStreamer (bugzilla.gnome.org)]

Do not reply to this via email (we are currently unable to handle email
responses and they get discarded).  You can add comments to this bug at
http://bugzilla.gnome.org/show_bug.cgi?id=338818
 GStreamer | gst-plugins-good | Ver: HEAD CVS


Andy Wingo changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |wingo at pobox.com




------- Comment #5 from Andy Wingo  2006-05-04 11:34 UTC -------
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!


-- 
Configure bugmail: http://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.