[Webkit-unassigned] [Bug 140337] [WinCairo][Video] Windows Media Foundation implementation is not completed.

[bugzilla-daemon at webkit.org]

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

Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #244540|review?                     |review-
              Flags|                            |

--- Comment #6 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 244540
  --> https://bugs.webkit.org/attachment.cgi?id=244540
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150113/8c167707/attachment-0002.html>