Re: [Evolution-hackers] improve PHOTO support in libebook/EDS

[Tristan Van Berkom <tristanvb openismus com>]

On Thu, 2011-07-07 at 10:48 +0200, Patrick Ohly wrote:
> On Wed, 2011-07-06 at 16:16 -0400, Tristan Van Berkom wrote:
> [staging directory for out-of-band photo data transmission]
> > This alternative proposal is strictly regarding the photo data for
> > which the server must take ownership of I assume, correct ?
> 
> Yes.
> 
> > I might not have been clear enough in my proposal but I only 
> > intend for new or modified photos to be sent to the addressbook.
> 
> Understood. I personally don't mind the D-Bus overhead in these cases
> because I consider them rare. But there are people who run massive write
> benchmarks against EDS anyway and do flag low performance in them as a
> problem. So if we can find a solution that also optimizes the write
> performance, then it would be better.
> 
> > The staging directory you propose if I understand it correctly
> > would also be used under only the same circumstances, I suppose
> > the questions are:
> >   o What is faster: writing a file to a staging directory and 
> >     then moving it to a permanent location or sending the blob
> >     once over D-Bus and saving it to the permanent location.
> 
> If the photo data needs to be stored as file and moving the file into
> its permanent location can be done with a rename, then the staging
> directory approach is faster. I expect writing the file to be equally
> fast, regardless of who does it. Sending inline data adds D-Bus overhead
> (considerable for large chunks of data!) and encoding/decoding overhead.
> 
> >   o What is more reliable ? (it seems that there are less
> >     points of failure when sending the blob but I could be wrong).
> 
> Agreed. I tried to mitigate that with the rules around cleaning up the
> staging directory.
> 
> > Perhaps the staging directory is a little faster when batching lots 
> > of photo modifications during a 'sync' operation ? (the client
> > gets to write to disk while waiting for the server to be ready
> > for the next batch of contact additions perhaps ?)
> > 
> > At any rate, if it's judged that the performance gain of using
> > a staging directory is worth the added complexity then let's do it.
> 
> I'd like to hear some other opinions about this. Ross?
> 
> > I think the more important issue at hand is that to offer
> > a reliable api for EBookView then the backend has to track
> > the views which have received notification of the contact
> > modifications.
> > 
> > In other words when I have an EBookView and I have been
> > notified that a contact exists with a photo at a given uri path,
> > then until I receive notification that that uri is invalid,
> > removed or replaced with a new one... I should be able to
> > access that file.
> 
> I don't see this as a problem. When a process fails to load a photo
> because the contact was already removed on the server together with that
> photo, then shortly after the attempt to read the photo the process will
> get the notification that the contact is gone and thus the process no
> longer needs the photo.
> 
> I also think that this is a very rare situation. Definitely doesn't
> justify adding complex code to the D-Bus interface between libebook and
> EDS.
> 
> > Another hard limitation is regarding storing uris for which
> > you don't want the addressbook to assume ownership of.
> > 
> > The clean way as far as I can see is whoever is responsible
> > for adding an 'alien uri' to an addressbook is responsible
> > for removing that entry from the addressbook at the time
> > that the uri might be deleted.
> 
> Agreed. But I don't think there is a clean and efficient way of solving
> this. It'll be much simpler to accept that there will be URIs that point
> to deleted files.
> 
> The intention is to not use URIs pointing to arbitrary local files.
> Therefore we don't need to provide a solution that assists apps who want
> to do that.
> 
> > Well... perhaps we should just swallow the ugly race and do nothing
> > about it and say that:
> >   "a uri might be invalid from time to time directly before your view
> >    receives notification of the removal" ?
> > 
> > Would that be acceptable ?
> 
> Yes.
> 

Ok actually that sounds reasonable, I wasn't sure and would have
been embarrassed to be told that 
  "my uri is invalid before I receive the remove notification".

I now have a much simpler patch up on the openismus-work branch[0].
(it's also in patch form on the bug[1]).

It was easy enough to manage the photo files with a 
simple algorithm like so:

in_add_modify_remove_contact_code () {

   /* Find a new filename for the incoming binary blobs and put
    * the new data on disk
    */
   maybe_transform_incoming_contact ();

   /* Check new uris vs old uris in the DB... if old unused uris
    * belong to us (that is if they are in our photo cache directory)
    * then delete the old uri.
    */
   maybe_delete_unused_uris ();
}

The branch has a test case that verifies modifications and additions of
binary blobs get properly transformed.

Currently I don't have any unit tests in place that ensure the files are
properly deleted.

Also there is a strange use case worth pointing out:

You are not allowed to use an addressbook generated uri as the uri
for another field of the same contact or another contact.

So you are allowed to set multiple contacts photo fields
to point to "http://hostyouravatar.com/buster.png";, but you
are not allowed to do:

   /* Get that contact named "Elvis Presley" */
   PhotoContact *photo = e_contact_get (contact_a, E_CONTACT_PHOTO);

   /* Use Elvis's face on this new contact I'm creating */
   PhotoContact *new_photo = create_photo (photo->data.uri);

   e_book_commit_contact (new_photo);

Not sure if that's an acceptable rule or not, if not then we
either need to add some complex code to implement internal
refcounting on the uris in the backend... or perhaps we
could trap the incoming shared uris and create copies
on disk instead...

In any case, I think we could try to close this first and
then look at the staging directory feature separately, seeing
that this code will very easily scale from:
  a.) "If the incoming photo is a binary blob" to...
  b.) "If the incoming photo is a binary blob or 
       a uri inside the staging directory..." 

Cheers,
      -Tristan


[0]http://git.gnome.org/browse/evolution-data-server/log/?h=openismus-work
[1]https://bugzilla.gnome.org/show_bug.cgi?id=652178