[Webkit-unassigned] [Bug 150941] [WinCairo][Video][MediaFoundation] Video should be rendered in provided graphics context.

[bugzilla-daemon at webkit.org]

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

Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #265074|review?                     |review+, commit-queue-
              Flags|                            |

--- Comment #16 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 265074
  --> https://bugs.webkit.org/attachment.cgi?id=265074
Patch

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

This looks great! I have a few suggestions before landing, but I think it looks good.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:834
> +ULONG STDMETHODCALLTYPE MediaPlayerPrivateMediaFoundation::CustomVideoPresenter::AddRef()

Please remove the STDMETHODCALLTYPE stuff from the *.cpp file.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:840
> +ULONG STDMETHODCALLTYPE MediaPlayerPrivateMediaFoundation::CustomVideoPresenter::Release()

Ditto.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:1081
> +        *ppv = static_cast<IMFVideoPresenter*>(this);

We should be checking for null ppv before dereferencing it.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:1678
> +static HRESULT GetVideoDisplayArea(IMFMediaType *type, MFVideoArea *area)

Do we need type and area null checks? Maybe we know that all uses have been null checked. The spacing looks wrong, since this isn't Objective C (i.e., "IMFMediaType *type" -> "IMFMediaType* type", "MFVideoArea *area" -> "MFVideoArea* area".

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:1684
> +    bPanScan = MFGetAttributeUINT32(type, MF_MT_PAN_SCAN_ENABLED, FALSE);

I prefer "BOOL bPanScan = ..." since nothing happens between declaring the BOOL and initializing it.

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

So we do check nulls in some of these functions. I feel like this should be consistent, unless we are in an especially tight loop that can't afford the test.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:1880
> +    hr = m_mixer->ProcessOutput(0, 1, &dataBuffer, &status);

Are there conditions where m_mixer can be null? If not, maybe we should access it through a reference instead.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:1957
> +    HRESULT hr = sample->QueryInterface(__uuidof(IMFTrackedSample), (void**)&tracked);

Do we ever have SUCCEEDED(hr) with a null "tracked"?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:1979
> +HRESULT MediaPlayerPrivateMediaFoundation::CustomVideoPresenter::onSampleFree(IMFAsyncResult* result)

Can 'result' ever be null?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:2043
> +    m_videoSampleQueue.append(sample);

Is it OK to append null samples?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:2749
> +        &device

This should really be reduced to a few lines. We don't typically use these vertically-extended MS-style formats.

-- 
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/20151109/cd90ed8c/attachment.html>