Bug 131830 – media foundation video element skeleton

• Component: Media
• Status: RESOLVED FIXED

[Alex Christensen]

I made a video player that can play videos from urls, and I started to implement MediaPlayerPrivate functions for it.  play, pause, seek, and maxTimeSeekable work.  I'm working on getting it to run from the WebKit side, and this skeleton compiles and shows the controls and poster.  I'd like to upstream it and get feedback.

[Alex Christensen]

Created attachment 229599 [details]
patch

[WebKit Commit Bot]

Attachment 229599 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:30:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:34:  The parameter name "registrar" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Comment on attachment 229599 [details]
patch

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

This looks like a great start. Please use "notImplemented()" in the various stubs you have. Also, are there specific build requirements you need to use MediaFoundation? Is it only available under Windows 8 or something?

We might want to protect the MediaFoundation stuff inside an SDK guard or something.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:55
> +{

I'd recommend putting "notImplemented()" in all of these locations. Then we can remove them as you flesh out support.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:69
> +void MediaPlayerPrivateMediaFoundation::load(const String&) { }

Ditto for all of these stubs:

load(const String&)
{
    notImplemented();
}

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:30
> + 

Get rid of this single space character!

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:34
> +    static void registerMediaEngine(MediaEngineRegistrar registrar);

Remove 'registrar' please.

> Source/WebKit/win/FullscreenVideoController.cpp:28
> +#if ENABLE(VIDEO) && !USE(GSTREAMER) && !USE(MEDIA_FOUNDATION)

Why don't you want to support full screen with media foundation?

[Alex Christensen]

(In reply to comment #3)
> Also, are there specific build requirements you need to use MediaFoundation? Is it only available under Windows 8 or something?
It's available on Windows Vista and newer.
> 
> We might want to protect the MediaFoundation stuff inside an SDK guard or something.
It is included with Visual Studio 2013.  I don't think this is necessary.
> Why don't you want to support full screen with media foundation?
It wasn't supported with GStreamer.  I'll support it later if I need it.

[Alex Christensen]

Created attachment 229602 [details]
Patch

[Alex Christensen]

Comment on attachment 229602 [details]
Patch

I got an ffmpeg-based player working instead, but it opens a separate window instead of drawing to the current window.  I've got some work to do, but I think I prefer ffmpeg over Media Foundation.

[Brent Fulgham]

(In reply to comment #6)
> (From update of attachment 229602 [details])
> I got an ffmpeg-based player working instead, but it opens a separate window instead of drawing to the current window.  I've got some work to do, but I think I prefer ffmpeg over Media Foundation.

That's too bad. The ffmpeg code has some weird licensing issues. If you have people who don't like CFLite's licensing, you are going to really enjoy the feedback from ffmpeg!  :-)

[Brent Fulgham]

Comment on attachment 229602 [details]
Patch

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

Looks good. I had a few minor suggestions (I thin you should mark the methods in MediaPlayerPrivateMediaFoundation as 'override' where appropriate.

I assume the new USE(MEDIA_FOUNDATION) stuff will be added in a future patch to FeatureDefines.props?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:160
> +        || !m_player->visible())

I think this could be one line.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:49
> +    virtual bool hasVideo() const;

Should these be marked 'override'?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:70
> +

Please get rid of this extra whitespace.

[Alex Christensen]

Committed to http://trac.webkit.org/changeset/167670

[Alex Christensen]

And sorry, I ignored some of your last comments.  I'll fix them in my next patch when I have something working.