Bug 106972 – Add a Setting to disable QTKit media engine.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Add a Setting to disable QTKit media engine.

[Jer Noble]

Created attachment 182922 [details]
Patch

[Jer Noble]

<rdar://problem/13019548>

[Early Warning System Bot]

Comment on attachment 182922 [details]
Patch

Attachment 182922 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15901333

[kov's GTK+ EWS bot]

Comment on attachment 182922 [details]
Patch

Attachment 182922 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15908264

[Jer Noble]

Created attachment 182924 [details]
Patch

Added #if guards to protect ports without QTKit support.

[Eric Carlson]

Comment on attachment 182924 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.cpp:229
> +#if PLATFORM(MAC) || (PLATFORM(QT) && USE(QTKIT))
> +        if (Settings::isQTKitEnabled())
> +            MediaPlayerPrivateQTKit::registerMediaEngine(addMediaEngine);
> +#endif
> +

Having the check here means it won't really be possible to have a control in an application's UI, because the user can't know if the media engines have already been registered or not (eg. loading a document WebCore doesn't handle natively has the side effect of registering all media engines to see if it is possible to create a media document). I think it would be more useful to always register the QTKit media engine, but then not use it when the pref is set by checking engineDescription() in bestMediaEngineForTypeAndCodecs().

[Jer Noble]

(In reply to comment #6)
> (From update of attachment 182924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182924&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:229
> > +#if PLATFORM(MAC) || (PLATFORM(QT) && USE(QTKIT))
> > +        if (Settings::isQTKitEnabled())
> > +            MediaPlayerPrivateQTKit::registerMediaEngine(addMediaEngine);
> > +#endif
> > +
> 
> Having the check here means it won't really be possible to have a control in an application's UI, because the user can't know if the media engines have already been registered or not (eg. loading a document WebCore doesn't handle natively has the side effect of registering all media engines to see if it is possible to create a media document). I think it would be more useful to always register the QTKit media engine, but then not use it when the pref is set by checking engineDescription() in bestMediaEngineForTypeAndCodecs().

We don't have access to engineDescription() at that point, as bestMediaEngineForTypeAndCodecs() just uses the MediaEngine factory methods, not an instance of MediaPlayerPrivate.  But even if it were available, that seems fragile (i.e., comparing strings returned from engineDescription()).

What if we recreate the list of media engine factories whenever one of those preferences change?

[Eric Carlson]

Comment on attachment 182924 [details]
Patch

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

>>> Source/WebCore/platform/graphics/MediaPlayer.cpp:229
>>> +
>> 
>> Having the check here means it won't really be possible to have a control in an application's UI, because the user can't know if the media engines have already been registered or not (eg. loading a document WebCore doesn't handle natively has the side effect of registering all media engines to see if it is possible to create a media document). I think it would be more useful to always register the QTKit media engine, but then not use it when the pref is set by checking engineDescription() in bestMediaEngineForTypeAndCodecs().
> 
> We don't have access to engineDescription() at that point, as bestMediaEngineForTypeAndCodecs() just uses the MediaEngine factory methods, not an instance of MediaPlayerPrivate.  But even if it were available, that seems fragile (i.e., comparing strings returned from engineDescription()).
> 
> What if we recreate the list of media engine factories whenever one of those preferences change?

Fair enough. Having to set the pref, quit, and relaunch is a pain but not the end of the world.

[Jer Noble]

Created attachment 183009 [details]
Patch

Notwithstanding ericc's r+, I've modified the patch to make the setting 'live'.

[Eric Carlson]

Comment on attachment 183009 [details]
Patch

Nice solution, thanks!

[Early Warning System Bot]

Comment on attachment 183009 [details]
Patch

Attachment 183009 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15903533

[Early Warning System Bot]

Comment on attachment 183009 [details]
Patch

Attachment 183009 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15912425

[WebKit Review Bot]

Comment on attachment 183009 [details]
Patch

Attachment 183009 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15925158

[Jer Noble]

Committed r139899: <http://trac.webkit.org/changeset/139899>