Bug 150941

Summary: [WinCairo][Video][MediaFoundation] Video should be rendered in provided graphics context.
Product: WebKit Reporter: peavo
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=198523
https://bugs.webkit.org/show_bug.cgi?id=235088
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch bfulgham: review+, bfulgham: commit-queue-

Description peavo 2015-11-05 13:13:41 PST
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.
Comment 1 peavo 2015-11-05 13:25:33 PST
Created attachment 264880 [details]
Patch
Comment 2 peavo 2015-11-05 13:28:20 PST
(In reply to comment #1)
> Created attachment 264880 [details]
> Patch

This patch is based on the EVR custom presenter example on MSDN.
Comment 3 Alex Christensen 2015-11-05 13:37:46 PST
This is exciting! I'll try this out and review it, but I probably won't get to it for a few days.
Comment 4 peavo 2015-11-05 13:39:10 PST
(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.
Comment 5 peavo 2015-11-05 13:41:29 PST
(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.
Comment 6 Alex Christensen 2015-11-05 22:06:09 PST
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.
Comment 7 Alex Christensen 2015-11-05 23:02:47 PST
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?
Comment 8 peavo 2015-11-06 12:08:48 PST
Created attachment 264947 [details]
Patch
Comment 9 peavo 2015-11-06 12:18:41 PST
(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.
Comment 10 peavo 2015-11-06 12:21:30 PST
(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 :)
Comment 11 peavo 2015-11-09 06:29:54 PST
Created attachment 265044 [details]
Patch
Comment 12 peavo 2015-11-09 07:25:45 PST
Created attachment 265048 [details]
Patch
Comment 13 peavo 2015-11-09 08:57:37 PST
(In reply to comment #12)
> Created attachment 265048 [details]
> Patch

I will upload yet another patch soon :)
Comment 14 peavo 2015-11-09 11:07:19 PST
Created attachment 265074 [details]
Patch
Comment 15 peavo 2015-11-09 11:10:09 PST
(In reply to comment #14)
> Created attachment 265074 [details]
> Patch

Fixed style issues, removed unneeded methods, and in general tightened the code.
Comment 16 Brent Fulgham 2015-11-09 11:45:42 PST
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.
Comment 17 peavo 2015-11-09 13:52:27 PST
(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 :)
Comment 18 peavo 2015-11-09 13:55:17 PST
Committed r192176: <http://trac.webkit.org/changeset/192176>.