-
Notifications
You must be signed in to change notification settings - Fork 177
Fix stack overflow in DPIUtil.autoScaleImageData #2358
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 stack overflow in DPIUtil.autoScaleImageData #2358
Conversation
739ebce
to
d3d106c
Compare
8dcb140
to
b1e2397
Compare
b1e2397
to
a2deff9
Compare
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 able to judge the technical side of this PR but isn't it correct to say that it fixes #2353 ? If it does then the commit text needs to be adapted.
a2deff9
to
0b95661
Compare
Thank you for adding the information. I'm not able to judge this PR at a technical level (maybe @akoch-yatta / @HeikoKlare can) but I can tell that at least this PR does get rid of the stack overflow, as it promises. ✔️ |
The failing test seems to be unrelated to the change. I have raised an issue about the failures #2363 |
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.
The change is sound, thank you!
I only have two minor comments.
@@ -394,6 +392,10 @@ public static void setMonitorSpecificScaling(boolean activate) { | |||
System.setProperty(SWT_AUTOSCALE_UPDATE_ON_RUNTIME, Boolean.toString(activate)); | |||
} | |||
|
|||
public static void setAutoScaleValue(String autoScaleValue) { |
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.
We should add a JavaDoc that states that this method is only supposed to be called in tests and may lead to unintended effects if called in production.
"1.0, 200, false", "2.0, 200, false", "2.0, quarter, false", }) | ||
public void autoScaleImageData0(float scaleFactor, String autoScale, boolean monitorSpecificScaling) { | ||
DPIUtil.setMonitorSpecificScaling(monitorSpecificScaling); | ||
DPIUtil.setAutoScaleValue(autoScale); |
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.
Since the value is only initialized on class loading (i.e., at JVM startup), changing this value in a test without resetting it will leave the JVM in an unexpected state for the subsequent tests. To avoid that, the original value should be stored and reapplied after executing the test. This has to be done in the finally block, as otherwise a test failure would still leave behind an unexpected system state.
f273153
to
bd1e417
Compare
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 just saw that we already have DPIUtil#runWithAutoScaleValue()
which can be used to run some code with a specific autoScale value making the setAutoScaleValue
method for test purposes obsolete. I have adapted the test implementation with a usage of that method and removed the obsolete new method from DPIUtil
, which renders my previous comments obsolete.
@arunjose696 please check if the change is fine for you.
bd1e417
to
5d514a4
Compare
Recently, a strict check was introduced that ImageData should be linearly scaled for other zoom levels. ImageDataProviders inside SWT not following this strict checks were modified (e.g., to return only image data at 100%). However, this change broke DPIUtil.autoScaleImageData. That function creates an Image using the system zoom level, draws it on a GC at scaled dimensions, and then requests ImageData at 100% zoom from the image. Since the ImageDataProvider was now restricted to 100% zoom only, requesting imageData at other zoom levels while creating an image at system zoom level triggered recursive calls, causing a stack overflow error. This commit reverts the behavior of the ImageDataProvider used in DPIUtil.autoScaleImageData to return the same ImageData at all zoom levels. We also disable the strict zoom check in this specific case, because the scaling is done by GC.drawImage() and the ImageData used to create the is expected to be same. Fixes eclipse-platform#2353
5d514a4
to
a2cf98c
Compare
The new changes look good to me, Additionally I have added image disposals for the new images created which I missed. |
Great, thanks! |
Fixes #2353
In PR #2249, a strict check was introduced that
ImageData
should be linearly scaled forzoom levels. And imageDataProviders in SWT repo not following this strict checks were modified (eg to return only imageData at 100%).
However, this change broke
DPIUtil.autoScaleImageData
. That function creates an Image using the system zoom level, draws it on a GC at scaled dimensions, and then requests ImageData at 100% zoom from the image. Since theImageDataProvider
was now restricted to 100% zoom only, requesting imageData at other zoom levels during creation of image at system zoom level triggered recursive calls, causing a stack overflow error.The current commit reverts the behavior of the
ImageDataProvider
used inDPIUtil.autoScaleImageData
to return the sameImageData
at all zoom levels.We also Disable the strict zoom check in this specific case, because the scaling is done by
GC.drawImage()
and theImageData
used to create the image at system zoom level is expected to be same as the passedimageData
.Steps to reproduce
Execute Issue0445_HiDPISmoothScaling when your device zoom is something other than 200%. This would give stackoverflow error without this change