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
peavo
2015-11-05 13:13:41 PST
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. 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. 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 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. 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>. |