Bug 142594 – [MediaFoundation] Implement seek

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Alex Christensen]

I found some old code from my experimenting with Media Foundation.  This should probably be upstreamed, but I don't have a machine to test this on right now, and the media controls are covered up by our child window.  Peavo, could you look at this?

[Alex Christensen]

Created attachment 248450 [details]
Patch

[Darin Adler]

Comment on attachment 248450 [details]
Patch

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

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:173
> +    const __int64 tenMegaHertz = 10000000;

Not sure this type should be __int64; given how the calculation is done, seems it should just be a double.

Also, megahertz is one word, not two, so there should not be an interact.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:182
> +    if (!SUCCEEDED(m_mediaSession->Start(&GUID_NULL, &propVariant))) {
> +        PropVariantClear(&propVariant);
> +        return;
> +    }
> +    PropVariantClear(&propVariant);

Why the if statement? We call PropVariantClear unconditionally.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:187
> +    const __int64 tenMegaHertz = 10000000;

Again, given how it’s used it looks like this constant should be a  double, not an __int64.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:198
> +        descriptor->Release();
> +        return 0;

Could just set duration to 0 here and fall through rather than repeating the descriptor->Release() code twice.

[Darin Adler]

Comment on attachment 248450 [details]
Patch

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

>> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:173
>> +    const __int64 tenMegaHertz = 10000000;
> 
> Not sure this type should be __int64; given how the calculation is done, seems it should just be a double.
> 
> Also, megahertz is one word, not two, so there should not be an interact.

An “intercap”, not an “interact”.

(In reply to comment #0)
> I found some old code from my experimenting with Media Foundation.  This
> should probably be upstreamed, but I don't have a machine to test this on
> right now, and the media controls are covered up by our child window. 
> Peavo, could you look at this?

This looks good to me :)

+double MediaPlayerPrivateMediaFoundation::durationDouble() const
+{
+    const __int64 tenMegaHertz = 10000000;
+    if (!m_mediaSource)
+        return 0;
+	
+    IMFPresentationDescriptor* descriptor;

Maybe use a smart pointer here.

(In reply to comment #0)
> I found some old code from my experimenting with Media Foundation.  This
> should probably be upstreamed, but I don't have a machine to test this on
> right now, and the media controls are covered up by our child window. 
> Peavo, could you look at this?

I hope to get some time to look at how to draw directly to the WebView window soon ... :)

[Alex Christensen]

http://trac.webkit.org/changeset/181863