XEP-0084: User Avatar (fetching only) #4

noonien-d · 2018-01-05T16:22:56Z

The User Avatar patch enables fetching of PEP based avatars. This is used by the Conversations Android client. Unfortunately, Conversations tends to use webp images, which are not supported by some systems or clients per default.

(still missing mime type detection)

ghost · 2018-01-06T13:53:08Z

I think I saw Conversations recently and finally went back to using PNG avatars, so the amount of webp avatars will drop to zero again as time unfolds. Many thanks for these patches.

Kaffeine · 2020-04-24T10:00:17Z

@rufferson can you please review this series?

rufferson · 2020-05-08T11:47:39Z

on it

rufferson

General comments until I keep it in mind:
this series implements the fetching of avatars sent via new 0084 (PEP meta push) mechanism rather than older 0153 (presence sha push). What it doesn't do however to complete the implementation is:

Publishing (and retrieval on connect/request) of own avatar
Caching of the retrieved avatars

in other words this seems to be kind of work-around for missing avatars (for newer clients) rather than full XEP-0084 implementation. Is my understanding correct or I'm missing something?

The PEP was introduced to 0084 with idea to save the bandwidth and this implementation will do slightly opposite - it will add additional traffic for PEP avatars (without caching) to existing (partially cached) vCards.
The current caching is tightly intertwined with vcard_manager which in turn is bound to presence's sha1 hash.
So need to hook up the meta's hash to get_known_tokens (decouple from presence) and make vcard_manager more generic for caching external data. And of course own avatar management.

rufferson · 2020-05-08T21:19:47Z

src/conn-avatars.c

+      return;
+    }
+
+  // FIXME: there may exist multiple child nodes


there must be at least one info with image/png - shouldn't you iterate through infos till png is found?

When writing this code there was at least Conversations failing to comply https://github.com/iNPUTmice/Conversations/issues/1442.

But you are right, we should at least iterate and check for best match.

rufferson · 2020-05-08T21:39:59Z

src/conn-avatars.c

+  DEBUG ("retrieved avatar from %d with size=%zd, sha1='%s'", handle, outlen, sha1);
+
+  // is this really needed?
+  if (sha1)


this shouldn't be needed, if contact wants to maintain interop they will send the hash explicitly or by xep-0398. if some clients implementations rely on this information - they should rather not.

but is needed if we don't maintain known tokens via pep. also there's no verification of whether this resulting sha1 matches to the pep id (which is also sha1). On the other hand we're not retrieving specific node id so we don't really know what have we fetched :)

Kaffeine · 2020-05-08T22:50:32Z

What it doesn't do however to complete the implementation is:
* Caching of the retrieved avatars

I think that all known telepathy clients cache avatars 🤔. I know that exact code doing that in KDE Telepathy and Sailfish OS (contacts/commhistory) backend.

I had no idea that there is any need in storing avatars on the CM side.

Kaffeine · 2020-05-08T23:01:28Z

How it works in clients:

CM exposes Avatars/token as a contact attribute and notifies the client about an avatar changed via AvatarUpdated signal.
Client checks if it has data for the given token and calls RequestAvatars to get the data for missing avatars.

rufferson · 2020-05-08T23:03:51Z

that's non-persistent in-memory cache (hash table) storing entire vcard in original form - raw xml definition. Not avatar-specific. The point is - many avatar-related calls are relying on vcard manager to retrieve them either from cache or from backend (server). Current implementation is just adding another mechanism to retrieve them from pep (if published). however if client asks for avatar it will still go to vcard-manager.

rufferson · 2020-05-08T23:05:41Z

right so that token/data thingy is dealing with vcards. While the token is actually sha hash which is retrieved from PEP metadata, and data is PEP node referenced by this hash (token)

rufferson · 2020-05-08T23:12:52Z

Or you mean that existing clients are never actually using gabble_connection_request_avatar and instead are always fetching entire bulk for the roster via request_avatars? Even if viewing contact?

Kaffeine · 2020-05-08T23:31:59Z

RequestAvatar is deprecated for 11 years. TelepathyIM/telepathy-spec@072bb0f

I hope that all clients use RequestAvatars instead. But even if clients cache avatar files, they do NOT cache contact attributes and the CM may need vCard for some of those. If the CM need a vCard to return an avatar token then it's wise to cache it. Even more, there is ContactInfo interface that should return vCard data. It also has a contact attribute ContactInfo/info that might be a part of the typical GetContactListAttributes request.

Kaffeine · 2020-05-08T23:38:30Z

Or you mean that existing clients are never actually using gabble_connection_request_avatar and instead are always fetching entire bulk for the roster via request_avatars? Even if viewing contact?

I didn't implement that call in any of the three TelepathyQt-based clients and I had no complaining about missing avatars.

rufferson · 2020-05-09T06:31:44Z

Ok, after sleeping through it and checking deprecations here are following considerations:

request_avatar as deprecated is optional indeed, perhaps worth marking it as such and explain why is it abandoned
known_tokens are currently maintained via presence which is kinda hack but will work (makes answer then to "// Is this really needed?" a "Yes")
since pep is preferred mechanism to manage avatars that should be checked first (in request_avatars first check pep_avatar_hashes, then vcard_manager_get_cached, then vcard_manager_request) but as you see this lacks local data cache (comparing to vcards) to avoid excessive pep data node querying. Ideally we'd need to call it once, only when hash changes.
and yes, total lack of self avatar management doesn't allow calling it "0084 implementation", only partial implementation. But if we call it out explicitly as such in commit/pr should be fine as a starter.

rufferson

The only major one is pep_avatar_hashes leak

would really be appreciated

known_tokens and fill_attribs rework to deal with tokens directly rather than via presence. this will also allow removal of presence hash injection hack
pep data cache ("If the subscriber already has a locally cached copy of the avatar image, it MUST NOT retrieve the image data.") and reorder in request_avatars (pep then vcard) since if there's cached vcard pep will never be retrieved.
change commit/mr title to reflect it's not full implementation of the xep

minor tweaks - comments, line reordering

rufferson · 2020-05-09T06:46:07Z

src/conn-avatars.c

+  DEBUG ("retrieved avatar from %d with size=%zd, sha1='%s'", handle, outlen, sha1);
+
+  // is this really needed?
+  if (sha1)


but is needed if we don't maintain known tokens via pep. also there's no verification of whether this resulting sha1 matches to the pep id (which is also sha1). On the other hand we're not retrieving specific node id so we don't really know what have we fetched :)

src/conn-avatars.c

rufferson · 2020-05-09T07:39:24Z

src/conn-avatars.c

+  const gchar *binval_value;
+  gchar *sha1;
+  /* todo: get mime type from metadata pep, if any */
+  const gchar *mime_type = "";


not unless we're fetching specific id and verifying the hashes are matching. As it stands - let's keep it empty to indicate - we don't know which mime type is being retrieved (update comment perhaps with this note).

rufferson · 2020-05-09T07:41:42Z

src/conn-avatars.c

+  gsize outlen;
+  GArray *arr;
+
+  g_slice_free (pep_request_ctx, ctx);


this makes hash contain dangling pointer for several cycles, let's better reorder these calls perhaps - remove then free. assert will abort the execution whatsoever.

rufferson · 2020-05-09T07:51:46Z

src/conn-avatars.c

@@ -915,6 +933,8 @@ conn_avatars_fill_contact_attributes (GObject *obj,

          if (NULL != presence->avatar_sha1)
            g_value_set_string (val, presence->avatar_sha1);
+          else if (g_hash_table_lookup (self->pep_avatar_hashes,  GINT_TO_POINTER(handle)))
+            g_value_set_string (val, g_hash_table_lookup (self->pep_avatar_hashes,  GINT_TO_POINTER(handle)));


We do not need presence technically to fill this attribute.
Eg. if we have pep_avatar_hash we don't even need to check for presence availability, but if we don't - we need to execute the old code to retrieve hash from presence.

Same logic need to be added to line 380 (get_known_avatar_tokens) as it doesn't seem to be deprecated.
Eg check hash - if exists fill the token, otherwise try to obtain presence, further code will work on presence if it exists and will skip it if token was filled (and presence remains null)

src/conn-avatars.c

rufferson

I've actually did the review already and suggested changes here :) So far I see the MR title was adjusted but other changes are not yet committed for next review round.

Kaffeine · 2020-05-12T20:51:10Z

I've actually did the review already

Yep. Both of you accepted the invitation and I used that to make the process a bit more clear and formal.
You actually can select "Request changes" (the issue with pep_avatar_hashes seems to be a strong point) so the PR status will be more visible (here and in the PRs list).

rufferson

I've tried to build that but gcc threw several warnings (fatal with werror) so suggested changes make gcc happy and also preventing leaks

src/conn-avatars.c

src/connection.c

rufferson · 2020-12-25T15:59:18Z

Hi @noonien-d - first of all merry Christmas and happy New Year!
I see you either don't have time for this or lost the motivation or both (stopped using it?) - hence I wonder if you would mind me pulling it manually making (as per my understanding) necessary adjustments?

noonien-d · 2020-12-26T19:22:51Z

Hi @noonien-d - first of all merry Christmas and happy New Year!
I see you either don't have time for this or lost the motivation or both (stopped using it?) - hence I wonder if you would mind me pulling it manually making (as per my understanding) necessary adjustments?

Hi @rufferson, also merry Christmas! I noticed you did a bunch of work, and I'm eager to test your PR #17.
Somehow, I was pretty busy during the last months. But I'll have some free time during the next days and will use it to revive this pull request. Thanks for reminding me ;-)

rufferson

Looks good now and compiles without warnings.
Thanks, should be good to merge and a good foundation to complete the full implementation.

rufferson · 2020-12-31T10:45:04Z

Thanks @noonien-d !

noonien-d force-pushed the 0.18_XEP-0084_User_Avatar branch from 733fc99 to 251f893 Compare April 19, 2019 00:16

rufferson reviewed May 8, 2020

View reviewed changes

rufferson requested changes May 9, 2020

View reviewed changes

noonien-d changed the title ~~XEP-0084: User Avatar~~ XEP-0084: User Avatar (fetching only) May 9, 2020

Kaffeine requested a review from rufferson May 12, 2020 20:34

rufferson reviewed May 12, 2020

View reviewed changes

noonien-d added 3 commits June 20, 2020 23:42

initial pep avatar fetching support

efc1abc

use correct format specifier for size_t

2f08ad3

pep-avatar: select png from the list of available avatars

bf0176a

noonien-d force-pushed the 0.18_XEP-0084_User_Avatar branch from 251f893 to bf0176a Compare June 20, 2020 22:22

rufferson requested changes Jul 10, 2020

View reviewed changes

src/conn-avatars.c Outdated Show resolved Hide resolved

src/conn-avatars.c Outdated Show resolved Hide resolved

src/conn-avatars.c Outdated Show resolved Hide resolved

src/conn-avatars.c Outdated Show resolved Hide resolved

src/connection.c Show resolved Hide resolved

noonien-d added 2 commits December 28, 2020 21:43

create pep hash table with destructors

a70a283

fix compiler warnings

2eb7663

noonien-d requested a review from rufferson December 28, 2020 21:02

rufferson approved these changes Dec 30, 2020

View reviewed changes

rufferson merged commit ec525c3 into TelepathyIM:master Dec 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XEP-0084: User Avatar (fetching only) #4

XEP-0084: User Avatar (fetching only) #4

noonien-d commented Jan 5, 2018

ghost commented Jan 6, 2018

Kaffeine commented Apr 24, 2020

rufferson commented May 8, 2020

rufferson left a comment •

edited

Loading

rufferson May 8, 2020

noonien-d May 8, 2020

rufferson May 8, 2020

rufferson May 9, 2020

Kaffeine commented May 8, 2020 •

edited

Loading

Kaffeine commented May 8, 2020

rufferson commented May 8, 2020 •

edited

Loading

rufferson commented May 8, 2020 •

edited

Loading

rufferson commented May 8, 2020

Kaffeine commented May 8, 2020

Kaffeine commented May 8, 2020

rufferson commented May 9, 2020

rufferson left a comment

rufferson May 9, 2020

rufferson May 9, 2020

rufferson May 9, 2020

rufferson May 9, 2020

rufferson May 9, 2020

rufferson left a comment

Kaffeine commented May 12, 2020 •

edited

Loading

rufferson left a comment •

edited

Loading

rufferson commented Dec 25, 2020

noonien-d commented Dec 26, 2020 •

edited

Loading

rufferson left a comment

rufferson commented Dec 31, 2020

XEP-0084: User Avatar (fetching only) #4

XEP-0084: User Avatar (fetching only) #4

Conversation

noonien-d commented Jan 5, 2018

ghost commented Jan 6, 2018

Kaffeine commented Apr 24, 2020

rufferson commented May 8, 2020

rufferson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kaffeine commented May 8, 2020 • edited Loading

Kaffeine commented May 8, 2020

rufferson commented May 8, 2020 • edited Loading

rufferson commented May 8, 2020 • edited Loading

rufferson commented May 8, 2020

Kaffeine commented May 8, 2020

Kaffeine commented May 8, 2020

rufferson commented May 9, 2020

rufferson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rufferson left a comment

Choose a reason for hiding this comment

Kaffeine commented May 12, 2020 • edited Loading

rufferson left a comment • edited Loading

Choose a reason for hiding this comment

rufferson commented Dec 25, 2020

noonien-d commented Dec 26, 2020 • edited Loading

rufferson left a comment

Choose a reason for hiding this comment

rufferson commented Dec 31, 2020

rufferson left a comment •

edited

Loading

Kaffeine commented May 8, 2020 •

edited

Loading

rufferson commented May 8, 2020 •

edited

Loading

rufferson commented May 8, 2020 •

edited

Loading

Kaffeine commented May 12, 2020 •

edited

Loading

rufferson left a comment •

edited

Loading

noonien-d commented Dec 26, 2020 •

edited

Loading