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
Fix #461 - ImageBufferQt scaling wrong for HiDPI environments #534
Fix #461 - ImageBufferQt scaling wrong for HiDPI environments #534
Conversation
As per qtwebkit#461 - the scale was not applied to the source, resulting in the filter attempting to copy from unallocated memory. This adds similar implementation of scaling to the one from the ImageBufferCG changes in https://trac.webkit.org/changeset/168577/webkit ("-webkit-filter prevents rendering at retina scale") Also fixes putByteArray creating too small image, and adds proper scale to images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jturcotte could you take a look at this change?
return getImageData<Unmultiplied>(rect, m_data, m_size); | ||
float scale = 1.0; | ||
if (coordinateSystem == LogicalCoordinateSystem) | ||
scale = m_resolutionScale; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code should be moved to getImageData to avoid duplication
@@ -155,24 +166,32 @@ PassRefPtr<Uint8ClampedArray> getImageData(const IntRect& rect, const ImageBuffe | |||
// Let drawImage deal with the conversion. | |||
// FIXME: This is inefficient for accelerated ImageBuffers when only part of the imageData is read. | |||
QPainter painter(&image); | |||
// this painter needs the same scaling as the whole buffer, to copy HiDPI images properly: | |||
painter.setWorldTransform(QTransform::fromScale(scale, scale)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure, but this may be an expensive operation. It might be a better idea to put it under scale != 1.0
(or maybe even !qFuzzyCompare(scale, 1.0f)
).
Also it might be a good idea to evaluate this QTransform
up front and save it alongside with m_resolutionScale
, if this code is executed often
// we need to copy it, to scale without converting to int first, to retain precision: | ||
FloatSize sizeScaled(size); | ||
sizeScaled.scale(resolutionScale); | ||
m_size = IntSize(sizeScaled); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_size = IntSize(size * resolutionScale);
should do the same thing and is cleaner
On HiDPI screen, is text quality in inverted mode as good as non-inverted with this patch? |
Asking because I'm getting different text rendering in test case when intermediate ImageBuffer is used and when it isn't |
Merged your changes as 7259518 |
As per #461 - the scale was not applied to the source,
resulting in the filter attempting to copy from unallocated memory.
This adds similar implementation of scaling to the one from the
ImageBufferCG changes in https://trac.webkit.org/changeset/168577/webkit
("-webkit-filter prevents rendering at retina scale")
Also fixes putByteArray creating too small image, and adds proper
scale to images.