[Webkit-unassigned] [Bug 156020] [WinCairo][MediaFoundation] Video size is not always set.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=156020

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #275192|review?                     |review+
              Flags|                            |

--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 275192
  --> https://bugs.webkit.org/attachment.cgi?id=275192
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”.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160330/63d5c1ae/attachment.html>