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?
@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.
Ok Jer, that totally makes sense indeed. I'll work on a patch in that direction.
Created attachment 345754 [details] Patch
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.
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?
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.
Created attachment 346480 [details] Patch
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".
Created attachment 346481 [details] Patch
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!
Created attachment 346484 [details] Patch
Created attachment 346492 [details] Patch
Created attachment 346498 [details] Patch
No idea what's wrong now with the win EWS... Welp :)
(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"
Committed r234582: <https://trac.webkit.org/changeset/234582>
<rdar://problem/42943258>