On Tue, 16 Nov 2021 02:24:11 GMT, Alexander Matveev <almat...@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
