Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#14936 closed patch (fixed)

Port to Farstream

Reported by: ocrete Owned by: Maiku
Milestone: 2.10.4 Component: Voice and Video
Version: 2.10.1 Keywords:
Cc:

Description

The Farsight2 library that pidgin currently uses will soon be deprecated and replaced by Farstream, here is a patch that does the replacement.

I expect that the next round of distributions (based on Gnome 3.4) will do the switch as Empathy is also switching.

Attachments (6)

port-to-farstream.patch (14.7 KB) - added by ocrete 6 years ago.
Patch that replaces Farsight2 with Farstream
port-to-farstream-v2.patch (14.7 KB) - added by foutrelis 5 years ago.
Same as original patch but also replaces the farsight include in farsight include in libpurple/mediamanager.c; patch is against pidgin 2.10.1.
port-to-farstream-v3.patch (14.7 KB) - added by haakon 5 years ago.
Fix crash in gst_handle_message_element()
port-to-farstream-v4.patch (15.1 KB) - added by ocrete 5 years ago.
Also "codecs-ready" is gone.. "codecs" just returns NULL if they are not ready
port-to-farstream-v5.patch (15.1 KB) - added by ocrete 5 years ago.
version that actually compiles.. and works
port-to-farstream-rlaager-v1.patch (19.5 KB) - added by rlaager 5 years ago.
Supports farsight and farstream

Download all attachments as: .zip

Change History (28)

Changed 6 years ago by ocrete

Patch that replaces Farsight2 with Farstream

comment:1 Changed 6 years ago by Robby

  • Milestone set to Patches Needing Review
  • Type changed from enhancement to patch

comment:2 Changed 6 years ago by QuLogic

I don't think we can switch to Farstream unconditionally.

Are all these changes required for the switch? For example, the stream->supports_add addition doesn't seem to need Farstream.

comment:3 Changed 6 years ago by ocrete

Actually, in farsight2, there is only a fs_stream_set_remote_candidates(). In Farstream, this has been split into fs_stream_add_remote_candidates() and fs_stream_force_remote_candidates(). The main difference between them is that _force_() implies that there is no negotiation, while _add_() gives them to some negotiation engine (like libnice).

So this change is purely for Farstream.

The only bit of the patch that can be used with Farsight2 is the use of the default element properties from Farsight2/Farstream, but even this requires a relatively recent version of Farsight2.

The rest is pure porting. Farstream has a couple new APIs that can reduce the amount of code a bit, but I didn't even port to those.

We can add ifdefs all over, and since Farsight2 and Farstream are not parallel installable, there is no point in having separate backends.

comment:4 Changed 5 years ago by foutrelis

The original patch is missing a farsight include in libpurple/mediamanager.c.

Changed 5 years ago by foutrelis

Same as original patch but also replaces the farsight include in farsight include in libpurple/mediamanager.c; patch is against pidgin 2.10.1.

Changed 5 years ago by haakon

Fix crash in gst_handle_message_element()

comment:5 Changed 5 years ago by haakon

port-to-farstream-v2.patch segfaults Pidgin in gst_handle_message_element() when initiating a call. This is caused by superfluous ampersand in create_participant() function where instead of

g_object_set(participant, "cname", &name, NULL);

there should be

g_object_set(participant, "cname", name, NULL);

Fixed in port-to-farstream-v3.patch.

comment:6 Changed 5 years ago by ocrete

Both changes look good to me.

comment:7 follow-up: Changed 5 years ago by BigBrownChunx

I would personally reject this patch: libpurple can support multiple media backends. The patch shouldn't overwrite Farsight2 but instead provide separate backend support for Farstream via use flags. This will also give the opportunity to remove the Farsight2-specific code out of libpurple and back into the media backend where it belongs.

Changed 5 years ago by ocrete

Also "codecs-ready" is gone.. "codecs" just returns NULL if they are not ready

Changed 5 years ago by ocrete

version that actually compiles.. and works

comment:8 in reply to: ↑ 7 Changed 5 years ago by bleeter

  • Milestone changed from Patches Needing Review to Patches Needing Improvement

Replying to BigBrownChunx:

I would personally reject this patch:

Same, for precisely the reasons cited by you.

comment:9 follow-up: Changed 5 years ago by rlaager

I don't know about that. The change to libpurple/media.c seems like it could be accepted immediately, as it seems like a trivial cleanup unrelated to farsight->farstream.

The configure test could support both with minor tweaks. (The farsight check could set FARSTREAM_CFLAGS, to avoid extra changes to the Makefile.am.)

The pile of #ifdefs is probably still less code than duplicating libpurple/media/backend-fs2.c.

I'm not sure why we have a dependency on farsight in purple_media_manager_get_pipeline(), but I don't really understand the vv code because I haven't looked at it much. I agree it'd be nice to get rid of that dependency, if possible.

comment:10 in reply to: ↑ 9 Changed 5 years ago by bleeter

Replying to rlaager:

I don't know about that.

I can see where you're coming from. I should be a bit more clear in my comment. There is some need for improvement and I do not mean the patch should be rejected outright. I hope the Milestone change I set more correctly indicated this than my actual comment. Apologies for any confusion caused.

Changed 5 years ago by rlaager

Supports farsight and farstream

comment:11 Changed 5 years ago by rlaager

ocrete: Can you test the patch I just attached, with farsight and with farstream? I'm not in a great position to test beyond just compiling it with farstream.

comment:12 Changed 5 years ago by ocrete

FS_ERROR_NO_CODECS_LEFT should never happen in that context. Only from fs_element_set_codec_preferences() I believe.

The defaults properties and FsElementAddedNotifier? exist in farsight2 since 0.0.24.

This bit should also be used with farsight2. There are other transmitters in recent farsight2 version.

if (is_nice
!strcmp(transmitter, "rawudp"))

The rest looks good.

comment:13 Changed 5 years ago by rlaager

The NO_CODECS_LEFT code isn't going to hurt anything, so if you're not positive, I'll just leave it. It isn't used on farstream and it'll get removed when we drop support for farsight2 at some point.

We definitely need to support versions older than 0.0.24. As far as I can tell, farsight2 doesn't have a version check macro (e.g. FARSIGHT_CHECK_VERSION). Nor does farstream. That makes it more difficult for us to support optional newer APIs, as we have to write configure checks for them.

This might be something you want to add to farstream. Since we can't rely on it always being present, we'll still need to do #if defined(FARSTREAM_CHECK_VERSION) && FARSTREAM_CHECK_VERSION(x,y) instead of just #if FARSTREAM_CHECK_VERSION(x,y), but that's not too bad.

You're saying this part of the patch, which checks for "nice" or "rawudp"...

#ifdef HAVE_FARSIGHT

if (is_nice
!strcmp(transmitter, "rawudp"))

#else

if (!!strcmp(transmitter, "multicast"))

#endif

...should use the following on both farsight and farstream, which just excludes "multicast"? if (!!strcmp(transmitter, "multicast"))

comment:14 Changed 5 years ago by ocrete

I'm not too big in supporting all versions since the beginning of time, but that seems to be a fashionable thing in the pidgin-land..

I'm saying: if (is_nice
!strcmp(transmitter, "rawudp")) should be used on farsight2 too.

comment:15 Changed 5 years ago by rlaager

if (is_nice
!strcmp(transmitter, "rawudp")) *is* used on farsight2, both before and after this patch. It is *not* used with farstream; farstream uses if (!!strcmp(transmitter, "multicast")) per your patch. So what are you saying again?

comment:16 Changed 5 years ago by ocrete

Sorry, I was confused... we shoudl use "!!multicast" everywhere, it's the only place where "no-rtcp-timeout" != 0 really makes sense. Sorry for the confusion

comment:18 Changed 5 years ago by olivier.crete@…

(In 1471a6c23306bd59c5b814cbaf05a2383f35b398):
Remove an unused variable

Refs #14936

comment:17 Changed 5 years ago by rlaager

  • Resolution set to fixed
  • Status changed from new to closed

Thanks for the changes!

comment:19 Changed 5 years ago by olivier.crete@…

(In c67a060046437347c5cc4f58bdff008d7b8c4496):
Set no-rtcp-timeout on more transmitters in farsight2 as well

Oliver says there are new transmitters in newer versions of farsight2.

Refs #14936

comment:20 Changed 5 years ago by rlaager

I also accepted the bulk of the changes, of course. I just forgot to add a "Refs 14936" on that commit.

comment:21 Changed 5 years ago by Robby

  • Milestone changed from Patches Needing Improvement to 3.0.0

All good now, right?

comment:22 Changed 5 years ago by rlaager

  • Milestone changed from 3.0.0 to 2.10.4
Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!