D29526: Thumbnails: make thumbnail generation dpr-aware

[Stefan Brüns]

bruns added a comment.


  In D29526#676402 , @meven wrote:
  
  > In D29526#676386 , @bruns wrote:
  >
  > > For all but text the DPR is completely irrelevant, large@1 is identical 
to normal@2.
  >
  >
  > Yes and that's up to thumbnail creators to decide. To take advantage of 
this, we would need to introduce some ThumbnailCreator type that would say 
whether or not generated thumbnail might be influenced by DPR (i.e) text. That 
would necessitate change the ThumbnailCreator API.
  
  
  No, just add a "DisplayPixelRatioDependent=true"  key to the service data of 
the thumbnailer.
  
  > But the implementation will stay a lot simpler if we don't this level of 
complexity and adding will have a limited interest. Storing twice large@1 would 
happen only when a user would change DPR, thumbnails cache size limit will stay 
enforced.
  
  You are changing every single thumbnailer implementation, although the 
results are binary identical for normal@2 and large@1. And that are only the 
thumbnailers in kio-extra.
  
  And on top of all this, the only thumbnailer which is DPR dependent (text) 
does not even cache the data on disk.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29526

To: meven, #frameworks, dfaure, broulik, sitter, ngraham, bruns
Cc: bruns, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, emmanuelp, rdieter, mikesomov

D29526: Thumbnails: make thumbnail generation dpr-aware

[Méven Car]

meven added a comment.


  In D29526#676386 , @bruns wrote:
  
  > For all but text the DPR is completely irrelevant, large@1 is identical to 
normal@2.
  
  
  Yes and that's up to thumbnail creators to decide. To take advantage of this, 
we would need to introduce some ThumbnailCreator type that would say whether or 
not generated thumbnail might be influenced by DPR (i.e) text. That would 
necessitate change the ThumbnailCreator API.
  
  But the implementation will stay a lot simpler if we don't this level of 
complexity and adding will have a limited interest. Storing twice large@1 would 
happen only when a user would change DPR, thumbnails cache size limit will stay 
enforced.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29526

To: meven, #frameworks, dfaure, broulik, sitter, ngraham, bruns
Cc: bruns, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, emmanuelp, rdieter, mikesomov

D29526: Thumbnails: make thumbnail generation dpr-aware

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> imagecreator.cpp:48
> +ir.setQuality(75); // set quality so that the jpeg handler will use a 
> high quality downscaler
> +
>  img = ir.read();

This whole hunk should not be part of this submission. Also, the comment 
("jpeg") is likely wrong.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29526

To: meven, #frameworks, dfaure, broulik, sitter, ngraham, bruns
Cc: bruns, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, emmanuelp, rdieter, mikesomov

D29526: Thumbnails: make thumbnail generation dpr-aware

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> thumbnail.cpp:143
>  
> +bool createThumbnail(ThumbCreator *creator, const QString , int width, 
> int height, QImage , qreal devicePixelRatio = 1.0)
> +{

Has to be static or in an anonymous namespace.

> thumbnail.cpp:149
> +} else {
> +return creator->create(path, width, height, img);
> +}

`width * dpr, height * dpr` gives the wanted result even without explicit 
support for DPR for the majority of the thumbnailer.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29526

To: meven, #frameworks, dfaure, broulik, sitter, ngraham, bruns
Cc: bruns, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, emmanuelp, rdieter, mikesomov

D29526: Thumbnails: make thumbnail generation dpr-aware

[Stefan Brüns]

bruns requested changes to this revision.
bruns added a comment.
This revision now requires changes to proceed.


  For all but text the DPR is completely irrelevant, large@1 is identical to 
normal@2.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29526

To: meven, #frameworks, dfaure, broulik, sitter, ngraham, bruns
Cc: bruns, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, emmanuelp, rdieter, mikesomov

D29526: Thumbnails: make thumbnail generation dpr-aware

[Méven Car]

meven added a comment.


  I just sent a specification evolution to cover this use case : 
https://gitlab.freedesktop.org/xdg/xdg-specs/-/merge_requests/35

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29526

To: meven, #frameworks, dfaure, broulik, sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29526: Thumbnails: make thumbnail generation dpr-aware

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> sitter wrote in thumbnail.cpp:774
> Var naming is a bit inconsistent across the code base now. In the 
> implementations there are maxWidth/H that are device-adjusted but in here 
> they are not. Not a huge concern, just noticed.

Will take care

> thumbnail.cpp:791
>  // for the same directory and sequence-item
>  qsrand(qHash(filePath));
>  

Not used can be removed.

> thumbnail.cpp:794
>  // Apply fake smooth scaling, as seen on several blogs
> -if (subThumbnail.width() > width * 4 || subThumbnail.height() > height * 
> 4) {
> -subThumbnail = subThumbnail.scaled(width*4, height*4, 
> Qt::KeepAspectRatio, Qt::FastTransformation);
> +if (subThumbnail.width() > width * 4 * subThumbnail.devicePixelRatio() 
> || subThumbnail.height() > height * 4 * subThumbnail.devicePixelRatio()) {
> +subThumbnail = subThumbnail.scaled(width * 4 * 
> subThumbnail.devicePixelRatio(), height * 4 * 
> subThumbnail.devicePixelRatio(), Qt::KeepAspectRatio, Qt::FastTransformation);

Because of line above `scaleDownImage` can never happen.
Can probably be removed.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29526

To: meven, #frameworks, dfaure, broulik, sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29526: Thumbnails: make thumbnail generation dpr-aware

[Harald Sitter]

sitter added a comment.


  You do seem to calculate the deviceWidth and height an awful lot, it makes 
reading a bit clunky. I'd much rather have the multiplication done once and 
then always use the var instead. In fact, perhaps it'd make sense to have 
createV3 feed the values into the implementations? Currently they all repate 
the same two lines over and over.

INLINE COMMENTS

> djvucreator.cpp:61
>QByteArray sizearg, fnamearg;
> -  sizearg = QByteArray::number(width) + 'x' + QByteArray::number(height);
> +  sizearg = QByteArray::number(width * devicePixelRatio) + 'x' + 
> QByteArray::number(height * devicePixelRatio);
>fnamearg = QFile::encodeName( path );

Surely multiplication results need converting to int. That being said, you do 
multiply below again, so maybe just move maxWidth/Height up here.

> thumbnail.cpp:774
>  
>  void ThumbnailProtocol::scaleDownImage(QImage& img, int maxWidth, int 
> maxHeight)
>  {

Var naming is a bit inconsistent across the code base now. In the 
implementations there are maxWidth/H that are device-adjusted but in here they 
are not. Not a huge concern, just noticed.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D29526

To: meven, #frameworks, dfaure, broulik, sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29526: Thumbnails: make thumbnail generation dpr-aware

[Méven Car]

meven updated this revision to Diff 82332.
meven added a comment.


  Store thumbnails with dpr 2 in @2x folders

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29526?vs=82254=82332

BRANCH
  thumbnail-dpr

REVISION DETAIL
  https://phabricator.kde.org/D29526

AFFECTED FILES
  CMakeLists.txt
  thumbnail/djvucreator.cpp
  thumbnail/djvucreator.h
  thumbnail/imagecreator.cpp
  thumbnail/imagecreator.h
  thumbnail/jpegcreator.cpp
  thumbnail/jpegcreator.h
  thumbnail/kritacreator.cpp
  thumbnail/kritacreator.h
  thumbnail/svgcreator.cpp
  thumbnail/svgcreator.h
  thumbnail/textcreator.cpp
  thumbnail/textcreator.h
  thumbnail/thumbnail.cpp
  thumbnail/thumbnail.h

To: meven, #frameworks, dfaure, broulik, sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29526: Thumbnails: make thumbnail generation dpr-aware

[Méven Car]

meven created this revision.
meven added reviewers: Frameworks, dfaure, broulik, sitter, ngraham.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
meven requested review of this revision.

REVISION SUMMARY
  Thumbnail ioslave can now receive a devicePixelRatio metadata parameter, that 
it will pass to its thumbnail creators plugins implementing ThumbCreatorV3.
  It updates the directory thumbnail generation.
  
  Port image, jpeg, djvu, krita, svg, text to new ThumbCreatorV3.
  
  Depends on D29397 

TEST PLAN
  Manualy tested
  With dolphin D29525 

REPOSITORY
  R320 KIO Extras

BRANCH
  thumbnail-dpr

REVISION DETAIL
  https://phabricator.kde.org/D29526

AFFECTED FILES
  CMakeLists.txt
  thumbnail/djvucreator.cpp
  thumbnail/djvucreator.h
  thumbnail/imagecreator.cpp
  thumbnail/imagecreator.h
  thumbnail/jpegcreator.cpp
  thumbnail/jpegcreator.h
  thumbnail/kritacreator.cpp
  thumbnail/kritacreator.h
  thumbnail/svgcreator.cpp
  thumbnail/svgcreator.h
  thumbnail/textcreator.cpp
  thumbnail/textcreator.h
  thumbnail/thumbnail.cpp
  thumbnail/thumbnail.h

To: meven, #frameworks, dfaure, broulik, sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov