Bug 156020 – [WinCairo][MediaFoundation] Video size is not always set.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

This happens because getting the video display control from the media session might fail the first time. If it fails the first time, we should try again later.

Created attachment 275192 [details]
Patch

[Darin Adler]

Comment on attachment 275192 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Getting the video display control object from the media session might fail the first time.
> +        In case it fails, we should try again when setting the video size.

Is keeping the video display control object an important optimization? Maybe we should just get it every time.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:353
> +    COMPtr<IMFVideoDisplayControl> videoDisplay = getVideoDisplay();

Probably a little nicer to use auto here.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:778
> +    if (FAILED(MFGetServicePtr()(m_mediaSession.get(), MR_VIDEO_RENDER_SERVICE, IID_PPV_ARGS(&m_videoDisplay))))
> +        return nullptr;
> +
> +    return m_videoDisplay;

Checking FAILED explicitly here means we don’t trust that m_videoDisplay will be nullptr if the function calls. We check FAILED and explicitly return nullptr rather than just returning m_videoDisplay, which will be nullptr.

But when this function is called again, whatever is stored in m_videoDisplay *will* be trusted.

So we should omit the FAILED check since it does us no good.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:794
> +    COMPtr<IMFVideoDisplayControl> videoDisplay = getVideoDisplay();
> +    if (videoDisplay) {

Would read nicer with auto and nesting the assignment in the if statement.

    if (auto videoDisplay = this->videoDisplay()) {

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:138
> +    COMPtr<IMFVideoDisplayControl> getVideoDisplay();

In WebKit coding style we do not use “get” in the names of functions like this one unless they produce values through "out arguments". We can either just call this videoDisplay or use some verb other than “get”.

Thanks for reviewing!

Committed r198848: <http://trac.webkit.org/changeset/198848>