RFR: 8273096: Add support for H.265/HEVC to JavaFX Media [v2]

[Kevin Rushforth]

On Tue, 16 Nov 2021 02:24:11 GMT, Alexander Matveev <almatvee at openjdk.org> wrote:

>> - Added support for H.265/HEVC for all 3 platforms.
>>  - Support is added only for .mp4 files over FILE/HTTP/HTTPS protocols. HTTP Live Streaming with H.265/HEVC is not supported.
>>  - On Windows mfwrapper was introduced which uses Media Foundation APIs to decode HEVC.
>>  - 10 and 12-bit HEVC was tested and also supported, however due to graphics pipeline not supporting 10-bit YUV rendering we will use color converter to convert video frame to 8-bit before sending it for rendering.
>>  - Resolution upto 4k is supported.
>> 
>> Additional runtime dependency requirements:
>> Windows: Windows 10 with HEVC Video Extensions installed.
>> macOS: macOS High Sierra and later
>> Linux: at least libavcodec56 and libswscale5
>> 
>> Additional build dependency:
>> Linux: libswscale-dev
>
> Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8273096: Add support for H.265/HEVC to JavaFX Media [v3]

Overall, this looks good. I left a few questions and comments inline.

modules/javafx.media/src/main/native/gstreamer/plugins/av/videodecoder.c line 525:

> 523:     if (decoder->sws_context == NULL || decoder->dest_frame == NULL ||
> 524:             decoder->sws_scale_func == NULL)
> 525:         return TRUE;

Should this be `return FALSE;`?

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 313:

> 311:     default:
> 312:         break;
> 313:     }

This block is a no-op. Is this intended? (and if so, is it needed?)

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 517:

> 515:     do
> 516:     {
> 517:         hr = decoder->pDecoder->GetOutputAvailableType(0, dwTypeIndex, &pDecoderOutputType);

Do you need to check `SUCCEEDED(hr)` here?

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 571:

> 569:         SafeRelease(&pDecoderOutputType);
> 570: 
> 571:     } while (hr != MF_E_NO_MORE_TYPES && hr != S_OK);

Should this be `FAILED(hr)` instead of `hr != S_OK`? Also, is it guaranteed to get success or `MF_E_NO_MORE_TYPES`? If not, this could lead to an infinite loop.

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 577:

> 575:     do
> 576:     {
> 577:         hr = decoder->pColorConvert->GetOutputAvailableType(0, dwTypeIndex, &pConverterOutputType);

Same questions for this loop as for the previous.

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 684:

> 682:         return FALSE;
> 683:     }
> 684:     else if (hr == MF_E_TRANSFORM_STREAM_CHANGE)

Is it OK that this case will return FALSE always?

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 723:

> 721: 
> 722:     if (SUCCEEDED(hr))
> 723:         hr = pMediaBuffer->Lock(&pBuffer, &cbMaxLength, &cbCurrentLength);

If this succeeds, but later operations fail, it seems possible that Unlock won't be called in some cases. Might this cause a problem?

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 730:

> 728:         GstMapInfo info;
> 729: 
> 730:         if (!gst_buffer_map(pGstBuffer, &info, GST_MAP_WRITE))

Do you need to check for `pGstBuffer == NULL`? My concern is that either `gst_buffer_map` will crash if it is, or else it return NULL and then `gst_buffer_unref` might crash.

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 791:

> 789:         return PO_FLUSHING;
> 790: 
> 791:     HRESULT hr = decoder->pDecoder->GetOutputStatus(&dwFlags);

Do you not need to check `hr` here?

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 811:

> 809:             do
> 810:             {
> 811:                 hr = decoder->pDecoder->GetOutputAvailableType(0, dwTypeIndex, &pOutputType);

Same question as earlier loops -- do you need to check `hr` (from previous time through loop) here?

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 963:

> 961:         // Ask decoder to produce all remaining data
> 962:         if (SUCCEEDED(hr))
> 963:             hr = decoder->pDecoder->ProcessMessage(MFT_MESSAGE_COMMAND_DRAIN, 0);

Does this `hr` need to be checked?

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 1101:

> 1099: 
> 1100:     // Get array count
> 1101:     arrayCount = *(guint8*)bdata;

Should this count be sanity checked?

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 1114:

> 1112:         nalUnitsCount = ((guint16)*(guint8*)bdata) << 8;
> 1113:         bdata++; in_bytes_count++;
> 1114:         nalUnitsCount |= (guint16)*(guint8*)bdata;

Should this count be sanity checked?

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 1123:

> 1121:             nalUnitLength = ((guint16)*(guint8*)bdata) << 8;
> 1122:             bdata++; in_bytes_count++;
> 1123:             nalUnitLength |= (guint16)*(guint8*)bdata;

Same question about count.

modules/javafx.media/src/main/native/gstreamer/plugins/mfwrapper/mfwrapper.cpp line 1346:

> 1344:     default:
> 1345:         break;
> 1346:     }

This is a no-op. Is it intended?

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

PR: https://git.openjdk.java.net/jfx/pull/649