Bug 142594 - [MediaFoundation] Implement seek
Summary: [MediaFoundation] Implement seek
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-11 14:14 PDT by Alex Christensen
Modified: 2015-03-23 13:33 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.00 KB, patch)
2015-03-11 14:33 PDT, Alex Christensen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2015-03-11 14:14:30 PDT
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?
Comment 1 Alex Christensen 2015-03-11 14:33:38 PDT
Created attachment 248450 [details]
Patch
Comment 2 Darin Adler 2015-03-11 15:56:55 PDT
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.
Comment 3 Darin Adler 2015-03-11 15:57:29 PDT
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”.
Comment 4 peavo 2015-03-16 10:24:20 PDT
(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.
Comment 5 peavo 2015-03-16 10:28:06 PDT
(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 ... :)
Comment 6 Alex Christensen 2015-03-23 13:33:01 PDT
http://trac.webkit.org/changeset/181863