Add call events and request of turn servers

[mariogrip]

This adds support for call events and request of turn servers. Needed for call support in uMatriks

This handles the WebRTC negotiation and call signaling between clients.

This follows https://matrix.org/docs/spec/client_server/r0.2.0.html#voice-over-ip

The WebRTC part not handled here, that must be handled by client. Example of this implemented will be in uMatriks once pushed.

[KitsuneRal]

Overall, I can't express my appreciation that you've taken on this task. VoIP has been a very much requested feature, and the Librem 5 phone might very much use this code as well (if they choose to use libqmatrixclient, that is...). So even though I have a lot of comments on your code, please accept my huge thanks for your effort, to begin with. Now to the critique :)

[KitsuneRal]

This blank line looks like a left-over from something; please drop.

[KitsuneRal]

I'm not sure how much it's worth it to store QJsonObject here. I'd probably simply store the SDP text instead (we only have type=="offer" anyway). In general, we try to not expose JSON to the clients because both the JSON processing engine and even the transfer protocol (JSON, that is) may, even though in theory yet, change for something else (Qt JSON -> RapidJson, or JSON -> COAP, e.g.).

[KitsuneRal]

I think it makes sense to use another bit (pick your favourite from the higher byte) to indicate that an event is a call event. Doesn't change much so far; but clients might want to check if an event is a call event to adorn it with a relevant icon, e.g. See the code for Event::isStateEvent(). On a related note, you should move the new events to the room non-state events block (before RoomStateEventBase).

[KitsuneRal]

Same as the comment above, please unroll _turnObject into actual data stored in it.

[mariogrip]

This also gets passed directly to the "javascript" side, thats why i store it as a jsonobj. Does it make sense to spit it up to then assemble it again?

[KitsuneRal]

Nope, in that case let it stay as it is now; we can parse it in another accessor when/if there comes the need.

[KitsuneRal]

The following two points apply to all the Room methods you're introducing:

• You're replicating Room::postMessage() here. Consider just passing a created event to postMessage instead (see line 611 above).
• As also mentioned below, Room::inviteCall() must only actually put a call event if the room has exactly two members. Specific way to do validation is up to you; I prefer clients to apply such validations on their side so I'd put a Q_ASSERT here (potentially blowing up a misbehaving client in debug mode). At the very least, there should be a qCWarning(MAIN) instead of sending a message event when memberCount() != 2.

[mariogrip]

The reason why i created separated function is so they can be called directly from QML, but i can move it to the client side (uMatriks) if this is not the correct place.

[KitsuneRal]

I meant a bit different thing: the functions are in place; but their bodies can be made one-liners calling postMessage instead of the current two-liners.

[KitsuneRal]

Scratch that, I understood the issue. Keep it like now.

[KitsuneRal]

This is not the right place to catch m.call events - The Spec defines them as message events, so you have to put this code in doAddNewMessages() and doAddHistoricalMessages() instead of processStateEvents(). What's worse, in order for clients to not go through hoops to correctly deal with messages you have to do some extra checking:

• At least in doAddHistoricalMessages() you should make sure that clients are not notified about already finished calls (or at least are notified in a more intelligent way). Note that historical message events come in reverse, hangup first, and moreover you might have a hangup event in one chunk and invite/candidates events in another chunk. doAddNewMessages() may benefit from the same logic but since call events come in natural order in this case, the standard logic for a missed call on the client side will perfectly work.
• Since The Spec is very strict that m.call events MUST only be used in 1:1 rooms, it makes sense to only emit the signal if memberCount() == 2.

[KitsuneRal]

One more thing here: by notifying about an event of one of several types with one common signal, you effectively mandate clients to put a switch statement in their code, to find out what exactly happened. Since that signal carries a RoomEvent, clients don't have even a single chance to process the call event uniformly (RoomEvent is too generic a type). So I'd really consider separating callEvent() into separate signals, maaybe only aggregating CallInvite and CallCandidates (and I'm not sure even that is proper).

[mariogrip]

The reason why i choose room to place this event, is so client don't need to listen to each room to be able to catch incoming calls. This is even more useful if clients want add a "listen for call" background services for mobile devices.

But I do agree on 1:1 room check, I have done this on the client side for uMatriks but it does make sense to add this here, I'll add that tomorrow.

[KitsuneRal]

I have no problem with Room being in the signal (although, unfortunately, you will anyway either have to put this signal to Connection - and listen for this signal from each connection object - or listen to Room::callEvent() from each room). I understand that calling is kind of a special case because it consumes much more attention of the end users (normally - all attention); so probably it really makes sense to make a separate signal on at least the level of Connection, or even setup a separate callback (although we mostly use direct connections that are only marginally heavier than callbacks).
My point in the original comment, though, was about passing a generic RoomEvent through a bottleneck of a single signal signature. When I get that RoomEvent in my slot on the client side, too many things have to be clarified before I can do anything sensible. Is that RoomEvent a call event at all (should I put a Q_ASSERT(evt->isCallEvent()) in each client?)? Should I even care about a hangup event if I don't have active calls? Why should I care about (probably invalid) call answer event if I didn't place a call invite (note that I might have several devices and the call answer will arrive to all of them)? That's why I'd prefer to have 4 signals instead of a single one; and only m.call.invite is worth that "globalization" I mentioned in the first paragraph.

[KitsuneRal]

As mentioned above, please try to not use QJsonArray unless you actually intend to pass some opaque JSON object. In this particular case, CS API describes the element contents explicitly so just pass a QVector of supplementary "call candidate" structures. To save effort on converting a QVector to/from QJsonArray, you might consider using facility functions from converters.h.

[mariogrip]

Using QJsonArray makes it easier to pass to directly to WebRTC handler which is written JavaScript, since all WebRTC will probably be handled by a browser (qwebengine or oxide) this makes sense.

[KitsuneRal]

Oh, I see, didn't know about that. Then sure, please keep it as is.

[KitsuneRal]

Would be great to see doc comments next to the declarations, by the way :) I'm not very consistent at putting them around, but 0.2 has an explicit goal about documenting the whole public API of the library, so these comments will have to arrive sooner or later.

[KitsuneRal]

const int&? Plain int will be more than enough :) The whole const-ref thing is a bit extraneous in case of Qt strings but we use it because it's more universal and also it still saves on refcounting. For int this just doesn't apply.

[mariogrip]

Yeah, i'm getting to used to references :P

[KitsuneRal]

Looking at those callIds in each call, I start to think that we might want to have a separate "1:1 call" class with its own lifecycle. The "call candidate" structure definition mentioned above would land inside that class as well.

[KitsuneRal]

If you use 0x2100 for calls, push Type::Reserved to 0x4000 or something like that. Or instead, something like 0x1100 would fit much better (because 0x2100 does not have the 0x1000 "room event" bit - we don't have any trouble with it right now because we don't have Event::isRoomEvent() but who knows what will come).

[KitsuneRal]

You don't need _answer anymore, do you?

[KitsuneRal]

Also note that Travis CI is unhappy now - as disappointing as it is, but Qt 5.2.1 doesn't have a QJsonObject constructor from a brace list initializer. You have to use QJsonObject::insert(), or factor out the code of BaseJob::Data() that augments that omission, or wait until December when I'm going to officially drop older Debian support and bump the required Qt version to 5.6.

[KitsuneRal]

That won't fly either. You now have CallInvite coinciding with RoomEncryptedMessage (so far unused, and that's why you're lucky to not see any issues from that). Please use 0x1100, as I advised; or 0x1200, or 0x1400. You can even get into bits 5-8 (like in 0x1080) but please make call events into their separate block.

[KitsuneRal]

QStringLiteral is better in such cases.

[mariogrip]

There we go, sorry for the confusion. Would it be ok that i can move the call things to it's own class (and add doc comments) later on? That way i can push my changes for uMatriks, I also have started briefly on a low power "mode" that only listed for the bare minimal to get push notifications (for both call and messages)

[mariogrip]

Here is the implementation on the client side mariogrip/uMatriks@a1f2cff it's still little of a mess, but it works really well! :)

[KitsuneRal]

I'm very much puzzled why you treat call events as state events. Having not tried it from the working code, I wonder if Riot also sends those in state batch rather than timeline batch. Can you confirm?

[KitsuneRal]

I mean, if Riot does send those as state events, then the spec is damn lying that those are message events.

As for the code in uMatriks - wow, that's a lot. I was thinking to borrow a thing or two for Quaternion but it's apparently not easy. Kudos.

[KitsuneRal]

Aside from state vs. timeline thing, I'm fine to move this forward and refine things later (even though it means breaking the API that's being put now). Speaking of refinement...
For the record (and I might move this out to a separate issue), I scrolled to the newEvent() function in uMatriks QML, and it looks like you really have to write quite a bit of boilerplate code in that function and in RoomListModel::callEventChanged() just because you are emitting one signal instead of 4. Otherwise you could divert those room signals through RoomListModel straight to QML, without necessity to do all that stuff in RoomListModel::callEventChanged(). The way you use the event JSON is very neat but I'd rather make call events Q_GADGETs and those fields in event objects accessible through Q_PROPERTYs - this saves you from overheads on reconstructing JSON and then parsing it again in JavaScript engine, while still providing with safeguards on C++ side. Or am I missing tiny details?

[KitsuneRal]

Ok, I at least understood the reason why putting this code into processStateEvents works. This code won't work, however, in one (admittedly, rather extreme) case - that is when a call arrives in historical events and is still up. I think I'll put this as a separate issue in the lib backlog because making historical events processed properly is quite a bit of work. So now I ask of 3 things (one of them optional):

• I urge you to split the callEvent() signal; this alters the API considerably, so I'd rather make it right from the onset;
• I'd rather move the current code from processStateEvents to doAddNewMessages anyway (but I can do that after the merge if you wish);
• Please change from using event JSON to event Q_PROPERTYs, as described in the previous comment; this only slightly changes the API (your QML should run unchanged, if I don't mistake) so this can also be done on my side after the merge (although making sure that it doesn't break uMatriks lays on uMatriks community's shoulders - but I'll do my best not to break things).