Fetch MUC participant avatars

[lissine0]

Add support for fetching MUC participant avatars, using vcard-temp (XEP-0153)
And display the avatars in the ChatView, and in ChannelMemberList.

Fixes #699

[lissine0]

Here are some points for discussion:

• When first joining a room, mam queries don't work until one of the following two things happens:
  • the avatars finish being fetched
  • 60 seconds have passed. At which point the following error is shown: Failed to query new messages for Group/ Channel (stanzaid) room@conference.domain.tld: remote-server-timeout (No response in 60 seconds).
    Avatar fetching also stops at this point, or slows down significantly.
• Currently, if some participants are present in multiple rooms, their avatars are fetched once for every room.
  It would be nice to optimize this case: before sending the iq, we could check if the avatar hash is known in other rooms. If so, we simply copy the file. otherwise we proceed with sending the iq.
• The avatar file storage on disk does not distinguish between different accounts.
  i.e. if two accounts are in the same room, they share the same avatar files.
  (the avatars are stored in documents directory/muc_participant_avatars/room/nick)
  I think this can be extended to the database as well, by dropping the account_id column from the muc_participants table.

• Minor: there's a comment that says //hashes should never be nil, should we enforce that on the DB side? (by making the column NOT NULL, and setting an empty string as a default value)
• ejabberd has a custom muc configuration form field that lets you disable iqs in the room:

<field var="allow_query_users" type="boolean" label="Allow users to query other users">
    <value>1</value>
</field>

Should we skip fetching avatars if we detect that this config option is set to false? If we don't, then we'll be sending a pointless iq every time we get a presence with a hash in it, which will return an error every time.
But this configuration is rarely used I think.

[tmolitor-stud-tu]

See my answers inline.

• When first joining a room, mam queries don't work until one of the following two things happens:

  • the avatars finish being fetched
  • 60 seconds have passed. At which point the following error is shown: Failed to query new messages for Group/ Channel (stanzaid) room@conference.domain.tld: remote-server-timeout (No response in 60 seconds).
    Avatar fetching also stops at this point, or slows down significantly.

Why do avatar fetches block mam? I don't think this is something we want.

• Currently, if some participants are present in multiple rooms, their avatars are fetched once for every room.
  It would be nice to optimize this case: before sending the iq, we could check if the avatar hash is known in other rooms. If so, we simply copy the file. otherwise we proceed with sending the iq.

Well, since different mucs could have different policies for direct communication, I think that optimization would cause more confusion than do good (its hard to understand for ordinary users, why a muc shows only avatars of some users and not others).

• The avatar file storage on disk does not distinguish between different accounts.
  i.e. if two accounts are in the same room, they share the same avatar files.
  (the avatars are stored in documents directory/muc_participant_avatars/room/nick)
  I think this can be extended to the database as well, by dropping the account_id column from the muc_participants table.

No, it should even be the other way round: we need to differentiate by account. My own affiliation might influence, if I'm able to retrieve the avatar or even see the jid of participants. Since the avatar hash of every participant is stored in the db, using something like this: documents directory/muc_participant_avatars/accountId/roomJid/avatarHash

• Minor: there's a comment that says //hashes should never be nil, should we enforce that on the DB side? (by making the column NOT NULL, and setting an empty string as a default value)

No, no. That simply means that we should return an empty string rather than nil to indicate a missing hash. Having NULL in the database it still beneficial, I think. Only the getAvatarHashForContact:andAccount: should always return a string to ease the code using that return value (e.g. calling isEqualToString: on it etc.).

• ejabberd has a custom muc configuration form field that lets you disable iqs in the room:

<field var="allow_query_users" type="boolean" label="Allow users to query other users">
    <value>1</value>
</field>

Should we skip fetching avatars if we detect that this config option is set to false? If we don't, then we'll be sending a pointless iq every time we get a presence with a hash in it, which will return an error every time. But this configuration is rarely used I think.

Yes, I think that would be beneficial, even if that is rarely used.

[tmolitor-stud-tu]

I did only a quick scroll-through

[lissine0]

See my answers inline.

• When first joining a room, mam queries don't work until one of the following two things happens:

  • the avatars finish being fetched
  • 60 seconds have passed. At which point the following error is shown: Failed to query new messages for Group/ Channel (stanzaid) room@conference.domain.tld: remote-server-timeout (No response in 60 seconds).
    Avatar fetching also stops at this point, or slows down significantly.

Why do avatar fetches block mam? I don't think this is something we want.

This is a bug, that I was hoping you'd have some insights about. my guess is it's somehow related to the sendQueue having too many iqs, because the mam query seems to be added to the sendQueue after the avatar queries. I'll investigate the issue more.

• Currently, if some participants are present in multiple rooms, their avatars are fetched once for every room.
  It would be nice to optimize this case: before sending the iq, we could check if the avatar hash is known in other rooms. If so, we simply copy the file. otherwise we proceed with sending the iq.

Well, since different mucs could have different policies for direct communication, I think that optimization would cause more confusion than do good (its hard to understand for ordinary users, why a muc shows only avatars of some users and not others).

If by policies for direct communication you mean blocking iqs, my last suggestion already covers this point. we can detect that policy and avoid copying the avatar files for MUCs that block iqs

• The avatar file storage on disk does not distinguish between different accounts.
  i.e. if two accounts are in the same room, they share the same avatar files.
  (the avatars are stored in documents directory/muc_participant_avatars/room/nick)
  I think this can be extended to the database as well, by dropping the account_id column from the muc_participants table.

No, it should even be the other way round: we need to differentiate by account. My own affiliation might influence, if I'm able to retrieve the avatar or even see the jid of participants. Since the avatar hash of every participant is stored in the db, using something like this: documents directory/muc_participant_avatars/accountId/roomJid/avatarHash

I get that my affiliation influences if I can see the jid of participants. But I don't get how it influences if I'm able to retrieve avatars.

[tmolitor-stud-tu]

If by policies for direct communication you mean blocking iqs, my last suggestion already covers this point. we can detect that policy and avoid copying the avatar files for MUCs that block iqs

yes and no. allowing iqs between participants isn't mandated by the spec and that ejabberd exposes the fact that it blocks them is entirely an implementation detail. other servers might block iqs but not expose it as such. especially bridges to other chat networks like biboumi.

--> you can't rely on this field to detect this.

[tmolitor-stud-tu]

I get that my affiliation influences if I can see the jid of participants. But I don't get how it influences if I'm able to retrieve avatars.

Because servers could choose to allow moderators/admins to send iqs to participants, but not ordinary users. Or allow it for members, but not for participants having no affiliation.

[lissine0]

He also said, that something must be wrong if we are hitting the ratelimit of an xmpp server and I tend to agree:
Could it be, that it isn't the server's ratelimit but parsing all incoming avatar stanzas that delay the MAM response? You can check the time between logging the RECV: carrying the MAM response and the RECV Stanza: handling the MAM response.

It is neither the server's rate limit nor the parsing of incoming stanzas.

The problem is: we send a bunch of vcard iqs, and then send a MAM iq.
The server answers iqs sequentially. So, it doesn't answer the mam query until it has answered all the vcard queries.

And answering vcard queries takes a lot of time because it involves contacting many other servers.
In a big room (400 participants) it can take 2-3 minutes.

[lissine0]

I updated the PR.
Notable changes since the inital version:

• Use the room + occupantId as identifier, instead of room + nick
• Use the occupantId for the avatar placeholder color
• Save the avatars per account (So if a room is joined by two accounts in Monal, they would both download the avatars separately)
• Have only one avatar query in flight at a time (for a given account). In the previous comment I explained why this is needed (not rate limits)

I did not implement a generic abstraction for this. Because I don't think this is needed in other situations. Fetching channel participant avatars is the only time where you can send dozens or hunderds of iqs at the same time. (plus answering them takes several minutes)
So I kept things simple and just implemented it in MLMucProcessor

I'd like to optimize things to reduce the number of avatar fetches:

• The way things are implemented currently, when a participant leaves, its avatar is deleted. (this is because unavailable presences don't contain the avatar hash)
  Ideally, we'd only delete the avatar if it's unpublished, or if the participant is banned

• The second potential optimization was already discussed in Fetch MUC participant avatars #1480 (comment)

Currently, if some participants are present in multiple rooms, their avatars are fetched once for every room.
It would be nice to optimize this case: before sending the iq, we could check if the avatar hash is known in other rooms. If so, we simply copy the file. otherwise we proceed with sending the iq.

Well, since different mucs could have different policies for direct communication, I think that optimization would cause more confusion than do good (its hard to understand for ordinary users, why a muc shows only avatars of some users and not others).

What changed: since version 26.01, ejabberd no longer blocks avatar IQs, even when muc#roomconfig_allow_query_users is set to false: processone/ejabberd@46737b1
The rationale is that avatar queries are answered by the server on behalf of the client. So no direct communication is involved. See processone/ejabberd#4489 for more details

[lissine0]

Notes:

• I didn't implement automatic view updates when the avatar changes. This PR is too big already and I wanted to limit its scope.
• I don't display the avatars of participants of private groups that aren't in our roster, even though their avatars are downloaded. Displaying them would complicate the code a bit and I don't have plans to implement it.

[lissine0]

Is this the correct way to check that we're no longer joined to the room?

[lissine0]

I think the entire cleanupHashes method can / should be removed.
I initially followed the existing code for contact avatars.

These are the only situations that I'm aware of where a hash does not have a corresponding avatar file:

• The PEP node of the participant avatar does not have open access model. as per XEP-0398, the server would still inject the hash to the presence in this case:

services MUST include the hash on behalf of their users in every available presence that does not contain an empty photo element wrapped in an x element qualified by the 'vcard-temp:x:update' namespace.

• The participant avatar is in a file format that Monal doesn't understand. For example I know a user who has an AVIF avatar. At least as of now, Monal tries to load the avatar image (in order to resize it, make it circular and convert it to PNG) and if it can't do that, it doesn't save the file

In both of these cases, if cleanupHashes removes the hash, the next time we receive a presence from the participant, the hash will get saved again, and the avatar will try to be fetched again, which is wasteful since we know it will fail.

As for situations where we have the avatar file but don't have a corresponding hash, I added separate commits that address them:

• When removing a contact
• When leaving a room
• When removing / deleting an account

To conclude, I think we should remove cleanupHashes, even the part concerning contact avatars, unless you have an objection.