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

RESOLVED FIXED in Firefox 17

Status

()

Core
Audio/Video
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

Trunk
mozilla18
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox16 wontfix, firefox17 fixed, firefox18 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Attachment #655204 - Flags: review?(gal)
(Assignee)

Comment 1

5 years ago
We could also use this pref to temporarily disable hardware decoding when this code is uplifted from Nightly 17 to Aurora 17 next week.
(Assignee)

Comment 2

5 years ago
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.
Attachment #655204 - Flags: review?(gal) → review?(chris.double)

Comment 3

5 years ago
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.
Attachment #655204 - Flags: review?(chris.double) → review-

Comment 4

5 years ago
Send a runnable and wait for the result. Should be trivial.
(Assignee)

Comment 5

5 years ago
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.
Attachment #655204 - Attachment is obsolete: true
Attachment #656615 - Flags: review?(chris.double)
(Assignee)

Comment 6

5 years ago
(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?

Updated

5 years ago
Attachment #656615 - Flags: review?(chris.double) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/af9abc3c3776
status-firefox16: --- → wontfix
status-firefox17: --- → affected
status-firefox18: --- → fixed
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/af9abc3c3776
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

5 years ago
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
Attachment #656615 - Flags: approval-mozilla-aurora?
Attachment #656615 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/01054a593d3d
status-firefox17: affected → fixed
Depends on: 791213
You need to log in before you can comment on or make changes to this bug.