Bug 140337 – [WinCairo][Video] Windows Media Foundation implementation is not completed.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

This patch aims to complete some of the methods which are not implemented. Currently, only MP4 is supported. Video is rendered in a child window of the main window. We should eventually render the video directly in the main window, by reading and painting individual video frames from the stream.

Created attachment 244411 [details]
Patch

I have used http://html5videoformatconverter.com/html5-video-tag-example.html for testing.

[Alex Christensen]

Comment on attachment 244411 [details]
Patch

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

This looks like some pretty dang good work in progress.  I wouldn't be opposed to having this in WebKit and it can be polished from here.  What do you think, Brent?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:212
> +        ::MoveWindow(m_hwndVideo, rect.x(), rect.y(), rect.width(), rect.height(), FALSE);

Does this just move the child window over where the video element is supposed to be?  Does it draw controls below the child window?  What if the parent window is moved or not in the coordinates (0,0)?

Created attachment 244540 [details]
Patch

(In reply to comment #3)
> Comment on attachment 244411 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244411&action=review
> 
> This looks like some pretty dang good work in progress.  I wouldn't be
> opposed to having this in WebKit and it can be polished from here.  What do
> you think, Brent?
> 
> > Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:212
> > +        ::MoveWindow(m_hwndVideo, rect.x(), rect.y(), rect.width(), rect.height(), FALSE);
> 
> Does this just move the child window over where the video element is
> supposed to be?  Does it draw controls below the child window?  What if the
> parent window is moved or not in the coordinates (0,0)?

Thanks for reviewing :)

I've changed this in the latest patch. The child window will be moved along with the parent, and scrolling should also work (with a little lag, though). Unfortunately, the controls are drawn beneath the child window, I think. This should be fixed by drawing individual frames directly into the main window, which the Media Foundation api supports. This should be the next step, I believe.

[Brent Fulgham]

Comment on attachment 244540 [details]
Patch

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

A really great start! Thank you for working on this. I think this patch needs a little attention before we land it, but it is getting very close.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:120
> +    HRESULT hr = m_mediaSession->Start(nullptr, &varStart);

What should we do if Start fails? Do we need to log anything that would help us understand why the media session failed to start? Maybe we should ASSERT on this?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:124
> +    m_paused = false;

Is this true, if m_mediaSession->Start fails?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:132
> +    m_mediaSession->Pause();

Does this have an HRESULT return code? If so, should we do something with it (e.g., maybe the media cannot be paused for some reason?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:236
> +    if (FAILED(hr))

You could probably combine these two lines if you don't care about hanging onto the HRESULT for logging or debugging.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:250
> +        m_mediaSession->Release();

Can we set m_mediaSession to nullptr here?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:253
> +    HRESULT hr = MFShutdown();

Maybe you should ASSERT here if HRESULT is some kind of failure so we can see this in debug builds?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:276
> +        );

We don't usually use this one-line-per-argument syntax. Could you tighten this into a couple of lines, please?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:345
> +    // TODO: cleanup on failure

OOPS! :-)

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:366
> +        if (!ok)

I think the two preceding lines could be combined.  "if (!addBranchToPartialTopology(i))"

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:377
> +    COMPtr<IMFTopologyNode> outputNode;

Could you please move these closer to the point where they are first used, rather than just having a block of declarations here?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:387
> +    if (selected) {

Please make this an early return.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:390
> +        if (!ok)

In general, if you aren't going to use 'ok' for logging, I think this should just be written "if (!createSourceStreamNode(...)"

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:393
> +        // Create the output node for the renderer.

This comment just states what the next line does. Please remove it.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:395
> +        if (!ok)

Ditto: "if (!createOutputNode(...)"

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:408
> +        hr = sourceNode->ConnectOutput(0, outputNode.get(), 0);

Should either of these 0's be nullptr?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:441
> +    wcex.lpszMenuName = 0;

nullptr

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:448
> +    return 0;

return nullptr; please!

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:471
> +{

Should you be checking for nullptr here, like you do in the other ::create methods you wrote?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:472
> +    COMPtr<IMFMediaTypeHandler> handler;

Please move declaration down to where it is first used.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:473
> +    COMPtr<IMFActivate> rendererActivate;

Ditto.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:475
> +    GUID guidMajorType = GUID_NULL;

Please move declaration down to where it is first used.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:479
> +    sourceSD->GetStreamIdentifier(&streamID); // Just for debugging, ignore any failures.

Should this be wrapped in an #ifndef NDEBUG?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:481
> +    // Get the media type handler for the stream.

I don't think this comment is helpful, since it just restates the next line.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:486
> +    // Get the major media type.

I don't think this comment is helpful.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:518
> +    return true;

Maybe this should just be "return SUCCEEDED(hr);" and get rid of the if case?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:557
> +        return;

This seems a little weird. Is there supposed to be more? Or maybe just a TODO?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:587
> +HRESULT STDMETHODCALLTYPE MediaPlayerPrivateMediaFoundation::AsyncCallback::QueryInterface(REFIID riid, __RPC__deref_out void __RPC_FAR *__RPC_FAR *ppvObject)

Please remove the STDMETHODCALLTYPE declarations in the implementation file. They are only needed in the method declarations.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:594
> +    }

I think the above might be clearer written as an early return for the !IsEqualGUIID case, then just include *ppvObject = this down where you do the AddRef.

[Alex Christensen]

Thanks for the more thorough review, Brent.  This patch is basically what I meant to do with https://bugs.webkit.org/show_bug.cgi?id=132143 so I'm marking that one as a duplicate.

[Alex Christensen]

*** Bug 132143 has been marked as a duplicate of this bug. ***

Created attachment 244624 [details]
Patch

Thanks for the review! The review comments are addressed in the latest patch.

[Alex Christensen]

Comment on attachment 244624 [details]
Patch

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

This is great!  Let's do another iteration of refining before putting it in, though.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:49
> +    , m_hwndVideo(0)

This should probably be nullptr.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:133
> +    HRESULT hr = m_mediaSession->Pause();

This could just be put into the next line.  We don't really need an explicit local variable name if we only use it once outside of an assert.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:239
> +    AsyncCallback* callback = new AsyncCallback(this, true);

Where is this deleted?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:240
> +    m_mediaSession->BeginGetEvent(callback, nullptr);

Should we check the return value of this to see if it succeeded?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:261
> +    HRESULT hr = MFCreateSourceResolver(&m_sourceResolver);

Again, this could just be put inside the next line.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:262
> +    if (!SUCCEEDED(hr))

That's what the FAILED macro is for.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:272
> +    return SUCCEEDED(hr);

This should always be true, right?  Otherwise it would have returned early.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:327
> +        AsyncCallback* callback = new AsyncCallback(this, true);

Again, should this be deleted?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:353
> +    for (int i = 0; i < sourceStreams; i++) {

i should be a DWORD to keep type comparisons as consistent as possible.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:420
> +    wcex.hInstance = 0;

nullptr

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:421
> +    wcex.hIcon = 0;

nullptr? Is HICON a pointer type?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:423
> +    wcex.hbrBackground = 0;

probably nullptr.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:426
> +    wcex.hIconSm = 0;

probably nullptr.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:436
> +    HWND hWndParent = 0;

nullptr

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:440
> +    if (view && view->hostWindow())

else return early, I think.  Otherwise CreateWindowEx could be given nullptr as its parent.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:543
> +    m_videoDisplay->SetVideoPosition(nullptr, &rc);

It would be good to assert m_videoDisplay and m_player before using them here.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:583
> +    int refCount = m_refCount;

ULONG.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:118
> +        virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID riid, __RPC__deref_out void __RPC_FAR *__RPC_FAR *ppvObject);

These should probably have override after them.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:126
> +        int m_refCount;

This should be a ULONG.

[Alex Christensen]

It works!
To get it to link, I had to add these libraries to WebKitCFLite.props:
Mfplat.lib;Mf.lib;mfuuid.lib;strmiids.lib;
It also crashes every time with http://www.w3.org/2010/05/video/mediaevents.html

Created attachment 244863 [details]
Patch

(In reply to comment #11)
> Comment on attachment 244624 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244624&action=review
> 

Thanks for reviewing :) Updated patch.

>Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:239
> > +    AsyncCallback* callback = new AsyncCallback(this, true);
> 
> Where is this deleted?
> 

It's deleted when Media Foundation releases its reference.

[Alex Christensen]

Comment on attachment 244863 [details]
Patch

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

Does this really link successfully without linking to additional libraries?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:451
> +        DestroyWindow(m_hwndVideo);

You should probably set m_hwndVideo to nullptr after this.

Created attachment 244914 [details]
Patch

(In reply to comment #15)
> Comment on attachment 244863 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244863&action=review
> 
> Does this really link successfully without linking to additional libraries?
> 

Sorry, forgot about this one, added in latest patch :)

[Alex Christensen]

Comment on attachment 244914 [details]
Patch

Hooray!

(In reply to comment #18)
> Comment on attachment 244914 [details]
> Patch
> 
> Hooray!

Thanks for the review and helpful feedback :)

[WebKit Commit Bot]

Comment on attachment 244914 [details]
Patch

Clearing flags on attachment: 244914

Committed r178671: <http://trac.webkit.org/changeset/178671>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.