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

[Alexander Matveev]

On Fri, 10 Dec 2021 19:34:25 GMT, Kevin Rushforth <kcr at 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