On Fri, 10 Dec 2021 19:34:25 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> 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]
>
> 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?)
Actually mfwrapper_change_state() is not needed, since it is not used at all.
Default handler should be just fine. It was copy-paste from dshowwrapper when I
created this element and forgot to remove it. In dshowwrapper it is used. I
will remove this function.
> 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?
No checks below should handle it in case if it fails.
> 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.
Fixed. Yes, it should be FAILED(hr) and it is not guaranteed to get success or
MF_E_NO_MORE_TYPES.
> 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.
Fixed as well.
> 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?
Yes, we return TRUE only if we got output from converter which is ready to be
delivered. MF_E_TRANSFORM_STREAM_CHANGE is needed to reconfigure converter. I
removed MF_E_TRANSFORM_NEED_MORE_INPUT since it is not needed.
> 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?
It can cause a problem, but it should be called, since nothing will change hr
value between Lock/Unlock. I will remove unnecessary check of hr value before
unlock. It will only miss unlock if cbCurrentLength == 0, which can happen for
empty buffers, but unlikely. I will fix it as well.
> 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.
Yes, we should. I fixed it.
> 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?
Yes, fixed.
> 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?
Fixed as well.
> 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?
No, I removed it. I think we still need to attempt reading data from decoder,
even if it refused DRAIN which asks decoder to produce all output it can
possible produce without any additional input.
> 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?
No, checks for in_bytes_count and out_bytes_count seems to handle any bad
values for arrayCount, nalUnitsCount or nalUnitLength just fine. I tested it
under debugger by modifying these variables.
> 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?
No, mfwrapper_src_query() is not needed. Removed. Same for
mfwrapper_src_event().
-------------
PR: https://git.openjdk.java.net/jfx/pull/649
