Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#16315 closed patch (fixed)

New API for private Raw and RTP data streams

Reported by: kakaroto Owned by:
Milestone: 2.10.12 Component: libpurple
Version: 3.0.0hg Keywords:
Cc: dwmw2, haakon, niklas.andersson@…, salinasv

Description

Hi, I've added a new API to libpurple to add support for private PurpleMedia? that plugins can use as well as new APIs for sending/receiving raw and RTP data. I have sent this proposal to the pidgin-devel mailing list in June (https://pidgin.im/pipermail/devel/2014-June/023514.html) and I've now finished the implementation. You will find all the commits here : https://github.com/tieto/pidgin/commits/appdata There are small differences between the implementation and the proposal. Mainly, the callbacks that get register will signal readable or writable, they won't send the actual data received. Also, I've added a receive_application_data API in order to read. When the readable callback gets called, the plugin must call receive_application_data until no data is left. You can see how the API can be used by a plugin in these commits to SIPE : https://github.com/tieto/sipe/commit/bae7b738dfe31baafbe50200f5895fc432fc170a https://github.com/tieto/sipe/commit/76a893337b4484d8c0736a8b651ccaa60bfee9e8 Note that the stream type is PURPLE_MEDIA_APPLICATION which is not shown in those commits, and the media is created using create_private_media so the UI doesn't get notified and the file transfer can use purple_xfer to notify the UI instead and the private PurpleMedia? internally to transfer the data with the peer.

I also fixed a couple of bugs I found in pidgin, and added a signal to PurpleMedia? to notify when a candidate pair has been established, and added a purple_media_set_send_rtcp_mux API to set the rtcp-mux option on the FsStream?.

Please review and merge these changes. Thank you.

Change History (16)

comment:1 Changed 5 years ago by Robby

  • Milestone set to Patches Needing Review

comment:2 Changed 5 years ago by Youness Alaoui <kakaroto@…>

(In [2b41ba1fde8a]):
Fix send-video enum typo

Refs #16315

comment:3 Changed 5 years ago by Youness Alaoui <kakaroto@…>

(In [b52be4fef1de]):
Fix gstreamer elements references In backend-fs2, the create_src will unref the src after it's done with it, if we simply return the created source, it will segfault. The issue never happened before because every source so far also has the UNIQUE flag, which causes it to go in a different branch of the code which does ref the element and add it to the bin.

Refs #16315

comment:4 Changed 5 years ago by Jakub Adam <jakub.adam@…>

(In [6a8f7f4ec06f]):
Fix bug in purple_xfer_read_file()

This was introduced in 499ffff1e77c and for purple_xfer_write_file() fixed in 2b7c4c034594. File reading was for sure also affected since the subsequent calculation of sent bytes and function's return value are completely wrong.

Refs #16315

comment:5 Changed 5 years ago by datallah

  • Cc haakon niklas.andersson@… added

Thanks for your efforts. I got around to taking a brief look at this (even though I'm not very knowledgeable about the existing media stuff).

I went ahead and committed 3 of the bugfixes - thanks for the granular commits, those make this process a lot easier.

The development was done against the default branch, which will become the 3.0.0 version of libpurple/pidgin.

I was able to backport it to the release-2.x.y branch without any significant conflicts, and it does look like it is additive and from that perspective could go into a 2.11.0 release.

I haven't fully finished reviewing the changes yet (and I haven't tried to build it yet), but I did notice an issue in that it depends on the glib main loop, which is problematic - pidgin uses the glib main loop, but libpurple itself has that abstracted out so that other applications can use their own native loop. See https://github.com/tieto/pidgin/commit/0d1866b868ee9882322644d1b6b088b2077fc2aa#diff-62d7c3b13ece872aa547d20dbd8e44c0R695 for example.

I'll try to get the rest of the review done before too long - hopefully you can fix the main loop issue in the meantime.

comment:6 follow-up: Changed 5 years ago by kakaroto

Thanks for the review!

I didn't know about the main loop abstraction, thanks for pointing it out.

By the way, there were a couple of commits recently on the 'launchpad' branch (instead of the 'appdata' branch) that fixes some deadlocks in the appdata code. https://github.com/tieto/pidgin/commits/launchpad. I will cherry pick them soon into the appdata branch as well.

As for the mainloop issue, the reason for that is we can get those signals from the gstreamer streaming thread and we need to send the signals from the libpurple thread. Should I just replace all of that with purple_timeout_add(0, func, data) ? It should be simple enough, I'll let you know when it's all done.

Thanks again for the review.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 5 years ago by datallah

Replying to kakaroto:

Thanks for the review!

I didn't know about the main loop abstraction, thanks for pointing it out.

By the way, there were a couple of commits recently on the 'launchpad' branch (instead of the 'appdata' branch) that fixes some deadlocks in the appdata code. https://github.com/tieto/pidgin/commits/launchpad. I will cherry pick them soon into the appdata branch as well.

As for the mainloop issue, the reason for that is we can get those signals from the gstreamer streaming thread and we need to send the signals from the libpurple thread. Should I just replace all of that with purple_timeout_add(0, func, data) ? It should be simple enough, I'll let you know when it's all done.

Yeah, it's most appropriate to use purple_timeout_add(0, ...) for that type of use.

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

Replying to datallah:

Yeah, it's most appropriate to use purple_timeout_add(0, ...) for that type of use.

Oups, I just realized I forgot to reply. I've fixed the main loop issue and pushed it to the repository. Thanks.

comment:9 Changed 4 years ago by kakaroto

Hi @datallah,

Is there any update on this, did you get time to review it some more and is there anything still needed before it can be merged ?

Thanks!

comment:10 Changed 4 years ago by dwmw2

The first patch in the sequence, adding MS-TURN TCP support, unconditionally uses new enum values (S_NETWORK_PROTOCOL_TCP_ACTIVE) which were added in farstream 0.2.5. This causes a build failure if we build with 0.2.4. It needs to be either made conditional, or we need to *require* farstream >= 0.2.5.

I note that a later patch checks for FS_MEDIA_TYPE_APPLICATION which was also added in farstream 0.2.5, so if we *do* change the requirement rather than making the TCP support conditional, that later check would now be redundant.

comment:11 Changed 4 years ago by dwmw2

OK, I've taken the contents of that branch and reworked it into a committable series of discrete patches. I have committed everything except (what is now) the last patch to both master and the 2.x.y branch.

The final outstanding patch is the one which adds the PURPLE_MEDIA_APPLICATION type, currently http://git.infradead.org/users/dwmw2/pidgin-collab.git/commitdiff/82ff350807

I haven't committed that last one because I don't like it. I don't like the fact that we unconditionally force the use of an appsrc/appsink and wrap our own APIs around it. In testing, I have observed serious UI misbehaviour because of the (mis)use of these new APIs in the SIPE plugin, doing blocking writes to the media stream from the main thread.

Surely it would be much simpler just to allow the caller to provide a GstElement of their choosing and let the pipeline run naturally? In the SIPE case that could be a filesrc or filesink for file transfers, and would it be fdsrc/fdsink for the screen sharing talking freerdp via a UNIX socket?

Of course, if the caller wanted to use the appsrc/appsink they could still do so, and we wouldn't need to wrap its API with our own.

comment:12 Changed 4 years ago by kakaroto

Hi David,

Thanks for all the work you've done with the patches.

One reason for using appsrc/appsink is because we can't let the plugin provide us with a GstElement? since the whole API is supposed to be independent of gstreamer/farstream as there are many abstraction layers there. Exposing the GstElement? kind of defeats the purpose. While there is the media-gst.h, I don't think having to use it should be needed in order to use the MEDIA_APPLICATION.

As for the blocking calls, they aren't actually blocking unless you specify it. Both the send and receive APIs have a blocking argument which if set to FALSE will make it non-blocking, otherwise, after it sends the data to the pipeline, it will wait until the data is flushed before returning. If you see them as blocking writes, it's possible that sipe is doing it that way. While data does travel through the main thread, it's only when it goes from/to pidgin, but otherwise, it gets sent to appsrc/appsink which have their own threads and all the processing is in that thread. It's possible something else needs to be optimized to avoid the issue you're experiencing.

As for using filesrc/fdsrc, I'm not sure it's a good idea, mostly because I don't think there's any protocol which takes raw data this way, it will always be encapsulated somehow, so you will always need to modify the data in one way or another. As you can see here : https://github.com/tieto/sipe/commit/bae7b738dfe31baafbe50200f5895fc432fc170a for file transfers, you still need to parse the packets to see if it's a start-of-file, data-chunk, or end-of-file packet, so even with filesrc/fdsrc or whatever, you still need data to go through the application in order to be parsed, which means you need appsrc/apssink. It might work with fdsrc/fdsink for RDP though since I don't think there is any processing other than what freeRDP does. I know you weren't saying it should be replaced with filesrc/filesink, but I'm just saying that removing the appsrc/appsink system will just cause it to be moved into sipe for file transfers and you may have the exact same issues which are making you disliking it at the moment.

It looks like some profiling may be in order...

Thanks.

comment:13 Changed 4 years ago by dwmw2

Thank you for the coherent response. Although I think it's mostly a fiction that we actually manage to hide the GStreamer dependency behind our own APIs, it does make a lot of sense. Committed to 3.x and 2.x.

The problem with the UI blocking is in the SIPE backend. As soon as there are incoming data available on the UDP socket from freerdp-shadow, we do a *blocking* write to the media stream. From the main thread. You should be waiting until it's writeable. The patch I've committed does provide that interface, but you don't use it in SIPE.

Likewise for outgoing file transfer you use g_idla_add() to spew data up the media stream from your send_file_chunk() function, but that's *still* blocking I/O on the main thread and it still hurts. But again, that isn't a limitation of the patch I just committed, it's all yours in SIPE :)

comment:14 Changed 4 years ago by Youness Alaoui <kakaroto@…>

  • Milestone changed from Patches Needing Review to 3.0.0
  • Resolution set to fixed
  • Status changed from new to closed

(In [9b4a94c113b4]):
Add application media type and APIs

Fixes #16315

comment:15 Changed 4 years ago by Youness Alaoui <kakaroto@…>

  • Milestone changed from 3.0.0 to 2.10.12

(In [4fe1034f3dce]):
Add application media type and APIs

Fixes #16315

comment:16 Changed 4 years ago by kakaroto

Thanks David!

I was afraid that I wasn't being coherent, but I'm glad it all made sense to you :) I know the abstraction layer is not really being useful since there's only FS2 as backend and the API doesn't really hide that...

Thanks for reviewing/fixing/merging these patches! Greatly appreciated.

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!