GNOME Bugzilla – Bug 709414
dvbsrc: Uses deprecated field frontend type and add DVB S2/T2 support
Last modified: 2014-05-26 11:33:14 UTC
1. in method gst_dvbsrc_open_frontend you use deprecated field dvb_frontend_info.type . It is not dvbv5 like. in frontend doc, section frontend type: "The usage of this field is deprecated, as it doesn't report all supported standards, and will provide an incomplete information for frontends that support multiple delivery systems. Please use DTV_ENUM_DELSYS instead." 2. please add a property delsys. 3. please add a method for information of all dvb resources.
Created attachment 256755 [details] [review] Change to DTV_ENUM_DELSYS
Created attachment 256756 [details] [review] add DVB-T2, DVB-S2
Review of attachment 256755 [details] [review]: ::: sys/dvb/gstdvbsrc.c @@ +115,3 @@ #define DEFAULT_DVB_BUFFER_SIZE (10*188*1024) /* Default is the same as the kernel default */ #define DEFAULT_BUFFER_SIZE 8192 /* not a property */ +#define DEFAULT_DELSYS 0 Use the actual enum value (SYS_UNDEFINED ?) @@ +866,3 @@ adapter_name = g_strdup (fe_info.name); + adapter_structure = gst_structure_new ("dvb-adapter", hmmm... Any way we could make this a "tiny bit" more readable ? :) Do we really need to have FALSE for the delsys that are not spuported ? Iterating over the known ones and just adding the values (gst_structure_set...) would make the resulting structure shorter and the code a bit more readable.
## Do we really need to have FALSE for the delsys that are not spuported ? ## Iterating over the known ones and just adding the values (gst_structure_set...) ## would make the resulting structure shorter and the code a bit more readable. Yes. Each apps which use this plugin must know the supported delvery systems. Okay, I can rewrite this structure (a bit more readable).
Edward is that better for each delsys? if (gst_dvbsrc_check_delsys (&dvb_prop[0], SYS_DVBC_ANNEX_A)) gst_structure_set (adapter_structure, "dvb-c-a", G_TYPE_STRING, "DVB-C ANNEX A", NULL);
I was going to say we could use a list ... but then I remembered that lists in structures aren't supported by most bindings :( That solution will have to do
Created attachment 256796 [details] [review] Change to DTV_ENUM_DELSYS ver 2
Created attachment 256797 [details] [review] add DVB-T2, DVB-S2 ver 2
Review of attachment 256796 [details] [review]: Looks good to me, needs to be tested by someone with DVB (Edward? :) ) ::: sys/dvb/gstdvbsrc.c @@ +875,3 @@ + G_TYPE_BOOLEAN, fe_info.caps & FE_CAN_GUARD_INTERVAL_AUTO, + "auto-hierarchy", G_TYPE_BOOLEAN, fe_info.caps % FE_CAN_HIERARCHY_AUTO, + "auto-fec", G_TYPE_BOOLEAN, fe_info.caps & FE_CAN_FEC_AUTO, NULL); You now set these auto-* fields always. Previously they were only set depending on the delsys. Does this make sense?
Review of attachment 256797 [details] [review]: Looks generally good to me ::: sys/dvb/gstdvbsrc.c @@ +555,3 @@ + g_param_spec_int ("stream-id", "stream-id", + "Stream ID (DVB-T2 and DVB-S2 max 255, ISDB max 65535)", + -1, 65535, DEFAULT_STREAM_ID, G_PARAM_READWRITE)); What's -1? Add that to the description :)
(In reply to comment #9) > Review of attachment 256796 [details] [review]: > > Looks good to me, needs to be tested by someone with DVB (Edward? :) ) > > ::: sys/dvb/gstdvbsrc.c > @@ +875,3 @@ > + G_TYPE_BOOLEAN, fe_info.caps & FE_CAN_GUARD_INTERVAL_AUTO, > + "auto-hierarchy", G_TYPE_BOOLEAN, fe_info.caps % FE_CAN_HIERARCHY_AUTO, > + "auto-fec", G_TYPE_BOOLEAN, fe_info.caps & FE_CAN_FEC_AUTO, NULL); > > You now set these auto-* fields always. Previously they were only set depending > on the delsys. Does this make sense? I don't know it for all delsys. Can I add the same fields?
(In reply to comment #10) > Review of attachment 256797 [details] [review]: > > Looks generally good to me > > ::: sys/dvb/gstdvbsrc.c > @@ +555,3 @@ > + g_param_spec_int ("stream-id", "stream-id", > + "Stream ID (DVB-T2 and DVB-S2 max 255, ISDB max 65535)", > + -1, 65535, DEFAULT_STREAM_ID, G_PARAM_READWRITE)); > > What's -1? Add that to the description :) -1 = disabled and is default.
(In reply to comment #11) > (In reply to comment #9) > > Review of attachment 256796 [details] [review] [details]: > > > > Looks good to me, needs to be tested by someone with DVB (Edward? :) ) > > > > ::: sys/dvb/gstdvbsrc.c > > @@ +875,3 @@ > > + G_TYPE_BOOLEAN, fe_info.caps & FE_CAN_GUARD_INTERVAL_AUTO, > > + "auto-hierarchy", G_TYPE_BOOLEAN, fe_info.caps % FE_CAN_HIERARCHY_AUTO, > > + "auto-fec", G_TYPE_BOOLEAN, fe_info.caps & FE_CAN_FEC_AUTO, NULL); > > > > You now set these auto-* fields always. Previously they were only set depending > > on the delsys. Does this make sense? > > I don't know it for all delsys. Can I add the same fields? Thinking about it, it should be fine as is. (In reply to comment #12) > (In reply to comment #10) > > Review of attachment 256797 [details] [review] [details]: > > > > Looks generally good to me > > > > ::: sys/dvb/gstdvbsrc.c > > @@ +555,3 @@ > > + g_param_spec_int ("stream-id", "stream-id", > > + "Stream ID (DVB-T2 and DVB-S2 max 255, ISDB max 65535)", > > + -1, 65535, DEFAULT_STREAM_ID, G_PARAM_READWRITE)); > > > > What's -1? Add that to the description :) > > -1 = disabled and is default. ok, just add that then :)
Created attachment 256926 [details] [review] Change to DTV_ENUM_DELSYS v3
Created attachment 256927 [details] [review] add DVB-T2, DVB-S2 v3
*** Bug 720350 has been marked as a duplicate of this bug. ***
Created attachment 264540 [details] dvbsrc test ouput, lock on dvbs-2 signal Tested features: - delsys - rolloff - 8PSK modulation Untested features: - stream_id - pilot
Created attachment 264541 [details] dvbbasebin test output, lock on dvb-s(2) signal Tested features: - delsys - rolloff - 8PSK modulation Untested features: - stream_id - pilot
commit cd11a38bf0780da9b20aac5e46f980f47c57786d Author: Stefan Ringel <linuxtv@stefanringel.de> Date: Thu Oct 10 18:25:46 2013 +0200 dvbsrc: Add dvb-s2, dvb-t2 support https://bugzilla.gnome.org/show_bug.cgi?id=709414 commit e34df02115574084c13cc88caa47198bcc0856fc Author: Stefan Ringel <linuxtv@stefanringel.de> Date: Thu Oct 10 18:23:20 2013 +0200 dvbsrc: Change from deprecated frontend type field to DTV_ENUM_DELSYS -add delsys property -add delivery system capability to the gstreamer adapter structure -ready for add new delivery systems Application must ask the adapter structure to know which delivery systems are avaible. The property delsys must be set. https://bugzilla.gnome.org/show_bug.cgi?id=709414
Nice, but this won't build on kernel 3.4, do we really want to drop support for older kernel ?
Probably not, 3.4 is not that old yet unfortunately. Who wants to provide a patch that works with 3.4 and older too?
I have Linux square 3.8.0-35-generic and it does not build. Can we use #ifdefs ?
Ok, there's bug #721869 for that created just now.
Those commits broke dvbbasebin (you now need to set delsys) and therefore the dvb:// usage
AFAICS, the fastest way forward to fix this would be to have parsechannels.[ch] also set the delsys property accordingly.
Created attachment 271523 [details] [review] parsechannels: add delsys property
Stefan, can you provide a version of the last patch but which uses the actual SYS_ defines ?
(In reply to comment #27) > Stefan, can you provide a version of the last patch but which uses the actual > SYS_ defines ? o.k. Another point, the channel file looks too old for current and future delivery systems. Should we rework the channel file implemetation now?
Created attachment 274094 [details] [review] parsechannels: add delsys property
Meh, we should even completely go away from a file-based system and switch to a daemon-based system (ask via dbus for settings, update them ,...). Let's just avoid regressions for now. I'm glad to discuss better ways somewhere else :)
(In reply to comment #30) > Meh, we should even completely go away from a file-based system and switch to a > daemon-based system (ask via dbus for settings, update them ,...). > > Let's just avoid regressions for now. I'm glad to discuss better ways somewhere > else :) Good idea.
in which ones we stored channel data, if a computer goes off?
commit f3489442468f0c1be747a0b26cde24aa4a39d4aa Author: Stefan Ringel <linuxtv@stefanringel.de> Date: Fri Apr 11 15:14:17 2014 +0200 dvb: parsechannels: add delsys property Signed-off-by: Stefan Ringel <linuxtv@stefanringel.de> https://bugzilla.gnome.org/show_bug.cgi?id=709414