Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v6]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Make getShell32Icon method return multi-resolition image in case of requested icon size and actual icon size mismatch. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/a481b29b..10bae9be Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2875=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2875=04-05 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 20:01:37 GMT, Alexey Ivanov wrote:
>> Wherever it is necessary down the line we are wrapping the result in
>> multi-resolution and if we wrap it here too we will have multi-resolution
>> icon that contains multi-resolution images as a resolution variant.
>> Unfortunately AWT can't handle painting of such abomination.
>
> No, it isn't wrapped: if `getShell32Icon` is called in `getIcon(final boolean
> getLargeIcon)`, the returned value is immediately returned to the caller, see
> lines 1157–1163 in the updated code:
>
> if (hIcon <= 0) {
> if (isDirectory()) {
> return getShell32Icon(FOLDER_ICON_ID, size);
> } else {
> return getShell32Icon(FILE_ICON_ID, size);
> }
> }
>
> It's not wrapped into multi-resolution icon when called from
> `Win32ShellFolder2.get()` for keys `shell32Icon *` and `shell32LargeIcon *`
> (lines 411/413–414).
>
> Neither is the returned value of `Win32ShellFolder2.getSystemIcon` wrapped;
> it's also called from `Win32ShellFolder2.get()` when getting icons for
> `optionPaneIcon *` (lines 405/407). These icons are supposed to be large
> 32×32 icons, thus if the size of the icon in `getSystemIcon(SystemIcon
> iconType)` differs from 32, it should be wrapped. Otherwise, it could cause
> regression for
> [JDK-8151385](https://bugs.openjdk.java.net/browse/JDK-8151385): _[hidpi]
> JOptionPane-Icons only partially visible when using Windows 10 L_.
I see - but still it has to be solved in the getShell32Icon method - which i'm
going to do in a moment.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v5]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Slightly changing javadoc wording Removing unnecessary boxing of int Releasing the pIcon in native code properly - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/237d4070..a481b29b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2875=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2875=03-04 Stats: 6 lines in 3 files changed: 3 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 10:07:30 GMT, Alexey Ivanov wrote: >> Alexander Zuev has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - Small fixes >>Added testing for MultiResolutionImage >> - Merge remote-tracking branch 'origin/master' into JDK-8182043 >> - Update src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp >> >>Select one icon at a time. >> >>Co-authored-by: Alexey Ivanov >> <70774172+aivanov-...@users.noreply.github.com> >> - Small fixes >>Remived size parameter from makeIcon >>Removed useVGAColors usage as irrelevant since it is ignored on all >>supported platforms now >> - 8182043: Access to Windows Large Icons >>Fix updated based on the previous review. > > src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java > line 267: > >> 265: * returned will be a multi-resolution icon image, >> 266: * which will allow better scaling with different >> 267: * scaling factors. > > “…which will allow better support for High DPI environments with different > scaling factors.” — does it look better? > “scaling with … scaling factors” doesn't sound good to me. Yes, much better. I updated the wording and once the PR is approved i will fix it in CSR too. > src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line > 1098: > >> 1096: imageCache = getLargeIcon ? >> smallSystemImages : largeSystemImages; >> 1097: } >> 1098: newIcon2 = >> imageCache.get(Integer.valueOf(index)); > > Probably, explicit boxing is unnecessary. Removed. > src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp line 929: > >> 927: } >> 928: HRESULT hres; >> 929: IExtractIconW* pIcon; > > `pIcon->Release()` must be called before returning as done in `extractIcon`. Fixed. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 18:48:31 GMT, Alexander Zuev wrote:
>> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line
>> 1196:
>>
>>> 1194: Image icon = makeIcon(hIcon);
>>> 1195: disposeIcon(hIcon);
>>> 1196: return icon;
>>
>> Shall it not be wrapped into multi-resolution image if the `size` is
>> different from the actual size of the icon?
>> Previously, it was inside `makeIcon`.
>
> Wherever it is necessary down the line we are wrapping the result in
> multi-resolution and if we wrap it here too we will have multi-resolution
> icon that contains multi-resolution images as a resolution variant.
> Unfortunately AWT can't handle painting of such abomination.
No, it isn't wrapped: if `getShell32Icon` is called in `getIcon(final boolean
getLargeIcon)`, the returned value is immediately returned to the caller, see
lines 1157–1163 in the updated code:
if (hIcon <= 0) {
if (isDirectory()) {
return getShell32Icon(FOLDER_ICON_ID, size);
} else {
return getShell32Icon(FILE_ICON_ID, size);
}
}
It's not wrapped into multi-resolution icon when called from
`Win32ShellFolder2.get()` for keys `shell32Icon *` and `shell32LargeIcon *`
(lines 411/413–414).
Neither is the returned value of `Win32ShellFolder2.getSystemIcon` wrapped;
it's also called from `Win32ShellFolder2.get()` when getting icons for
`optionPaneIcon *` (lines 405/407). These icons are supposed to be large 32×32
icons, thus if the size of the icon in `getSystemIcon(SystemIcon iconType)`
differs from 32, it should be wrapped. Otherwise, it could cause regression for
[JDK-8151385](https://bugs.openjdk.java.net/browse/JDK-8151385): _[hidpi]
JOptionPane-Icons only partially visible when using Windows 10 L_.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 18:50:13 GMT, Alexander Zuev wrote:
>> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line
>> 1192:
>>
>>> 1190: */
>>> 1191: static Image getShell32Icon(int iconID, int size) {
>>> 1192: long hIcon = getIconResource("shell32.dll", iconID, size,
>>> size);
>>
>> It's outside the scope for this code review but still, should
>> `getIconResource` free the loaded library? With each call, the library gets
>> loaded but it's never freed and thus it's never unloaded even if it's not
>> needed any more.
>
> I will create a separate bug to track this - i do not know if library loading
> gets cached on some level and what impact on performance will we have if we
> release it every call. But i will check it out separately.
Sure! I just wanted to bring it up. I could've submitted the bug myself… yet I
decided to ask at first.
As far as I can see `JDK_LoadSystemLibrary` has no caching, it just loads the
library. If any icons are accessed `shell32.dll` will already be loaded.
Probably, we never fully release it from COM. So in this case, loading and
freeing will just increase and decrease the counters. Even if it's never really
unloaded, we should release the resources that's not needed any more.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 10:53:06 GMT, Alexey Ivanov wrote:
>> Alexander Zuev has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Small fixes
>>Added testing for MultiResolutionImage
>> - Merge remote-tracking branch 'origin/master' into JDK-8182043
>> - Update src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp
>>
>>Select one icon at a time.
>>
>>Co-authored-by: Alexey Ivanov
>> <70774172+aivanov-...@users.noreply.github.com>
>> - Small fixes
>>Remived size parameter from makeIcon
>>Removed useVGAColors usage as irrelevant since it is ignored on all
>>supported platforms now
>> - 8182043: Access to Windows Large Icons
>>Fix updated based on the previous review.
>
> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line
> 1192:
>
>> 1190: */
>> 1191: static Image getShell32Icon(int iconID, int size) {
>> 1192: long hIcon = getIconResource("shell32.dll", iconID, size,
>> size);
>
> It's outside the scope for this code review but still, should
> `getIconResource` free the loaded library? With each call, the library gets
> loaded but it's never freed and thus it's never unloaded even if it's not
> needed any more.
I will create a separate bug to track this - i do not know if library loading
gets cached on some level and what impact on performance will we have if we
release it every call. But i will check it out separately.
> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line
> 1196:
>
>> 1194: Image icon = makeIcon(hIcon);
>> 1195: disposeIcon(hIcon);
>> 1196: return icon;
>
> Shall it not be wrapped into multi-resolution image if the `size` is
> different from the actual size of the icon?
> Previously, it was inside `makeIcon`.
Wherever it is necessary down the line we are wrapping the result in
multi-resolution and if we wrap it here too we will have multi-resolution icon
that contains multi-resolution images as a resolution variant. Unfortunately
AWT can't handle painting of such abomination.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8250658: Performance of ClipFlatOval Renderperf test is very low
On Fri, 7 May 2021 22:29:53 GMT, Alexey Ushakov wrote: > Implemented indirect rendering (via stencil texture attachment) to stencil > texture src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLClip.h line 52: > 50: @property (readonly) id stencilTextureRef; > 51: @property (readonly) BOOL stencilMaskGenerationInProgress; > 52: @property (readwrite ) BOOL stencilMaskGenerationStarted; Is 'stencilMaskGenerationStarted' property needed? I don't see it being used anywhere to compare. - PR: https://git.openjdk.java.net/jdk/pull/3929
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]
On Sat, 8 May 2021 00:19:21 GMT, Alexander Zuev wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Small fixes
>Added testing for MultiResolutionImage
> - Merge remote-tracking branch 'origin/master' into JDK-8182043
> - Update src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp
>
>Select one icon at a time.
>
>Co-authored-by: Alexey Ivanov
> <70774172+aivanov-...@users.noreply.github.com>
> - Small fixes
>Remived size parameter from makeIcon
>Removed useVGAColors usage as irrelevant since it is ignored on all
>supported platforms now
> - 8182043: Access to Windows Large Icons
>Fix updated based on the previous review.
Changes requested by aivanov (Reviewer).
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1098:
> 1096: imageCache = getLargeIcon ?
> smallSystemImages : largeSystemImages;
> 1097: }
> 1098: newIcon2 =
> imageCache.get(Integer.valueOf(index));
Probably, explicit boxing is unnecessary.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1192:
> 1190: */
> 1191: static Image getShell32Icon(int iconID, int size) {
> 1192: long hIcon = getIconResource("shell32.dll", iconID, size, size);
It's outside the scope for this code review but still, should `getIconResource`
free the loaded library? With each call, the library gets loaded but it's never
freed and thus it's never unloaded even if it's not needed any more.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1196:
> 1194: Image icon = makeIcon(hIcon);
> 1195: disposeIcon(hIcon);
> 1196: return icon;
Shall it not be wrapped into multi-resolution image if the `size` is different
from the actual size of the icon?
Previously, it was inside `makeIcon`.
src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp line 929:
> 927: }
> 928: HRESULT hres;
> 929: IExtractIconW* pIcon;
`pIcon->Release()` must be called before returning as done in `extractIcon`.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]
On Fri, 7 May 2021 17:30:15 GMT, Alexander Zuev wrote: >> src/java.desktop/share/classes/sun/awt/shell/ShellFolder.java line 213: >> >>> 211: * Returns the icon of the specified size used to display this >>> shell folder. >>> 212: * >>> 213: * @param size size of the icon > 0 (Valid range: 1 to 256) >> >> I'm unsure the valid range of 1 to 256 makes sense provided the icon smaller >> than 16×16 is never returned (on Windows at least). > > Well, user still can request 1x1 icon - we will return the multiresolution > image with minimal (1x1) icon inside that will be scaled every time the > actual painting occurs. The size will define the layout of the component with > the icon and can be auto-generated from the user code so i do not see why we > should limit the lowest requested size - especially in the shared instance > that not only specific for Windows platform. Even though 1×1 icon doesn't make much sense, you're right imposing a limitation is no good either. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v4]
On Sat, 8 May 2021 00:19:21 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Small fixes >Added testing for MultiResolutionImage > - Merge remote-tracking branch 'origin/master' into JDK-8182043 > - Update src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp > >Select one icon at a time. > >Co-authored-by: Alexey Ivanov > <70774172+aivanov-...@users.noreply.github.com> > - Small fixes >Remived size parameter from makeIcon >Removed useVGAColors usage as irrelevant since it is ignored on all >supported platforms now > - 8182043: Access to Windows Large Icons >Fix updated based on the previous review. src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 267: > 265: * returned will be a multi-resolution icon image, > 266: * which will allow better scaling with different > 267: * scaling factors. “…which will allow better support for High DPI environments with different scaling factors.” — does it look better? “scaling with … scaling factors” doesn't sound good to me. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8250658: Performance of ClipFlatOval Renderperf test is very low
On Fri, 7 May 2021 22:29:53 GMT, Alexey Ushakov wrote: > Implemented indirect rendering (via stencil texture attachment) to stencil > texture Its good that we have removed intermediate stencil blit operations using this approach. LGTM. jtreg/JCK all test run is fine. J2DDemo, SwingSet2, Font2DTest is running fine in my Intel SoC Macbook pro. RenderPerfTest : ClipFlatOval and ClipFlatOvalAA show improvement in FPS from ~13 to ~24 in my Intel SoC Macbook pro. @aghaisas mentioned that he is also testing on discrete GPU Macbook Pro. Lets wait for his test output. - Marked as reviewed by jdv (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3929
