Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port QXmppCallManager to use GStreamer #207

Merged
merged 6 commits into from Mar 16, 2020
Merged

Conversation

olesalscheider
Copy link
Contributor

@olesalscheider olesalscheider commented May 17, 2019

This PR ports QXmppCallManager to use QtGStreamer.
The disadvantage is that it adds QtGStreamer as a dependency. But it also brings the following advantages:

  • Nearly 4000 lines less code (-> less maintenance burden)
  • We can easily support all codecs supported by GStreamer, also in the future
  • The client application has easy access to GStreamer elements, e. g.
    o Widgets for Qt and QML
    o Different audio and video sources including support for screen casting
    o A lot of filters like echo cancelation and noise reduction (cf. webrtcdsp)
  • We can easily support secure communication using SRTP (will be added in another patch)

What do you think? Before I continue to clean up this patch I would like to hear your oppinion.

@lnjX
Copy link
Member

lnjX commented May 18, 2019

Just a question: Would it be possible to do this with QtMultimedia?

@olesalscheider
Copy link
Contributor Author

QtMultimedia provides device access and encoders / decoders. But it does not have any of the RTP handling functionality (payloader, depayloader, RTP session management, jitter buffers, encryption, ...) and less filters. I assume we could use QtMultimedia and write these other parts on our own / use some of the code that we already have.

But I don't see the advantages. GStreamer seems to be available on the important platforms (Linux, Windows, OS X, Android, iOS and more) and it has all that is needed for a good RTP experience.
Do you have a reason why you would prefer QtMultimedia?

@lnjX
Copy link
Member

lnjX commented May 18, 2019

Do you have a reason why you would prefer QtMultimedia?

AFAIK most Plasma Mobile/Ubuntu Touch (android) devices don't have hardware acceleration support with GStreamer, but with QtMultimedia. And also just because it's an official part of Qt (less problems with new dependencies).

@olesalscheider
Copy link
Contributor Author

I think we can get hardware acceleration on android by trying the gst-omx codecs before the regular ones. They use the OpenMAX API.

@lnjX
Copy link
Member

lnjX commented May 18, 2019

I think we can get hardware acceleration on android by trying the gst-omx codecs before the regular ones. They use the OpenMAX API.

I'm not talking about android, I'm talking about Halium/libhybris on an android device.
However this is just a nice-to-have and isn't that important (at least for QXmpp).

@olesalscheider
Copy link
Contributor Author

Ah, I see. Well, at least SailfishOS also has hardware acceleration with gst-omx: https://github.com/sailfishos/gst-omx
I'm not sure about the other libhybris based mobile OS though. Sailfish has a few patches for gst-omx and I don't know if they are upstream.

@jbruechert
Copy link
Contributor

QtMultimedia uses gstreamer itself (at least on some platforms), and the accelerated decoding on Plasma Mobile is done as a gstreamer plugin which is then used by QtMultimedia, so using gstreamer here is perfectly fine.

@codecov
Copy link

codecov bot commented Jun 14, 2019

Codecov Report

Merging #207 into master will increase coverage by 3.08%.
The diff coverage is 61.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
+ Coverage   70.36%   73.44%   +3.08%     
==========================================
  Files         203      197       -6     
  Lines       17874    15692    -2182     
==========================================
- Hits        12577    11525    -1052     
+ Misses       5297     4167    -1130
Impacted Files Coverage Δ
src/base/QXmppStun.h 100% <ø> (ø) ⬆️
src/client/QXmppCall_p.h 100% <100%> (ø)
src/client/QXmppCallStream.h 100% <100%> (ø)
...ests/qxmppiceconnection/tst_qxmppiceconnection.cpp 100% <100%> (ø) ⬆️
src/client/QXmppCall.h 100% <100%> (ø)
src/client/QXmppCallManager_p.h 100% <100%> (ø)
src/client/QXmppCallStream.cpp 49.7% <49.7%> (ø)
src/client/QXmppCallManager.cpp 64.34% <60%> (+0.43%) ⬆️
src/client/QXmppCall.cpp 65.04% <65.04%> (ø)
src/base/QXmppStun.cpp 62.7% <88.88%> (-0.48%) ⬇️
... and 109 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46c5e6b...f761bd6. Read the comment docs.

@olesalscheider olesalscheider changed the title WIP RFC Port QXmppCallManager to use GStreamer Port QXmppCallManager to use GStreamer Jun 14, 2019
@olesalscheider
Copy link
Contributor Author

The build fails because we don't install qt-gstreamer with homebrew.
I think only gstreamer (but not qt-gstreamer) is available there. But I don't know anything about MacOS - are there other sources for packages? Or would we have to compile it from source?

@lnjX
Copy link
Member

lnjX commented Oct 14, 2019

@olesalscheider The wiki page and README of qt-gstreamer say it's unmaintained and out of date:
https://gstreamer.freedesktop.org/bindings/qt.html
https://github.com/GStreamer/qt-gstreamer/blob/master/README

Is this whole thing a good idea then? How hard would it be to use gstreamer directly or to use the C++ bindings instead?

@olesalscheider
Copy link
Contributor Author

Oh, I was not aware of that. Yes, it seems like using the qt bindings is not a good idea then.
Using gstreamer directly should not be too hard and I would be willing to port the code there if we decide that using gstreamer is a good idea.

@lnjX
Copy link
Member

lnjX commented Oct 15, 2019

@olesalscheider The listed advantages of gstreamer convince me and since it will be an optional dependency (like the codecs before) I see absolutely no problem. :)

@olesalscheider
Copy link
Contributor Author

I ported this to the gstreamer C API. It needs some real-world testing though.
Also now there are a lot of conflicts with the master branch.

Copy link
Member

@lnjX lnjX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only had a quick look, but looks good so far. Splitting up the private headers in some places is also a good idea.

README.md Outdated Show resolved Hide resolved
src/base/QXmppRtcpPacket.h Show resolved Hide resolved
src/base/QXmppRtpChannel.h Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/client/QXmppCall.h Outdated Show resolved Hide resolved
src/client/QXmppCallStream.cpp Outdated Show resolved Hide resolved
QList<GstCodec> videoCodecs = {
// vp9enc and x265enc seems to be very slow. Don't offer it for now.
//{.pt = 101, .name = "H265", .channels = 1, .clockrate = 90000, .gstPay = "rtph265pay", .gstDepay = "rtph265depay", .gstEnc = "x265enc", .gstDec = "avdec_h265", .encProps = {{"tune", 4}, {"speed-preset", 1}, {"bitrate", 256}}},
//{.pt = 100, .name = "VP9", .channels = 1, .clockrate = 90000, .gstPay = "rtpvp9pay", .gstDepay = "rtpvp9depay", .gstEnc = "vp9enc", .gstDec = "vp9dec", .encProps = {{"deadline", 20000}, {"target-bitrate", 256000}}},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The codecs are just very complex. I think we should generally support H.265/VP9, maybe just don't prefer them over H.264.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VP9 is basically unusable, even on very fast computers. But I added them for now with a lower priority.

We can also add the hardware encoders / decoders that are supported by GStreamer to the list. But I think there are none so far for these new codecs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardware encoders would also be nice for H.264/VP8 - They could be relevant on mobile devices?
(However this isn't that important for now.)

src/client/QXmppCall_p.h Outdated Show resolved Hide resolved
src/client/QXmppCallStream_p.h Show resolved Hide resolved
src/client/QXmppCallStream.h Show resolved Hide resolved
src/client/QXmppCall_p.h Outdated Show resolved Hide resolved
@lnjX
Copy link
Member

lnjX commented Nov 27, 2019

There are still some things that aren't tested, many qFatal calls (I'm not sure how you can test those?), but also other code. If it's not too hard, I'd be glad if you could add a tests for those stuff. Here's the coverage diff: https://codecov.io/gh/qxmpp-project/qxmpp/pull/207/diff

Apart from that I think we can merge it.

src/client/QXmppCallStream.h Outdated Show resolved Hide resolved
src/client/QXmppCall.cpp Outdated Show resolved Hide resolved
@olesalscheider
Copy link
Contributor Author

I tested this over the internet with a friend and it worked quite well and quality was good.

More real-world testing with slow hardware or a slow internet connection would be good to see how quality, packet drop and latency are there. Depending on that we can tune the codec parameters a bit and implement the bitrate controller. But maybe that can be done in a second step with some feedback.

@lnjX
Copy link
Member

lnjX commented Mar 16, 2020

Alright, I think we're ready to go then. Do you want me to merge your commits as they are or do you want to squash some of them before?

@olesalscheider
Copy link
Contributor Author

I think it would be good to keep them separate. The first is huge already and the following ones are nicely separated and touch other parts of the code.

@lnjX lnjX merged commit bb47e1b into qxmpp-project:master Mar 16, 2020
@jbruechert
Copy link
Contributor

Hi @olesalscheider,
do you have an example on how to use the new QXmppCallStream API?
I'm struggling with the GstPad callback functions currently. As far as I understand I can only use static functions there, but I also need an object to distinguish between the video and the audio stream. Once I understand how to use them I'd look into writing a QIODevice wrapper again for easier usage with QtMultimedia.

@olesalscheider
Copy link
Contributor Author

You can find a small test program that I wrote here:
https://github.com/olesalscheider/qxmpp/blob/gsttest/voiptest/main.cpp#L11

But I think I understand what your problem is. Should we add an opaque pointer to the callbacks?

@jbruechert
Copy link
Contributor

Thanks for your response. What made the API hard to use for me was that I was trying to use QtMultimedia instead of gstreamer in the client.
Using QAudioInput for this seems more natural for a Qt API, so I think we should ideally have a QAudioInput API for it. Since I now know I can use lambdas for the callbacks (which I tried, but for some reason failed earlier), maybe I can actually implement that myself.

@olesalscheider
Copy link
Contributor Author

Having the GStreamer API also has advantages, though. You can add a lot more elements to the pipeline, e.g. for echo cancellation or other post processing. There are also elements for QML or Qt Widgets to display the video, and elements for screen sharing.

Lambdas work and you can capture the objects you need. The only question is if that is a good design, or if we should add the user object pointer to the API before it is released.

@lnjX lnjX added the section:client QXmpp Client label Mar 25, 2020
@lnjX
Copy link
Member

lnjX commented Mar 25, 2020

Having the GStreamer API also has advantages, though. You can add a lot more elements to the pipeline, e.g. for echo cancellation or other post processing. There are also elements for QML or Qt Widgets to display the video, and elements for screen sharing.

Maybe it's possible to provide access to the gstreamer pipeline, but still to wrap the output of it into a QAudioOutput and to accept a QAudioInput. If using the gstreamer APIs works fine, we can maybe also just keep that, though.

For the QML elements you probably refer to QtGstreamer, but since it's unmaintained I don't think we should recommend using that.

You can find a small test program that I wrote here:
https://github.com/olesalscheider/qxmpp/blob/gsttest/voiptest/main.cpp#L11

I think we should also add your example to QXmpp. I think that's very helpful as there's no detailed description in the documentation on how to use it.

A note about STUN:
Some XMPP servers already provide STUN servers - Do you know whether we can automatically recognize and use them?

@olesalscheider
Copy link
Contributor Author

No, there are also Qt elements (e.g. qmlglsink) in gst-plugins-good:
https://github.com/GStreamer/gst-plugins-good/tree/master/ext/qt

But so far I haven't used them, though.

We can add my example but it needs to be cleaned up a bit, I guess.

About STUN: I think XEP-0215 is not implemented. How common is it for servers to support this?

@lnjX
Copy link
Member

lnjX commented Mar 25, 2020

No, there are also Qt elements (e.g. qmlglsink) in gst-plugins-good:
https://github.com/GStreamer/gst-plugins-good/tree/master/ext/qt

Ah, thanks, that's good to know.

About STUN: I think XEP-0215 is not implemented. How common is it for servers to support this?

There's a module for prosody, but it's probably not used by many servers. I know that ejabberd has a STUN server enabled by default, but ejabberd (at least the open-source edition) is not supporting XEP-0215. There's also https://wiki.xmpp.org/web/SRV_Records which recommends setting up SRV records for STUN/TURN. Maybe this is also an option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
section:client QXmpp Client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants