[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

Alex Christensen <achristensen at apple.com> changed:

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

--- Comment #6 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 264880
  --> https://bugs.webkit.org/attachment.cgi?id=264880
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.

-- 
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/20151106/2ca86f42/attachment-0001.html>