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.

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.

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.

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.

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"))

!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?

Download in other formats:

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!