Bug 23797 – A platform should be able to use more than one media engine for <video> and <audio>

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Eric Carlson]

It is very difficult for a port to use more than one media engine to implement <video> and <audio> (eg. QuickTime and Windows Media on Windows) because there can be only one type of  MediaPlayerPrivate. MediaPlayer should be able to create more than one type of private player, and should have a heuristic for choose the correct engine based on MIME type.

[Eric Carlson]

Created attachment 27409 [details]
Prototype patch

This patch only supports Mac build for now.

[Eric Carlson]

Created attachment 27414 [details]
patch cleaned up a bit

[Darin Adler]

Comment on attachment 27414 [details]
patch cleaned up a bit

> -    String pickMedia();
> +    String pickMedia(String& mediaMIMEType);

It's a little mysterious here what the return value is. Is it a URL? Probably needs a comment.

> +    String type = mimeType.stripWhiteSpace();

I worry a little about a low-ish level function that always strips whitespace. Is this stripping called for? Is it done by other browsers? I only want to strip when it's really needed. It's one thing to skip whitespace around a separator and another to just "strip" on general principles.

> +    static String getParameterFromMIMEType(const String& type, const String& parameter);

I think the argument should be named "parameterName". The returned thing is what I'd call the parameter. Maybe.

> +    MediaPlayerFactory(NewMediaEnginePlayer ctor, MediaEngineSupportedTypes getTypes, MediaEngineSupportsType supportsType) 
> +        : constructor(ctor)
> +        , getSupportedTypes(getTypes)
> +        , supportsTypeAndCodecs(supportsType)  { };

Formatting here should be different. The braces should go on their own separate lines. No semicolon. Also, no need to abbreviate to "ctor". You can even have the member be named the same thing as the initializer parameter. We've tested that elsewhere.

> +static void addMediaEngine(NewMediaEnginePlayer ctor, MediaEngineSupportedTypes getSupportedTypes, MediaEngineSupportsType supportsType);

Should omit the argument names here.

> +static MediaPlayerFactory *chooseBestEngineForTypeAndCodecs(const String& type, const String& codecs);

Should move the * to the left here.

> +static Vector<MediaPlayerFactory*>& installedMediaEngines();

Should not have this declaration since the function is defined below. We only need declarations of internal functions if there are forward references.

> +static MediaPlayerFactory *chooseBestEngineForTypeAndCodecs(const String& type, const String& codecs)

The * needs to be moved left.

> +    Vector<MediaPlayerFactory*>::const_iterator it = engines.begin();
> +    const Vector<MediaPlayerFactory*>::const_iterator end = engines.end();

We normally iterate vectors using indices rather than iterators.

> +        m_private = engine->constructor(this);
> +
> +    if (m_private)
> +        m_private->load(url);

Maybe we should create a class for a null engine so we don't have to pay for null checks everywhere. It doesn't seem like a case we want to optimize for.

> +    RefPtr<MediaPlayerPrivateInterface> m_private;

It seems strange to ref-count the private interface. Do we need to do that?

> +typedef PassRefPtr<MediaPlayerPrivateInterface> (*NewMediaEnginePlayer)(MediaPlayer *player);

Please leave out the name "player" and move the *.

> +typedef void (*MediaEngineRegistrar)(NewMediaEnginePlayer ctor, MediaEngineSupportedTypes getTypes, MediaEngineSupportsType supportsType); 

Please leave out the argument names here.

Generally looks great!

[Antti Koivisto]

Comment on attachment 27414 [details]
patch cleaned up a bit

I like it. A few random comments from a quick scan, nothing very important:

- MIME stuff seems to be calling for a new type 
- unnecessary parenthesis like this in a few places:
return (m_private ? m_private->currentTime() : 0);
- "Supported" is too generic name for public WebCore namespace, how about scoping it to MediaPlayer (as in MediaPlayer::Supported)
- MayBeSupported -> MaybeSupported?

[Eric Carlson]

Created attachment 27451 [details]
updated patch

Updated for Antti and Darin's comments except for putting MIME type functions into a new type, will do that in a future patch. Includes untested changes for all platforms.

[Eric Carlson]

Created attachment 27469 [details]
updated patch

Tested on Windows, fixed a few typos.

[Eric Carlson]

Created attachment 27538 [details]
Updated patch

Updated with fixes based on GTK testing by zecke.

[Eric Carlson]

Created attachment 27571 [details]
patch

Final clean up.

[Darin Adler]

Comment on attachment 27571 [details]
patch

I really think we should come up with a better name than "private" for the back end of the media player.

> +                if (end != -1)
> +                    end = type.length();

I think this should be end == -1. How did you test this parsing? Do we have test suite for this mini-parser?

> +    static String getParameterFromMIMEType(const String& type, const String& parameterName);
> +    static String stripParametersFromMIMEType(const String& type);

In other places in our API we don't use the word get -- we just use nouns for getters. However, you're just being consistent with the rest of the registry so I'm having a hard time finding fault.

> + * Copyright (C) 2007-2009 Apple Inc.  All rights reserved.

We use comma-separated years, no ranges. (And also just one space, not two spaces after a period.)

> +struct MediaPlayerFactory {
> +    MediaPlayerFactory(CreateMediaEnginePlayer constructor, MediaEngineSupportedTypes getSupportedTypes, MediaEngineSupportsType supportsTypeAndCodecs) 
> +        : constructor(constructor)
> +        , getSupportedTypes(getSupportedTypes)
> +        , supportsTypeAndCodecs(supportsTypeAndCodecs)  
> +    { }

We'd put the braces on separate lines in a case like this.

> +// ++++++++++++++++++++

We rarely use things like this, and when we do, we use --- rather than +++. Maybe just leave it out?

> +    //  if we didn't find an engine that claims the MIME type, just use the first engine

Extra space at the star of the comment here.

> +        delete m_private;
> +        m_private = 0;
> +        m_private = engine->constructor(this);

This makes it look like m_private should be an OwnPtr.

> +    Vector<MediaPlayerFactory*>::const_iterator it = engines.begin();
> +    const Vector<MediaPlayerFactory*>::const_iterator end = engines.end();

Preferred style is to iterate vectors using indices, not iterators.

> +class NULLMediaPlayerPrivate : public MediaPlayerPrivateInterface {

I don't think the word Null should be capitalized here. Also, we normally put class definitions at the top of the file.

> +public:
> +    NULLMediaPlayerPrivate(MediaPlayer*) {};
> +    virtual ~NULLMediaPlayerPrivate() {};

These declarations aren't needed. You get them automatically. Also, no semicolons after function bodies.

> +    virtual void cancelLoad() { };

No semicolons after function bodies.

> -class MediaPlayerPrivate;
> +class MediaPlayerPrivateInterface;

I guess you're doing this so the concrete implementations for each platform can still be named MediaPlayerPrivate, but I think that's only good short term. Eventually their names should be specific to the platform, because we'll want to compile more than one at a time.

>  class String;
>  
>  class MediaPlayerClient {
> @@ -52,19 +52,23 @@ public:
>      virtual void mediaPlayerRepaint(MediaPlayer*) { }
>  };
>  
> +
>  class MediaPlayer : Noncopyable {

We normally don't use multiple blank lines for formatting.

> +#include "IntRect.h"

You don't need the definition of IntRect to compile a function that takes a const IntRect& and ignores it, so this should just be forward-declared in this file.

> +#include "MediaPlayer.h"
> +#include "Timer.h"

Why is this included in MediaPlayerPrivate.h? I see no timer.

> +#include "TimeRanges.h"

Same question. I see no use of TimeRanges.h.

> +class String;

Since we have to include MediaPlayer.h, we don't need this forward declaration. But I think the real fix is to eliminate the need to include MediaPlayer.h.

> +class MediaPlayerPrivateInterface
> +{

Brace goes on same line with class name.

> +public:
> +    virtual ~MediaPlayerPrivateInterface() {};

Lets be consistent about including spaces between the braces. This should not have a semicolon after it.

> +    virtual void setRate(float inRate) = 0;

We don't use the "in" prefix.

> +    virtual void setVolume(float inVolume) = 0;

Ditto.

> +    virtual MediaPlayer::NetworkState networkState() const = 0;
> +    virtual MediaPlayer::ReadyState readyState() const = 0;

We should separate these types out and make them non-members. Then we can put them in a separate header and eliminate the need to include MediaPlayer.h here. Or not, if you think that's too much of a pain.

> +    virtual bool totalBytesKnown() const { return totalBytes() > 0; }

Does this need to be virtual or not?

> +    virtual void setRect(const IntRect& r) = 0;

No need for the "r" here.

> +    // FIXME: do the real thing
> +    notImplemented();
> +    return type == "video/x-theora+ogg" ? (!codecs.isEmpty() ? MediaPlayer::MayBeSupported : MediaPlayer::IsSupported) : MediaPlayer::IsNotSupported;

Could you write a better FIXME here? This one's a little vague.

> +        MediaPlayerPrivate(MediaPlayer*);
> +Ê ÊÊÊÊÊÊstatic MediaPlayerPrivateInterface* create(MediaPlayer* player);

Looks like this won't compile.

I'm going to say r=me, but there still seems like some room for improvement as above.

[Eric Carlson]

Update for all of Darin's comments except for changing the names of the concrete platform implementations, will do that in a future check-in.

[Eric Carlson]

Committed revision 40925.

[Dimitri Glazkov (Google)]

Adding MIMETypeRegistry::getParameterFromMIMEType/stripParametersFromMIMEType breaks both chromium and wx builds (they both have their own impls of MIMETypeRegistry). Since the new methods are static, what do you think about just turning them info functions and moving to header?

[Dimitri Glazkov (Google)]

.. or perhaps a new MimeTypeParameterParser class would work better? I am open to ideas.

[Eric Carlson]

I think a new class makes the most sense (this is also what Antti suggested in his review below). "MIMEType" seems the logical class to me, but unfortunately it already exists in WebCore/plugins. Maybe that class can be renamed since it is specific to plug-ins?

[Dimitri Glazkov (Google)]

Eeek. That would be a rather large change, considering that there's also an IDL by the same name.

Can we instead just have MIMETypeString or something like it?

[Antti Koivisto]

ContentType and rename to ContentTypeRegistry?

[Dimitri Glazkov (Google)]

anttik, you are awesome. See progress of this change in bug 23999.