On Tue, 11 Oct 2022 15:44:46 GMT, Andy Goryachev <ango...@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
