Last Comment Bug 785536 - Add MPAPI GetPref so platform decoders can query about:config prefs
: Add MPAPI GetPref so platform decoders can query about:config prefs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: ARM Android
: P3 normal (vote)
: mozilla18
Assigned To: Chris Peterson [:cpeterson]
:
:
Mentors:
Depends on: 791213
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-24 15:51 PDT by Chris Peterson [:cpeterson]
Modified: 2012-09-14 05:38 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed
fixed


Attachments
add-PluginHost-GetPref.patch (4.88 KB, patch)
2012-08-24 15:51 PDT, Chris Peterson [:cpeterson]
cajbir.bugzilla: review-
Details | Diff | Splinter Review
add-PluginHost-GetIntPref-v2.patch (6.67 KB, patch)
2012-08-29 14:29 PDT, Chris Peterson [:cpeterson]
cajbir.bugzilla: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2012-08-24 15:51:53 PDT
Created attachment 655204 [details] [diff] [review]
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] 2012-08-24 16:02:44 PDT
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] 2012-08-28 16:24:36 PDT
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) 2012-08-28 17:30:49 PDT
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 2012-08-28 17:36:31 PDT
Send a runnable and wait for the result. Should be trivial.
Comment 5 Chris Peterson [:cpeterson] 2012-08-29 14:29:52 PDT
Created attachment 656615 [details] [diff] [review]
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] 2012-09-05 17:15:11 PDT
(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] 2012-09-10 12:53:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/af9abc3c3776
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-09-10 18:44:30 PDT
https://hg.mozilla.org/mozilla-central/rev/af9abc3c3776
Comment 9 Chris Peterson [:cpeterson] 2012-09-11 15:05:45 PDT
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] 2012-09-12 16:49:46 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/01054a593d3d

Note You need to log in before you can comment on or make changes to this bug.