Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[office] Correct the issue of stretched cover when rotated. #65

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dcaliste
Copy link
Contributor

Currently, the cover for a document is stretched when the device is rotated. This patch, adding the fillMode line to the image corrects this (either starting form landscape mode and switching to portrait in the home view or reverse).

To avoid to scale up image when switching from portrait to landscape, the image is generated with the width corresponding to the largest cover dimension. That's the var ratio = lines in updatePreview() function. This makes for a 16/9 ratio an increase in memory size for the cover of a factor 3 (ratio * ratio). I think it's acceptable for small covers we have.

Nevertheless (it's the case before the PR), grabToImage() uses a nearest value algorithm to downscale item into a smaller image. Thus the result is quite ugly. The PR improves a bit this issue by generating an image that is a bit bigger when in portrait, but the issue remains for landscape orientation. In fact, result is much more pleasing when not using the Qt.size() argument and keeping an image the size of the screen before reduction to the cover size. But I'm afraid it's too memory demanding. Having the times 3 factor can be a good balance. What do you think ?

@rainemak
Copy link
Member

rainemak commented Jan 4, 2016

Hmm... One option is to use window.documentItem.width and window.documentItem.height for grabbing and scale those values down a bit and adjust by the cover aspect ratio. Maybe something in between of 0.5 - 0.6 as the larger cover is ~0.4 of the fullscreen page.

@pvuorela
Copy link
Contributor

pvuorela commented Jan 4, 2016

3 times cover size is roughly same as screen resolution * 0.5. Out of those simpler code should win.

On the other hand, half the resolution is already few megabytes on full hd, which is a bit unfortunate and borderline acceptable. For rotateable screenshot it should be enough that other dimension has more pixels, which should bring memory overhead to screen ratio, but not sure how to put that into code. Another option would be to ask document rendering backend for screenshot, but that would increase complexity. Though would make the other PR for screenshot time unnecessary.

@dcaliste
Copy link
Contributor Author

dcaliste commented Jan 4, 2016

@pvuorela, the current PR indeed calls grabToImage() with the smallest oversampling so when we rotate we are 1pixel in memory = 1 pixel on cover, as you suggest in your second paragraph (as far as I understand it). This oversampling corresponds to the ratio [r = max(width, height) / min(width, height)]. Thus the ×3 factor for a 16/9 screen ratio (because the increase in memory is r*r, not r only).

The issue, in my opinion, that is already present anyway, is that the downscaling from document window to cover is done by grabToImage() which uses a nearest neighbour and gives thus poor results when displaying 1 pixel image for 1 pixel cover. So my question was : would it be possible to pay in memory a bigger oversampling with grabToImage() so the downscaling is done by the QML (with a better algorithm) and thus gives better results. But as you said this represents already megabytes on HD display, which is IMHO too expemsive for a simple cover.

Would you agree to keep the PR as it is, which gives already better results than what we have now with a reasonable increase in memory for the cover ?

About asking the backend to render a new image when rotated, it may be not, in my opinion, a good solution because rotating the device when being in the cover view can happen quite often (because not paying attention to the device, passing it over to other people…) and thus will drain the battery quicker for no obvious reason, having a snapshot just rotated is cheap in CPU and can happen quickly.

Anyway thanks for your comments.

@pvuorela
Copy link
Contributor

pvuorela commented Jan 5, 2016

But as you said this represents already megabytes on HD display, which is IMHO too expemsive for a simple cover.
Would you agree to keep the PR as it is, which gives already better results than what we have now with a reasonable increase in memory for the cover ?

Contradictory sentences. The PR as is does that few megabyte memory increase on a full hd display.

But now to think of it. The whole rotation is silly, no other covers do that on phone, and tablet which does some rotations this doesn't even work, rotates then too much. So, should be enough avoiding changes after cover is shown. No extra memory needed. Right? :)

@dcaliste
Copy link
Contributor Author

dcaliste commented Jan 5, 2016

@pvuorela sorry, wasn't clear in the context of the two sentences. First one refers to the call to grabToImage() without the optional Qt.size() argument, thus putting a size-of-the-screen image into memory. This is not in the current PR, but only suggested in my first comment as a possible improvement to obtain a better rendering of the cover (downscaling done by QML with a better algo than nearest). The second sentence, deals with the current PR, with a slight increase in the Qt.size() argument to avoid upscaling after rotation. This slight increase correspond to the ×3 factor already mentioned.

Concerning the main issue this PR tries to solve, it's the fact that cover is currently rotating with ugly rendering due to default fillMode attribute being Image.stretched. We have thus several choices:

  • A: put fillMode: Image.pad → no memory increase wrt. current situation, less ugly than Image.stretched. For a cover of let say 768×432 pixels, it means ~1.3MB.
  • B: put fillMode: Image.preserveAspectCrop → current PR, need to overdimension the image in memory, thus a ×3 memory consumption for the cover (~4.2MB).
  • C: put fillMode: Image.preserveAspectCrop without Qt.size() optional argument when calling grabToImage() → far better rendering but megabytes of memory consumption (~8.3MB).
  • D: don't rotate the cover (~1.3MB).

I personnaly prefer option B, but it's up to your choice of course !

@pvuorela
Copy link
Contributor

pvuorela commented Jan 7, 2016

There are quite a few things going wrong now. First, image is grabbed regardless of pdf rendering status. I think currently on rotation first image is grabbed while a scaled version is shown and after that re-rendered version gets shown. Then there are these window visibility issues Raine is partly working around. These changes alone seem problematic to test as grabbing fails too often.

For cover contents, as said, all the other apps don't do any rotations. For consistency, this shouldn't be different. Tablet does rotate covers themselves together with rotating home screen, but the covers remain as portrait aspect ratio items.

Then part of the scaling problem is documentItem changing size. On either mode, there should be enough pixels for cover screenshot, though from landscape app to portrait cover some clipping is needed. grabToImage() annoyingly doesn't support such.

@ghost
Copy link

ghost commented Mar 10, 2016

At least the browser cover does actually rotate, but in some weird way, so it's probably more of a bug than a feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants