RFR: 8282999: Add support for EXT-X-MEDIA tag in HTTP Live Streaming [v3]

[Kevin Rushforth]

On Fri, 26 Apr 2024 23:36:26 GMT, Alexander Matveev <almatvee at openjdk.org> wrote:

>> - Added support for #EXT-X-MEDIA tag to HTTP Live Streaming.
>> - Following audio renditions via #EXT-X-MEDIA tag will be supported (see CSR for more details):
>>   - MP2T streams with one H.264/AVC video track and elementary AAC audio stream via #EXT-X-MEDIA tag.
>>   - fMP4 streams with one H.264/AVC or H.265/HEVC video track and elementary AAC audio stream via #EXT-X-MEDIA tag.
>>   - fMP4 streams with one H.264/AVC or H.265/HEVC video track and fMP4 streams with one AAC audio track via #EXT-X-MEDIA tag.
>> - Separate audio stream will be playback via separate chain of GStreamer elements inside one pipeline. Which means two "javasource" elements will be used inside one pipeline and they will be reading data independently of each other via two separate HLSConnectionHolders. GStreamer will handle audio and video synchronization based on PTS as for other streams. Other solutions were considered such as one "javasource" with multiple source pads, but such implementation will be more complex and does not provide any benefits.
>> - HLSConnectionHolder which handles video stream will also parse all information for separate audio stream and then create child HLSConnectionHolder for separate audio stream which will be responsible for downloading audio segments and seek of audio streams.
>> - Parser in HLSConnectionHolder was reworked to make it more readable and easy to maintain and extend.
>> - JavaDoc updated to point to latest HLS implementation vs old draft. It also updated with information on #EXT-X-MEDIA tag. Also, added missing information on AAC elementary streams and fMP4 from previous fixes.
>> - Fixed and improved debug output in Linux AV plugins.
>> - Added new property to "dshowwrapper" to disable PTS reset for each new segment, since with #EXT-X-MEDIA tag audio and video segments are not align and they can start at different time.
>> - Fixed missing PTS on first buffer after seek in MP2T demuxer in "dshowwrapper". Without it audio and video synchronization breaks with two separate streams.
>> - Removed dead code from MediaManager.
>> - Added handling for GST_MESSAGE_LATENCY. Based on GStreamer doc we need to call gst_bin_recalculate_latency() when such message received. Not sure if we really need to do this, but with separate video and audio streams we do receive this message when seek is done. Most likely due to video and audio is not align perfectly when we seek. For other streams this message is not received in most cases.
>
> Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8282999: Add for support EXT-X-MEDIA tag in HTTP Live Streaming [v2]

Testing looks good on all three platforms. I verified that HLS streams that use `EXT-X-MEDIA` tag for separate audio channel works (before the fix, it didn't on Windows and Linux).

I left one comment on the multiple top-level classes in `HLSConnectionHolder`, and I still need to review the changes in that class, but will wait until you address the feedback I left.

modules/javafx.media/src/main/java/com/sun/media/jfxmedia/locator/HLSConnectionHolder.java line 58:

> 56: import static com.sun.media.jfxmedia.locator.HLSConnectionHolder.HLS_VALUE_MIMETYPE_MP3;
> 57: import static com.sun.media.jfxmedia.locator.HLSConnectionHolder.HLS_VALUE_MIMETYPE_UNKNOWN;
> 58: import static com.sun.media.jfxmedia.locator.HLSConnectionHolder.stripParameters;

If you make the other classes in this file nested static classes, then you won't need these static imports.

modules/javafx.media/src/main/java/com/sun/media/jfxmedia/locator/HLSConnectionHolder.java line 480:

> 478: }
> 479: 
> 480: class PlaylistLoader extends Thread {

Having multiple top-level classes in the same source file is an anti-pattern. Is there a good reason that these can't be nested static classes? If not, then please make this change. You might be able to then make the nested classes private, although there is no harm in leaving them package scope.

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

PR Review: https://git.openjdk.org/jfx/pull/1435#pullrequestreview-2044045296
PR Review Comment: https://git.openjdk.org/jfx/pull/1435#discussion_r1592990614
PR Review Comment: https://git.openjdk.org/jfx/pull/1435#discussion_r1592989352