RFR: 8287822: [macos] Remove support of duplicated formats from macOS

Kevin Rushforth kcr at openjdk.org
Tue Oct 11 21:52:17 UTC 2022


On Tue, 11 Oct 2022 15:44:46 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> - Added support for JAR and JRT protocol to AVFoundation platform.
>>  - Removed H.264/MP3 and AAC support from GStreamer platform, which was primary used to playback these formats for JAR and JRT protocols.
>>  - Added ability to FXMediaPlayer sample to generate playlist for JAR and JRT protocols for testing. See FXMedia.java for how to use it.
>>  - H.265/HEVC via JAR/JRT protocols should work on macOS after this change. Before it did not work because GStreamer platform did not support H.265/HEVC and AVFoundation did not support JAR/JRT protocols.
>>  - Minor code clean up.
>> 
>> After this changes:
>> GSTPlatform: AIFF and WAV for all protocols.
>> AVFoundation: MP3, AAC, HLS, H.264 and H.265 for all protocols.
>> 
>> This change is transparent for end user and does not affect list of supported formats by JavaFX Media.
>
> modules/javafx.media/src/main/native/jfxmedia/Locator/Locator.cpp line 73:
> 
>> 71:     if (javaEnv.clearException())
>> 72:         return NULL;
>> 73: 
> 
> minor: would it make sense to wrap return in { }, line 67 and 72?

A lot of our third-party native code doesn't (I would insist on the `{}` if this were Java code), so if this were a gstreamer file it would be better to follow their convention, but since this is entirely our code, it makes sense to enclose in `{}`.

> tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java line 66:
> 
>> 64:  * [Windows] jlink --output dist/FXMediaPlayer -p ../../../../build/jmods;dist --add-modules javafx.base,javafx.controls,javafx.media,javafx.graphics,FXMediaPlayer --launcher FXMediaPlayer=FXMediaPlayer/fxmediaplayer.FXMediaPlayer
>> 65:  * [Windows] dist\FXMediaPlayer\bin\FXMediaPlayer.bat
>> 66:  */
> 
> minor: this javadoc will likely not render as expected.  perhaps use <pre> from line 50 to 61 or 65.
> also, spelling "protcol" on line 50

Also minor: you can shorten the command lines by removing `javafx.base` and `javafx.graphics` from the list of added modules, since `javafx.controls` transiently requires them.

> tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java line 97:
> 
>> 95:                 uri = FXMedia.class.getResource("/fxmediaplayer/media").toURI();
>> 96: 
>> 97:                 if (uri.getScheme().equals("jar")) {
> 
> is it possible for uri scheme to be null?
> may be "jar".equals(uri.getScheme()) ?

Related question: Can `uri` itself be null?

> tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java line 131:
> 
>> 129:             });
>> 130:         } catch (URISyntaxException | IOException ex) {
>> 131:             System.out.println("Exception: " + ex);
> 
> should System.out be removed?
> 
> general question: do we want to use logging in places like this?

Since this is a test app, I wouldn't bother with logging. We generally would use `System.err` rather than `System.out` for printing exceptions, but that's a minor point.

-------------

PR: https://git.openjdk.org/jfx/pull/909


More information about the openjfx-dev mailing list