search

andig / carddav2fb

Download CardDAV VCards and upload as phonebook to AVM FRITZ!Box
63 stars 19 forks source link

Image upload: some improvement ideas [PR exists] #75

Closed derwok closed 5 years ago

derwok commented 5 years ago

I really like the image upload feature! Thanks!! But I also see some improvement ideas.

TL;DR: I will submit a pull request on this!

  1. Image upload was not stable on my FritzBox. Non-deterministically after 25 to 30 successful ftp uploads all(!) further images produces this error message:

    ftp access denied for 68609498-7e94-40a7-a4a9-08da58ad55e5.jpg 
to /Sony-StorageMedia-01/FRITZ/fonpix

    Assumption: server or client runs out of FTP connections? Don't know for sure. => Suggestion: We should use one sticky FTP session for all image uploads.

  2. The image handling with the functions imagecreatefromstring and imagejpeg uncompress the jpeg data and then recompress with lossy JPEG the image data. This costs time and results in less image quality. Users might assume that images have "same quality". => Suggestion: Just use the already existing JPEG data in vcard->rawPhoto and perform upload via fast in-memory stream

  3. Current implementation always uploads ALL images. With lots of contact images with higher resolutions this might take quite some time and will stress FritzBox server unneccessary. As JPEG almost always changes +/- some bytes if content changes, we might simply rely upon file size. => Suggestion: Only upload if file size of photo changes. User can always force upload by removing all photos via FritzBox NAS.

  4. As image upload of many high res images might take some time add a nice progress bar.

blacksenator commented 5 years ago

Everything sounds very convincing. Thank you for your participation. I'm curious when Andreas finds the time to review the improvements :)

derwok commented 5 years ago

Thanks. :-) Hope he likes it?

Pull request now exists here: https://github.com/andig/carddav2fb/pull/76

The PR also fixes this issue:

  1. the "example" config.php specifies the fritz box as "http://fritz.box" - when users do NOT change this, then the old image upload code could not connect to the Fritz FTP server. I had to remove the http:// to "fritz.box". The PR code now can handle this situation by removing http:// locally, if present.
andig commented 5 years ago

Assumption: server or client runs out of FTP connections? Don't know for sure. => Suggestion: We should use one sticky FTP session for all image uploads.

I see. Here is the explanation for #73. Please: ref the PRs from the Issues and add "fixes xyz" to the PRs. Will make cross-referencing much easier. Also please do single PRs for single issues instead of batches.

blacksenator commented 5 years ago

The image handling with the functions imagecreatefromstring and imagejpeg uncompress the jpeg data and then recompress with lossy JPEG the image data. This costs time and results in less image quality. Users might assume that images have "same quality".

I think, in order to ensure a stable image synchronization between FRITZ!Box and Fritz!Fon and to take account of the small screen size, it is rather quite useful to reduce the images appropriately and into the correct decomposition hierarchy imageinterlace (image $, 0).

AVMs recommendations are:

Wir empfehlen für die Bilder eine Auflösung von mindestens 240x320 Pixel*, optimal ist ein Seitenverhältnis von 1:1. Wenn Sie ein Bild in einem Bildbearbeitungsprogramm als JPEG speichern, empfehlen wir die Kodierung "Baseline" (üblicherweise voreingestellt).

(*) display size Fritz!Fon C4/C5