785536 - Add MPAPI GetPref so platform decoders can query about:config prefs

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P3)

Description

[Chris Peterson [:cpeterson]]

Attachment: add-PluginHost-GetPref.patch

This patch adds a GetPref() function to PluginHost that allows platform decoders to query about:config prefs.

In particular, I would like to be able to override the OMXCodec::CreationFlags we pass to Stagefright's OMXCodec::Create() to toggle hardware or software decoding. This would be useful for A/B testing hardware and software decoders for performance and bugs. doublec suggested the pref name "media.stagefright.omxcodec.flags".

Can you suggest there a better way for a platform decoder plugin to query a pref than extending MPAPI's PluginHost?

OmxPlugin.cpp is not able to call Preferences::GetUint() directly because Preferences.h has an #error message that it is a MOZILLA_INTERNAL_API only available within libxul.

Comment 1

[Chris Peterson [:cpeterson]]

We could also use this pref to temporarily disable hardware decoding when this code is uplifted from Nightly 17 to Aurora 17 next week.

Comment 2

[Chris Peterson [:cpeterson]]

Comment on attachment 655204 [details] [diff] [review]
add-PluginHost-GetPref.patch

doublec, this patch adds a GetPref() function to PluginHost that allows platform decoders to query about:config prefs. There are more notes above in this bug report.

This change is a little ugly. Can you suggest a cleaner way to expose user-configurable prefs to a media plugin?

You and I had previously discussed passing a plugin-specific configuration struct to CreateDecoder(), but that API change would require changing all media plugins' CreateDecoder() APIs. Plus, the core code has to know which configuration prefs each plugin wants. Adding a GetPref() function to PluginHost only affects media plugins that care to call it.

Comment 3

[cajbir (:cajbir)]

Comment on attachment 655204 [details] [diff] [review]
add-PluginHost-GetPref.patch

Unfortunately the Mozilla preferences API can only be used on the main thread and in this patch the call happens in the decoder thread (from nsBuiltinDecoderStateMachine::DecodeMetadata).

I do not know of an easy non-code-intrusive way of calling this on the main thread and passing it through to the plugin unfortunately.

Comment 4

[Andreas Gal :gal]

Send a runnable and wait for the result. Should be trivial.

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: add-PluginHost-GetIntPref-v2.patch

Changes in patch v2:

* Renames GetPref() to GetIntPref(), so getters for other pref types may be added 
later.
* GetIntPref() dispatches a synchronous runnable from the decoder thread to the 
main thread. This synchronous call only affects the video initialization code 
path for media plugins that call GetIntPref(), which is currently just Android.
* Log a warning message if the Stagefright pref is overriding the default decoder 
behavior by forcing hardware or software decoder.

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson (:cpeterson) from comment #5)
> Created attachment 656615 [details] [diff] [review]
> add-PluginHost-GetIntPref-v2.patch

doublec, have you had a chance to review my updated patch for adding a GetIntPref() API to MPAPI?

Comment 7

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af9abc3c3776

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af9abc3c3776

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 656615 [details] [diff] [review]
add-PluginHost-GetIntPref-v2.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: This patch adds an about:config pref to force Android H264 video to use a software decoder or the hardware decoder. This will make debugging and performance testing much easier.
Testing completed (on m-c, etc.): m-c and local testing.
Risk to taking this patch (and alternatives if risky): Low risk because this code path only affects Android H264 video playback.
String or UUID changes made by this patch: N/A

Comment 10

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/01054a593d3d