Bug 150941 – [WinCairo][Video][MediaFoundation] Video should be rendered in provided graphics context.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

On WinCairo, we currently render video in a child window of the main browser window. This makes it difficult to render things on top of the video, like video controls and context menus. We should render the video in the graphics context provided by the paint method.

Created attachment 264880 [details]
Patch

(In reply to comment #1)
> Created attachment 264880 [details]
> Patch

This patch is based on the EVR custom presenter example on MSDN.

[Alex Christensen]

This is exciting! I'll try this out and review it, but I probably won't get to it for a few days.

(In reply to comment #3)
> This is exciting! I'll try this out and review it, but I probably won't get
> to it for a few days.

Thanks, no rush ;)

I've tested this patch on easyhtml5video.com. I've also been testing YouTube; there are some repaint issues there, since the invalidation of the frame view does not lead to a repaint of the video for some reason. I will look into that soon.

(In reply to comment #3)
> This is exciting! I'll try this out and review it, but I probably won't get
> to it for a few days.

Also, I haven't tested it in debug mode, so there might be some failing asserts in debug.

[Alex Christensen]

Comment on attachment 264880 [details]
Patch

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

http://www.w3.org/2010/05/video/mediaevents.html didn't draw in my debug build.  I'm sure this is very close to good, but r- because it needs a little more polishing.

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:28
> +const MFTIME oneSecond = 10000000; // One second in hns
> +const LONG oneMsec = 1000; // One msec in hns 

I assume hns means hundred nanoseconds, in which case oneMsec should be 10000 hns, not 1000.  I also think these should have better names.

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:1013
> +//////////////////////////////////////////////////////////////////////////

Probably not necessary.  This isn't done elsewhere in WebKit.

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:1021
> +    offset.fract = WORD(65536 * (v-offset.value));

Give 65536 a name.  std::numeric_limits<WORD>::max() + 1
Here and in MFOffsetToFloat, which should probably be right next to this.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:86
> +static const GUID MFSamplePresenterSampleCounter =
> +{ 0xb0bb83cc, 0xf10f, 0x4e2e, { 0xaa, 0x2b, 0x29, 0xea, 0x5e, 0x92, 0xef, 0x85 } };

Where did these come from?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:790
> +    : m_refCount(0)

Use m_refCount { 0 }; etc. in the header instead of having the default values here.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:1861
> +    if ((srcPAR.Numerator != destPAR.Numerator) || (srcPAR.Denominator != destPAR.Denominator)) {

Is the greatest common factor of Numerator and Denominator always 1?  I think a better way to determine if two fractions are equal is to cross multiply.  That way 1/2 would be equal to 2/4

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:2155
> +    ASSERT(MFGetAttributeUINT32(pSample, MFSamplePresenter_SampleCounter, (UINT32)-1) == m_TokenCounter);

pSample -> pSample.get()
MFSamplePresenter_SampleCounter -> MFSamplePresenterSampleCounter
There are two other places where you have underscores in comments that don't exist in the variable names.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:3437
> +        ctxt->drawSurfaceToContext(image, rect, rect, context);

This is where the magic happens!  Cool!

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:3454
> +//-----------------------------------------------------------------------------
> +// InitializeD3D
> +// 
> +// Initializes Direct3D and the Direct3D device manager.
> +//-----------------------------------------------------------------------------

Again, I don't think comments like this are necessary.

[Alex Christensen]

Comment on attachment 264880 [details]
Patch

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

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:9
> +#define CheckPointer(x, hr) if (x == nullptr) { return hr; }

WebKit style: if (!x)
Also, I think this macro isn't good.  Just have lots of early returns.  That'll make it more clear and consistent with the rest of WebKit.

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:82
> +        return m_pType != nullptr;

Again, no comparing with nullptr.  !!m_pType

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:87
> +        assert(IsValid());

ASSERT here and elsewhere.

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:100
> +        : m_pType(nullptr)
> +    {
> +        if (pType) {
> +            m_pType = pType;
> +            m_pType->AddRef();  
> +        }

This is strange.  I'd prefer this:
    : m_pType(pType)
{
    if (m_pType)
        m_pType->AddRef();
}

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:104
> +    // Copy ctor
> +    MediaType(const MediaType&  mt)

Comments like this not needed.

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:1133
> +template<class T>
> +class AsyncCallback : public IMFAsyncCallback {

This is cool, but it's only used once.  Is it always called on the main thread, or do we need Locks when using this?

Created attachment 264947 [details]
Patch

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

Thanks for the swift review :)

> 
> http://www.w3.org/2010/05/video/mediaevents.html didn't draw in my debug
> build.  I'm sure this is very close to good, but r- because it needs a
> little more polishing.
> 

I'll have a look at this testcase.

> > Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:86
> > +static const GUID MFSamplePresenterSampleCounter =
> > +{ 0xb0bb83cc, 0xf10f, 0x4e2e, { 0xaa, 0x2b, 0x29, 0xea, 0x5e, 0x92, 0xef, 0x85 } };
> 
> Where did these come from?
> 

These are GUIDs generated by Visual Studio.

(In reply to comment #7)
> Comment on attachment 264880 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264880&action=review
> 
> 
> > Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:1133
> > +template<class T>
> > +class AsyncCallback : public IMFAsyncCallback {
> 
> This is cool, but it's only used once.  Is it always called on the main
> thread, or do we need Locks when using this?

I removed this template, since the same thing can be accomplished by letting CustomVideoPresenter inherit from IMFAsyncCallback and implement the required methods :)

Created attachment 265044 [details]
Patch

Created attachment 265048 [details]
Patch

(In reply to comment #12)
> Created attachment 265048 [details]
> Patch

I will upload yet another patch soon :)

Created attachment 265074 [details]
Patch

(In reply to comment #14)
> Created attachment 265074 [details]
> Patch

Fixed style issues, removed unneeded methods, and in general tightened the code.

[Brent Fulgham]

Comment on attachment 265074 [details]
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.

(In reply to comment #16)
> Comment on attachment 265074 [details]
> 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.
> 

Thanks for reviewing!

> 
> > 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.
> 

Yes, it can be null during shutdown :)

Committed r192176: <http://trac.webkit.org/changeset/192176>.