Bug 187850 – [MediaCapabilities] Platform integration

• Component: Media
• Status: RESOLVED FIXED

[Philippe Normand]

The current implementation of MediaCapabilities::decodingInfo() and MediaCapabilities::encodingInfo() are incomplete.
I wonder if the platform probing should be done by extending MediaPlayer with new static functions, would that be acceptable or should I implement a new platform abstraction for this?

[Jer Noble]

@Philippe, we've introduced new platform abstractions, even for MediaPlayer-like capabilities, for Modern EME. I think it would be acceptable to introduce a new abstraction rather than re-use MediaPlayer.

[Philippe Normand]

Ok Jer, that totally makes sense indeed. I'll work on a patch in that direction.

[Philippe Normand]

Created attachment 345754 [details]
Patch

[Philippe Normand]

This is my current WIP patch which has a decodingInfo() platform integration and a mock test. Before going further I'd like to gather some initial feedback on the approach taken.

[Eric Carlson]

Comment on attachment 345754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345754&action=review

> Source/WebCore/Modules/mediacapabilities/MediaCapabilities.cpp:203
> +        auto info = MediaCapabilitiesInfo::create();
> +        auto engineConfiguration = MediaEngineConfigurationFactory::createDecodingConfiguration(configuration);

Nit: I wonder if it would be better to pass a lambda to createDecodingConfiguration in case a platform isn't able to get the platform capabilities synchronously?

> Source/WebCore/platform/mediacapabilities/MediaEngineConfiguration.cpp:42
> +    if (ok)
> +        return;

Shouldn't this be "if (!ok)"?

> Source/WebCore/platform/mediacapabilities/MediaEngineConfiguration.cpp:60
> +    double numerator = frameratePieces[0].toDouble(&ok);
> +    if (!ok)
> +        return;
> +
> +    double denominator = frameratePieces[1].toDouble(&ok);
> +    if (!ok)
> +        return;
> +
> +    if (!std::isfinite(numerator) || !std::isfinite(denominator))
> +        return;
> +
> +    m_framerate = numerator / denominator;

Nit: I think it would be better to store the numerator and denominator in case a platform can use a rational number. You can always keep the frameRate() method and do the division there.

> Source/WebCore/testing/Internals.cpp:512
> +    MediaEngineConfigurationFactory::disableMock();

This will leave GStreamer and AVFoundation disabled.

> Source/WebCore/testing/Internals.cpp:3447
> +#if USE(AVFOUNDATION)
> +    WebCore::DeprecatedGlobalSettings::setAVFoundationEnabled(false);
> +#endif
> +#if USE(GSTREAMER)
> +    WebCore::DeprecatedGlobalSettings::setGStreamerEnabled(false);
> +#endif

Why is this necessary?

[Philippe Normand]

Comment on attachment 345754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345754&action=review

Thanks for the early feedback Eric. I'll update the patch, add audio support to the decoding config, add a mock implementation for the encoding side, add a few more tests and fix EWS

>> Source/WebCore/Modules/mediacapabilities/MediaCapabilities.cpp:203
>> +        auto engineConfiguration = MediaEngineConfigurationFactory::createDecodingConfiguration(configuration);
> 
> Nit: I wonder if it would be better to pass a lambda to createDecodingConfiguration in case a platform isn't able to get the platform capabilities synchronously?

Good point! The GStreamer implementation shouldn't need this but I can change the code already if the AVF backend will require it.

>> Source/WebCore/platform/mediacapabilities/MediaEngineConfiguration.cpp:42
>> +        return;
> 
> Shouldn't this be "if (!ok)"?

Oh, indeed :)

>> Source/WebCore/platform/mediacapabilities/MediaEngineConfiguration.cpp:60
>> +    m_framerate = numerator / denominator;
> 
> Nit: I think it would be better to store the numerator and denominator in case a platform can use a rational number. You can always keep the frameRate() method and do the division there.

Makes sense!

>> Source/WebCore/testing/Internals.cpp:512
>> +    MediaEngineConfigurationFactory::disableMock();
> 
> This will leave GStreamer and AVFoundation disabled.

Right, but the code below should be removed, as you hinted already.

>> Source/WebCore/testing/Internals.cpp:3447
>> +#endif
> 
> Why is this necessary?

Ah no, it isn't, because the factory checks the mock case first anyway.

[Philippe Normand]

Created attachment 346480 [details]
Patch

[Eric Carlson]

Comment on attachment 346480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346480&action=review

> Source/WebCore/Modules/mediacapabilities/MediaCapabilities.cpp:202
> +        MediaEngineConfigurationFactory::DecodingConfigurationCallback callback = [promise = WTFMove(promise)] (RefPtr<MediaEngineDecodingConfiguration> engineConfiguration) mutable {

Nit: I think you can use "auto" here.

> Source/WebCore/Modules/mediacapabilities/MediaCapabilities.cpp:252
> +        MediaEngineConfigurationFactory::EncodingConfigurationCallback callback = [promise = WTFMove(promise)] (RefPtr<MediaEngineEncodingConfiguration> engineConfiguration) mutable {

Ditto.

> Source/WebCore/platform/mediacapabilities/MediaEngineConfiguration.cpp:56
> +    double numerator = frameratePieces[0].toDouble(&ok);
> +    if (!ok)
> +        return;
> +
> +    double denominator = frameratePieces[1].toDouble(&ok);
> +    if (!ok)
> +        return;

You should also fail if numerator or denominator is zero.

> Source/WebCore/platform/mock/MediaEngineDecodingConfigurationMock.cpp:43
> +    RefPtr<MediaEngineVideoConfiguration> videoConfig = videoConfiguration();

Nit: you can use "auto" here.

> Source/WebCore/platform/mock/MediaEngineDecodingConfigurationMock.cpp:51
> +    RefPtr<MediaEngineAudioConfiguration> audioConfig = audioConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineDecodingConfigurationMock.cpp:60
> +    RefPtr<MediaEngineVideoConfiguration> videoConfig = videoConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineDecodingConfigurationMock.cpp:66
> +    RefPtr<MediaEngineAudioConfiguration> audioConfig = audioConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineDecodingConfigurationMock.cpp:75
> +    RefPtr<MediaEngineVideoConfiguration> videoConfig = videoConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineDecodingConfigurationMock.cpp:81
> +    RefPtr<MediaEngineAudioConfiguration> audioConfig = audioConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:43
> +    RefPtr<MediaEngineVideoConfiguration> videoConfig = videoConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:51
> +    RefPtr<MediaEngineAudioConfiguration> audioConfig = audioConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:60
> +    RefPtr<MediaEngineVideoConfiguration> videoConfig = videoConfiguration();

Ditto.

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:62
> +    if (videoConfig) {
> +        if (videoConfig->framerate() > 30)

Nit: these two lines can be collapsed.

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:66
> +    RefPtr<MediaEngineAudioConfiguration> audioConfig = audioConfiguration();

"auto".

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:77
> +    RefPtr<MediaEngineVideoConfiguration> videoConfig = videoConfiguration();
> +    if (videoConfig) {
> +        if (videoConfig->contentType().containerType() != "video/mp4")

Ditto the above.

> Source/WebCore/platform/mock/MediaEngineEncodingConfigurationMock.cpp:81
> +    RefPtr<MediaEngineAudioConfiguration> audioConfig = audioConfiguration();

"auto".

[Philippe Normand]

Created attachment 346481 [details]
Patch

[Philippe Normand]

Comment on attachment 346481 [details]
Patch

Ah, sorry I didn't notice your review. This patch was a win EWS build fix attempt. Will update the patch again taking into account your review :) Thanks!

[Philippe Normand]

Created attachment 346484 [details]
Patch

[Philippe Normand]

Created attachment 346492 [details]
Patch

[Philippe Normand]

Created attachment 346498 [details]
Patch

[Philippe Normand]

No idea what's wrong now with the win EWS... Welp :)

[Eric Carlson]

(In reply to Philippe Normand from comment #14)
> No idea what's wrong now with the win EWS... Welp :)

Not likely your fault: "Unable to build without patch"

[Philippe Normand]

Committed r234582: <https://trac.webkit.org/changeset/234582>

[Radar WebKit Bug Importer]

<rdar://problem/42943258>