Closed Bug 785536 Opened 12 years ago Closed 12 years ago

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


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




Tracking Status
firefox16 --- wontfix
firefox17 --- fixed
firefox18 --- fixed


(Reporter: cpeterson, Assigned: cpeterson)




(1 file, 1 obsolete file)

Attached patch add-PluginHost-GetPref.patch (obsolete) — Splinter Review
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.
Attachment #655204 - Flags: review?(gal)
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 on attachment 655204 [details] [diff] [review]

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.
Attachment #655204 - Flags: review?(gal) → review?(chris.double)
Comment on attachment 655204 [details] [diff] [review]

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.
Attachment #655204 - Flags: review?(chris.double) → review-
Send a runnable and wait for the result. Should be trivial.
Changes in patch v2:

* Renames GetPref() to GetIntPref(), so getters for other pref types may be added 
* 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.
Attachment #655204 - Attachment is obsolete: true
Attachment #656615 - Flags: review?(chris.double)
(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?
Attachment #656615 - Flags: review?(chris.double) → review+
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 656615 [details] [diff] [review]

[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
Attachment #656615 - Flags: approval-mozilla-aurora?
Attachment #656615 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.