On Fri, 5 May 2023 09:33:00 GMT, Maxim Kartashev <mkartas...@openjdk.org> wrote:
>> Alexander Zvegintsev has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - update, based on review comments
>> - remove wayland detection
>> - BUILD_LIBPIPEWIRE_HEADER_DIRS -> LIBPIPEWIRE_HEADER_DIRS
>
> src/java.desktop/unix/classes/sun/awt/UNIXToolkit.java line 428:
>
>> 426:
>> 427: @SuppressWarnings("removal")
>> 428: public static boolean isOnWayland() {
>
> It seems that this method doesn't have to part of a public interface of
> `UNIXToolkit`.
You're right. Furthermore, Wayland detection is no longer used in this change,
so I am removing it.
It is now in #13830, where it belongs.
> src/java.desktop/unix/classes/sun/awt/screencast/ScreencastHelper.java line
> 56:
>
>> 54: }
>> 55:
>> 56: private static final int DENIED = -11; //defined in native code
>
> The comment is confusing as the field is defined right here, not in native
> code.
removed.
> src/java.desktop/unix/classes/sun/awt/screencast/ScreencastHelper.java line
> 58:
>
>> 56: private static final int DENIED = -11; //defined in native code
>> 57:
>> 58: private static synchronized native int getRGBPixelsImpl(
>
> Is `synchronized` really needed on the declaration of `getRGBPixelsImpl()`?
This code was written with the understanding that it will not run in parallel,
so the `synchronized` is used.
Moreover, we will get a significant benefit from removing `synchronized` only
if `getRGBPixelsImpl` is called frequently from different threads.
If it is called from a single thread(which seems to me to be the main use
case), the results of my quick tests with JMH show that the difference between
synchronized and not synchronized is within the margin of error.
Removing the `synchronized` keyword would require additional testing, which at
this point I want to avoid.
And since this is not a public API we have no problem revisiting it later.
P.S. I removed `synchronized` modifier from `getRGBPixels`
> src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 50:
>
>> 48: jmethodID storeTokenMethodID = NULL;
>> 49:
>> 50: inline void debug_screencast(
>
> Does this one need to be externally visible? If not, it can be (also) made
> `static`.
It is used by `screencast_portal.c` and `screencast_pipewire.c`.
> src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 138:
>
>> 136: }
>> 137:
>> 138: static inline void convertRGBxToBGRx(int* in) {
>
> Will this work correctly on little-endian ARM systems?
I haven't tested it, but I'll try to find a machine to test it on.
> src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 602:
>
>> 600: }
>> 601:
>> 602: pipewire_libhandle = dlopen("libpipewire-0.3.so.0",
>
> There's a useful macro for wrapping library names:
> `VERSIONED_JNI_LIB_NAME("pipewire-0.3", "0")`
Updated.
> src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 722:
>
>> 720:
>> 721: const gchar *token = jtoken
>> 722: ? token = (*env)->GetStringUTFChars(env,
>> jtoken, NULL)
>
> The variable `token` is unnecessarily used in its own initializer here.
Fixed.
> src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 789:
>
>> 787: env, pixelArray,
>> 788: start, len,
>> 789: ((jint *) screenProps->captureData)
>
> I'm not sure if endianness of `captureData` matches the expected endianness
> of `pixelArray`.
For now, it is going to be supported on x86-64 only, but it is a good candidate
for investigation.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1186164127
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1186163925
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1186163799
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1186163601
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1186163575
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1186163520
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1186163465
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1186163401
